Skip to content

Commit

Permalink
Fix #4275 fail URIs with ambiguous segments
Browse files Browse the repository at this point in the history 
Fix #4275 fail URIs with ambiguous segments.
  • Loading branch information
@gregw
gregw committed Feb 8, 2021
1 parent f768e2e commit 372b407
Show file tree
Hide file tree
Showing 7 changed files with 144 additions and 5 deletions.
2 changes: 1 addition & 1 deletion jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t
/**
* A Legacy compliance mode to match jetty's behavior prior to RFC2616 and RFC7230.
*/
LEGACY(sectionsBySpec("0,METHOD_CASE_SENSITIVE")),
LEGACY(sectionsBySpec("0,METHOD_CASE_SENSITIVE,AMBIGUOUS_PATH_SEGMENTS")),

/**
* The legacy RFC2616 support, which incorrectly excludes
Expand Down
3 changes: 2 additions & 1 deletion jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ public enum HttpComplianceSection
NO_FIELD_FOLDING("https://tools.ietf.org/html/rfc7230#section-3.2.4", "No line Folding"),
NO_HTTP_0_9("https://tools.ietf.org/html/rfc7230#appendix-A.2", "No HTTP/0.9"),
TRANSFER_ENCODING_WITH_CONTENT_LENGTH("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Transfer-Encoding and Content-Length"),
MULTIPLE_CONTENT_LENGTHS("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Multiple Content-Lengths");
MULTIPLE_CONTENT_LENGTHS("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Multiple Content-Lengths"),
AMBIGUOUS_PATH_SEGMENTS("", "Allows ambiguous URI path segments");

final String url;
final String description;
Expand Down
5 changes: 5 additions & 0 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,11 @@ public HttpHandler getHandler()
return _handler;
}

public HttpCompliance getHttpCompliance()
{
return _compliance;
}

/**
* Check RFC compliance violation
*
Expand Down
80 changes: 77 additions & 3 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;

import org.eclipse.jetty.util.ArrayTrie;
import org.eclipse.jetty.util.MultiMap;
import org.eclipse.jetty.util.Trie;
import org.eclipse.jetty.util.TypeUtil;
import org.eclipse.jetty.util.URIUtil;
import org.eclipse.jetty.util.UrlEncoded;
Expand Down Expand Up @@ -65,6 +67,18 @@ private enum State
ASTERISK
}

private static final Trie<Boolean> __ambiguousSegments = new ArrayTrie<>();

static
{
__ambiguousSegments.put("%2e", Boolean.TRUE);
__ambiguousSegments.put("%2e%2e", Boolean.TRUE);
__ambiguousSegments.put(".%2e", Boolean.TRUE);
__ambiguousSegments.put("%2e.", Boolean.TRUE);
__ambiguousSegments.put("..", Boolean.FALSE);
__ambiguousSegments.put(".", Boolean.FALSE);
}

private String _scheme;
private String _user;
private String _host;
Expand All @@ -77,6 +91,8 @@ private enum State
String _uri;
String _decodedPath;

boolean _ambiguousSegment;

/**
* Construct a normalized URI.
* Port is not set if it is the default port.
Expand Down Expand Up @@ -208,6 +224,8 @@ private void parse(State state, final String uri, final int offset, final int en
boolean encoded = false;
int mark = offset;
int pathMark = 0;
int segment = 0;
boolean encodedSegment = false;

for (int i = offset; i < end; i++)
{
Expand Down Expand Up @@ -241,14 +259,18 @@ private void parse(State state, final String uri, final int offset, final int en
_path = "*";
state = State.ASTERISK;
break;

case '%':
encoded = encodedSegment = true;
mark = pathMark = segment = i;
state = State.PATH;
break;
default:
mark = i;
if (_scheme == null)
state = State.SCHEME_OR_PATH;
else
{
pathMark = i;
pathMark = segment = i;
state = State.PATH;
}
}
Expand All @@ -269,6 +291,7 @@ private void parse(State state, final String uri, final int offset, final int en

case '/':
// must have been in a path and still are
segment = i + 1;
state = State.PATH;
break;

Expand All @@ -288,6 +311,7 @@ private void parse(State state, final String uri, final int offset, final int en
case '%':
// must have be in an encoded path
encoded = true;
encodedSegment = true;
state = State.PATH;
break;

Expand Down Expand Up @@ -317,11 +341,13 @@ private void parse(State state, final String uri, final int offset, final int en
// was a path, look again
i--;
pathMark = mark;
segment = mark + 1;
state = State.PATH;
break;
default:
// it is a path
pathMark = mark;
segment = mark + 1;
state = State.PATH;
}
continue;
Expand All @@ -334,6 +360,7 @@ private void parse(State state, final String uri, final int offset, final int en
case '/':
_host = uri.substring(mark, i);
pathMark = mark = i;
segment = mark + 1;
state = State.PATH;
break;
case ':':
Expand Down Expand Up @@ -396,6 +423,7 @@ else if (c == '/')
{
_port = TypeUtil.parseInt(uri, mark, i - mark, 10);
pathMark = mark = i;
segment = i + 1;
state = State.PATH;
}
continue;
Expand All @@ -406,21 +434,30 @@ else if (c == '/')
switch (c)
{
case ';':
checkSegment(encodedSegment, true, uri, segment, i);
mark = i + 1;
state = State.PARAM;
break;
case '?':
checkSegment(encodedSegment, false, uri, segment, i);
_path = uri.substring(pathMark, i);
mark = i + 1;
state = State.QUERY;
break;
case '#':
checkSegment(encodedSegment, false, uri, segment, i);
_path = uri.substring(pathMark, i);
mark = i + 1;
state = State.FRAGMENT;
break;
case '%':
encoded = true;
encodedSegment = true;
break;
case '/':
checkSegment(encodedSegment, false, uri, segment, i);
encodedSegment = false;
segment = i + 1;
break;
}
continue;
Expand All @@ -443,8 +480,10 @@ else if (c == '/')
state = State.FRAGMENT;
break;
case '/':
encoded = true;
// ignore internal params
encoded = true;
encodedSegment = false;
segment = i + 1;
state = State.PATH;
break;
case ';':
Expand Down Expand Up @@ -516,6 +555,7 @@ else if (c == '/')
break;

case PATH:
checkSegment(encodedSegment, false, uri, segment, end);
_path = uri.substring(pathMark, end);
break;

Expand All @@ -533,6 +573,38 @@ else if (c == '/')
}
}

/**
* Check for ambiguous path segments.
*
* An ambiguous path segment is one that is perhaps technically legal, but is considered undesirable to handle
* due to possible ambiguity. Examples include segments like '..;', '%2e', '%2e%2e' etc.
* @param segmentEncoded True if the segment is encoded
* @param param true if the segment has a parameter
* @param uri The URI string
* @param segment The inclusive starting index of the segment (excluding any '/')
* @param end The exclusive end index of the segment
*/
private void checkSegment(boolean segmentEncoded, boolean param, String uri, int segment, int end)
{
// if the segment is encoded or there is a segment parameter
if (!_ambiguousSegment && (segmentEncoded || param))
{
// Look for possible bad segments
Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment);

// A segment is ambiguous if it is always ambiguous (TRUE) or ambiguous only with params (FALSE) and we have a param
_ambiguousSegment |= ambiguous == Boolean.TRUE || (param && ambiguous == Boolean.FALSE);
}
}

