From 866f4517dbaa977e7eb2357df93c4d6eebefe4c1 Mon Sep 17 00:00:00 2001
From: Lachlan Roberts <lachlan@webtide.com>
Date: Tue, 12 Oct 2021 18:21:59 +1100
Subject: [PATCH] Improve #4275 ambiguous URIs (#6939)

* 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.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
---
 .../main/java/org/eclipse/jetty/http/HttpURI.java    | 11 +++++------
 .../java/org/eclipse/jetty/http/UriCompliance.java   |  2 +-
 .../java/org/eclipse/jetty/http/HttpURITest.java     | 12 ++++++------
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
index 1294874db368..d9e4d8bdbe1f 100644
--- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
+++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
@@ -1414,15 +1414,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.AMBIGUOUS_PATH_SEGMENT);
-            }
-            else if (param && ambiguous == Boolean.FALSE)
-            {
+                if (Boolean.TRUE.equals(ambiguous))
+                    _violations.add(Violation.AMBIGUOUS_PATH_SEGMENT);
                 // The segment is ambiguous only when followed by a parameter.
-                _violations.add(Violation.AMBIGUOUS_PATH_PARAMETER);
+                if (param)
+                    _violations.add(Violation.AMBIGUOUS_PATH_PARAMETER);
             }
         }
     }
diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
index 8baef6b1ac55..76cb2ee57497 100644
--- a/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
+++ b/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
@@ -59,7 +59,7 @@ public enum Violation implements ComplianceViolation
          */
         AMBIGUOUS_PATH_SEPARATOR("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path separator"),
         /**
-         * Allow 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> or <code>/foo/%2e%2e;param/bar</code>
          */
         AMBIGUOUS_PATH_PARAMETER("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path parameter"),
         /**
diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
index 83701d915624..13601ede56e4 100644
--- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
+++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
@@ -395,9 +395,9 @@ public static Stream<Arguments> decodePathTests()
                 {"/path/%2e/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
                 {"path/%2e/info/", "path/info/", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
                 {"/path/%2e%2e/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
-                {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
-                {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
-                {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
+                {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_PARAMETER)},
+                {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_PARAMETER)},
+                {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_PARAMETER)},
                 {"%2e/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
                 {"%u002e/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.UTF16_ENCODINGS)},
 
@@ -510,9 +510,9 @@ public static Stream<Arguments> testPathQueryTests()
                 {"/path/%2e/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
                 {"path/%2e/info/", "path/info/", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
                 {"/path/%2e%2e/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
-                {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
-                {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
-                {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
+                {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_PARAMETER)},
+                {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_PARAMETER)},
+                {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_PARAMETER)},
                 {"%2e/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},
                 {"%2e", "", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)},