imparse: reject HTTP methods as initial tag only

Author: elliefm

cunit/imparse.testc

@@ -78,36 +78,50 @@ static void test_isatom(void)
 static void test_istag(void)
 {
     const char hello[] = "hello";
+    const char *const forbidden_http_methods[] = {
+        "ACL", "BIND", "LOCK", "MKCALENDAR", "MKCOL", "PATCH", "POST",
+        "PROPFIND", "PROPPATCH", "PUT", "REPORT", "SEARCH", "UNBIND",
+    };
+    const int n_forbidden_http_methods = sizeof(forbidden_http_methods)
+                                         / sizeof(forbidden_http_methods[0]);
     char tmp[2] = { 0 };
     int i;
 
     /* if it's not an atom, it definitely can't be a tag */
     for (i = 0; i <= 0xff; i++) {
         tmp[0] = (char) i;
         if (!imparse_isatom(tmp))
-            CU_ASSERT_EQUAL(imparse_istag(tmp), 0);
+            CU_ASSERT_EQUAL(imparse_istag(tmp, 0), 0);
     }
 
     /* there used to be an explicit (albeit redundant) check for this case */
     tmp[0] = '*';
-    CU_ASSERT_EQUAL(imparse_istag(tmp), 0);
+    CU_ASSERT_EQUAL(imparse_istag(tmp, 0), 0);
 
     /* "." tag idiomatic when telnetting to imap server, don't break that */
     tmp[0] = '.';
-    CU_ASSERT_NOT_EQUAL(imparse_istag(tmp), 0);
+    CU_ASSERT_NOT_EQUAL(imparse_istag(tmp, 0), 0);
 
     /* angle brackets exploitable in cross-protocol reflection attacks */
     tmp[0] = '<';
-    CU_ASSERT_EQUAL(imparse_istag(tmp), 0);
+    CU_ASSERT_EQUAL(imparse_istag(tmp, 0), 0);
     tmp[0] = '>';
-    CU_ASSERT_EQUAL(imparse_istag(tmp), 0);
+    CU_ASSERT_EQUAL(imparse_istag(tmp, 0), 0);
 
     /* colon character in tag suggests confused HTTP client */
     tmp[0] = ':';
-    CU_ASSERT_EQUAL(imparse_istag(tmp), 0);
+    CU_ASSERT_EQUAL(imparse_istag(tmp, 0), 0);
 
     /* make sure it doesn't just always return zero... */
-    CU_ASSERT_NOT_EQUAL(imparse_istag(hello), 0);
+    CU_ASSERT_NOT_EQUAL(imparse_istag(hello, 0), 0);
+
+    for (i = 0; i < n_forbidden_http_methods; i++) {
+        /* reject forbidden HTTP method used as tag on the first command */
+        CU_ASSERT_EQUAL(imparse_istag(forbidden_http_methods[i], 0), 0);
+
+        /* but permit during an established session */
+        CU_ASSERT_NOT_EQUAL(imparse_istag(forbidden_http_methods[i], 1), 0);
+    }
 }
 
 static void test_parse_range(void)

imap/httpd.c

@@ -510,7 +510,11 @@ static struct sasl_callback mysasl_cb[] = {
     { SASL_CB_LIST_END, NULL, NULL }
 };
 
-/* Array of HTTP methods known by our server. */
+/* Array of HTTP methods known by our server.
+ * Keep this up to date with reject_http_method_tag() in imparse.c
+ * XXX Would be nice if this table were in libcyrus somewhere rather than
+ * XXX directly in httpd, so that imparse could use it directly.
+ */
 const struct known_meth_t http_methods[] = {
     { "ACL",           0,                          CYRUS_HTTP_ACL_TOTAL },
     { "BIND",          0,                          CYRUS_HTTP_BIND_TOTAL },

imap/imapd.c

@@ -1493,6 +1493,7 @@ static void cmdloop(void)
     int readonly = config_getswitch(IMAPOPT_READONLY);
     int syntax_errors = 0;
     const int syntax_errors_limit = 10; /* XXX make this configurable? */
+    unsigned command_count = 0;
 
     prot_printf(imapd_out, "* OK [CAPABILITY");
     capa_response(CAPA_PREAUTH);
@@ -1599,7 +1600,7 @@ static void cmdloop(void)
             }
             goto done;
         }