/**
* @return True if the URI has a possibly ambiguous segment like '..;' or '%2e%2e'
*/
public boolean hasAmbiguousSegment()
{
return _ambiguousSegment;
}

public String getScheme()
{
return _scheme;
Expand Down Expand Up @@ -633,6 +705,8 @@ public void clear()
_fragment = null;

_decodedPath = null;

_ambiguousSegment = false;
}

public boolean isAbsolute()
Expand Down
39 changes: 39 additions & 0 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,12 @@

import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.stream.Stream;

import org.eclipse.jetty.util.MultiMap;
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 @@ -221,4 +224,40 @@ public void testBasicAuthCredentials() throws Exception
assertEquals(uri.getAuthority(), "example.com:8888");
assertEquals(uri.getUser(), "user:password");
}

public static Stream<String> pathsWithAmbiguousSegments()
{
// @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck
return Stream.of(
"/foo/%2e%2e/bar",
"/foo/%2e%2e;/bar",
"/foo/%2e%2e;param/bar",
"/foo/..;/bar",
"/foo/..;param/bar",
"foo/%2e%2e/bar",
"%2e%2e/bar",
"//host/%2e%2e/bar",
"scheme://host/%2e%2e/bar",
"scheme:/%2e%2e/bar",
"/foo/%2e%2e",
"/foo/%2e%2e?query",
"/foo/%2e%2e#fragment",
"foo/%2e.",
".%2e",
"//host/%2e%2e",
"scheme://host/.%2e",
"scheme:/%2e%2e",
"%2e",
"%2e.",
".%2e",
"%2e%2e"
);
}

@ParameterizedTest(name = "[{index}] {0}")
@MethodSource("pathsWithAmbiguousSegments")
public void testAmbiguousSegments(String uriWithBadSegment)
{
assertTrue(new HttpURI(uriWithBadSegment).hasAmbiguousSegment());
}
}
6 changes: 6 additions & 0 deletions jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,12 @@ public HttpConnection(HttpConfiguration config, Connector connector, EndPoint en
LOG.debug("New HTTP Connection {}", this);
}

@Deprecated
public HttpCompliance getHttpCompliance()
{
return _parser.getHttpCompliance();
}

public HttpConfiguration getHttpConfiguration()
{
return _config;
Expand Down
14 changes: 14 additions & 0 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@

import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.HostPortHttpField;
import org.eclipse.jetty.http.HttpComplianceSection;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpFields;
Expand All @@ -77,6 +78,7 @@
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.http.MetaData;
import org.eclipse.jetty.http.MimeTypes;
import org.eclipse.jetty.io.Connection;
import org.eclipse.jetty.io.RuntimeIOException;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.server.handler.ContextHandler.Context;
Expand Down Expand Up @@ -1815,6 +1817,18 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request)

setMethod(request.getMethod());
HttpURI uri = request.getURI();

if (uri.hasAmbiguousSegment())
{
// TODO replace in jetty-10 with HttpCompliance from the HttpConfiguration
Connection connection = _channel.getConnection();
boolean allow = connection instanceof HttpConnection
? ((HttpConnection)connection).getHttpCompliance().sections().contains(HttpComplianceSection.AMBIGUOUS_PATH_SEGMENTS)
: _channel.getServer().getAttribute(HttpComplianceSection.AMBIGUOUS_PATH_SEGMENTS.toString()) == Boolean.TRUE;
if (!allow)
throw new BadMessageException("Ambiguous segment in URI");
}

_originalURI = uri.isAbsolute() && request.getHttpVersion() != HttpVersion.HTTP_2 ? uri.toString() : uri.getPathQuery();

String encoded = uri.getPath();
Expand Down

0 comments on commit 372b407

Please sign in to comment.