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 all 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
}
102 changes: 67 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,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
}
}
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
90 changes: 85 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,91 @@ 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"),
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<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