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

Fix CVE-2024-6763 #12532

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Fix CVE-2024-6763 #12532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ static EnumSet<HttpComplianceSection> sectionsBySpec(String spec)
HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS,
HttpComplianceSection.NO_AMBIGUOUS_PATH_SEPARATORS,
HttpComplianceSection.NO_UTF16_ENCODINGS,
HttpComplianceSection.NO_USER_INFO,
HttpComplianceSection.NO_AMBIGUOUS_EMPTY_SEGMENT,
HttpComplianceSection.NO_AMBIGUOUS_PATH_ENCODING));
break;
Expand Down Expand Up @@ -217,6 +218,7 @@ public EnumSet<HttpComplianceSection> sections()
}

private static final EnumMap<HttpURI.Violation, HttpComplianceSection> __uriViolations = new EnumMap<>(HttpURI.Violation.class);

static
{
// create a map from Violation to compliance in a loop, so that any new violations added are detected with ISE
Expand All @@ -242,6 +244,9 @@ public EnumSet<HttpComplianceSection> sections()
case UTF16:
__uriViolations.put(violation, HttpComplianceSection.NO_UTF16_ENCODINGS);
break;
case USER_INFO:
__uriViolations.put(violation, HttpComplianceSection.NO_USER_INFO);
break;
default:
throw new IllegalStateException();
}
Expand Down
1 change: 1 addition & 0 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public enum HttpComplianceSection
NO_AMBIGUOUS_PATH_SEPARATORS("https://tools.ietf.org/html/rfc3986#section-3.3", "No ambiguous URI path separators"),
NO_AMBIGUOUS_PATH_PARAMETERS("https://tools.ietf.org/html/rfc3986#section-3.3", "No ambiguous URI path parameters"),
NO_UTF16_ENCODINGS("https://www.w3.org/International/iri-edit/draft-duerst-iri.html#anchor29", "UTF16 encoding"),
NO_USER_INFO("https://datatracker.ietf.org/doc/html/rfc9110#name-deprecation-of-userinfo-in-", "User info in authority"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird place for this, but oh well, it is Jetty 9.x ...

NO_AMBIGUOUS_EMPTY_SEGMENT("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI empty segment"),
NO_AMBIGUOUS_PATH_ENCODING("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path encoding");

Expand Down
127 changes: 120 additions & 7 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,11 @@ enum Violation
/**
* Non standard UTF-16 encoding eg {@code /foo%u2192bar}.
*/
UTF16("Non standard UTF-16 encoding");
UTF16("Non standard UTF-16 encoding"),
/**
* User info in authority
*/
USER_INFO("User info in Authority");

private final String _message;

Expand Down Expand Up @@ -155,6 +159,84 @@ String getMessage()
__ambiguousSegments.put("%u002e%u002e", Boolean.TRUE);
}

private static final boolean[] __unreservedPctEncodedSubDelims;

private static boolean isDigit(char c)
{
return (c >= '0') && (c <= '9');
}

private static boolean isHexDigit(char c)
{
return (((c >= 'a') && (c <= 'f')) || // ALPHA (lower)
((c >= 'A') && (c <= 'F')) || // ALPHA (upper)
((c >= '0') && (c <= '9')));
}

private static boolean isUnreserved(char c)
{
return (((c >= 'a') && (c <= 'z')) || // ALPHA (lower)
((c >= 'A') && (c <= 'Z')) || // ALPHA (upper)
((c >= '0') && (c <= '9')) || // DIGIT
(c == '-') || (c == '.') || (c == '_') || (c == '~'));
}

private static boolean isSubDelim(char c)
{
return c == '!' || c == '$' || c == '&' || c == '\'' || c == '(' || c == ')' || c == '*' || c == '+' || c == ',' || c == ';' || c == '=';
}

static boolean isUnreservedPctEncodedOrSubDelim(char c)
{
return c < __unreservedPctEncodedSubDelims.length && __unreservedPctEncodedSubDelims[c];
}

static
{
// Establish allowed and disallowed characters per the path rules of
// https://datatracker.ietf.org/doc/html/rfc3986#section-3.3
// ABNF
// path = path-abempty ; begins with "/" or is empty
// / path-absolute ; begins with "/" but not "//"
// / path-noscheme ; begins with a non-colon segment
// / path-rootless ; begins with a segment
// / path-empty ; zero characters
// path-abempty = *( "/" segment )
// path-absolute = "/" [ segment-nz *( "/" segment ) ]
// path-noscheme = segment-nz-nc *( "/" segment )
// path-rootless = segment-nz *( "/" segment )
// path-empty = 0<pchar>
//
// segment = *pchar
// segment-nz = 1*pchar
// segment-nz-nc = 1*( unreserved / pct-encoded / sub-delims / "@" )
// ; non-zero-length segment without any colon ":"
// pchar = unreserved / pct-encoded / sub-delims / ":" / "@"
// pct-encoded = "%" HEXDIG HEXDIG
//
// unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
// reserved = gen-delims / sub-delims
// gen-delims = ":" / "/" / "?" / "#" / "[" / "]" / "@"
// sub-delims = "!" / "$" / "&" / "'" / "(" / ")"
// / "*" / "+" / "," / ";" / "="
//
// authority = [ userinfo "@" ] host [ ":" port ]
// userinfo = *( unreserved / pct-encoded / sub-delims / ":" )
// host = IP-literal / IPv4address / reg-name
// port = *DIGIT
//
// reg-name = *( unreserved / pct-encoded / sub-delims )
//
// we are limited to US-ASCII per https://datatracker.ietf.org/doc/html/rfc3986#section-2
__unreservedPctEncodedSubDelims = new boolean[128];

for (int i = 0; i < __unreservedPctEncodedSubDelims.length; i++)
{
char c = (char)i;
__unreservedPctEncodedSubDelims[i] = isUnreserved(c) || c == '%' || isSubDelim(c);
}
}

