From 4d146412c8feac05c25d171b15c4f6ab4d42719b Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 21 Feb 2023 21:31:43 +1100 Subject: [PATCH] Fix #9334 Cookie Compliance (#9402) Fix incorrect change to RFC6265 to not support dollars in cookie names. Signed-off-by: gregw --- .../eclipse/jetty/http/CookieCompliance.java | 75 ++++--- .../org/eclipse/jetty/http/CookieParser.java | 30 ++- .../org/eclipse/jetty/http/HttpCookie.java | 2 +- .../jetty/http/RFC6265CookieParser.java | 46 ++-- .../http/RFC6265CookieParserLenientTest.java | 137 +++++++++++ .../jetty/http/RFC6265CookieParserTest.java | 95 ++++++-- jetty-server/src/main/config/etc/jetty.xml | 4 +- .../src/main/config/modules/server.mod | 2 +- .../org/eclipse/jetty/server/Cookies.java | 11 +- .../jetty/server/ServerHttpCookieTest.java | 212 ++++++++++++++++++ 10 files changed, 524 insertions(+), 90 deletions(-) create mode 100644 jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserLenientTest.java create mode 100644 jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java 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 6397038d9efd..28557b8800eb 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 @@ -128,13 +128,12 @@ public String getDescription() *

A CookieCompliance mode that enforces RFC 6265 compliance, * but allows:

