From 166736db554dda8f7fe5623230da2bee22aca0b6 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 15 May 2017 21:01:15 +0200 Subject: [PATCH] Issue #1546 - more cookie leniency fixes --- .../eclipse/jetty/server/CookieCutter.java | 65 ++++++++++++------- .../jetty/server/CookieCutterTest.java | 17 ----- .../server/CookieCutter_LenientTest.java | 12 +++- 3 files changed, 50 insertions(+), 44 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java index d17e38c5be3..189ad45c5d2 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java @@ -126,17 +126,19 @@ public class CookieCutter Cookie cookie = null; boolean invalue=false; + boolean inQuoted=false; boolean quoted=false; - boolean unquotedToken=false; boolean escaped=false; int tokenstart=-1; int tokenend=-1; for (int i = 0, length = hdr.length(), last=length-1; i < length; i++) { char c = hdr.charAt(i); - + + // System.err.printf("i=%d c=%s v=%b q=%b e=%b u=%s s=%d e=%d%n" ,i,""+c,invalue,inQuoted,escaped,unquoted,tokenstart,tokenend); + // Handle quoted values for name or value - if (quoted) + if (inQuoted) { if (escaped) { @@ -148,25 +150,43 @@ public class CookieCutter switch (c) { case '"': - quoted=false; + inQuoted=false; if (i==last) { value = unquoted.toString(); } else { - unquotedToken=true; + quoted=true; tokenstart=i; tokenend=-1; } break; case '\\': - escaped=true; + if (i==last) + { + unquoted.setLength(0); + inQuoted = false; + i--; + } + else + { + escaped=true; + } continue; default: - unquoted.append(c); + if (i==last) + { + unquoted.setLength(0); + inQuoted = false; + i--; + } + else + { + unquoted.append(c); + } continue; } } @@ -183,16 +203,14 @@ public class CookieCutter continue; case ';': - if (unquotedToken) + if (quoted) { value = unquoted.toString(); unquoted.setLength(0); - unquotedToken = false; + quoted = false; } else if(tokenstart>=0 && tokenend>=0) value = hdr.substring(tokenstart, tokenend+1); - else - value=""; tokenstart = -1; invalue=false; @@ -201,7 +219,8 @@ public class CookieCutter case '"': if (tokenstart<0) { - quoted=true; + tokenstart=i; + inQuoted=true; if (unquoted==null) unquoted=new StringBuilder(); continue; @@ -209,12 +228,12 @@ public class CookieCutter // fall through to default case default: - if (unquotedToken) + if (quoted) { // must have been a bad internal quote. let's fix as best we can unquoted.append(hdr.substring(tokenstart,i)); - quoted = true; - unquotedToken = false; + inQuoted = true; + quoted = false; i--; continue; } @@ -239,27 +258,26 @@ public class CookieCutter continue; case ';': - if (unquotedToken) + if (quoted) { name = unquoted.toString(); unquoted.setLength(0); - unquotedToken = false; + quoted = false; } else if(tokenstart>=0 && tokenend>=0) { name = hdr.substring(tokenstart, tokenend+1); } - value = ""; tokenstart = -1; break; case '=': - if (unquotedToken) + if (quoted) { name = unquoted.toString(); unquoted.setLength(0); - unquotedToken = false; + quoted = false; } else if(tokenstart>=0 && tokenend>=0) { @@ -271,12 +289,12 @@ public class CookieCutter continue; default: - if (unquotedToken) + if (quoted) { // must have been a bad internal quote. let's fix as best we can unquoted.append(hdr.substring(tokenstart,i)); - quoted = true; - unquotedToken = false; + inQuoted = true; + quoted = false; i--; continue; } @@ -286,7 +304,6 @@ public class CookieCutter if (i==last) { name = hdr.substring(tokenstart, tokenend+1); - value = ""; break; } continue; diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java index 21e2db965d6..02f53e46719 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java @@ -193,21 +193,4 @@ public class CookieCutterTest assertThat("Cookies.length", cookies.length, is(1)); assertCookie("Cookies[0]", cookies[0], "key", "value", 0, null); } - - /** - * Multiple name=value, heavy abuse, badly terminated quotes, lenient behavior test - */ - @Test - public void testMultiName_BadQuoteTerminate() - { - // TODO: this seems very hokey, and allowing this as 3 separate entries is probably a security issue. - String rawCookie = "a=\"b; $Path=/a; c=d; $PATH=/c; e=f\"; $Path=/e/"; - - Cookie cookies[] = parseCookieHeaders(rawCookie); - - assertThat("Cookies.length", cookies.length, is(3)); - assertCookie("Cookies[0]", cookies[0], "a", "\"b", 0, "/a"); - assertCookie("Cookies[1]", cookies[1], "c", "d", 0, "/c"); - assertCookie("Cookies[2]", cookies[2], "e", "f\"", 0, "/e/"); - } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java index 4eb0f7d29a8..7fab2dc1f72 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java @@ -64,6 +64,8 @@ public class CookieCutter_LenientTest // RFC2109 - quoted-string values // quoted-string = ( <"> *(qdtext) <"> ) // qdtext = > + + ret.add(new String[]{"abc=\"\"", "abc", ""}); // The backslash character ("\") may be used as a single-character quoting // mechanism only within quoted-string and comment constructs. // quoted-pair = "\" CHAR @@ -98,7 +100,9 @@ public class CookieCutter_LenientTest // Unterminated Quotes ret.add(new String[]{"x=\"abc", "x", "\"abc"}); // Unterminated Quotes with valid cookie params after it - ret.add(new String[]{"x=\"abc $Path=/", "x", "\"abc"}); + ret.add(new String[]{"x=\"abc $Path=/", "x", "\"abc $Path=/"}); + // Unterminated Quotes with trailing escape + ret.add(new String[]{"x=\"abc\\", "x", "\"abc\\"}); // UTF-8 values ret.add(new String[]{"2sides=\u262F", "2sides", "\u262f"}); // 2 byte @@ -111,9 +115,9 @@ public class CookieCutter_LenientTest ret.add(new String[]{"z=a b c d e f g", "z", "a b c d e f g"}); // Bad tspecials usage - ret.add(new String[]{"foo=bar;baz", "foo", "bar;baz"}); // TODO: not sure supporting this is sane + ret.add(new String[]{"foo=bar;baz", "foo", "bar"}); ret.add(new String[]{"foo=\"bar;baz\"", "foo", "bar;baz"}); - ret.add(new String[]{"z=a;b,c:d;e/f[g]", "z", "a;b,c:d;e/f[g]"}); + ret.add(new String[]{"z=a;b,c:d;e/f[g]", "z", "a"}); ret.add(new String[]{"z=\"a;b,c:d;e/f[g]\"", "z", "a;b,c:d;e/f[g]"}); // Quoted with other Cookie keywords @@ -121,6 +125,7 @@ public class CookieCutter_LenientTest ret.add(new String[]{"x=\"$Path=/\"", "x", "$Path=/"}); ret.add(new String[]{"x=\"$Path=/ $Domain=.foo.com\"", "x", "$Path=/ $Domain=.foo.com"}); ret.add(new String[]{"x=\" $Path=/ $Domain=.foo.com \"", "x", " $Path=/ $Domain=.foo.com "}); + ret.add(new String[]{"a=\"b; $Path=/a; c=d; $PATH=/c; e=f\"; $Path=/e/", "a", "b; $Path=/a; c=d; $PATH=/c; e=f"}); // Lots of equals signs ret.add(new String[]{"query=b=c&d=e", "query", "b=c&d=e"}); @@ -161,4 +166,5 @@ public class CookieCutter_LenientTest assertThat("Cookie.value", cookies[0].getValue(), is(expectedValue)); } } + }