Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #4275 #6001 separate compliance modes for ambiguous URI segments and se… #6003

Merged
merged 3 commits into from
Feb 24, 2021
Merged

Fix #4275 #6001 separate compliance modes for ambiguous URI segments and se… #6003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68988c6
Fix #4275 separate compliance modes for ambiguous URI segments and se…
gregw Feb 23, 2021
9c9da67
Fix #4275 separate compliance modes for ambiguous URI segments and se…
gregw Feb 23, 2021
a24e27f
Fix #4275 separate compliance modes for ambiguous URI segments and se…
gregw Feb 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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