* */ public static final CookieCompliance RFC6265 = new CookieCompliance("RFC6265", of( - Violation.ATTRIBUTES, Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE) + Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE) ); /** @@ -146,6 +145,7 @@ public String getDescription() *

A CookieCompliance mode that enforces RFC 6265 compliance, * but allows:

* */ - public static final CookieCompliance RFC6265_LEGACY = new CookieCompliance("RFC6265_LEGACY", of( - Violation.BAD_QUOTES, Violation.ESCAPE_IN_QUOTES, Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE, Violation.SPECIAL_CHARS_IN_QUOTES) + public static final CookieCompliance RFC6265_LEGACY = new CookieCompliance("RFC6265_LEGACY", EnumSet.of( + Violation.ATTRIBUTES, Violation.BAD_QUOTES, Violation.ESCAPE_IN_QUOTES, Violation.INVALID_COOKIES, Violation.OPTIONAL_WHITE_SPACE, Violation.SPECIAL_CHARS_IN_QUOTES) ); /** @@ -214,40 +214,45 @@ public static CookieCompliance valueOf(String name) */ public static CookieCompliance from(String spec) { - Set violations; - String[] elements = spec.split("\\s*,\\s*"); - switch (elements[0]) + CookieCompliance compliance = valueOf(spec); + if (compliance == null) { - case "0": - violations = noneOf(Violation.class); - break; - - case "*": - violations = allOf(Violation.class); - break; + String[] elements = spec.split("\\s*,\\s*"); + Set violations; + switch (elements[0]) + { + case "0" : + violations = noneOf(Violation.class); + break; + + case "*" : + violations = allOf(Violation.class); + break; + + default : + { + CookieCompliance mode = valueOf(elements[0]); + if (mode == null) + throw new IllegalArgumentException("Unknown base mode: " + elements[0]); + violations = (mode.getAllowed().isEmpty()) ? noneOf(Violation.class) : copyOf(mode.getAllowed()); + } + } - default: + for (int i = 1; i < elements.length; i++) { - CookieCompliance mode = valueOf(elements[0]); - violations = (mode == null) ? noneOf(Violation.class) : copyOf(mode.getAllowed()); - break; + String element = elements[i]; + boolean exclude = element.startsWith("-"); + if (exclude) + element = element.substring(1); + Violation section = Violation.valueOf(element); + if (exclude) + violations.remove(section); + else + violations.add(section); } - } - for (int i = 1; i < elements.length; i++) - { - String element = elements[i]; - boolean exclude = element.startsWith("-"); - if (exclude) - element = element.substring(1); - Violation section = Violation.valueOf(element); - if (exclude) - violations.remove(section); - else - violations.add(section); + compliance = new CookieCompliance("CUSTOM" + __custom.getAndIncrement(), violations); } - - CookieCompliance compliance = new CookieCompliance("CUSTOM" + __custom.getAndIncrement(), violations); if (LOG.isDebugEnabled()) LOG.debug("CookieCompliance from {}->{}", spec, compliance); return compliance; @@ -290,4 +295,10 @@ public boolean compliesWith(CookieCompliance mode) { return this == mode || getAllowed().containsAll(mode.getAllowed()); } + + @Override + public String toString() + { + return String.format("%s@%x%s", _name, hashCode(), _violations); + } } diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/CookieParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/CookieParser.java index 3b90c464e70a..8c6c16d5c781 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/CookieParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/CookieParser.java @@ -41,9 +41,9 @@ static CookieParser newParser(Handler handler, CookieCompliance compliance, List return new RFC6265CookieParser(handler, compliance, complianceListener); } - void parseField(String field); + void parseField(String field) throws InvalidCookieException; - default void parseFields(List rawFields) + default void parseFields(List rawFields) throws InvalidCookieException { // For each cookie field for (String field : rawFields) @@ -57,4 +57,30 @@ interface Handler { void addCookie(String name, String value, int version, String domain, String path, String comment); } + + /** + *

The exception thrown when a cookie cannot be parsed and {@link CookieCompliance.Violation#INVALID_COOKIES} is not allowed.

+ */ + class InvalidCookieException extends IllegalArgumentException + { + public InvalidCookieException() + { + super(); + } + + public InvalidCookieException(String s) + { + super(s); + } + + public InvalidCookieException(String message, Throwable cause) + { + super(message, cause); + } + + public InvalidCookieException(Throwable cause) + { + super(cause); + } + } } diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java index db75abfe4130..757daee181ff 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java @@ -281,7 +281,7 @@ private static boolean isQuoteNeededForCookie(String s) public String getSetCookie(CookieCompliance compliance) { - if (CookieCompliance.RFC6265.compliesWith(compliance)) + if (compliance == null || CookieCompliance.RFC6265_LEGACY.compliesWith(compliance)) return getRFC6265SetCookie(); return getRFC2965SetCookie(); } diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265CookieParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265CookieParser.java index b195fc58733d..55ca55b6f30f 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265CookieParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265CookieParser.java @@ -85,7 +85,7 @@ public void parseField(String field) if (token == null) { if (!_complianceMode.allows(INVALID_COOKIES)) - throw new IllegalArgumentException("Invalid Cookie character"); + throw new InvalidCookieException("Invalid Cookie character"); state = State.INVALID_COOKIE; continue; } @@ -100,7 +100,7 @@ public void parseField(String field) if (token.isRfc2616Token()) { - if (!StringUtil.isBlank(cookieName) && c != '$') + if (!StringUtil.isBlank(cookieName) && !(c == '$' && (_complianceMode.allows(ATTRIBUTES) || _complianceMode.allows(ATTRIBUTE_VALUES)))) { _handler.addCookie(cookieName, cookieValue, cookieVersion, cookieDomain, cookiePath, cookieComment); cookieName = null; @@ -120,7 +120,7 @@ else if (_complianceMode.allows(INVALID_COOKIES)) } else { - throw new IllegalArgumentException("Bad Cookie name"); + throw new InvalidCookieException("Bad Cookie name"); } break; @@ -158,7 +158,7 @@ else if (_complianceMode.allows(INVALID_COOKIES)) } else { - throw new IllegalArgumentException("Bad Cookie name"); + throw new InvalidCookieException("Bad Cookie name"); } break; @@ -181,7 +181,7 @@ else if (_complianceMode.allows(INVALID_COOKIES)) } else { - throw new IllegalArgumentException("Bad Cookie"); + throw new InvalidCookieException("Bad Cookie"); } break; @@ -215,7 +215,7 @@ else if (_complianceMode.allows(INVALID_COOKIES)) } else { - throw new IllegalArgumentException("Bad Cookie value"); + throw new InvalidCookieException("Bad Cookie value"); } break; @@ -237,7 +237,7 @@ else if (_complianceMode.allows(INVALID_COOKIES)) } else { - throw new IllegalArgumentException("Bad Cookie value"); + throw new InvalidCookieException("Bad Cookie value"); } break; @@ -277,7 +277,7 @@ else if (_complianceMode.allows(INVALID_COOKIES)) } else { - throw new IllegalArgumentException("Bad Cookie quoted value"); + throw new InvalidCookieException("Bad Cookie quoted value"); } break; @@ -299,7 +299,7 @@ else if (_complianceMode.allows(INVALID_COOKIES)) } else { - throw new IllegalArgumentException("Bad Cookie quoted value"); + throw new InvalidCookieException("Bad Cookie quoted value"); } break; @@ -323,7 +323,7 @@ else if (_complianceMode.allows(INVALID_COOKIES)) } else { - throw new IllegalStateException("Comma cookie separator"); + throw new InvalidCookieException("Comma cookie separator"); } } else if ((c == ' ' || c == '\t') && _complianceMode.allows(OPTIONAL_WHITE_SPACE)) @@ -332,7 +332,6 @@ else if ((c == ' ' || c == '\t') && _complianceMode.allows(OPTIONAL_WHITE_SPACE) continue; } - boolean knownAttribute = true; if (StringUtil.isBlank(attributeName)) { cookieValue = value; @@ -358,7 +357,10 @@ else if ((c == ' ' || c == '\t') && _complianceMode.allows(OPTIONAL_WHITE_SPACE) cookieVersion = Integer.parseInt(value); break; default: - knownAttribute = false; + if (!_complianceMode.allows(INVALID_COOKIES)) + throw new IllegalArgumentException("Invalid Cookie attribute"); + reportComplianceViolation(INVALID_COOKIES, field); + state = State.INVALID_COOKIE; break; } } @@ -366,31 +368,17 @@ else if (_complianceMode.allows(ATTRIBUTES)) { reportComplianceViolation(ATTRIBUTES, field); } - else if (_complianceMode.allows(INVALID_COOKIES)) - { - reportComplianceViolation(INVALID_COOKIES, field); - state = State.INVALID_COOKIE; - continue; - } else { - throw new IllegalArgumentException("Invalid Cookie with attributes"); + cookieName = attributeName; + cookieValue = value; } attributeName = null; } value = null; - if (!knownAttribute) - { - if (!_complianceMode.allows(INVALID_COOKIES)) - throw new IllegalArgumentException("Invalid Cookie attribute"); - reportComplianceViolation(INVALID_COOKIES, field); - state = State.INVALID_COOKIE; - continue; - } - if (state == State.END) - throw new IllegalStateException("Invalid cookie"); + throw new InvalidCookieException("Invalid cookie"); break; case INVALID_COOKIE: diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserLenientTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserLenientTest.java new file mode 100644 index 000000000000..2ba3d3d10764 --- /dev/null +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserLenientTest.java @@ -0,0 +1,137 @@ +// +// ======================================================================== +// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.http; + +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Stream; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + +/** + * Tests of poor various name=value scenarios and expectations of results + * due to our efforts at being lenient with what we receive. + */ +public class RFC6265CookieParserLenientTest +{ + public static Stream data() + { + return Stream.of( + // Simple test to verify behavior + Arguments.of("x=y", "x", "y"), + Arguments.of("key=value", "key", "value"), + + // Tests that conform to RFC2109 + // RFC2109 - token values + // token = 1* + // CHAR = + // CTL = + // SP = + // HT = + // tspecials = "(" | ")" | "<" | ">" | "@" + // | "," | ";" | ":" | "\" | <"> + // | "/" | "[" | "]" | "?" | "=" + // | "{" | "}" | SP | HT + Arguments.of("abc=xyz", "abc", "xyz"), + Arguments.of("abc=!bar", "abc", "!bar"), + Arguments.of("abc=#hash", "abc", "#hash"), + Arguments.of("abc=#hash", "abc", "#hash"), + // RFC2109 - quoted-string values + // quoted-string = ( <"> *(qdtext) <"> ) + // qdtext = > + + // 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= x", "abc", "x"), + Arguments.of("abc = ", "abc", ""), + Arguments.of("abc = ;", "abc", ""), + Arguments.of("abc = ; ", "abc", ""), + Arguments.of("abc = x ", "abc", "x"), + Arguments.of("abc=\"\"", "abc", ""), + Arguments.of("abc= \"\" ", "abc", ""), + Arguments.of("abc= \"x\" ", "abc", "x"), + Arguments.of("abc= \"x\" ;", "abc", "x"), + Arguments.of("abc= \"x\" ; ", "abc", "x"), + + // The backslash character ("\") may be used as a single-character quoting + // mechanism only within quoted-string and comment constructs. + // quoted-pair = "\" CHAR + Arguments.of("abc=\"xyz\"", "abc", "xyz"), + Arguments.of("abc=\"!bar\"", "abc", "!bar"), + Arguments.of("abc=\"#hash\"", "abc", "#hash"), + // RFC2109 - other valid name types that conform to 'attr'/'token' syntax + Arguments.of("!f!o!o!=wat", "!f!o!o!", "wat"), + Arguments.of("__MyHost=Foo", "__MyHost", "Foo"), + Arguments.of("some-thing-else=to-parse", "some-thing-else", "to-parse"), + Arguments.of("$foo=bar", "$foo", "bar"), + + // Tests that conform to RFC6265 + Arguments.of("abc=foobar!", "abc", "foobar!"), + Arguments.of("abc=\"foobar!\"", "abc", "foobar!") + + ); + } + + @ParameterizedTest + @MethodSource("data") + public void testLenientBehavior(String rawHeader, String expectedName, String expectedValue) + { + TestCutter cutter = new TestCutter(); + cutter.parseField(rawHeader); + + if (expectedName == null) + assertThat("Cookies.length", cutter.names.size(), is(0)); + else + { + assertThat("Cookies.length", cutter.names.size(), is(1)); + assertThat("Cookie.name", cutter.names.get(0), is(expectedName)); + assertThat("Cookie.value", cutter.values.get(0), is(expectedValue)); + } + } + + static class TestCutter implements CookieParser.Handler + { + CookieParser parser; + List names = new ArrayList<>(); + List values = new ArrayList<>(); + + protected TestCutter() + { + parser = new RFC6265CookieParser(this, CookieCompliance.RFC6265, null); + } + + public void parseField(String field) + { + parser.parseField(field); + } + + @Override + public void addCookie(String cookieName, String cookieValue, int cookieVersion, String cookieDomain, String cookiePath, String cookieComment) + { + names.add(cookieName); + values.add(cookieValue); + } + } +} diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java index 2f4170a69e9c..5fbf65bd657e 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java @@ -23,7 +23,6 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; -import static org.junit.jupiter.api.Assertions.assertThrows; public class RFC6265CookieParserTest { @@ -36,7 +35,7 @@ public void testRFC2965Single() String rawCookie = "$Version=\"1\"; Customer=\"WILE_E_COYOTE\"; $Path=\"/acme\""; // Test with RFC 2965. - TestCookieParser parser = new TestCookieParser(CookieCompliance.RFC2965_LEGACY); + TestCookieParser parser = new TestCookieParser(CookieCompliance.RFC2965); List cookies = parser.parseFields(rawCookie); assertThat("Cookies.length", cookies.size(), is(1)); @@ -44,18 +43,45 @@ public void testRFC2965Single() // There are 2 attributes, so 2 violations. assertThat(parser.violations.size(), is(2)); - // Same test with RFC 6265. + // Same test with RFC6265. parser = new TestCookieParser(CookieCompliance.RFC6265); cookies = parser.parseFields(rawCookie); + assertThat("Cookies.length", cookies.size(), is(3)); + assertCookie("Cookies[0]", cookies.get(0), "$Version", "1", 0, null); + assertCookie("Cookies[1]", cookies.get(1), "Customer", "WILE_E_COYOTE", 0, null); + assertCookie("Cookies[2]", cookies.get(2), "$Path", "/acme", 0, null); + // There attributes are seen as just normal cookies, so no violations + assertThat(parser.violations.size(), is(0)); + + // Same again, but allow attributes which are ignored + parser = new TestCookieParser(CookieCompliance.from("RFC6265,ATTRIBUTES")); + cookies = parser.parseFields(rawCookie); assertThat("Cookies.length", cookies.size(), is(1)); assertCookie("Cookies[0]", cookies.get(0), "Customer", "WILE_E_COYOTE", 0, null); - // There are 2 attributes, so 2 violations. + + // There attributes are seen as just normal cookies, so no violations + assertThat(parser.violations.size(), is(2)); + + // Same again, but allow attributes which are not ignored + parser = new TestCookieParser(CookieCompliance.from("RFC6265,ATTRIBUTE_VALUES")); + cookies = parser.parseFields(rawCookie); + assertThat("Cookies.length", cookies.size(), is(1)); + assertCookie("Cookies[0]", cookies.get(0), "Customer", "WILE_E_COYOTE", 1, "/acme"); + + // There attributes are seen as just normal cookies, so no violations assertThat(parser.violations.size(), is(2)); // Same test with RFC 6265 strict should throw. - TestCookieParser strictParser = new TestCookieParser(CookieCompliance.RFC6265_STRICT); - assertThrows(IllegalArgumentException.class, () -> strictParser.parseFields(rawCookie)); + parser = new TestCookieParser(CookieCompliance.RFC6265_STRICT); + cookies = parser.parseFields(rawCookie); + assertThat("Cookies.length", cookies.size(), is(3)); + assertCookie("Cookies[0]", cookies.get(0), "$Version", "1", 0, null); + assertCookie("Cookies[1]", cookies.get(1), "Customer", "WILE_E_COYOTE", 0, null); + assertCookie("Cookies[2]", cookies.get(2), "$Path", "/acme", 0, null); + + // There attributes are seen as just normal cookies, so no violations + assertThat(parser.violations.size(), is(0)); } /** @@ -68,11 +94,29 @@ public void testRFC2965Double() "Customer=\"WILE_E_COYOTE\"; $Path=\"/acme\"; " + "Part_Number=\"Rocket_Launcher_0001\"; $Path=\"/acme\""; - Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie); + Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie); assertThat("Cookies.length", cookies.length, is(2)); assertCookie("Cookies[0]", cookies[0], "Customer", "WILE_E_COYOTE", 1, "/acme"); assertCookie("Cookies[1]", cookies[1], "Part_Number", "Rocket_Launcher_0001", 1, "/acme"); + + cookies = parseCookieHeaders(CookieCompliance.RFC6265, rawCookie); + assertThat("Cookies.length", cookies.length, is(5)); + assertCookie("Cookies[0]", cookies[0], "$Version", "1", 0, null); + assertCookie("Cookies[1]", cookies[1], "Customer", "WILE_E_COYOTE", 0, null); + assertCookie("Cookies[2]", cookies[2], "$Path", "/acme", 0, null); + assertCookie("Cookies[3]", cookies[3], "Part_Number", "Rocket_Launcher_0001", 0, null); + assertCookie("Cookies[4]", cookies[4], "$Path", "/acme", 0, null); + + cookies = parseCookieHeaders(CookieCompliance.from("RFC6265,ATTRIBUTES"), rawCookie); + assertThat("Cookies.length", cookies.length, is(2)); + assertCookie("Cookies[0]", cookies[0], "Customer", "WILE_E_COYOTE", 0, null); + assertCookie("Cookies[1]", cookies[1], "Part_Number", "Rocket_Launcher_0001", 0, null); + + cookies = parseCookieHeaders(CookieCompliance.from("RFC6265,ATTRIBUTE_VALUES"), rawCookie); + assertThat("Cookies.length", cookies.length, is(2)); + assertCookie("Cookies[0]", cookies[0], "Customer", "WILE_E_COYOTE", 1, "/acme"); + assertCookie("Cookies[1]", cookies[1], "Part_Number", "Rocket_Launcher_0001", 1, "/acme"); } /** @@ -86,7 +130,7 @@ public void testRFCTriple() "Part_Number=\"Rocket_Launcher_0001\"; $Path=\"/acme\"; " + "Shipping=\"FedEx\"; $Path=\"/acme\""; - Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie); + Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie); assertThat("Cookies.length", cookies.length, is(3)); assertCookie("Cookies[0]", cookies[0], "Customer", "WILE_E_COYOTE", 1, "/acme"); @@ -104,7 +148,7 @@ public void testRFCPathExample() "Part_Number=\"Riding_Rocket_0023\"; $Path=\"/acme/ammo\"; " + "Part_Number=\"Rocket_Launcher_0001\"; $Path=\"/acme\""; - Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie); + Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie); assertThat("Cookies.length", cookies.length, is(2)); assertCookie("Cookies[0]", cookies[0], "Part_Number", "Riding_Rocket_0023", 1, "/acme/ammo"); @@ -121,7 +165,7 @@ public void testRFC2109CookieSpoofingExample() "session_id=\"1234\"; " + "session_id=\"1111\"; $Domain=\".cracker.edu\""; - Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie); + Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie); assertThat("Cookies.length", cookies.length, is(2)); assertCookie("Cookies[0]", cookies[0], "session_id", "1234", 1, null); @@ -137,15 +181,25 @@ public void testRFC2965CookieSpoofingExample() String rawCookie = "$Version=\"1\"; session_id=\"1234\", " + "$Version=\"1\"; session_id=\"1111\"; $Domain=\".cracker.edu\""; - Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie); + 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(1)); - assertCookie("Cookies[1]", cookies[0], "session_id", "1111", 0, null); + assertThat("Cookies.length", cookies.length, is(3)); + assertCookie("Cookies[0]", cookies[0], "$Version", "1", 0, null); + assertCookie("Cookies[1]", cookies[1], "session_id", "1111", 0, null); + assertCookie("Cookies[2]", cookies[2], "$Domain", ".cracker.edu", 0, null); + + cookies = parseCookieHeaders(CookieCompliance.from("RFC6265,COMMA_SEPARATOR"), rawCookie); + assertThat("Cookies.length", cookies.length, is(5)); + assertCookie("Cookies[0]", cookies[0], "$Version", "1", 0, null); + assertCookie("Cookies[1]", cookies[1], "session_id", "1234", 0, null); + assertCookie("Cookies[3]", cookies[2], "$Version", "1", 0, null); + assertCookie("Cookies[3]", cookies[3], "session_id", "1111", 0, null); + assertCookie("Cookies[4]", cookies[4], "$Domain", ".cracker.edu", 0, null); } /** @@ -201,7 +255,8 @@ public void testDollarName() Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC6265, rawCookie); - assertThat("Cookies.length", cookies.length, is(0)); + assertThat("Cookies.length", cookies.length, is(1)); + assertCookie("Cookies[0]", cookies[0], "$key", "value", 0, null); } @Test @@ -236,7 +291,7 @@ public void testExcessiveSemicolons() public void testRFC2965QuotedEscape() { String rawCookie = "A=\"double\\\"quote\""; - Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie); + Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie); assertThat("Cookies.length", cookies.length, is(1)); assertCookie("Cookies[0]", cookies[0], "A", "double\"quote", 0, null); @@ -246,16 +301,12 @@ public void testRFC2965QuotedEscape() public void testRFC2965QuotedSpecial() { String rawCookie = "A=\", ;\""; - Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965_LEGACY, rawCookie); + Cookie[] cookies = parseCookieHeaders(CookieCompliance.RFC2965, rawCookie); assertThat("Cookies.length", cookies.length, is(1)); assertCookie("Cookies[0]", cookies[0], "A", ", ;", 0, null); } - // TODO: - // $X; N=V - // $X=Y; N=V - public static List parameters() { return Arrays.asList( @@ -281,8 +332,8 @@ public static List parameters() 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") + new Param("$X=Y; N=V", "$X=Y", "N=V"), + new Param("N=V; $X=Y", "N=V", "$X=Y") ); } diff --git a/jetty-server/src/main/config/etc/jetty.xml b/jetty-server/src/main/config/etc/jetty.xml index c40f51a8920c..0405da0ab45d 100644 --- a/jetty-server/src/main/config/etc/jetty.xml +++ b/jetty-server/src/main/config/etc/jetty.xml @@ -73,8 +73,8 @@ - - + + diff --git a/jetty-server/src/main/config/modules/server.mod b/jetty-server/src/main/config/modules/server.mod index 5650b1248dc0..08b0b17151e6 100644 --- a/jetty-server/src/main/config/modules/server.mod +++ b/jetty-server/src/main/config/modules/server.mod @@ -75,7 +75,7 @@ etc/jetty.xml ## URI Compliance: DEFAULT, LEGACY, RFC3986, RFC3986_UNAMBIGUOUS, UNSAFE # jetty.httpConfig.uriCompliance=DEFAULT -## Cookie compliance mode for parsing request Cookie headers: RFC2965, RFC6265 +## Cookie compliance mode for parsing request Cookie headers: RFC6265_STRICT, RFC6265, RFC6265_LEGACY, RFC2965, RFC2965_LEGACY # jetty.httpConfig.requestCookieCompliance=RFC6265 ## Cookie compliance mode for generating response Set-Cookie: RFC2965, RFC6265 diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Cookies.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Cookies.java index a3feaa99ed16..498304299cd5 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Cookies.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Cookies.java @@ -17,9 +17,11 @@ import java.util.List; import javax.servlet.http.Cookie; +import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.CookieParser; +import org.eclipse.jetty.http.HttpStatus; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -95,7 +97,14 @@ public Cookie[] getCookies() if (_parsed) return _cookies; - _parser.parseFields(_rawFields); + try + { + _parser.parseFields(_rawFields); + } + catch (CookieParser.InvalidCookieException invalidCookieException) + { + throw new BadMessageException(HttpStatus.BAD_REQUEST_400, invalidCookieException.getMessage(), invalidCookieException); + } _cookies = (Cookie[])_cookieList.toArray(new Cookie[_cookieList.size()]); _cookieList.clear(); _parsed = true; diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java new file mode 100644 index 000000000000..c78c6498b9ea --- /dev/null +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java @@ -0,0 +1,212 @@ +// +// ======================================================================== +// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.server; + +import java.io.IOException; +import java.util.List; +import java.util.stream.Stream; +import javax.servlet.ServletException; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.http.CookieCompliance; +import org.eclipse.jetty.http.CookieParser; +import org.eclipse.jetty.http.HttpHeader; +import org.eclipse.jetty.http.HttpTester; +import org.eclipse.jetty.server.handler.AbstractHandler; +import org.eclipse.jetty.util.UrlEncoded; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.eclipse.jetty.http.CookieCompliance.RFC2965; +import static org.eclipse.jetty.http.CookieCompliance.RFC2965_LEGACY; +import static org.eclipse.jetty.http.CookieCompliance.RFC6265; +import static org.eclipse.jetty.http.CookieCompliance.RFC6265_LEGACY; +import static org.eclipse.jetty.http.CookieCompliance.RFC6265_STRICT; +import static org.eclipse.jetty.http.CookieCompliance.from; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; + +public class ServerHttpCookieTest +{ + private Server _server; + private LocalConnector _connector; + private HttpConfiguration _httpConfiguration; + + @BeforeEach + public void beforeEach() throws Exception + { + _server = new Server(); + _connector = new LocalConnector(_server); + _server.addConnector(_connector); + _server.setHandler(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + HttpConfiguration config = baseRequest.getHttpChannel().getHttpConfiguration(); + baseRequest.setHandled(true); + String setCookie = baseRequest.getParameter("SetCookie"); + if (setCookie != null) + { + CookieParser parser = CookieParser.newParser((name, value, version, domain, path, comment) -> + { + Cookie cookie = new Cookie(name, value); + if (version > 0) + cookie.setVersion(version); + if (domain != null) + cookie.setDomain(domain); + if (path != null) + cookie.setPath(path); + if (comment != null) + cookie.setComment(comment); + response.addCookie(cookie); + }, RFC2965, null); + parser.parseField(setCookie); + } + + Cookie[] cookies = request.getCookies(); + StringBuilder out = new StringBuilder(); + if (cookies != null) + { + for (Cookie cookie : cookies) + { + out + .append("[") + .append(cookie.getName()) + .append('=') + .append(cookie.getValue()); + + if (cookie.getVersion() > 0) + out.append(";Version=").append(cookie.getVersion()); + if (cookie.getPath() != null) + out.append(";Path=").append(cookie.getPath()); + if (cookie.getDomain() != null) + out.append(";Domain=").append(cookie.getDomain()); + out.append("]\n"); + } + } + response.getWriter().println(out); + } + }); + _httpConfiguration = _connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration(); + _server.start(); + } + + public static Stream requestCases() + { + return Stream.of( + Arguments.of(RFC6265_STRICT, "Cookie: name=value", 200, "Version=", List.of("[name=value]").toArray(new String[0])), + + + // Attribute tests + // TODO $name attributes are ignored because servlet 5.0 Cookie class rejects them. They are not ignored in servlet 6.0 + Arguments.of(RFC6265_STRICT, "Cookie: $version=1; name=value", 200, "Version=", List.of("[name=value]").toArray(new String[0])), + Arguments.of(RFC6265, "Cookie: $version=1; name=value", 200, "Version=", List.of("[name=value]").toArray(new String[0])), + Arguments.of(RFC6265, "Cookie: name=value;$path=/path", 200, "Path=", List.of("[name=value]").toArray(new String[0])), + Arguments.of(from("RFC6265,ATTRIBUTES"), "Cookie: name=value;$path=/path", 200, "/path", List.of("name=value").toArray(new String[0])), + Arguments.of(from("RFC6265_STRICT,ATTRIBUTE_VALUES"), "Cookie: name=value;$path=/path", 200, null, List.of("name=value;Path=/path").toArray(new String[0])), + Arguments.of(RFC2965, "Cookie: name=value;$path=/path", 200, null, List.of("name=value;Path=/path").toArray(new String[0])), + Arguments.of(RFC2965, "Cookie: $Version=1;name=value;$path=/path", 200, null, List.of("name=value;Version=1;Path=/path").toArray(new String[0])), + Arguments.of(RFC2965, "Cookie: $Version=1;name=value;$path=/path;$Domain=host", 200, null, List.of("name=value;Version=1;Path=/path;Domain=host").toArray(new String[0])), + + // multiple cookie tests + Arguments.of(RFC6265_STRICT, "Cookie: name=value; other=extra", 200, "Version=", List.of("[name=value]", "[other=extra]").toArray(new String[0])), + Arguments.of(RFC6265_STRICT, "Cookie: name=value, other=extra", 400, null, List.of("BadMessageException", "Comma cookie separator").toArray(new String[0])), + Arguments.of(from("RFC6265_STRICT,COMMA_SEPARATOR,"), "Cookie: name=value, other=extra", 200, "Version=", List.of("[name=value]", "[other=extra]").toArray(new String[0])), + Arguments.of(RFC6265, "Cookie: name=value, other=extra", 200, "name=value", null), + Arguments.of(RFC6265_LEGACY, "Cookie: name=value, other=extra", 200, null, List.of("[name=value, other=extra]").toArray(new String[0])), + Arguments.of(RFC6265_LEGACY, "Cookie: name=value; other=extra", 200, null, List.of("[name=value]", "other=extra]").toArray(new String[0])), + Arguments.of(RFC2965, "Cookie: name=value, other=extra", 200, "Version=", List.of("[name=value]", "[other=extra]").toArray(new String[0])), + Arguments.of(RFC2965_LEGACY, "Cookie: name=value, other=extra", 200, "Version=", List.of("[name=value]", "[other=extra]").toArray(new String[0])), + + // white space + Arguments.of(RFC6265_STRICT, "Cookie: name =value", 400, null, List.of("BadMessageException", "Bad Cookie name").toArray(new String[0])), + Arguments.of(from("RFC6265,OPTIONAL_WHITE_SPACE"), "Cookie: name =value", 200, null, List.of("name=value").toArray(new String[0])), + + // bad characters + Arguments.of(RFC6265_STRICT, "Cookie: name=va\\ue", 400, null, List.of("BadMessageException", "Bad Cookie value").toArray(new String[0])), + Arguments.of(RFC6265_STRICT, "Cookie: name=\"value\"", 200, "Version=", List.of("[name=value]").toArray(new String[0])), + Arguments.of(RFC6265_STRICT, "Cookie: name=\"value;other=extra\"", 400, null, List.of("BadMessageException", "Bad Cookie quoted value").toArray(new String[0])), + Arguments.of(RFC6265, "Cookie: name=\"value;other=extra\"", 200, "name=value", null), + Arguments.of(RFC6265_LEGACY, "Cookie: name=\"value;other=extra\"", 200, null, List.of("[name=value;other=extra]").toArray(new String[0])), + Arguments.of(RFC2965, "Cookie: name=\"value;other=extra\"", 200, null, List.of("[name=value;other=extra]").toArray(new String[0])), + Arguments.of(RFC2965, "Cookie: name=\"value;other=extra", 200, "name=value", null), + Arguments.of(RFC2965_LEGACY, "Cookie: name=\"value;other=extra\"", 200, null, List.of("[name=value;other=extra]").toArray(new String[0])), + Arguments.of(RFC2965_LEGACY, "Cookie: name=\"value;other=extra", 200, null, List.of("[name=\"value;other=extra]").toArray(new String[0])), + + // TCK check + Arguments.of(RFC6265, "Cookie: $Version=1; name1=value1; $Domain=hostname; $Path=/servlet_jsh_cookie_web", 200, null, List.of("name1=value1").toArray(new String[0])) + ); + } + + @ParameterizedTest + @MethodSource("requestCases") + public void testRequestCookies(CookieCompliance compliance, String cookie, int status, String unexpected, String... expectations) throws Exception + { + _httpConfiguration.setRequestCookieCompliance(compliance); + + HttpTester.Response response = HttpTester.parseResponse(_connector.getResponse("GET / HTTP/1.0\r\n" + cookie + "\r\n\r\n")); + + assertThat(response.getStatus(), equalTo(status)); + + if (unexpected != null) + assertThat(response.getContent(), not(containsString(unexpected))); + + if (expectations != null) + for (String expected : expectations) + assertThat(response.getContent(), containsString(expected)); + } + + public static Stream responseCases() + { + return Stream.of( + Arguments.of(RFC6265_STRICT, "name=value", "name=value"), + Arguments.of(RFC6265, "name=value", "name=value"), + Arguments.of(RFC6265_LEGACY, "name=value", "name=value"), + Arguments.of(RFC2965, "name=value", "name=value"), + Arguments.of(RFC2965_LEGACY, "name=value", "name=value"), + + Arguments.of(RFC6265_STRICT, "name=value;$version=1;$path=/path;$domain=domain", "name=value; Path=/path; Domain=domain"), + Arguments.of(RFC6265, "name=value;$version=1;$path=/path;$domain=domain", "name=value; Path=/path; Domain=domain"), + Arguments.of(RFC6265_LEGACY, "name=value;$version=1;$path=/path;$domain=domain", "name=value; Path=/path; Domain=domain"), + Arguments.of(RFC2965, "name=value;$version=1;$path=/path;$domain=domain", "name=value;Version=1;Path=/path;Domain=domain"), + Arguments.of(RFC2965_LEGACY, "name=value;$version=1;$path=/path;$domain=domain", "name=value;Version=1;Path=/path;Domain=domain"), + + Arguments.of(RFC6265, "name=value", "name=value") + ); + } + + @ParameterizedTest + @MethodSource("responseCases") + public void testResponseCookies(CookieCompliance compliance, String cookie, String expected) throws Exception + { + _httpConfiguration.setResponseCookieCompliance(compliance); + + HttpTester.Response response = HttpTester.parseResponse(_connector.getResponse("GET /?SetCookie=" + UrlEncoded.encodeString(cookie) + " HTTP/1.0\r\n\r\n")); + assertThat(response.getStatus(), equalTo(200)); + + String setCookie = response.get(HttpHeader.SET_COOKIE); + if (expected == null) + assertThat(setCookie, nullValue()); + else + assertThat(setCookie, equalTo(expected)); + } +}