Skip to content

Commit

Permalink
Improve #4275 ambiguous URIs (#6939)
Browse files Browse the repository at this point in the history 
* Improve #4275 ambiguous URIs

A URI like `/foo/%2e%2e;/bar` should be ambiguous both because of the encoded dots and because of the parameters.  This means that the default setting of jetty-9 is a bit more secure as this path is considered ambiguous if either Violation.SEGMENT or Violation.PARAM is set.
  • Loading branch information
@gregw
gregw authored Oct 11, 2021
1 parent 479d40b commit 3f82d69
Show file tree
Hide file tree
Showing 6 changed files with 23 additions and 23 deletions.
2 changes: 1 addition & 1 deletion jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ else if (contentLength != field.getLongValue())
close = true;
_persistent = false;
}
if (keepAlive && _persistent == Boolean.FALSE)
if (keepAlive && Boolean.FALSE.equals(_persistent))
{
field = new HttpField(HttpHeader.CONNECTION,
Stream.of(field.getValues()).filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))
Expand Down
17 changes: 8 additions & 9 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 @@ -108,7 +108,7 @@ enum Violation
*/
SEPARATOR("Ambiguous path separator"),
/**
* Ambiguous path parameters within a URI segment e.g. {@code /foo/..;/bar}
* Ambiguous path parameters within a URI segment e.g. {@code /foo/..;/bar} or {@code /foo/%2e%2e;param/bar}
*/
PARAM("Ambiguous path parameters"),
/**
Expand Down Expand Up @@ -782,15 +782,14 @@ private void checkSegment(String uri, int segment, int end, boolean param)

// Look for segment in the ambiguous segment index.
Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment);
if (ambiguous == Boolean.TRUE)
if (ambiguous != null)
{
// The segment is always ambiguous.
_violations.add(Violation.SEGMENT);
}
else if (param && ambiguous == Boolean.FALSE)
{
// The segment is ambiguous only when followed by a parameter.
_violations.add(Violation.PARAM);
// Is the segment intrinsically ambiguous
if (Boolean.TRUE.equals(ambiguous))
_violations.add(Violation.SEGMENT);
// Is the segment ambiguous with a parameter?
if (param)
_violations.add(Violation.PARAM);
}
}

Expand Down
12 changes: 6 additions & 6 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 @@ -358,9 +358,9 @@ public static Stream<Arguments> decodePathTests()
{"/path/%2e/info", "/path/info", EnumSet.of(Violation.SEGMENT)},
{"path/%2e/info/", "path/info/", EnumSet.of(Violation.SEGMENT)},
{"/path/%2e%2e/info", "/info", EnumSet.of(Violation.SEGMENT)},
{"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.SEGMENT)},
{"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.SEGMENT)},
{"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.SEGMENT)},
{"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.SEGMENT, Violation.PARAM)},
{"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.SEGMENT, Violation.PARAM)},
{"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.SEGMENT, Violation.PARAM)},
{"%2e/info", "info", EnumSet.of(Violation.SEGMENT)},
{"%u002e/info", "info", EnumSet.of(Violation.SEGMENT, Violation.UTF16)},

Expand Down Expand Up @@ -473,9 +473,9 @@ public static Stream<Arguments> testPathQueryTests()
{"/path/%2e/info", "/path/info", EnumSet.of(Violation.SEGMENT)},
{"path/%2e/info/", "path/info/", EnumSet.of(Violation.SEGMENT)},
{"/path/%2e%2e/info", "/info", EnumSet.of(Violation.SEGMENT)},
{"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.SEGMENT)},
{"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.SEGMENT)},
{"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.SEGMENT)},
{"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.SEGMENT, Violation.PARAM)},
{"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.SEGMENT, Violation.PARAM)},
{"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.SEGMENT, Violation.PARAM)},
{"%2e/info", "info", EnumSet.of(Violation.SEGMENT)},
{"%2e", "", EnumSet.of(Violation.SEGMENT)},

Expand Down
5 changes: 3 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 @@ -1947,13 +1947,14 @@ public void setComplianceModes(HttpCompliance compliance, HttpComplianceSection.
@Test
public void testAmbiguousPaths() throws Exception
{
// TODO this is a poor test as it is not very exhaustive. See HttpURITest for better tests.
_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" +
String request = "GET /unnormal/.././path/ambiguous%2f%2e%2e/info HTTP/1.0\r\n" +
"Host: whatever\r\n" +
"\r\n";

Expand All @@ -1962,7 +1963,7 @@ public void testAmbiguousPaths() throws Exception
startsWith("HTTP/1.1 200"),
containsString("pathInfo=/path/info")));

setComplianceModes(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS);
setComplianceModes(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEPARATORS);
assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400"));
}

Expand Down
4 changes: 2 additions & 2 deletions jetty-util/src/main/java/org/eclipse/jetty/util/IncludeExcludeSet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,12 +259,12 @@ public static <T1, T2> boolean matchCombined(T1 item1, IncludeExcludeSet<?, T1>
Boolean match2 = set2.isIncludedAndNotExcluded(item2);

// if we are excluded from either set, then we do not match
if (match1 == Boolean.FALSE || match2 == Boolean.FALSE)
if (Boolean.FALSE.equals(match1) || Boolean.FALSE.equals(match2))
return false;

// If either set has any includes, then we must be included by one of them
if (set1.hasIncludes() || set2.hasIncludes())
return match1 == Boolean.TRUE || match2 == Boolean.TRUE;
return Boolean.TRUE.equals(match1) || Boolean.TRUE.equals(match2);

// If not excluded and no includes, then we match
return true;
Expand Down
6 changes: 3 additions & 3 deletions jetty-webapp/src/main/java/org/eclipse/jetty/webapp/ClasspathPattern.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -742,20 +742,20 @@ static boolean combine(IncludeExcludeSet<Entry, String> names, String name, Incl
Boolean byName = names.isIncludedAndNotExcluded(name);

// If we excluded by name, then no match
if (Boolean.FALSE == byName)
if (Boolean.FALSE.equals(byName))
return false;

// check the location set
URI uri = location.get();
Boolean byLocation = uri == null ? null : locations.isIncludedAndNotExcluded(uri);

// If we excluded by location or couldn't check location exclusion, then no match
if (Boolean.FALSE == byLocation || (locations.hasExcludes() && uri == null))
if (Boolean.FALSE.equals(byLocation) || (locations.hasExcludes() && uri == null))
return false;

// If there are includes, then we must be included to match.
if (names.hasIncludes() || locations.hasIncludes())
return byName == Boolean.TRUE || byLocation == Boolean.TRUE;
return Boolean.TRUE.equals(byName) || Boolean.TRUE.equals(byLocation);

// Otherwise there are no includes and it was not excluded, so match
return true;
Expand Down

0 comments on commit 3f82d69

Please sign in to comment.