private String _scheme;
private String _user;
private String _host;
Expand Down Expand Up @@ -334,7 +416,7 @@ private void parse(State state, final String uri, final int offset, final int en
int mark = offset; // the start of the current section being parsed
int pathMark = 0; // the start of the path section
int segment = 0; // the start of the current segment within the path
boolean encodedPath = false; // set to true if the path contains % encoded characters
boolean encoded = false; // set to true if the path contains % encoded characters
boolean encodedUtf16 = false; // Is the current encoding for UTF16?
int encodedCharacters = 0; // partial state of parsing a % encoded character<x>
int encodedValue = 0; // the partial encoded value
Expand Down Expand Up @@ -378,7 +460,7 @@ private void parse(State state, final String uri, final int offset, final int en
state = State.ASTERISK;
break;
case '%':
encodedPath = true;
encoded = true;
encodedCharacters = 2;
encodedValue = 0;
mark = pathMark = segment = i;
Expand Down Expand Up @@ -431,7 +513,7 @@ private void parse(State state, final String uri, final int offset, final int en
break;
case '%':
// must have been in an encoded path
encodedPath = true;
encoded = true;
encodedCharacters = 2;
encodedValue = 0;
state = State.PATH;
Expand Down Expand Up @@ -481,7 +563,10 @@ private void parse(State state, final String uri, final int offset, final int en
switch (c)
{
case '/':
if (encodedCharacters > 0)
throw new IllegalArgumentException("Bad authority");
_host = uri.substring(mark, i);
encoded = false;
pathMark = mark = i;
segment = mark + 1;
state = State.PATH;
Expand All @@ -496,12 +581,35 @@ private void parse(State state, final String uri, final int offset, final int en
if (_user != null)
throw new IllegalArgumentException("Bad authority");
_user = uri.substring(mark, i);
_violations.add(Violation.USER_INFO);
mark = i + 1;
break;
case '[':
if (i != mark)
throw new IllegalArgumentException("Bad authority");
state = State.IPV6;
break;
case '%':
if (encodedCharacters > 0)
throw new IllegalArgumentException("Bad authority");
encodedCharacters = 2;
encoded = true;
break;
case '#':
case ';':
throw new IllegalArgumentException("Bad authority");

default:
if (encodedCharacters > 0)
{
if (!isHexDigit(c))
throw new IllegalArgumentException("Bad authority");
encodedCharacters--;
}
else if (!isUnreservedPctEncodedOrSubDelim(c))
{
throw new IllegalArgumentException("Bad authority");
}
break;
}
break;
Expand All @@ -526,7 +634,11 @@ private void parse(State state, final String uri, final int offset, final int en
state = State.PATH;
}
break;
case ':':
break;
default:
if (!isHexDigit(c))
throw new IllegalArgumentException("Bad authority");
break;
}
break;
Expand All @@ -539,6 +651,7 @@ private void parse(State state, final String uri, final int offset, final int en
throw new IllegalArgumentException("Bad authority");
// It wasn't a port, but a password!
_user = _host + ":" + uri.substring(mark, i);
_violations.add(Violation.USER_INFO);
mark = i + 1;
state = State.HOST;
}
Expand Down Expand Up @@ -614,7 +727,7 @@ else if (c == '/')
dot |= segment == i;
break;
case '%':
encodedPath = true;
encoded = true;
encodedUtf16 = false;
encodedCharacters = 2;
encodedValue = 0;
Expand Down Expand Up @@ -642,7 +755,7 @@ else if (c == '/')
state = State.FRAGMENT;
break;
case '/':
encodedPath = true;
encoded = true;
segment = i + 1;
state = State.PATH;
break;
Expand Down Expand Up @@ -721,7 +834,7 @@ else if (c == '/')
throw new IllegalStateException(state.toString());
}

if (!encodedPath && !dot)
if (!encoded && !dot)
{
if (_param == null)
_decodedPath = _path;
Expand Down
37 changes: 32 additions & 5 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@ public void testParse()
uri.parse("http://foo/bar");
assertThat(uri.getHost(), is("foo"));
assertThat(uri.getPath(), is("/bar"));

// We do allow nulls if not encoded. This can be used for testing 2nd line of defence.
uri.parse("http://fo\000/bar");
assertThat(uri.getHost(), is("fo\000"));
assertThat(uri.getPath(), is("/bar"));
}

@Test
Expand Down Expand Up @@ -875,4 +870,36 @@ public void testRelativePathWithAuthority()
uri.setPath("");
assertEquals("//host", uri.toString());
}

public static Stream<String> badAuthorities()
{
return Stream.of(
"http://#host/path",
"https:// host/path",
"https://h st/path",
"https://h\000st/path",
"https://h%GGst/path",
"https://host%/path",
"https://host%0/path",
"https://host%u001f/path",
"https://host%:8080/path",
"https://host%0:8080/path",
"https://user%@host/path",
"https://user%0@host/path",
"https://host:notport/path",
"https://user@host:notport/path",
"https://user:password@host:notport/path",
"https://user @host.com/",
"https://user#@host.com/",
"https://[notIpv6]/",
"https://bad[0::1::2::3::4]/"
);
}

@ParameterizedTest
@MethodSource("badAuthorities")
public void testBadAuthority(String uri)
{
assertThrows(IllegalArgumentException.class, () -> new HttpURI(uri));
}
}