From b56edf511ab4399122ea2c6162a4a5988870f479 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 8 Apr 2021 12:03:30 +1000 Subject: [PATCH] UriCompliance mode improvements #6132 (#6137) Resolve #6132 Improve configuration of ambiguous URI handling. Added NON_CANONICAL_AMBIGUOUS_PATHS --- .../org/eclipse/jetty/http/UriCompliance.java | 69 +++++++++++++------ .../org/eclipse/jetty/server/Request.java | 10 +-- .../org/eclipse/jetty/server/RequestTest.java | 34 +++++++++ 3 files changed, 88 insertions(+), 25 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java index 656c25e9036..0eb0d557ddd 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java @@ -25,7 +25,9 @@ import org.slf4j.LoggerFactory; import static java.util.Arrays.asList; import static java.util.Collections.unmodifiableSet; import static java.util.EnumSet.allOf; +import static java.util.EnumSet.complementOf; import static java.util.EnumSet.noneOf; +import static java.util.EnumSet.of; /** * URI compliance modes for Jetty request handling. @@ -37,23 +39,29 @@ public final class UriCompliance implements ComplianceViolation.Mode protected static final Logger LOG = LoggerFactory.getLogger(UriCompliance.class); /** - * These are URI compliance violations, which may be allowed by the compliance mode. Currently all these - * violations are for additional criteria in excess of the strict requirements of rfc3986. + * These are URI compliance "violations", which may be allowed by the compliance mode. These are actual + * violations of the RFC, as they represent additional requirements in excess of the strict compliance of rfc3986. + * A compliance mode that contains one or more of these Violations, allows request to violate the corresponding + * additional requirement. */ public enum Violation implements ComplianceViolation { /** - * Ambiguous path segments e.g. /foo/%2e%2e/bar + * Allow ambiguous path segments e.g. /foo/%2e%2e/bar */ AMBIGUOUS_PATH_SEGMENT("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path segment"), /** - * Ambiguous path separator within a URI segment e.g. /foo/b%2fr + * Allow ambiguous path separator within a URI segment e.g. /foo/b%2fr */ AMBIGUOUS_PATH_SEPARATOR("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path separator"), /** - * Ambiguous path parameters within a URI segment e.g. /foo/..;/bar + * Allow ambiguous path parameters within a URI segment e.g. /foo/..;/bar */ - AMBIGUOUS_PATH_PARAMETER("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path parameter"); + AMBIGUOUS_PATH_PARAMETER("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path parameter"), + /** + * Allow Non canonical ambiguous paths. eg /foo/x%2f%2e%2e%/bar provided to applications as /foo/x/../bar + */ + NON_CANONICAL_AMBIGUOUS_PATHS("https://tools.ietf.org/html/rfc3986#section-3.3", "Non canonical ambiguous paths"); private final String _url; private final String _description; @@ -84,19 +92,28 @@ public final class UriCompliance implements ComplianceViolation.Mode } /** - * The default compliance mode that extends RFC3986 compliance with additional violations to avoid ambiguous URIs + * The default compliance mode that extends RFC3986 compliance with additional violations to avoid most ambiguous URIs. + * This mode does allow {@link Violation#AMBIGUOUS_PATH_SEPARATOR}, but disallows + * {@link Violation#AMBIGUOUS_PATH_PARAMETER} and {@link Violation#AMBIGUOUS_PATH_SEGMENT}. + * Ambiguous paths are not allowed by {@link Violation#NON_CANONICAL_AMBIGUOUS_PATHS}. */ - public static final UriCompliance DEFAULT = new UriCompliance("DEFAULT", noneOf(Violation.class)); + public static final UriCompliance DEFAULT = new UriCompliance("DEFAULT", of(Violation.AMBIGUOUS_PATH_SEPARATOR)); /** - * LEGACY compliance mode that disallows only ambiguous path parameters as per Jetty-9.4 + * LEGACY compliance mode that models Jetty-9.4 behavior by allowing {@link Violation#AMBIGUOUS_PATH_SEGMENT} and {@link Violation#AMBIGUOUS_PATH_SEPARATOR} */ - public static final UriCompliance LEGACY = new UriCompliance("LEGACY", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR)); + public static final UriCompliance LEGACY = new UriCompliance("LEGACY", of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR)); /** - * Compliance mode that exactly follows RFC3986, including allowing all additional ambiguous URI Violations + * Compliance mode that exactly follows RFC3986, including allowing all additional ambiguous URI Violations, + * except {@link Violation#NON_CANONICAL_AMBIGUOUS_PATHS}, thus ambiguous paths are canonicalized for safety. */ - public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", allOf(Violation.class)); + public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", complementOf(of(Violation.NON_CANONICAL_AMBIGUOUS_PATHS))); + + /** + * Compliance mode that allows all URI Violations, including allowing ambiguous paths in non canonicalized form. + */ + public static final UriCompliance UNSAFE = new UriCompliance("UNSAFE", allOf(Violation.class)); /** * @deprecated equivalent to DEFAULT @@ -125,6 +142,17 @@ public final class UriCompliance implements ComplianceViolation.Mode return null; } + /** + * Create compliance set from a set of allowed Violations. + * + * @param violations A string of violations to allow: + * @return the compliance from the string spec + */ + public static UriCompliance from(Set violations) + { + return new UriCompliance("CUSTOM" + __custom.getAndIncrement(), violations); + } + /** * Create compliance set from string. *

