Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/jetty 9.4.x cookie cutter legacy #9352

Merged
merged 6 commits into from Feb 15, 2023
Merged

Fix/jetty 9.4.x cookie cutter legacy #9352

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6d331bf
improve rfc6265 handling
lorban Feb 10, 2023
06ba736
allow whitespaces in values
lorban Feb 10, 2023
93f6586
align test suite
lorban Feb 10, 2023
4dd5d66
Added RFC6265_LEGACY mode
gregw Feb 14, 2023
21bce64
USed RFC6265_LEGACY mode for test
gregw Feb 15, 2023
13ba312
Updates from review
gregw Feb 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions jetty-http/src/main/java/org/eclipse/jetty/http/CookieCompliance.java
View file
Open in desktop
Expand Up @@ -24,5 +24,6 @@
public enum CookieCompliance
{
RFC6265,
RFC6265_LEGACY, // Forgiving of bad quotes.
RFC2965
}
108 changes: 73 additions & 35 deletions jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -182,22 +189,38 @@ 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
{
// 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)
{
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -390,28 +413,43 @@ 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
{
/* 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
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove the comment because it's not what the code does (the code does not check for , " and \.
Alternatively, can this block be merged with the block below which is identical and has a clarifying comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove the comment.
It is not identical to the code below. This is the original code and the code below is updated by @lorban.

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
}
}
3 changes: 2 additions & 1 deletion jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterLenientTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -162,7 +163,7 @@ public static Stream<Arguments> 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)
Expand Down
92 changes: 87 additions & 5 deletions jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -266,12 +267,93 @@ 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);

assertThat("Cookies.length", cookies.length, is(2));
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<Param> 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"), // Why should A be rejected? Shouldn't it be A=<1 B=2>?
Copy link
Contributor

@lorban lorban Feb 15, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment either is outdated and should go away, or space characters should be rejected and there's a bug left.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to keep us much legacy behaviour as possible for these lenient cases, so I don't want to change the code. I'll remove the comment.

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"),
/* TODO Use Legacy mode for these
new Param("A=\" 1\"; B=2; C=3", "A=1", "B=2", "C=3"), // Why should the whitespaces be trimmed? They were not in the prev impl.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These assertions should be adapted. In these four cases, A should be ignored but B and C are still valid, aren't they?

new Param("A=\"1 \"; B=2; C=3", "A=1", "B=2", "C=3"), // ditto
new Param("A=\" 1 \"; B=2; C=3", "A=1", "B=2", "C=3"), // ditto
new Param("A=\" 1 1 \"; B=2; C=3", "A=1 1", "B=2", "C=3"), // ditto
*/
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<String> expected;

public Param(String input, String... expected)
{
this.input = input;
this.expected = Arrays.asList(expected);
}

@Override
public String toString()
{
return input + " -> " + expected.toString();
}
}
}
2 changes: 2 additions & 0 deletions jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down