From 20ef71fe5d709a90c2a5698834fff07b9b4e7ad7 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 16 Feb 2021 14:47:41 +0100 Subject: [PATCH] Fix #4275 fail URIs with ambiguous segments (#5954) Handle URIs by first resolving relative paths and then decoding. Added compliance mode to return 400 if there are ambiguous path segments. Signed-off-by: Greg Wilkins --- .../eclipse/jetty/http/HttpCompliance.java | 24 +- .../jetty/http/HttpComplianceSection.java | 3 +- .../org/eclipse/jetty/http/HttpParser.java | 5 + .../java/org/eclipse/jetty/http/HttpURI.java | 261 ++++++++++++------ .../org/eclipse/jetty/http/HttpURITest.java | 141 +++++++++- .../eclipse/jetty/server/HttpConnection.java | 6 + .../org/eclipse/jetty/server/Request.java | 18 +- .../jetty/server/HttpConnectionTest.java | 23 +- .../org/eclipse/jetty/server/RequestTest.java | 22 ++ .../jetty/servlet/AsyncContextTest.java | 12 +- .../jetty/servlet/DefaultServletTest.java | 3 + .../eclipse/jetty/servlet/RequestURITest.java | 3 + .../java/org/eclipse/jetty/util/URIUtil.java | 81 +++--- .../jetty/util/URIUtilCanonicalPathTest.java | 14 +- 14 files changed, 456 insertions(+), 160 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..8d6334cc307d 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 @@ -56,13 +56,14 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t LEGACY(sectionsBySpec("0,METHOD_CASE_SENSITIVE")), /** - * The legacy RFC2616 support, which incorrectly excludes + * The legacy RFC2616 support, which excludes * {@link HttpComplianceSection#METHOD_CASE_SENSITIVE}, * {@link HttpComplianceSection#FIELD_COLON}, * {@link HttpComplianceSection#TRANSFER_ENCODING_WITH_CONTENT_LENGTH}, - * {@link HttpComplianceSection#MULTIPLE_CONTENT_LENGTHS}, + * {@link HttpComplianceSection#MULTIPLE_CONTENT_LENGTHS} and + * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEGMENTS}. */ - RFC2616_LEGACY(sectionsBySpec("RFC2616,-FIELD_COLON,-METHOD_CASE_SENSITIVE,-TRANSFER_ENCODING_WITH_CONTENT_LENGTH,-MULTIPLE_CONTENT_LENGTHS")), + RFC2616_LEGACY(sectionsBySpec("RFC2616,-FIELD_COLON,-METHOD_CASE_SENSITIVE,-TRANSFER_ENCODING_WITH_CONTENT_LENGTH,-MULTIPLE_CONTENT_LENGTHS,-NO_AMBIGUOUS_PATH_SEGMENTS")), /** * The strict RFC2616 support mode @@ -70,9 +71,11 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t RFC2616(sectionsBySpec("RFC2616")), /** - * Jetty's current RFC7230 support, which incorrectly excludes {@link HttpComplianceSection#METHOD_CASE_SENSITIVE} + * Jetty's current RFC7230 support, which excludes + * {@link HttpComplianceSection#METHOD_CASE_SENSITIVE} and + * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEGMENTS}. */ - RFC7230_LEGACY(sectionsBySpec("RFC7230,-METHOD_CASE_SENSITIVE")), + RFC7230_LEGACY(sectionsBySpec("RFC7230,-METHOD_CASE_SENSITIVE,-NO_AMBIGUOUS_PATH_SEGMENTS")), /** * The RFC7230 support mode @@ -123,11 +126,6 @@ static EnumSet sectionsBySpec(String spec) i++; break; - case "*": - i++; - sections = EnumSet.allOf(HttpComplianceSection.class); - break; - case "RFC2616": sections = EnumSet.complementOf(EnumSet.of( HttpComplianceSection.NO_FIELD_FOLDING, @@ -135,6 +133,7 @@ static EnumSet sectionsBySpec(String spec) i++; break; + case "*": case "RFC7230": i++; sections = EnumSet.allOf(HttpComplianceSection.class); @@ -152,11 +151,6 @@ static EnumSet sectionsBySpec(String spec) if (exclude) element = element.substring(1); HttpComplianceSection section = HttpComplianceSection.valueOf(element); - if (section == null) - { - LOG.warn("Unknown section '" + element + "' in HttpCompliance spec: " + spec); - continue; - } if (exclude) sections.remove(section); else 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..ce5fa6645a3b 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"), + NO_AMBIGUOUS_PATH_SEGMENTS("https://tools.ietf.org/html/rfc3986#section-3.3", "No 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..74d5f2471d13 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 @@ -23,8 +23,11 @@ import java.net.URISyntaxException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.util.Objects; +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 +68,30 @@ private enum State ASTERISK } + /** + * The concept of URI path parameters was originally specified in + * RFC2396, but that was + * obsoleted by + * RFC3986 which removed + * a normative definition of path parameters. Specifically it excluded them from the + * Remove Dot Segments + * algorithm. This results in some ambiguity as dot segments can result from later + * parameter removal or % encoding expansion, that are not removed from the URI + * by {@link URIUtil#canonicalPath(String)}. Thus this class flags such ambiguous + * path segments, so that they may be rejected by the server if so configured. + */ + 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; @@ -73,9 +100,9 @@ private enum State private String _param; private String _query; private String _fragment; - - String _uri; - String _decodedPath; + private String _uri; + private String _decodedPath; + private boolean _ambiguousSegment; /** * Construct a normalized URI. @@ -108,16 +135,29 @@ public HttpURI(String scheme, String host, int port, String path, String param, _scheme = scheme; _host = host; _port = port; - _path = path; - _param = param; - _query = query; - _fragment = fragment; + if (path != null) + parse(State.PATH, path, 0, path.length()); + if (param != null) + _param = param; + if (query != null) + _query = query; + if (fragment != null) + _fragment = fragment; } public HttpURI(HttpURI uri) { - this(uri._scheme, uri._host, uri._port, uri._path, uri._param, uri._query, uri._fragment); + _scheme = uri._scheme; + _user = uri._user; + _host = uri._host; + _port = uri._port; + _path = uri._path; + _param = uri._param; + _query = uri._query; + _fragment = uri._fragment; _uri = uri._uri; + _decodedPath = uri._decodedPath; + _ambiguousSegment = uri._ambiguousSegment; } public HttpURI(String uri) @@ -129,40 +169,44 @@ public HttpURI(String uri) public HttpURI(URI uri) { _uri = null; - _scheme = uri.getScheme(); _host = uri.getHost(); if (_host == null && uri.getRawSchemeSpecificPart().startsWith("//")) _host = ""; _port = uri.getPort(); _user = uri.getUserInfo(); - _path = uri.getRawPath(); - - _decodedPath = uri.getPath(); - if (_decodedPath != null) - { - int p = _decodedPath.lastIndexOf(';'); - if (p >= 0) - _param = _decodedPath.substring(p + 1); - } + String path = uri.getRawPath(); + if (path != null) + parse(State.PATH, path, 0, path.length()); _query = uri.getRawQuery(); _fragment = uri.getFragment(); - - _decodedPath = null; } public HttpURI(String scheme, String host, int port, String pathQuery) { _uri = null; - _scheme = scheme; _host = host; _port = port; - if (pathQuery != null) parse(State.PATH, pathQuery, 0, pathQuery.length()); } + public void clear() + { + _uri = null; + _scheme = null; + _user = null; + _host = null; + _port = -1; + _path = null; + _param = null; + _query = null; + _fragment = null; + _decodedPath = null; + _ambiguousSegment = false; + } + public void parse(String uri) { clear(); @@ -205,9 +249,12 @@ public void parse(String uri, int offset, int length) private void parse(State state, final String uri, final int offset, final int end) { - boolean encoded = false; - int mark = offset; - int pathMark = 0; + 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 encoded = false; // set to true if the path contains % encoded characters + boolean dot = false; // set to true if the path containers . or .. segments + int escapedSlash = 0; // state of parsing a %2f for (int i = offset; i < end; i++) { @@ -241,21 +288,30 @@ private void parse(State state, final String uri, final int offset, final int en _path = "*"; state = State.ASTERISK; break; - + case '%': + encoded = true; + escapedSlash = 1; + mark = pathMark = segment = i; + state = State.PATH; + break; + case '.' : + dot = true; + 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; } } continue; } - case SCHEME_OR_PATH: { switch (c) @@ -266,40 +322,38 @@ private void parse(State state, final String uri, final int offset, final int en // Start again with scheme set state = State.START; break; - case '/': // must have been in a path and still are + segment = i + 1; state = State.PATH; break; - case ';': // must have been in a path mark = i + 1; state = State.PARAM; break; - case '?': // must have been in a path _path = uri.substring(mark, i); mark = i + 1; state = State.QUERY; break; - case '%': // must have be in an encoded path encoded = true; + escapedSlash = 1; state = State.PATH; break; - case '#': // must have been in a path _path = uri.substring(mark, i); state = State.FRAGMENT; break; + default: + break; } continue; } - case HOST_OR_PATH: { switch (c) @@ -310,23 +364,26 @@ private void parse(State state, final String uri, final int offset, final int en state = State.HOST; break; + case '%': case '@': case ';': case '?': case '#': + case '.': // 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; } - case HOST: { switch (c) @@ -334,6 +391,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 ':': @@ -348,14 +406,14 @@ private void parse(State state, final String uri, final int offset, final int en _user = uri.substring(mark, i); mark = i + 1; break; - case '[': state = State.IPV6; break; + default: + break; } continue; } - case IPV6: { switch (c) @@ -376,11 +434,11 @@ private void parse(State state, final String uri, final int offset, final int en state = State.PATH; } break; + default: + break; } - continue; } - case PORT: { if (c == '@') @@ -396,36 +454,57 @@ else if (c == '/') { _port = TypeUtil.parseInt(uri, mark, i - mark, 10); pathMark = mark = i; + segment = i + 1; state = State.PATH; } continue; } - case PATH: { switch (c) { case ';': + checkSegment(uri, segment, i, true); mark = i + 1; state = State.PARAM; break; case '?': + checkSegment(uri, segment, i, false); _path = uri.substring(pathMark, i); mark = i + 1; state = State.QUERY; break; case '#': + checkSegment(uri, segment, i, false); _path = uri.substring(pathMark, i); mark = i + 1; state = State.FRAGMENT; break; + case '/': + checkSegment(uri, segment, i, false); + segment = i + 1; + break; + case '.': + dot |= segment == i; + break; case '%': encoded = true; + escapedSlash = 1; + break; + case '2': + escapedSlash = escapedSlash == 1 ? 2 : 0; + break; + case 'f': + case 'F': + _ambiguousSegment |= (escapedSlash == 2); + escapedSlash = 0; + break; + default: + escapedSlash = 0; break; } continue; } - case PARAM: { switch (c) @@ -444,17 +523,18 @@ else if (c == '/') break; case '/': encoded = true; - // ignore internal params + segment = i + 1; state = State.PATH; break; case ';': // multiple parameters mark = i + 1; break; + default: + break; } continue; } - case QUERY: { if (c == '#') @@ -465,17 +545,18 @@ else if (c == '/') } continue; } - case ASTERISK: { throw new IllegalArgumentException("Bad character '*'"); } - case FRAGMENT: { _fragment = uri.substring(mark, end); i = end; + break; } + default: + break; } } @@ -486,51 +567,78 @@ else if (c == '/') case SCHEME_OR_PATH: _path = uri.substring(mark, end); break; - case HOST_OR_PATH: _path = uri.substring(mark, end); break; - case HOST: if (end > mark) _host = uri.substring(mark, end); break; - case IPV6: throw new IllegalArgumentException("No closing ']' for ipv6 in " + uri); - case PORT: _port = TypeUtil.parseInt(uri, mark, end - mark, 10); break; - case ASTERISK: break; - case FRAGMENT: _fragment = uri.substring(mark, end); break; - case PARAM: _path = uri.substring(pathMark, end); _param = uri.substring(mark, end); break; - case PATH: + checkSegment(uri, segment, end, false); _path = uri.substring(pathMark, end); break; - case QUERY: _query = uri.substring(mark, end); break; + default: + break; } - if (!encoded) + if (!encoded && !dot) { if (_param == null) _decodedPath = _path; else _decodedPath = _path.substring(0, _path.length() - _param.length() - 1); } + else if (_path != null) + { + String canonical = URIUtil.canonicalPath(_path); + if (canonical == null) + throw new BadMessageException("Bad URI"); + _decodedPath = URIUtil.decodePath(canonical); + } + } + + /** + * 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 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(String uri, int segment, int end, boolean param) + { + if (!_ambiguousSegment) + { + Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment); + _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() @@ -561,10 +669,12 @@ public String getPath() return _path; } + /** + * @return The decoded canonical path. + * @see URIUtil#canonicalPath(String) + */ public String getDecodedPath() { - if (_decodedPath == null && _path != null) - _decodedPath = URIUtil.decodePath(_path); return _decodedPath; } @@ -575,10 +685,14 @@ public String getParam() public void setParam(String param) { - _param = param; - if (_path != null && !_path.contains(_param)) + if (!Objects.equals(_param, param)) { - _path += ";" + _param; + if (_param != null && _path.endsWith(";" + _param)) + _path = _path.substring(0, _path.length() - 1 - _param.length()); + _param = param; + if (_param != null) + _path = (_path == null ? "" : _path) + ";" + _param; + _uri = null; } } @@ -620,21 +734,6 @@ public void decodeQueryTo(MultiMap parameters, Charset encoding) throws UrlEncoded.decodeTo(_query, parameters, encoding); } - public void clear() - { - _uri = null; - - _scheme = null; - _host = null; - _port = -1; - _path = null; - _param = null; - _query = null; - _fragment = null; - - _decodedPath = null; - } - public boolean isAbsolute() { return _scheme != null && !_scheme.isEmpty(); @@ -688,6 +787,12 @@ public boolean equals(Object o) return toString().equals(o.toString()); } + @Override + public int hashCode() + { + return toString().hashCode(); + } + public void setScheme(String scheme) { _scheme = scheme; @@ -711,8 +816,9 @@ public void setAuthority(String host, int port) public void setPath(String path) { _uri = null; - _path = path; - _decodedPath = null; + _path = null; + if (path != null) + parse(State.PATH, path, 0, path.length()); } public void setPathQuery(String path) @@ -722,6 +828,7 @@ public void setPathQuery(String path) _decodedPath = null; _param = null; _fragment = null; + _query = null; if (path != null) parse(State.PATH, path, 0, path.length()); } 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..24b2a37e48a1 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,14 @@ import java.net.URLEncoder; import java.nio.charset.StandardCharsets; +import java.util.Arrays; +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.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; @@ -214,11 +219,137 @@ public void testSchemeAndOrAuthority() throws Exception } @Test - public void testBasicAuthCredentials() throws Exception + public void testSetters() throws Exception { - HttpURI uri = new HttpURI("http://user:password@example.com:8888/blah"); - assertEquals("http://user:password@example.com:8888/blah", uri.toString()); - assertEquals(uri.getAuthority(), "example.com:8888"); - assertEquals(uri.getUser(), "user:password"); + HttpURI uri = new HttpURI(); + assertEquals("", uri.toString()); + + uri = new HttpURI(null, null, 0, null, null, null, null); + assertEquals("", uri.toString()); + + uri.setPath("/path/info"); + assertEquals("/path/info", uri.toString()); + + uri.setAuthority("host", 8080); + assertEquals("//host:8080/path/info", uri.toString()); + + uri.setParam("param"); + assertEquals("//host:8080/path/info;param", uri.toString()); + + uri.setQuery("a=b"); + assertEquals("//host:8080/path/info;param?a=b", uri.toString()); + + uri.setScheme("http"); + assertEquals("http://host:8080/path/info;param?a=b", uri.toString()); + + uri.setPathQuery("/other;xxx/path;ppp?query"); + assertEquals("http://host:8080/other;xxx/path;ppp?query", uri.toString()); + + assertThat(uri.getScheme(), is("http")); + assertThat(uri.getAuthority(), is("host:8080")); + assertThat(uri.getHost(), is("host")); + assertThat(uri.getPort(), is(8080)); + assertThat(uri.getPath(), is("/other;xxx/path;ppp")); + assertThat(uri.getDecodedPath(), is("/other/path")); + assertThat(uri.getParam(), is("ppp")); + assertThat(uri.getQuery(), is("query")); + assertThat(uri.getPathQuery(), is("/other;xxx/path;ppp?query")); + + uri.setPathQuery(null); + assertEquals("http://host:8080", uri.toString()); + + uri.setPathQuery("/other;xxx/path;ppp?query"); + assertEquals("http://host:8080/other;xxx/path;ppp?query", uri.toString()); + + uri.setScheme(null); + assertEquals("//host:8080/other;xxx/path;ppp?query", uri.toString()); + + uri.setAuthority(null,-1); + assertEquals("/other;xxx/path;ppp?query", uri.toString()); + + uri.setParam(null); + assertEquals("/other;xxx/path?query", uri.toString()); + + uri.setQuery(null); + assertEquals("/other;xxx/path", uri.toString()); + + uri.setPath(null); + assertEquals("", uri.toString()); + } + + public static Stream decodePathTests() + { + return Arrays.stream(new Object[][] + { + // Simple path example + {"http://host/path/info", "/path/info", false}, + {"//host/path/info", "/path/info", false}, + {"/path/info", "/path/info", false}, + + // legal non ambiguous relative paths + {"http://host/../path/info", null, false}, + {"http://host/path/../info", "/info", false}, + {"http://host/path/./info", "/path/info", false}, + {"//host/path/../info", "/info", false}, + {"//host/path/./info", "/path/info", false}, + {"/path/../info", "/info", false}, + {"/path/./info", "/path/info", false}, + {"path/../info", "info", false}, + {"path/./info", "path/info", false}, + + // illegal paths + {"//host/../path/info", null, false}, + {"/../path/info", null, false}, + {"../path/info", null, false}, + {"/path/%XX/info", null, false}, + {"/path/%2/F/info", null, false}, + + // ambiguous dot encodings or parameter inclusions + {"scheme://host/path/%2e/info", "/path/./info", true}, + {"scheme:/path/%2e/info", "/path/./info", true}, + {"/path/%2e/info", "/path/./info", true}, + {"path/%2e/info/", "path/./info/", true}, + {"/path/%2e%2e/info", "/path/../info", true}, + {"/path/%2e%2e;/info", "/path/../info", true}, + {"/path/%2e%2e;param/info", "/path/../info", true}, + {"/path/%2e%2e;param;other/info;other", "/path/../info", true}, + {"/path/.;/info", "/path/./info", true}, + {"/path/.;param/info", "/path/./info", true}, + {"/path/..;/info", "/path/../info", true}, + {"/path/..;param/info", "/path/../info", true}, + {"%2e/info", "./info", true}, + {"%2e%2e/info", "../info", true}, + {"%2e%2e;/info", "../info", true}, + {".;/info", "./info", true}, + {".;param/info", "./info", true}, + {"..;/info", "../info", true}, + {"..;param/info", "../info", true}, + {"%2e", ".", true}, + {"%2e.", "..", true}, + {".%2e", "..", true}, + {"%2e%2e", "..", true}, + + // ambiguous segment separators + {"/path/%2f/info", "/path///info", true}, + {"%2f/info", "//info", true}, + {"%2F/info", "//info", true}, + + }).map(Arguments::of); + } + + @ParameterizedTest + @MethodSource("decodePathTests") + public void testDecodedPath(String input, String decodedPath, boolean ambiguous) + { + try + { + HttpURI uri = new HttpURI(input); + assertThat(uri.getDecodedPath(), is(decodedPath)); + assertThat(uri.hasAmbiguousSegment(), is(ambiguous)); + } + catch (Exception e) + { + assertThat(decodedPath, nullValue()); + } } } 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..4bda67ae2891 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,8 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HostPortHttpField; +import org.eclipse.jetty.http.HttpCompliance; +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 +79,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 +1818,19 @@ 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(); + HttpCompliance compliance = connection instanceof HttpConnection + ? ((HttpConnection)connection).getHttpCompliance() + : _channel.getConnector().getBean(HttpCompliance.class); + boolean allow = compliance != null && !compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS); + 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(); @@ -1826,7 +1842,7 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) } else if (encoded.startsWith("/")) { - path = (encoded.length() == 1) ? "/" : URIUtil.canonicalPath(uri.getDecodedPath()); + path = (encoded.length() == 1) ? "/" : uri.getDecodedPath(); } else if ("*".equals(encoded) || HttpMethod.CONNECT.is(getMethod())) { diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java index a480503e1d18..72197e6867a3 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java @@ -500,43 +500,28 @@ public void testOKPathDotDotPath() throws Exception public void testBadPathDotDotPath() throws Exception { String response = connector.getResponse("GET /ooops/../../path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 Bad URI"); - } - - @Test - public void testOKPathEncodedDotDotPath() throws Exception - { - String response = connector.getResponse("GET /ooops/%2e%2e/path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 200 OK"); - checkContains(response, 0, "pathInfo=/path"); - } - - @Test - public void testBadPathEncodedDotDotPath() throws Exception - { - String response = connector.getResponse("GET /ooops/%2e%2e/%2e%2e/path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 Bad URI"); + checkContains(response, 0, "HTTP/1.1 400 "); } @Test public void testBadDotDotPath() throws Exception { String response = connector.getResponse("GET ../path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 Bad URI"); + checkContains(response, 0, "HTTP/1.1 400 "); } @Test public void testBadSlashDotDotPath() throws Exception { String response = connector.getResponse("GET /../path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 Bad URI"); + checkContains(response, 0, "HTTP/1.1 400 "); } @Test public void testEncodedBadDotDotPath() throws Exception { String response = connector.getResponse("GET %2e%2e/path HTTP/1.0\r\nHost: localhost:80\r\n\n"); - checkContains(response, 0, "HTTP/1.1 400 Bad URI"); + checkContains(response, 0, "HTTP/1.1 400 "); } @Test diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index 730944a3a916..1f796ab7a13a 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -1836,6 +1836,28 @@ public void testGetterSafeFromNullPointerException() assertEquals(0, request.getParameterMap().size()); } + @Test + public void testAmbiguousPaths() throws Exception + { + _handler._checker = (request, response) -> true; + + String request = "GET /ambiguous/..;/path HTTP/1.0\r\n" + + "Host: whatever\r\n" + + "\r\n"; + + _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400")); + + _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_LEGACY); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); + + _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC2616); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400")); + + _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC2616_LEGACY); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); + } + private static long getFileCount(Path path) { try (Stream s = Files.list(path)) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java index 87c08dc3545d..d6e9b838c2a2 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/AsyncContextTest.java @@ -229,7 +229,7 @@ public void testDispatchAsyncContext() throws Exception @Test public void testDispatchAsyncContextEncodedUrl() throws Exception { - String request = "GET /ctx/test/hello%2fthere?dispatch=true HTTP/1.1\r\n" + + String request = "GET /ctx/test/hello%20there?dispatch=true HTTP/1.1\r\n" + "Host: localhost\r\n" + "Content-Type: application/x-www-form-urlencoded\r\n" + "Connection: close\r\n" + @@ -253,16 +253,16 @@ public void testDispatchAsyncContextEncodedUrl() throws Exception // async run attributes assertThat("async run attr servlet path is original", responseBody, containsString("async:run:attr:servletPath:/test")); - assertThat("async run attr path info has correct encoding", responseBody, containsString("async:run:attr:pathInfo:/hello/there")); + assertThat("async run attr path info has correct encoding", responseBody, containsString("async:run:attr:pathInfo:/hello there")); assertThat("async run attr query string", responseBody, containsString("async:run:attr:queryString:dispatch=true")); assertThat("async run context path", responseBody, containsString("async:run:attr:contextPath:/ctx")); - assertThat("async run request uri has correct encoding", responseBody, containsString("async:run:attr:requestURI:/ctx/test/hello%2fthere")); + assertThat("async run request uri has correct encoding", responseBody, containsString("async:run:attr:requestURI:/ctx/test/hello%20there")); } @Test public void testDispatchAsyncContextSelfEncodedUrl() throws Exception { - String request = "GET /ctx/self/hello%2fthere?dispatch=true HTTP/1.1\r\n" + + String request = "GET /ctx/self/hello%20there?dispatch=true HTTP/1.1\r\n" + "Host: localhost\r\n" + "Content-Type: application/x-www-form-urlencoded\r\n" + "Connection: close\r\n" + @@ -272,8 +272,8 @@ public void testDispatchAsyncContextSelfEncodedUrl() throws Exception String responseBody = response.getContent(); - assertThat("servlet request uri initial", responseBody, containsString("doGet.REQUEST.requestURI:/ctx/self/hello%2fthere")); - assertThat("servlet request uri async", responseBody, containsString("doGet.ASYNC.requestURI:/ctx/self/hello%2fthere")); + assertThat("servlet request uri initial", responseBody, containsString("doGet.REQUEST.requestURI:/ctx/self/hello%20there")); + assertThat("servlet request uri async", responseBody, containsString("doGet.ASYNC.requestURI:/ctx/self/hello%20there")); } @Test diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java index b2c063503f84..1661109d9a5c 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java @@ -47,12 +47,14 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.DateGenerator; +import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpContent; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.server.HttpConfiguration; +import org.eclipse.jetty.server.HttpConnectionFactory; import org.eclipse.jetty.server.LocalConnector; import org.eclipse.jetty.server.ResourceContentFactory; import org.eclipse.jetty.server.ResourceService; @@ -116,6 +118,7 @@ public void init() throws Exception connector = new LocalConnector(server); connector.getConnectionFactory(HttpConfiguration.ConnectionFactory.class).getHttpConfiguration().setSendServerVersion(false); + connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_LEGACY); // allow ambiguous path segments File extraJarResources = MavenTestingUtils.getTestResourceFile(ODD_JAR); URL[] urls = new URL[]{extraJarResources.toURI().toURL()}; diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java index de3814accf20..7de53075e785 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java @@ -34,6 +34,8 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.HttpCompliance; +import org.eclipse.jetty.server.HttpConnectionFactory; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; import org.hamcrest.Matchers; @@ -112,6 +114,7 @@ public static void startServer() throws Exception ServerConnector connector = new ServerConnector(server); connector.setPort(0); server.addConnector(connector); + connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_LEGACY); // Allow ambiguous segments ServletContextHandler context = new ServletContextHandler(); context.setContextPath("/"); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java index 371e15387fac..6818e5440928 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java @@ -782,11 +782,9 @@ public static String parentPath(String p) } /** - * Convert a decoded path to a canonical form. + * Convert an encoded path to a canonical form. *

