owasp-modsecurity · martinhsv · Nov 12, 2019 · Nov 14, 2019
diff --git a/CHANGES b/CHANGES
@@ -1,6 +1,8 @@
 v3.0.4 - YYYY-MMM-DD (to be released)
 -------------------------------------
 
+ - Fix Cookie header parsing issues
+   [Issue #2201 - @airween, @martinhsv]
  - Fix rules with nolog are logging to part H
    [Issue #2196 - @martinhsv]
  - Fix argument key-value pair parsing cases

diff --git a/src/transaction.cc b/src/transaction.cc
@@ -548,20 +548,64 @@ int Transaction::addRequestHeader(const std::string& key,
 
     if (keyl == "cookie") {
         size_t localOffset = m_variableOffset;
+        size_t pos;
+
         std::vector<std::string> cookies = utils::string::ssplit(value, ';');
+
+        // Get rid of any optional whitespace after the cookie-string
+        // (i.e. after the end of the final cookie-pair)
+        if (!cookies.empty()) {
+            std::string& final_cookie_pair = cookies.back();
+            while (!final_cookie_pair.empty() && isspace(final_cookie_pair.back())) {
+                final_cookie_pair.pop_back();
+            }
+        }
+
         for (const std::string &c : cookies) {
-            std::vector<std::string> s = utils::string::split(c,
-               '=');
-            if (s.size() > 1) {
-                if (s[0].at(0) == ' ') {
-                    s[0].erase(0, 1);
-                }
-                m_variableRequestCookiesNames.set(s[0],
-                    s[0], localOffset);
+            // skip empty substring, eg "Cookie: ;;foo=bar"
+            if (c.empty() == true) {
+                localOffset++; // add length of ';'
+                continue;
+            }
 
-                localOffset = localOffset + s[0].size() + 1;
-                m_variableRequestCookies.set(s[0], s[1], localOffset);
-                localOffset = localOffset + s[1].size() + 2;
+            // find the first '='
+            pos = c.find_first_of("=", 0);
+            std::string ckey = "";
+            std::string cval = "";
+
+            // if the cookie doesn't contains '=', its just a key
+            if (pos == std::string::npos) {
+                ckey = c;
+            }
+            // else split to two substrings by first =
+            else {
+                ckey = c.substr(0, pos);
+                // value will contains the next '=' chars if exists
+                // eg. foo=bar=baz -> key: foo, value: bar=baz
+                cval = c.substr(pos+1);
+            }
+
+            // ltrim the key - following the modsec v2 way
+            while (ckey.empty() == false && ckey.at(0) == ' ') {
+                ckey.erase(0, 1);
+                localOffset++;
+            }
+
+            // if the key is empty (eg: "Cookie:   =bar;") skip it
+            if (ckey.empty() == true) {
+                localOffset = localOffset + c.length() + 1;
+                continue;
+            }
+            else {
+                // handle cookie only if the key is not empty
+                // set cookie name
+                m_variableRequestCookiesNames.set(ckey,
+                        ckey, localOffset);
+                localOffset = localOffset + ckey.size() + 1;
+                // set cookie value
+                m_variableRequestCookies.set(ckey, cval,
+                        localOffset);
+                localOffset = localOffset + cval.size() + 1;
             }
         }
     }

diff --git a/test/test-cases/regression/variable-REQUEST_COOKIES.json b/test/test-cases/regression/variable-REQUEST_COOKIES.json
@@ -2,7 +2,7 @@
   {
     "enabled":1,
     "version_min":300000,
-    "title":"Testing Variables :: REQUEST_COOKIES (1/3)",
+    "title":"Testing Variables :: REQUEST_COOKIES (1/6)",
     "client":{
       "ip":"200.249.12.31",
       "port":123
@@ -42,7 +42,7 @@
   {
     "enabled":1,
     "version_min":300000,
-    "title":"Testing Variables :: REQUEST_COOKIES (2/3)",
+    "title":"Testing Variables :: REQUEST_COOKIES (2/6)",
     "client":{
       "ip":"200.249.12.31",
       "port":123
@@ -82,7 +82,7 @@
   {
     "enabled":1,
     "version_min":300000,
-    "title":"Testing Variables :: REQUEST_COOKIES (3/3)",
+    "title":"Testing Variables :: REQUEST_COOKIES (3/6)",
     "client":{
       "ip":"200.249.12.31",
       "port":123
@@ -118,6 +118,126 @@
       "SecRuleEngine On",
       "SecRule REQUEST_COOKIES \"@contains test \" \"id:1,pass,t:trim\""
     ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Testing Variables :: REQUEST_COOKIES (4/6)",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Cookie":"USER_TOKEN=Yes; a=z; t=b;  foo= bar"
+      },
+      "uri":"/?key=value&key=other_value",
+      "method":"GET"
+    },
+    "response":{
+      "headers":{
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[
+        "no need."
+      ]
+    },
+    "expected":{
+      "debug_log":"Target value: \"bar\""
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule REQUEST_COOKIES \"@contains test \" \"id:1,pass,t:trim\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Testing Variables :: REQUEST_COOKIES (5/6)",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Cookie":"USER_TOKEN=Yes; a=z; t=b;  foo= bar; = ; = = ; baz=value1=insert here something"
+      },
+      "uri":"/?key=value&key=other_value",
+      "method":"GET"
+    },
+    "response":{
+      "headers":{
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[
+        "no need."
+      ]
+    },
+    "expected":{
+      "debug_log":"Target value: \"value1=insert here something\""
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule REQUEST_COOKIES \"@contains test \" \"id:1,pass,t:trim\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Testing Variables :: REQUEST_COOKIES (6/6)",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Cookie":"aaa=bbb;abc=def  "
+      },
+      "uri":"/?key=value&key=other_value",
+      "method":"GET"
+    },
+    "response":{
+      "headers":{
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[
+        "no need."
+      ]
+    },
+    "expected":{
+      "debug_log":"Target value: \"def\" \\(Variable: REQUEST_COOKIES:abc\\)"
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule REQUEST_COOKIES:abc \"@rx ^def$\" \"id:1,pass\""
+    ]
   }
 ]
 
diff --git a/test/test-cases/regression/variable-REQUEST_COOKIES_NAMES.json b/test/test-cases/regression/variable-REQUEST_COOKIES_NAMES.json
@@ -2,7 +2,7 @@
   {
     "enabled":1,
     "version_min":300000,
-    "title":"Testing Variables :: REQUEST_COOKIES_NAMES (1/3)",
+    "title":"Testing Variables :: REQUEST_COOKIES_NAMES (1/4)",
     "client":{
       "ip":"200.249.12.31",
       "port":123
@@ -42,7 +42,7 @@
   {
     "enabled":1,
     "version_min":300000,
-    "title":"Testing Variables :: REQUEST_COOKIES_NAMES (2/3)",
+    "title":"Testing Variables :: REQUEST_COOKIES_NAMES (2/4)",
     "client":{
       "ip":"200.249.12.31",
       "port":123
@@ -82,7 +82,7 @@
   {
     "enabled":1,
     "version_min":300000,
-    "title":"Testing Variables :: REQUEST_COOKIES_NAMES (3/3)",
+    "title":"Testing Variables :: REQUEST_COOKIES_NAMES (3/4)",
     "client":{
       "ip":"200.249.12.31",
       "port":123
@@ -118,6 +118,46 @@
       "SecRuleEngine On",
       "SecRule REQUEST_COOKIES_NAMES \"@contains test \" \"id:1,pass,t:trim\""
     ]
+  },
+{
+    "enabled":1,
+    "version_min":300000,
+    "title":"Testing Variables :: REQUEST_COOKIES_NAMES (4/4)",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Cookie":"USER_TOKEN=Yes; a=z; t=b;  foobar"
+      },
+      "uri":"/?key=value&key=other_value",
+      "method":"GET"
+    },
+    "response":{
+      "headers":{
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[
+        "no need."
+      ]
+    },
+    "expected":{
+      "debug_log":"Target value: \"foobar\""
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule REQUEST_COOKIES_NAMES \"@contains foobar \" \"id:1,pass,t:trim\""
+    ]
   }
 ]