From 372b407d04b11f685f8135e0925f2e5b2765fd59 Mon Sep 17 00:00:00 2001 From: gregw Date: Mon, 8 Feb 2021 15:46:01 +0100 Subject: [PATCH] Fix #4275 fail URIs with ambiguous segments Fix #4275 fail URIs with ambiguous segments. --- .../eclipse/jetty/http/HttpCompliance.java | 2 +- .../jetty/http/HttpComplianceSection.java | 3 +- .../org/eclipse/jetty/http/HttpParser.java | 5 ++ .../java/org/eclipse/jetty/http/HttpURI.java | 80 ++++++++++++++++++- .../org/eclipse/jetty/http/HttpURITest.java | 39 +++++++++ .../eclipse/jetty/server/HttpConnection.java | 6 ++ .../org/eclipse/jetty/server/Request.java | 14 ++++ 7 files changed, 144 insertions(+), 5 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index 1488c89622e5..2d840e193767 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -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 diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java index e1dcc025a6dc..4e0f515caf16 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java @@ -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; diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index 237e9613057f..4637006c8644 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -311,6 +311,11 @@ public HttpHandler getHandler() return _handler; } + public HttpCompliance getHttpCompliance() + { + return _compliance; + } + /** * Check RFC compliance violation * diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 6b5cdff29952..d810dda3b9e4 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -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; @@ -65,6 +67,18 @@ private enum State ASTERISK } + private static final Trie __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; @@ -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. @@ -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++) { @@ -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; } } @@ -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; @@ -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; @@ -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; @@ -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 ':': @@ -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; @@ -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; @@ -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 ';': @@ -516,6 +555,7 @@ else if (c == '/') break; case PATH: + checkSegment(encodedSegment, false, uri, segment, end); _path = uri.substring(pathMark, end); break; @@ -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; @@ -633,6 +705,8 @@ public void clear() _fragment = null; _decodedPath = null; + + _ambiguousSegment = false; } public boolean isAbsolute() diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java index b341e57b3334..3a25920127b1 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java @@ -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; @@ -221,4 +224,40 @@ public void testBasicAuthCredentials() throws Exception assertEquals(uri.getAuthority(), "example.com:8888"); assertEquals(uri.getUser(), "user:password"); } + + public static Stream 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()); + } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index 3ae8bbe4bc67..024d57ec2459 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -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; diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index ff0f46a8e97d..b81511230014 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -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; @@ -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; @@ -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();