-        if (c != ' ' || !imparse_istag(tag.s)) {
+        if (c != ' ' || !imparse_istag(tag.s, command_count)) {
             syntax_errors ++;
             prot_printf(imapd_out, "* BAD Invalid tag\r\n");
             eatline(imapd_in, c);
@@ -1618,6 +1619,10 @@ static void cmdloop(void)
         xstrncpy(cmdname, cmd.s, 99);
         cmd.s[0] = toupper((unsigned char) cmd.s[0]);
 
+        /* that looks like a command, count it (but saturate, not overflow) */
+        if (command_count != UINT_MAX)
+            command_count ++;
+
         if (config_getswitch(IMAPOPT_CHATTY))
             syslog(LOG_NOTICE, "command: %s %s", tag.s, cmd.s);
 
@@ -2312,6 +2317,9 @@ static void cmdloop(void)
                 }
                 cmd_starttls(tag.s, 0);
 
+                /* reset command count, the real imap session starts here */
+                command_count = 0;
+
                 prometheus_increment(CYRUS_IMAP_STARTTLS_TOTAL);
                 continue;
             }

lib/imparse.c

@@ -43,6 +43,7 @@
 #include <config.h>
 #include <errno.h>
 #include <stdio.h>
+#include <string.h>
 
 #include "imparse.h"
 #include "util.h"
@@ -216,10 +217,64 @@ EXPORTED int imparse_isnumber(const char *s)
     return 1;
 }
 
+static int reject_http_method_tag(const char *s)
+{
+    /* Don't like tags that match HTTP methods that accept a request body!
+     * Keep this up to date with http_methods[] in httpd.c
+     * and test_istag() in imparse.testc
+     */
+    switch (s[0]) {
+    case 'A':
+        if (0 == strcmp(s, "ACL"))
+            return 1;
+        break;
+    case 'B':
+        if (0 == strcmp(s, "BIND"))
+            return 1;
+        break;
+    case 'L':
+        if (0 == strcmp(s, "LOCK"))
+            return 1;
+        break;
+    case 'M':
+        if (0 == strcmp(s, "MKCALENDAR"))
+            return 1;
+        else if (0 == strcmp(s, "MKCOL"))
+            return 1;
+        break;
+    case 'P':
+        if (0 == strcmp(s, "PATCH"))
+            return 1;
+        else if (0 == strcmp(s, "POST"))
+            return 1;
+        else if (0 == strcmp(s, "PROPFIND"))
+            return 1;
+        else if (0 == strcmp(s, "PROPPATCH"))
+            return 1;
+        else if (0 == strcmp(s, "PUT"))
+            return 1;
+        break;
+    case 'R':
+        if (0 == strcmp(s, "REPORT"))
+            return 1;
+        break;
+    case 'S':
+        if (0 == strcmp(s, "SEARCH"))
+            return 1;
+        break;
+    case 'U':
+        if (0 == strcmp(s, "UNBIND"))
+            return 1;
+        break;
+    }
+
+    return 0;
+}
+
 /*
  * Return nonzero if we like 's' as an IMAP tag
  */
-EXPORTED int imparse_istag(const char *s)
+EXPORTED int imparse_istag(const char *s, unsigned command_count)
 {
     static const char reject[] = {
     /*       0  1  2  3  4  5  6  7  8  9  A  B  C  D  E  F */
@@ -242,6 +297,9 @@ EXPORTED int imparse_istag(const char *s)
             return 0;
     }
 
+    if (command_count == 0 && reject_http_method_tag(s))
+        return 0;
+
     return 1;
 }
 

lib/imparse.h

@@ -56,7 +56,7 @@ extern int imparse_isnatom (const char *s, int len);
 extern int imparse_isatom (const char *s);
 extern int imparse_issequence (const char *s);
 extern int imparse_isnumber (const char *s);
-extern int imparse_istag (const char *s);
+extern int imparse_istag (const char *s, unsigned command_count);
 extern int imparse_range (const char *s, range_t *range);
 
 #endif /* INCLUDED_IMPARSE_H */