From 49e73dfb75909fbd8728c4a0d6dbe74686e8dfd4 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 24 Feb 2021 10:05:05 +0100 Subject: [PATCH] =?UTF-8?q?Fix=20#4275=20#6001=20separate=20compliance=20m?= =?UTF-8?q?odes=20for=20ambiguous=20URI=20segments=20and=20se=E2=80=A6=20(?= =?UTF-8?q?#6003)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix #4275 separate compliance modes for ambiguous URI segments and separators --- .../eclipse/jetty/http/HttpCompliance.java | 19 ++-- .../jetty/http/HttpComplianceSection.java | 3 +- .../java/org/eclipse/jetty/http/HttpURI.java | 39 ++++++-- .../org/eclipse/jetty/http/HttpURITest.java | 98 ++++++++++--------- .../org/eclipse/jetty/server/Request.java | 14 +-- .../org/eclipse/jetty/server/RequestTest.java | 25 ++++- 6 files changed, 124 insertions(+), 74 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 8d6334cc307d..670c792a6af0 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 @@ -22,9 +22,6 @@ import java.util.HashMap; import java.util.Map; -import org.eclipse.jetty.util.log.Log; -import org.eclipse.jetty.util.log.Logger; - /** * HTTP compliance modes for Jetty HTTP parsing and handling. * A Compliance mode consists of a set of {@link HttpComplianceSection}s which are applied @@ -60,10 +57,11 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t * {@link HttpComplianceSection#METHOD_CASE_SENSITIVE}, * {@link HttpComplianceSection#FIELD_COLON}, * {@link HttpComplianceSection#TRANSFER_ENCODING_WITH_CONTENT_LENGTH}, - * {@link HttpComplianceSection#MULTIPLE_CONTENT_LENGTHS} and - * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEGMENTS}. + * {@link HttpComplianceSection#MULTIPLE_CONTENT_LENGTHS}, + * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEGMENTS} and + * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEPARATORS}. */ - RFC2616_LEGACY(sectionsBySpec("RFC2616,-FIELD_COLON,-METHOD_CASE_SENSITIVE,-TRANSFER_ENCODING_WITH_CONTENT_LENGTH,-MULTIPLE_CONTENT_LENGTHS,-NO_AMBIGUOUS_PATH_SEGMENTS")), + RFC2616_LEGACY(sectionsBySpec("RFC2616,-FIELD_COLON,-METHOD_CASE_SENSITIVE,-TRANSFER_ENCODING_WITH_CONTENT_LENGTH,-MULTIPLE_CONTENT_LENGTHS,-NO_AMBIGUOUS_PATH_SEGMENTS,-NO_AMBIGUOUS_PATH_SEPARATORS")), /** * The strict RFC2616 support mode @@ -72,10 +70,11 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t /** * Jetty's current RFC7230 support, which excludes - * {@link HttpComplianceSection#METHOD_CASE_SENSITIVE} and - * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEGMENTS}. + * {@link HttpComplianceSection#METHOD_CASE_SENSITIVE}, + * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEGMENTS} and + * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEPARATORS}. */ - RFC7230_LEGACY(sectionsBySpec("RFC7230,-METHOD_CASE_SENSITIVE,-NO_AMBIGUOUS_PATH_SEGMENTS")), + RFC7230_LEGACY(sectionsBySpec("RFC7230,-METHOD_CASE_SENSITIVE,-NO_AMBIGUOUS_PATH_SEGMENTS,-NO_AMBIGUOUS_PATH_SEPARATORS")), /** * The RFC7230 support mode @@ -105,8 +104,6 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t public static final String VIOLATIONS_ATTR = "org.eclipse.jetty.http.compliance.violations"; - private static final Logger LOG = Log.getLogger(HttpParser.class); - private static EnumSet sectionsByProperty(String property) { String s = System.getProperty(HttpCompliance.class.getName() + property); 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 ce5fa6645a3b..f70f975345ff 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 @@ -32,7 +32,8 @@ public enum HttpComplianceSection 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"), - NO_AMBIGUOUS_PATH_SEGMENTS("https://tools.ietf.org/html/rfc3986#section-3.3", "No ambiguous URI path segments"); + NO_AMBIGUOUS_PATH_SEGMENTS("https://tools.ietf.org/html/rfc3986#section-3.3", "No ambiguous URI path segments"), + NO_AMBIGUOUS_PATH_SEPARATORS("https://tools.ietf.org/html/rfc3986#section-3.3", "No ambiguous URI path separators"); final String url; final String description; 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 c3453671809e..91554086fa88 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,6 +23,7 @@ import java.net.URISyntaxException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.util.EnumSet; import java.util.Objects; import org.eclipse.jetty.util.ArrayTrie; @@ -68,6 +69,12 @@ private enum State ASTERISK } + enum Ambiguous + { + SEGMENT, + SEPARATOR + } + /** * The concept of URI path parameters was originally specified in * RFC2396, but that was @@ -102,7 +109,7 @@ private enum State private String _fragment; private String _uri; private String _decodedPath; - private boolean _ambiguousSegment; + private final EnumSet _ambiguous = EnumSet.noneOf(Ambiguous.class); /** * Construct a normalized URI. @@ -157,7 +164,7 @@ public HttpURI(HttpURI uri) _fragment = uri._fragment; _uri = uri._uri; _decodedPath = uri._decodedPath; - _ambiguousSegment = uri._ambiguousSegment; + _ambiguous.addAll(uri._ambiguous); } public HttpURI(String uri) @@ -204,7 +211,7 @@ public void clear() _query = null; _fragment = null; _decodedPath = null; - _ambiguousSegment = false; + _ambiguous.clear(); } public void parse(String uri) @@ -496,7 +503,8 @@ else if (c == '/') break; case 'f': case 'F': - _ambiguousSegment |= (escapedSlash == 2); + if (escapedSlash == 2) + _ambiguous.add(Ambiguous.SEPARATOR); escapedSlash = 0; break; default: @@ -626,10 +634,11 @@ else if (_path != null) */ private void checkSegment(String uri, int segment, int end, boolean param) { - if (!_ambiguousSegment) + if (!_ambiguous.contains(Ambiguous.SEGMENT)) { Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment); - _ambiguousSegment |= ambiguous == Boolean.TRUE || (param && ambiguous == Boolean.FALSE); + if (ambiguous == Boolean.TRUE || (param && ambiguous == Boolean.FALSE)) + _ambiguous.add(Ambiguous.SEGMENT); } } @@ -638,7 +647,23 @@ private void checkSegment(String uri, int segment, int end, boolean param) */ public boolean hasAmbiguousSegment() { - return _ambiguousSegment; + return _ambiguous.contains(Ambiguous.SEGMENT); + } + + /** + * @return True if the URI has a possibly ambiguous separator of %2f + */ + public boolean hasAmbiguousSeparator() + { + return _ambiguous.contains(Ambiguous.SEPARATOR); + } + + /** + * @return True if the URI has either an {@link #hasAmbiguousSegment()} or {@link #hasAmbiguousSeparator()}. + */ + public boolean isAmbiguous() + { + return !_ambiguous.isEmpty(); } public String getScheme() 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 3713b5247b5d..c51120a406b5 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 @@ -285,73 +285,77 @@ 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}, + {"http://host/path/info", "/path/info", false, false}, + {"//host/path/info", "/path/info", false, false}, + {"/path/info", "/path/info", false, 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}, + {"http://host/../path/info", null, false, false}, + {"http://host/path/../info", "/info", false, false}, + {"http://host/path/./info", "/path/info", false, false}, + {"//host/path/../info", "/info", false, false}, + {"//host/path/./info", "/path/info", false, false}, + {"/path/../info", "/info", false, false}, + {"/path/./info", "/path/info", false, false}, + {"path/../info", "info", false, false}, + {"path/./info", "path/info", false, 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}, + {"//host/../path/info", null, false, false}, + {"/../path/info", null, false, false}, + {"../path/info", null, false, false}, + {"/path/%XX/info", null, false, false}, + {"/path/%2/F/info", null, false, 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}, + {"scheme://host/path/%2e/info", "/path/./info", true, false}, + {"scheme:/path/%2e/info", "/path/./info", true, false}, + {"/path/%2e/info", "/path/./info", true, false}, + {"path/%2e/info/", "path/./info/", true, false}, + {"/path/%2e%2e/info", "/path/../info", true, false}, + {"/path/%2e%2e;/info", "/path/../info", true, false}, + {"/path/%2e%2e;param/info", "/path/../info", true, false}, + {"/path/%2e%2e;param;other/info;other", "/path/../info", true, false}, + {"/path/.;/info", "/path/./info", true, false}, + {"/path/.;param/info", "/path/./info", true, false}, + {"/path/..;/info", "/path/../info", true, false}, + {"/path/..;param/info", "/path/../info", true, false}, + {"%2e/info", "./info", true, false}, + {"%2e%2e/info", "../info", true, false}, + {"%2e%2e;/info", "../info", true, false}, + {".;/info", "./info", true, false}, + {".;param/info", "./info", true, false}, + {"..;/info", "../info", true, false}, + {"..;param/info", "../info", true, false}, + {"%2e", ".", true, false}, + {"%2e.", "..", true, false}, + {".%2e", "..", true, false}, + {"%2e%2e", "..", true, false}, // ambiguous segment separators - {"/path/%2f/info", "/path///info", true}, - {"%2f/info", "//info", true}, - {"%2F/info", "//info", true}, + {"/path/%2f/info", "/path///info", false, true}, + {"%2f/info", "//info", false, true}, + {"%2F/info", "//info", false, true}, + {"/path/%2f../info", "/path//../info", false, true}, + {"/path/%2f/..;/info", "/path///../info", true, true}, // Non ascii characters - {"http://localhost:9000/x\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", "/x\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", false}, - {"http://localhost:9000/\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", "/\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", false}, + {"http://localhost:9000/x\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", "/x\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", false, false}, + {"http://localhost:9000/\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", "/\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", false, false}, }).map(Arguments::of); } @ParameterizedTest @MethodSource("decodePathTests") - public void testDecodedPath(String input, String decodedPath, boolean ambiguous) + public void testDecodedPath(String input, String decodedPath, boolean ambiguousSegment, boolean ambiguousSeparator) { try { HttpURI uri = new HttpURI(input); assertThat(uri.getDecodedPath(), is(decodedPath)); - assertThat(uri.hasAmbiguousSegment(), is(ambiguous)); + assertThat(uri.hasAmbiguousSegment(), is(ambiguousSegment)); + assertThat(uri.hasAmbiguousSeparator(), is(ambiguousSeparator)); + assertThat(uri.isAmbiguous(), is(ambiguousSegment || ambiguousSeparator)); } catch (Exception e) { 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 bcd2777a311c..eba35c6f41a4 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 @@ -1824,16 +1824,18 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) setMethod(request.getMethod()); HttpURI uri = request.getURI(); - if (uri.hasAmbiguousSegment()) + if (uri.isAmbiguous()) { - // TODO replace in jetty-10 with HttpCompliance from the HttpConfiguration - Connection connection = _channel.getConnection(); + // replaced in jetty-10 with URICompliance from the HttpConfiguration + Connection connection = _channel == null ? null : _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) + : _channel != null ? _channel.getConnector().getBean(HttpCompliance.class) : null; + + if (uri.hasAmbiguousSegment() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS))) throw new BadMessageException("Ambiguous segment in URI"); + if (uri.hasAmbiguousSeparator() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEPARATORS))) + throw new BadMessageException("Ambiguous separator in URI"); } _originalURI = uri.isAbsolute() && request.getHttpVersion() != HttpVersion.HTTP_2 ? uri.toString() : uri.getPathQuery(); 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 1f796ab7a13a..48305a18b3fa 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 @@ -1837,7 +1837,7 @@ public void testGetterSafeFromNullPointerException() } @Test - public void testAmbiguousPaths() throws Exception + public void testAmbiguousSegments() throws Exception { _handler._checker = (request, response) -> true; @@ -1858,6 +1858,28 @@ public void testAmbiguousPaths() throws Exception assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); } + @Test + public void testAmbiguousSeparators() throws Exception + { + _handler._checker = (request, response) -> true; + + String request = "GET /ambiguous/%2f/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)) @@ -1961,7 +1983,6 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques ((Request)request).setHandled(true); try { - MultipartConfigElement mpce = new MultipartConfigElement(tmpDir.getAbsolutePath(), -1, -1, 2); request.setAttribute(Request.MULTIPART_CONFIG_ELEMENT, mpce);