Skip to content

Commit

Permalink
Fix/jetty 9.4.x cookie cutter legacy (#9352)
Browse files Browse the repository at this point in the history 
* 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 <lorban@bitronix.be>
Co-authored-by: Ludovic Orban <lorban@bitronix.be>
  • Loading branch information
@gregw @lorban
gregw and lorban authored Feb 15, 2023
1 parent c302159 commit 1be1401
Show file tree
Hide file tree
Showing 5 changed files with 157 additions and 41 deletions.
1 change: 1 addition & 0 deletions jetty-http/src/main/java/org/eclipse/jetty/http/CookieCompliance.java
View file
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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

0 comments on commit 1be1401

Please sign in to comment.