Merge pull request #3994 from eclipse/jetty-9.4.x-3985-cookie-parsing

Fixes #3985 - Updates to CookieCutter to reject no-equal cookies

jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java

@@ -144,14 +144,13 @@ public class CookieCutter
             boolean inQuoted = false;
             boolean quoted = false;
             boolean escaped = false;
+            boolean reject = false;
             int tokenstart = -1;
             int tokenend = -1;
             for (int i = 0, length = hdr.length(); i <= length; i++)
             {
                 char c = i == length ? 0 : hdr.charAt(i);
 
-                // System.err.printf("i=%d/%d c=%s v=%b q=%b/%b e=%b u=%s s=%d e=%d \t%s=%s%n" ,i,length,c==0?"|":(""+c),invalue,inQuoted,quoted,escaped,unquoted,tokenstart,tokenend,name,value);
-
                 // Handle quoted values for name or value
                 if (inQuoted)
                 {
@@ -199,7 +198,7 @@ public class CookieCutter
                     // Handle name and value state machines
                     if (invalue)
                     {
-                        // parse the value
+                        // parse the cookie-value
                         switch (c)
                         {
                             case ' ':
@@ -273,9 +272,12 @@ public class CookieCutter
                                         cookie = new Cookie(name, value);
                                         if (version > 0)
                                             cookie.setVersion(version);
+                                        if (!reject)
+                                        {
                                             cookies.add(cookie);
                                         }
                                     }
+                                }
                                 catch (Exception e)
                                 {
                                     LOG.debug(e);
@@ -284,6 +286,7 @@ public class CookieCutter
                                 name = null;
                                 tokenstart = -1;
                                 invalue = false;
+                                reject = false;
 
                                 break;
                             }
@@ -308,6 +311,15 @@ public class CookieCutter
                                     quoted = false;
                                     continue;
                                 }
+
+                                if (_compliance == CookieCompliance.RFC6265)
+                                {
+                                    if (isRFC6265RejectedCharacter(inQuoted, c))
+                                    {
+                                        reject = true;
+                                    }
+                                }
+
                                 if (tokenstart < 0)
                                     tokenstart = i;
                                 tokenend = i;
@@ -316,13 +328,20 @@ public class CookieCutter
                     }
                     else
                     {
-                        // parse the name
+                        // parse the cookie-name
                         switch (c)
                         {
                             case ' ':
                             case '\t':
                                 continue;
 
+                            case ';':
+                                // a cookie terminated with no '=' sign.
+                                tokenstart = -1;
+                                invalue = false;
+                                reject = false;
+                                continue;
+
                             case '=':
                                 if (quoted)
                                 {
@@ -346,6 +365,15 @@ public class CookieCutter
                                     quoted = false;
                                     continue;
                                 }
+
+                                if (_compliance == CookieCompliance.RFC6265)
+                                {
+                                    if (isRFC6265RejectedCharacter(inQuoted, c))
+                                    {
+                                        reject = true;
+                                    }
+                                }
+
                                 if (tokenstart < 0)
                                     tokenstart = i;
                                 tokenend = i;
@@ -356,7 +384,34 @@ public class CookieCutter
             }
         }
 
-        _cookies = cookies.toArray(new Cookie[cookies.size()]);
+        _cookies = cookies.toArray(new Cookie[0]);
         _lastCookies = _cookies;
     }
+
+    protected boolean isRFC6265RejectedCharacter(boolean inQuoted, char c)
+    {
+        if (inQuoted)
+        {
+            // We only reject if a Control Character is encountered
+            if (Character.isISOControl(c))
+            {
+                return true;
+            }
+        }
+        else
+        {
+            /* From RFC6265 - Section 4.1.1 - Syntax
+             *  cookie-octet  = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
+             *                  ; US-ASCII characters excluding CTLs,
+             *                  ; whitespace DQUOTE, comma, semicolon,
+             *                  ; and backslash
+             */
+            return Character.isISOControl(c) || // control characters
+                c > 127 || // 8-bit characters
+                c == ',' || // comma
+                c == ';'; // semicolon
+        }
+
+        return false;
+    }
 }

jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java

@@ -245,4 +245,18 @@ public class CookieCutterTest
 
         assertThat("Cookies.length", cookies.length, is(0));
     }
+
+    @Test
+    public void testMultipleCookies()
+    {
+        String rawCookie = "testcookie; server.id=abcd; server.detail=cfg";
+
+        // The first cookie "testcookie" should be ignored, per RFC6265, as it's missing the "=" sign.
+
+        Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC6265, rawCookie);
+
+        assertThat("Cookies.length", cookies.length, is(2));
+        assertCookie("Cookies[0]", cookies[0], "server.id", "abcd", 0, null);
+        assertCookie("Cookies[1]", cookies[1], "server.detail", "cfg", 0, null);
+    }
 }

jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java

@@ -60,12 +60,17 @@ public class CookieCutter_LenientTest
             //   quoted-string  = ( <"> *(qdtext) <"> )
             //   qdtext         = <any TEXT except <">>
 
+            // rejected, as name cannot be DQUOTED
+            Arguments.of("\"a\"=bcd", null, null),
+            Arguments.of("\"a\"=\"b c d\"", null, null),
+
             // lenient with spaces and EOF
             Arguments.of("abc=", "abc", ""),
             Arguments.of("abc = ", "abc", ""),
             Arguments.of("abc = ;", "abc", ""),
             Arguments.of("abc = ; ", "abc", ""),
             Arguments.of("abc = x ", "abc", "x"),
+            Arguments.of("abc = e f g ", "abc", "e f g"),
             Arguments.of("abc=\"\"", "abc", ""),
             Arguments.of("abc= \"\" ", "abc", ""),
             Arguments.of("abc= \"x\" ", "abc", "x"),
@@ -110,28 +115,29 @@ public class CookieCutter_LenientTest
             // Unterminated Quotes with trailing escape
             Arguments.of("x=\"abc\\", "x", "\"abc\\"),
 
-            // UTF-8 values
+            // UTF-8 raw values (not encoded) - VIOLATION of RFC6265
-            Arguments.of("2sides=\u262F", "2sides", "\u262f"), // 2 byte
+            Arguments.of("2sides=\u262F", null, null), // 2 byte (YIN YANG) - rejected due to not being DQUOTED
-            Arguments.of("currency=\"\u20AC\"", "currency", "\u20AC"), // 3 byte
+            Arguments.of("currency=\"\u20AC\"", "currency", "\u20AC"), // 3 byte (EURO SIGN)
-            Arguments.of("gothic=\"\uD800\uDF48\"", "gothic", "\uD800\uDF48"), // 4 byte
+            Arguments.of("gothic=\"\uD800\uDF48\"", "gothic", "\uD800\uDF48"), // 4 byte (GOTHIC LETTER HWAIR)
 
             // Spaces
             Arguments.of("foo=bar baz", "foo", "bar baz"),
             Arguments.of("foo=\"bar baz\"", "foo", "bar baz"),
             Arguments.of("z=a b c d e f g", "z", "a b c d e f g"),
 
-            // Bad tspecials usage
+            // Bad tspecials usage - VIOLATION of RFC6265
             Arguments.of("foo=bar;baz", "foo", "bar"),
             Arguments.of("foo=\"bar;baz\"", "foo", "bar;baz"),
             Arguments.of("z=a;b,c:d;e/f[g]", "z", "a"),
             Arguments.of("z=\"a;b,c:d;e/f[g]\"", "z", "a;b,c:d;e/f[g]"),
+            Arguments.of("name=quoted=\"\\\"badly\\\"\"", "name", "quoted=\"\\\"badly\\\"\""), // someone attempting to escape a DQUOTE from within a DQUOTED pair)
 
             // Quoted with other Cookie keywords
             Arguments.of("x=\"$Version=0\"", "x", "$Version=0"),
             Arguments.of("x=\"$Path=/\"", "x", "$Path=/"),
             Arguments.of("x=\"$Path=/ $Domain=.foo.com\"", "x", "$Path=/ $Domain=.foo.com"),
             Arguments.of("x=\" $Path=/ $Domain=.foo.com \"", "x", " $Path=/ $Domain=.foo.com "),
-            Arguments.of("a=\"b; $Path=/a; c=d; $PATH=/c; e=f\"; $Path=/e/", "a", "b; $Path=/a; c=d; $PATH=/c; e=f"),
+            Arguments.of("a=\"b; $Path=/a; c=d; $PATH=/c; e=f\"; $Path=/e/", "a", "b; $Path=/a; c=d; $PATH=/c; e=f"), // VIOLATES RFC6265
 
             // Lots of equals signs
             Arguments.of("query=b=c&d=e", "query", "b=c&d=e"),
@@ -142,7 +148,7 @@ public class CookieCutter_LenientTest
             // Google cookies (seen in wild, has `tspecials` of ':' in value)
             Arguments.of("GAPS=1:A1aaaAaAA1aaAAAaa1a11a:aAaaAa-aaA1-", "GAPS", "1:A1aaaAaAA1aaAAAaa1a11a:aAaaAa-aaA1-"),
 
-            // Strong abuse of cookie spec (lots of tspecials)
+            // Strong abuse of cookie spec (lots of tspecials) - VIOLATION of RFC6265
             Arguments.of("$Version=0; rToken=F_TOKEN''!--\"</a>=&{()}", "rToken", "F_TOKEN''!--\"</a>=&{()}"),
 
             // Commas that were not commas