From 1be1401bafb4c46fae3c234c8e93743a661dcf21 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 16 Feb 2023 08:24:19 +1100 Subject: [PATCH] Fix/jetty 9.4.x cookie cutter legacy (#9352) * improve rfc6265 handling * allow whitespaces in values * align test suite * Added RFC6265_LEGACY mode * Use RFC6265_LEGACY mode for test --------- Signed-off-by: Ludovic Orban Co-authored-by: Ludovic Orban --- .../eclipse/jetty/http/CookieCompliance.java | 1 + .../eclipse/jetty/server/CookieCutter.java | 102 ++++++++++++------ .../jetty/server/CookieCutterLenientTest.java | 3 +- .../jetty/server/CookieCutterTest.java | 90 +++++++++++++++- .../org/eclipse/jetty/server/RequestTest.java | 2 + 5 files changed, 157 insertions(+), 41 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCompliance.java index f8696d48f582..40853a6f0104 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCompliance.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCompliance.java @@ -24,5 +24,6 @@ public enum CookieCompliance { RFC6265, + RFC6265_LEGACY, // Forgiving of bad quotes. RFC2965 } 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 3753a8253064..60a9f5208db1 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 @@ -154,6 +154,13 @@ protected void parseFields() // Handle quoted values for name or value if (inQuoted) { + boolean eol = c == 0 && i == hdr.length(); + if (!eol && _compliance != CookieCompliance.RFC2965 && isRFC6265RejectedCharacter(inQuoted, c)) + { + reject = true; + continue; + } + if (escaped) { escaped = false; @@ -182,15 +189,24 @@ protected void parseFields() continue; case 0: - // unterminated quote, let's ignore quotes + // unterminated quote + if (_compliance == CookieCompliance.RFC6265) + continue; + // let's ignore quotes unquoted.setLength(0); inQuoted = false; i--; continue; + case ';': + if (_compliance == CookieCompliance.RFC6265) + reject = true; + else + unquoted.append(c); + continue; + default: unquoted.append(c); - continue; } } else @@ -198,6 +214,13 @@ protected void parseFields() // Handle name and value state machines if (invalue) { + boolean eol = c == 0 && i == hdr.length(); + if (!eol && _compliance == CookieCompliance.RFC6265 && isRFC6265RejectedCharacter(inQuoted, c)) + { + reject = true; + continue; + } + // parse the cookie-value switch (c) { @@ -300,9 +323,20 @@ else if (tokenstart >= 0) unquoted = new StringBuilder(); break; } + else if (_compliance == CookieCompliance.RFC6265) + { + reject = true; + continue; + } // fall through to default case default: + if (_compliance == CookieCompliance.RFC6265 && quoted) + { + reject = true; + continue; + } + if (quoted) { // must have been a bad internal quote. let's fix as best we can @@ -312,18 +346,12 @@ else if (tokenstart >= 0) continue; } - if (_compliance == CookieCompliance.RFC6265) - { - if (isRFC6265RejectedCharacter(inQuoted, c)) - { - reject = true; - } - } + if (_compliance == CookieCompliance.RFC6265_LEGACY && isRFC6265RejectedCharacter(inQuoted, c)) + reject = true; if (tokenstart < 0) tokenstart = i; tokenend = i; - continue; } } else @@ -366,13 +394,8 @@ else if (tokenstart >= 0) continue; } - if (_compliance == CookieCompliance.RFC6265) - { - if (isRFC6265RejectedCharacter(inQuoted, c)) - { - reject = true; - } - } + if (_compliance != CookieCompliance.RFC2965 && isRFC6265RejectedCharacter(inQuoted, c)) + reject = true; if (tokenstart < 0) tokenstart = i; @@ -390,28 +413,37 @@ else if (tokenstart >= 0) protected boolean isRFC6265RejectedCharacter(boolean inQuoted, char c) { - if (inQuoted) + // LEGACY test + if (_compliance == CookieCompliance.RFC6265_LEGACY) { - // We only reject if a Control Character is encountered - if (Character.isISOControl(c)) + if (inQuoted) { - return true; + // 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 + else + { + return Character.isISOControl(c) || // control characters + c > 127 || // 8-bit characters + c == ',' || // comma + c == ';'; // semicolon + } + return false; } - return false; + /* 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 + * + * Note: DQUOTE and semicolon are used as separator by the parser, + * so we can consider them authorized. + */ + return c > 127 || // 8-bit characters + Character.isISOControl(c) || // control characters + c == ',' || // comma + c == '\\'; // backslash } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterLenientTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterLenientTest.java index 3eceac6e3974..1d06c82a5ada 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterLenientTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterLenientTest.java @@ -21,6 +21,7 @@ import java.util.stream.Stream; import javax.servlet.http.Cookie; +import org.eclipse.jetty.http.CookieCompliance; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -162,7 +163,7 @@ public static Stream data() @MethodSource("data") public void testLenientBehavior(String rawHeader, String expectedName, String expectedValue) { - CookieCutter cutter = new CookieCutter(); + CookieCutter cutter = new CookieCutter(CookieCompliance.RFC6265_LEGACY); cutter.addCookieField(rawHeader); Cookie[] cookies = cutter.getCookies(); if (expectedName == null) 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 494ff03282e9..3e84ce6ce467 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 @@ -19,10 +19,13 @@ package org.eclipse.jetty.server; import java.util.Arrays; +import java.util.List; import javax.servlet.http.Cookie; import org.eclipse.jetty.http.CookieCompliance; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; @@ -162,15 +165,13 @@ public void testRFC2965CookieSpoofingExample() "$Version=\"1\"; session_id=\"1111\"; $Domain=\".cracker.edu\""; Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie); - assertThat("Cookies.length", cookies.length, is(2)); assertCookie("Cookies[0]", cookies[0], "session_id", "1234", 1, null); assertCookie("Cookies[1]", cookies[1], "session_id", "1111", 1, null); cookies = parseCookieHeaders(CookieCompliance.RFC6265, rawCookie); - assertThat("Cookies.length", cookies.length, is(2)); - assertCookie("Cookies[0]", cookies[0], "session_id", "1234\", $Version=\"1", 0, null); - assertCookie("Cookies[1]", cookies[1], "session_id", "1111", 0, null); + assertThat("Cookies.length", cookies.length, is(1)); + assertCookie("Cookies[0]", cookies[0], "session_id", "1111", 0, null); } /** @@ -266,7 +267,7 @@ public void testExcessiveSemicolons() { char[] excessive = new char[65535]; Arrays.fill(excessive, ';'); - String rawCookie = "foo=bar; " + excessive + "; xyz=pdq"; + String rawCookie = "foo=bar; " + new String(excessive) + "; xyz=pdq"; Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC6265, rawCookie); @@ -274,4 +275,83 @@ public void testExcessiveSemicolons() assertCookie("Cookies[0]", cookies[0], "foo", "bar", 0, null); assertCookie("Cookies[1]", cookies[1], "xyz", "pdq", 0, null); } + + @ParameterizedTest + @MethodSource("rfc6265Cookies") + public void testRFC6265CookieParsing(Param param) + { + Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC6265, param.input); + + assertThat("Cookies.length (" + dump(cookies) + ")", cookies.length, is(param.expected.size())); + for (int i = 0; i < cookies.length; i++) + { + Cookie cookie = cookies[i]; + assertThat("Cookies[" + i + "] (" + dump(cookies) + ")", cookie.getName() + "=" + cookie.getValue(), is(param.expected.get(i))); + } + } + + public static List rfc6265Cookies() + { + return Arrays.asList( + new Param("A=1; B=2; C=3", "A=1", "B=2", "C=3"), + new Param("A=\"1\"; B=2; C=3", "A=1", "B=2", "C=3"), + new Param("A=\"1\"; B=\"2\"; C=\"3\"", "A=1", "B=2", "C=3"), + new Param("A=1; B=2; C=\"3", "A=1", "B=2"), + new Param("A=1 ; B=2; C=3", "A=1", "B=2", "C=3"), + new Param("A= 1; B=2; C=3", "A=1", "B=2", "C=3"), + new Param("A=\"1; B=2\"; C=3", "C=3"), + new Param("A=\"1; B=2; C=3"), + new Param("A=\"1 B=2\"; C=3", "A=1 B=2", "C=3"), + new Param("A=\"\"1; B=2; C=3", "B=2", "C=3"), + new Param("A=\"\" ; B=2; C=3", "A=", "B=2", "C=3"), + new Param("A=\"\"; B=2; C=3", "A=", "B=2", "C=3"), + new Param("A=1\"\"; B=2; C=3", "B=2", "C=3"), + new Param("A=1\"; B=2; C=3", "B=2", "C=3"), + new Param("A=1\"1; B=2; C=3", "B=2", "C=3"), + new Param("A=\" 1\"; B=2; C=3", "A= 1", "B=2", "C=3"), + new Param("A=\"1 \"; B=2; C=3", "A=1 ", "B=2", "C=3"), + new Param("A=\" 1 \"; B=2; C=3", "A= 1 ", "B=2", "C=3"), + new Param("A=\" 1 1 \"; B=2; C=3", "A= 1 1 ", "B=2", "C=3"), + new Param("A=1,; B=2; C=3", "B=2", "C=3"), + new Param("A=\"1,\"; B=2; C=3", "B=2", "C=3"), + new Param("A=\\1; B=2; C=3", "B=2", "C=3"), + new Param("A=\"\\1\"; B=2; C=3", "B=2", "C=3"), + new Param("A=1\u0007; B=2; C=3", "B=2", "C=3"), + new Param("A=\"1\u0007\"; B=2; C=3", "B=2", "C=3"), + new Param("€"), + new Param("@={}"), + new Param("$X=Y; N=V", "N=V"), + new Param("N=V; $X=Y", "N=V") + ); + } + + private static String dump(Cookie[] cookies) + { + StringBuilder sb = new StringBuilder(); + for (Cookie cookie : cookies) + { + sb.append("<").append(cookie.getName()).append(">=<").append(cookie.getValue()).append("> | "); + } + if (sb.length() > 0) + sb.delete(sb.length() - 2, sb.length() - 1); + return sb.toString(); + } + + private static class Param + { + private final String input; + private final List expected; + + public Param(String input, String... expected) + { + this.input = input; + this.expected = Arrays.asList(expected); + } + + @Override + public String toString() + { + return input + " -> " + expected.toString(); + } + } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index f77f39b63018..a4e9125964fc 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -53,6 +53,7 @@ import javax.servlet.http.Part; import org.eclipse.jetty.http.BadMessageException; +import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpComplianceSection; import org.eclipse.jetty.http.HttpStatus; @@ -140,6 +141,7 @@ protected String formatAddrOrHost(String addr) http.getHttpConfiguration().setRequestHeaderSize(512); http.getHttpConfiguration().setResponseHeaderSize(512); http.getHttpConfiguration().setOutputBufferSize(2048); + http.getHttpConfiguration().setRequestCookieCompliance(CookieCompliance.RFC6265_LEGACY); http.getHttpConfiguration().addCustomizer(new ForwardedRequestCustomizer()); _connector = new LocalConnector(_server, http); _server.addConnector(_connector);