* All instances of "." and ".." are factored out. - *

- *

* Null is returned if the path tries to .. above its root. *

* @@ -795,31 +793,35 @@ public static String parentPath(String p) */ public static String canonicalPath(String path) { + // See https://tools.ietf.org/html/rfc3986#section-5.2.4 + if (path == null || path.isEmpty()) return path; - boolean slash = true; int end = path.length(); int i = 0; + int dots = 0; - loop: - while (i < end) + loop: while (i < end) { char c = path.charAt(i); switch (c) { case '/': - slash = true; + dots = 0; break; case '.': - if (slash) + if (dots == 0) + { + dots = 1; break loop; - slash = false; + } + dots = -1; break; default: - slash = false; + dots = -1; } i++; @@ -831,7 +833,6 @@ public static String canonicalPath(String path) StringBuilder canonical = new StringBuilder(path.length()); canonical.append(path, 0, i); - int dots = 1; i++; while (i <= end) { @@ -839,14 +840,18 @@ public static String canonicalPath(String path) switch (c) { case '\0': + if (dots == 2) + { + if (canonical.length() < 2) + return null; + canonical.setLength(canonical.length() - 1); + canonical.setLength(canonical.lastIndexOf("/") + 1); + } + break; + case '/': switch (dots) { - case 0: - if (c != '\0') - canonical.append(c); - break; - case 1: break; @@ -858,36 +863,42 @@ public static String canonicalPath(String path) break; default: - while (dots-- > 0) - { - canonical.append('.'); - } - if (c != '\0') - canonical.append(c); + canonical.append(c); } - - slash = true; dots = 0; break; case '.': - if (dots > 0) - dots++; - else if (slash) - dots = 1; - else - canonical.append('.'); - slash = false; + switch (dots) + { + case 0: + dots = 1; + break; + case 1: + dots = 2; + break; + case 2: + canonical.append("..."); + dots = -1; + break; + default: + canonical.append('.'); + } break; default: - while (dots-- > 0) + switch (dots) { - canonical.append('.'); + case 1: + canonical.append('.'); + break; + case 2: + canonical.append(".."); + break; + default: } canonical.append(c); - dots = 0; - slash = false; + dots = -1; } i++; diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java index 396004ff7057..82f2771a42cb 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java @@ -34,6 +34,10 @@ public static Stream data() { String[][] canonical = { + // Examples from RFC + {"/a/b/c/./../../g", "/a/g"}, + {"mid/content=5/../6", "mid/6"}, + // Basic examples (no changes expected) {"/hello.html", "/hello.html"}, {"/css/main.css", "/css/main.css"}, @@ -56,8 +60,12 @@ public static Stream data() {"/aaa/./bbb/", "/aaa/bbb/"}, {"/aaa/./bbb", "/aaa/bbb"}, {"./bbb/", "bbb/"}, + {"./aaa", "aaa"}, + {"./aaa/", "aaa/"}, + {"/./aaa/", "/aaa/"}, {"./aaa/../bbb/", "bbb/"}, {"/foo/.", "/foo/"}, + {"/foo/./", "/foo/"}, {"./", ""}, {".", ""}, {".//", "/"}, @@ -121,6 +129,10 @@ public static Stream data() {"/foo/.;/bar", "/foo/.;/bar"}, {"/foo/..;/bar", "/foo/..;/bar"}, {"/foo/..;/..;/bar", "/foo/..;/..;/bar"}, + + // Trailing / is preserved + {"/foo/bar/..", "/foo/"}, + {"/foo/bar/../", "/foo/"}, }; ArrayList ret = new ArrayList<>(); @@ -135,6 +147,6 @@ public static Stream data() @MethodSource("data") public void testCanonicalPath(String input, String expectedResult) { - assertThat("Canonical", URIUtil.canonicalPath(input), is(expectedResult)); + assertThat(URIUtil.canonicalPath(input), is(expectedResult)); } }