Skip to content

Commit

Permalink
Fix #4275 #6001 separate compliance modes for ambiguous URI segments …
Browse files Browse the repository at this point in the history 
…and se… (#6003)

Fix #4275 separate compliance modes for ambiguous URI segments and separators
  • Loading branch information
@gregw
gregw authored Feb 24, 2021
1 parent c9cd1e4 commit 49e73df
Show file tree
Hide file tree
Showing 6 changed files with 124 additions and 74 deletions.
19 changes: 8 additions & 11 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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<HttpComplianceSection> sectionsByProperty(String property)
{
String s = System.getProperty(HttpCompliance.class.getName() + property);
Expand Down
3 changes: 2 additions & 1 deletion jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
39 changes: 32 additions & 7 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -68,6 +69,12 @@ private enum State
ASTERISK
}

enum Ambiguous
{
SEGMENT,
SEPARATOR
}

/**
* The concept of URI path parameters was originally specified in
* <a href="https://tools.ietf.org/html/rfc2396#section-3.3">RFC2396</a>, but that was
Expand Down Expand Up @@ -102,7 +109,7 @@ private enum State
private String _fragment;
private String _uri;
private String _decodedPath;
private boolean _ambiguousSegment;
private final EnumSet<Ambiguous> _ambiguous = EnumSet.noneOf(Ambiguous.class);

/**
* Construct a normalized URI.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -204,7 +211,7 @@ public void clear()
_query = null;
_fragment = null;
_decodedPath = null;
_ambiguousSegment = false;
_ambiguous.clear();
}

public void parse(String uri)
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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()
Expand Down
98 changes: 51 additions & 47 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,73 +285,77 @@ public static Stream<Arguments> 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)
{
Expand Down
14 changes: 8 additions & 6 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
25 changes: 23 additions & 2 deletions jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1837,7 +1837,7 @@ public void testGetterSafeFromNullPointerException()
}

@Test
public void testAmbiguousPaths() throws Exception
public void testAmbiguousSegments() throws Exception
{
_handler._checker = (request, response) -> true;

Expand All @@ -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<Path> s = Files.list(path))
Expand Down Expand Up @@ -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);

Expand Down

0 comments on commit 49e73df

Please sign in to comment.