Skip to content

Commit

Permalink
UriCompliance mode improvements #6132 (#6137)
Browse files Browse the repository at this point in the history 
Resolve #6132

Improve configuration of ambiguous URI handling.
Added NON_CANONICAL_AMBIGUOUS_PATHS
  • Loading branch information
@gregw
gregw committed Apr 8, 2021
1 parent e3c87fc commit b56edf5
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 25 deletions.
69 changes: 49 additions & 20 deletions jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
View file
Expand Up @@ -25,7 +25,9 @@
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.
Expand All @@ -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. <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 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 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 _description;
Expand Down Expand Up @@ -84,19 +92,28 @@ public String getDescription()
}

/**
* 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
Expand Down Expand Up @@ -125,6 +142,17 @@ public static UriCompliance valueOf(String name)
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.
* <p>
Expand All @@ -151,22 +179,23 @@ public static UriCompliance valueOf(String name)
*/
public static UriCompliance from(String spec)
{
Set<Violation> sections;
Set<Violation> 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;
}
}

Expand All @@ -178,12 +207,12 @@ public static UriCompliance from(String spec)
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;
Expand All @@ -192,7 +221,7 @@ public static UriCompliance from(String spec)
private final String _name;
private final Set<Violation> _allowed;

private UriCompliance(String name, Set<Violation> violations)
public UriCompliance(String name, Set<Violation> violations)
{
Objects.requireNonNull(violations);
_name = name;
Expand Down
10 changes: 5 additions & 5 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Expand Up @@ -67,7 +67,6 @@
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;
Expand Down Expand Up @@ -1692,10 +1691,11 @@ public void setMetaData(MetaData.Request request)
_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)))
Expand Down Expand Up @@ -1746,9 +1746,9 @@ else if (encoded.startsWith("/"))
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()))
Expand Down
34 changes: 34 additions & 0 deletions jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
View file
Expand Up @@ -28,6 +28,7 @@
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;
Expand Down Expand Up @@ -1733,12 +1734,45 @@ public void testAmbiguousSeparators() throws Exception
"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()
Expand Down

0 comments on commit b56edf5

Please sign in to comment.