@@ -151,22 +179,23 @@ public final class UriCompliance implements ComplianceViolation.Mode */ public static UriCompliance from(String spec) { - Set sections; + Set violations; String[] elements = spec.split("\\s*,\\s*"); switch (elements[0]) { case "0": - sections = noneOf(Violation.class); + violations = noneOf(Violation.class); break; case "*": - sections = allOf(Violation.class); + violations = allOf(Violation.class); break; default: { UriCompliance mode = UriCompliance.valueOf(elements[0]); - sections = (mode == null) ? noneOf(Violation.class) : copyOf(mode.getAllowed()); + violations = (mode == null) ? noneOf(Violation.class) : copyOf(mode.getAllowed()); + break; } } @@ -178,12 +207,12 @@ public final class UriCompliance implements ComplianceViolation.Mode element = element.substring(1); Violation section = Violation.valueOf(element); if (exclude) - sections.remove(section); + violations.remove(section); else - sections.add(section); + violations.add(section); } - UriCompliance compliance = new UriCompliance("CUSTOM" + __custom.getAndIncrement(), sections); + UriCompliance compliance = new UriCompliance("CUSTOM" + __custom.getAndIncrement(), violations); if (LOG.isDebugEnabled()) LOG.debug("UriCompliance from {}->{}", spec, compliance); return compliance; @@ -192,7 +221,7 @@ public final class UriCompliance implements ComplianceViolation.Mode private final String _name; private final Set _allowed; - private UriCompliance(String name, Set violations) + public UriCompliance(String name, Set violations) { Objects.requireNonNull(violations); _name = name; 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 27bf9d2e3b7..95866fb9910 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 @@ -67,7 +67,6 @@ import javax.servlet.http.WebConnection; import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HostPortHttpField; -import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField; import org.eclipse.jetty.http.HttpField; @@ -1692,10 +1691,11 @@ public class Request implements HttpServletRequest _httpFields = request.getFields(); final HttpURI uri = request.getURI(); + UriCompliance compliance = null; boolean ambiguous = uri.isAmbiguous(); if (ambiguous) { - UriCompliance compliance = _channel == null || _channel.getHttpConfiguration() == null ? null : _channel.getHttpConfiguration().getUriCompliance(); + compliance = _channel == null || _channel.getHttpConfiguration() == null ? null : _channel.getHttpConfiguration().getUriCompliance(); if (uri.hasAmbiguousSegment() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT))) throw new BadMessageException("Ambiguous segment in URI"); if (uri.hasAmbiguousSeparator() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR))) @@ -1746,9 +1746,9 @@ public class Request implements HttpServletRequest path = (encoded.length() == 1) ? "/" : _uri.getDecodedPath(); // Strictly speaking if a URI is legal and encodes ambiguous segments, then they should be // reflected in the decoded string version. However, it can be ambiguous to provide a decoded path as - // a string, so we normalize again. If an application wishes to see ambiguous URIs, then they can look - // at the encoded form of the URI - if (ambiguous) + // a string, so we normalize again. If an application wishes to see ambiguous URIs, then they must + // set the {@link UriCompliance.Violation#NON_CANONICAL_AMBIGUOUS_PATHS} compliance. + if (ambiguous && (compliance == null || !compliance.allows(UriCompliance.Violation.NON_CANONICAL_AMBIGUOUS_PATHS))) path = URIUtil.canonicalPath(path); } else if ("*".equals(encoded) || HttpMethod.CONNECT.is(getMethod())) 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 28a736d5b72..2661e609e23 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 @@ -28,6 +28,7 @@ import java.security.Principal; import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; +import java.util.EnumSet; import java.util.Enumeration; import java.util.List; import java.util.Locale; @@ -1733,12 +1734,45 @@ public class RequestTest "Host: whatever\r\n" + "\r\n"; _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.DEFAULT); + assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); + _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(new UriCompliance("Test", EnumSet.noneOf(UriCompliance.Violation.class))); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400")); _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.LEGACY); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.RFC3986); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); } + + @Test + public void testAmbiguousPaths() throws Exception + { + _handler._checker = (request, response) -> + { + response.getOutputStream().println("servletPath=" + request.getServletPath()); + response.getOutputStream().println("pathInfo=" + request.getPathInfo()); + return true; + }; + String request = "GET /unnormal/.././path/ambiguous%2f%2e%2e/%2e;/info HTTP/1.0\r\n" + + "Host: whatever\r\n" + + "\r\n"; + + _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.from(EnumSet.of( + UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR, + UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT, + UriCompliance.Violation.AMBIGUOUS_PATH_PARAMETER))); + assertThat(_connector.getResponse(request), Matchers.allOf( + startsWith("HTTP/1.1 200"), + containsString("pathInfo=/path/info"))); + + _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.from(EnumSet.of( + UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR, + UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT, + UriCompliance.Violation.AMBIGUOUS_PATH_PARAMETER, + UriCompliance.Violation.NON_CANONICAL_AMBIGUOUS_PATHS))); + assertThat(_connector.getResponse(request), Matchers.allOf( + startsWith("HTTP/1.1 200"), + containsString("pathInfo=/path/ambiguous/.././info"))); + } @Test public void testPushBuilder()