Skip to content

Commit

Permalink
Improve #4275 ambiguous URIs
Browse files Browse the repository at this point in the history
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.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
  • Loading branch information
gregw committed Sep 28, 2021
1 parent d38ed4b commit 53818e5
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 15 deletions.
17 changes: 8 additions & 9 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
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 (ambiguous == Boolean.TRUE)
_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
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

0 comments on commit 53818e5

Please sign in to comment.