UriCompliance mode improvements #6132 (#6137)

Resolve #6132

Improve configuration of ambiguous URI handling.
Added NON_CANONICAL_AMBIGUOUS_PATHS
This commit is contained in:
Greg Wilkins 2021-04-08 12:03:30 +10:00 committed by GitHub
parent e3c87fc2af
commit b56edf511a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 88 additions and 25 deletions

View File

@ -25,7 +25,9 @@ import org.slf4j.LoggerFactory;
import static java.util.Arrays.asList; import static java.util.Arrays.asList;
import static java.util.Collections.unmodifiableSet; import static java.util.Collections.unmodifiableSet;
import static java.util.EnumSet.allOf; import static java.util.EnumSet.allOf;
import static java.util.EnumSet.complementOf;
import static java.util.EnumSet.noneOf; import static java.util.EnumSet.noneOf;
import static java.util.EnumSet.of;
/** /**
* URI compliance modes for Jetty request handling. * 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); 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 * These are URI compliance "violations", which may be allowed by the compliance mode. These are actual
* violations are for additional criteria in excess of the strict requirements of rfc3986. * 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 public enum Violation implements ComplianceViolation
{ {
/** /**
* Ambiguous path segments e.g. <code>/foo/%2e%2e/bar</code> * Allow ambiguous path segments e.g. <code>/foo/%2e%2e/bar</code>
*/ */
AMBIGUOUS_PATH_SEGMENT("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path segment"), 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. <code>/foo/b%2fr</code> * Allow ambiguous path separator within a URI segment e.g. <code>/foo/b%2fr</code>
*/ */
AMBIGUOUS_PATH_SEPARATOR("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path separator"), 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. <code>/foo/..;/bar</code> * Allow ambiguous path parameters within a URI segment e.g. <code>/foo/..;/bar</code>
*/ */
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 <code>/foo/x%2f%2e%2e%/bar</code> provided to applications as <code>/foo/x/../bar</code>
*/
NON_CANONICAL_AMBIGUOUS_PATHS("https://tools.ietf.org/html/rfc3986#section-3.3", "Non canonical ambiguous paths");
private final String _url; private final String _url;
private final String _description; 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 * @deprecated equivalent to DEFAULT
@ -125,6 +142,17 @@ public final class UriCompliance implements ComplianceViolation.Mode
return null; 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<Violation> violations)
{
return new UriCompliance("CUSTOM" + __custom.getAndIncrement(), violations);
}
/** /**
* Create compliance set from string. * Create compliance set from string.
* <p> * <p>
@ -151,22 +179,23 @@ public final class UriCompliance implements ComplianceViolation.Mode
*/ */
public static UriCompliance from(String spec) public static UriCompliance from(String spec)
{ {
Set<Violation> sections; Set<Violation> violations;
String[] elements = spec.split("\\s*,\\s*"); String[] elements = spec.split("\\s*,\\s*");
switch (elements[0]) switch (elements[0])
{ {
case "0": case "0":
sections = noneOf(Violation.class); violations = noneOf(Violation.class);
break; break;
case "*": case "*":
sections = allOf(Violation.class); violations = allOf(Violation.class);
break; break;
default: default:
{ {
UriCompliance mode = UriCompliance.valueOf(elements[0]); 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); element = element.substring(1);
Violation section = Violation.valueOf(element); Violation section = Violation.valueOf(element);
if (exclude) if (exclude)
sections.remove(section); violations.remove(section);
else 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()) if (LOG.isDebugEnabled())
LOG.debug("UriCompliance from {}->{}", spec, compliance); LOG.debug("UriCompliance from {}->{}", spec, compliance);
return compliance; return compliance;
@ -192,7 +221,7 @@ public final class UriCompliance implements ComplianceViolation.Mode
private final String _name; private final String _name;
private final Set<Violation> _allowed; private final Set<Violation> _allowed;
private UriCompliance(String name, Set<Violation> violations) public UriCompliance(String name, Set<Violation> violations)
{ {
Objects.requireNonNull(violations); Objects.requireNonNull(violations);
_name = name; _name = name;

View File

@ -67,7 +67,6 @@ import javax.servlet.http.WebConnection;
import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.ComplianceViolation;
import org.eclipse.jetty.http.HostPortHttpField; import org.eclipse.jetty.http.HostPortHttpField;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField; import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField;
import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpField;
@ -1692,10 +1691,11 @@ public class Request implements HttpServletRequest
_httpFields = request.getFields(); _httpFields = request.getFields();
final HttpURI uri = request.getURI(); final HttpURI uri = request.getURI();
UriCompliance compliance = null;
boolean ambiguous = uri.isAmbiguous(); boolean ambiguous = uri.isAmbiguous();
if (ambiguous) 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))) if (uri.hasAmbiguousSegment() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT)))
throw new BadMessageException("Ambiguous segment in URI"); throw new BadMessageException("Ambiguous segment in URI");
if (uri.hasAmbiguousSeparator() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR))) 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(); path = (encoded.length() == 1) ? "/" : _uri.getDecodedPath();
// Strictly speaking if a URI is legal and encodes ambiguous segments, then they should be // 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 // 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 // a string, so we normalize again. If an application wishes to see ambiguous URIs, then they must
// at the encoded form of the URI // set the {@link UriCompliance.Violation#NON_CANONICAL_AMBIGUOUS_PATHS} compliance.
if (ambiguous) if (ambiguous && (compliance == null || !compliance.allows(UriCompliance.Violation.NON_CANONICAL_AMBIGUOUS_PATHS)))
path = URIUtil.canonicalPath(path); path = URIUtil.canonicalPath(path);
} }
else if ("*".equals(encoded) || HttpMethod.CONNECT.is(getMethod())) else if ("*".equals(encoded) || HttpMethod.CONNECT.is(getMethod()))

View File

@ -28,6 +28,7 @@ import java.security.Principal;
import java.time.Duration; import java.time.Duration;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Arrays; import java.util.Arrays;
import java.util.EnumSet;
import java.util.Enumeration; import java.util.Enumeration;
import java.util.List; import java.util.List;
import java.util.Locale; import java.util.Locale;
@ -1733,12 +1734,45 @@ public class RequestTest
"Host: whatever\r\n" + "Host: whatever\r\n" +
"\r\n"; "\r\n";
_connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.DEFAULT); _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")); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400"));
_connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.LEGACY); _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.LEGACY);
assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200"));
_connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.RFC3986); _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.RFC3986);
assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200")); 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 @Test
public void testPushBuilder() public void testPushBuilder()