Issue #6473 - Improve alias checking in PathResource. (#6477)

* Issue #6473 - Improve alias checking in PathResource.

* Reverted %-escape handling for URI query parts.
* Performing canonicalization in ServletContext.getResource(),
  and improving alias checking in ContextHandler.getResource().
* Performing canonicalization checks in Resource.addPath() to avoid
  navigation above of the root.
* Test added and fixed.
* Various cleanups.
* Improved javadoc and comments

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Co-authored-by: Greg Wilkins <gregw@webtide.com>

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

@@ -36,7 +36,7 @@ import org.eclipse.jetty.util.UrlEncoded;
 /**
  * Http URI.
  * Parse an HTTP URI from a string or byte array.  Given a URI
- * <code>http://user@host:port/path;param1/%2e/info;param2?query#fragment</code>
+ * {@code http://user@host:port/path;param1/%2e/info;param2?query#fragment}
  * this class will split it into the following optional elements:<ul>
  * <li>{@link #getScheme()} - http:</li>
  * <li>{@link #getAuthority()} - //name@host:port</li>
@@ -65,11 +65,13 @@ import org.eclipse.jetty.util.UrlEncoded;
  * Thus this class avoid and/or detects such ambiguities. Furthermore, by decoding characters and
  * removing parameters before relative path normalization, ambiguous paths will be resolved in such
  * a way to be non-standard-but-non-ambiguous to down stream interpretation of the decoded path string.
- * The violations are recorded and available by API such as {@link #hasViolation(Violation)} so that requests
+ * The violations are recorded and available by API such as {@link #hasAmbiguousSegment()} so that requests
  * containing them may be rejected in case the non-standard-but-non-ambiguous interpretations
- * are not satisfactory for a given compliance configuration.   Implementations that wish to
+ * are not satisfactory for a given compliance configuration.
- * process ambiguous URI paths must configure the compliance modes to accept them and then perform
+ * </p>
- * their own decoding of {@link #getPath()}.
+ * <p>
+ * Implementations that wish to process ambiguous URI paths must configure the compliance modes
+ * to accept them and then perform their own decoding of {@link #getPath()}.
  * </p>
  * <p>
  * If there are multiple path parameters, only the last one is returned by {@link #getParam()}.
@@ -95,30 +97,30 @@ public class HttpURI
     /**
      * Violations of safe URI interpretations
      */
-    public enum Violation
+    enum Violation
     {
         /**
-         * Ambiguous path segments e.g. <code>/foo/%2E%2E/bar</code>
+         * Ambiguous path segments e.g. {@code /foo/%2E%2E/bar}
          */
         SEGMENT("Ambiguous path segments"),
         /**
-         * Ambiguous path separator within a URI segment e.g. <code>/foo%2Fbar</code>
+         * Ambiguous path separator within a URI segment e.g. {@code /foo%2Fbar}
          */
         SEPARATOR("Ambiguous path separator"),
         /**
-         * Ambiguous path parameters within a URI segment e.g. <code>/foo/..;/bar</code>
+         * Ambiguous path parameters within a URI segment e.g. {@code /foo/..;/bar}
          */
         PARAM("Ambiguous path parameters"),
         /**
-         * Ambiguous double encoding within a URI segment e.g. <code>/%2557EB-INF</code>
+         * Ambiguous double encoding within a URI segment e.g. {@code /%2557EB-INF}
          */
         ENCODING("Ambiguous double encoding"),
         /**
-         * Ambiguous empty segments e.g. <code>/foo//bar</code>
+         * Ambiguous empty segments e.g. {@code /foo//bar}
          */
         EMPTY("Ambiguous empty segments"),
         /**
-         * Non standard UTF-16 encoding eg <code>/foo%u2192bar</code>.
+         * Non standard UTF-16 encoding eg {@code /foo%u2192bar}.
          */
         UTF16("Non standard UTF-16 encoding");
 
@@ -338,7 +340,7 @@ public class HttpURI
         int encodedValue = 0; // the partial encoded value
         boolean dot = false; // set to true if the path contains . or .. segments
 
-        for (int i = 0; i < end; i++)
+        for (int i = offset; i < end; i++)
         {
             char c = uri.charAt(i);
 
@@ -567,6 +569,8 @@ public class HttpURI
                             switch (encodedValue)
                             {
                                 case 0:
+                                    // Byte 0 cannot be present in a UTF-8 sequence in any position
+                                    // other than as the NUL ASCII byte which we do not wish to allow.
                                     throw new IllegalArgumentException("Illegal character in path");
                                 case '/':
                                     _violations.add(Violation.SEPARATOR);
@@ -653,26 +657,11 @@ public class HttpURI
                 }
                 case QUERY:
                 {
-                    switch (c)
+                    if (c == '#')
                     {
-                        case '%':
+                        _query = uri.substring(mark, i);
-                            encodedCharacters = 2;
+                        mark = i + 1;
-                            break;
+                        state = State.FRAGMENT;
-                        case 'u':
-                        case 'U':
-                            if (encodedCharacters == 1)
-                                _violations.add(Violation.UTF16);
-                            encodedCharacters = 0;
-                            break;
-                        case '#':
-                            _query = uri.substring(mark, i);
-                            mark = i + 1;
-                            state = State.FRAGMENT;
-                            encodedCharacters = 0;
-                            break;
-                        default:
-                            encodedCharacters = 0;
-                            break;
                     }
                     break;
                 }
@@ -687,7 +676,9 @@ public class HttpURI
                     break;
                 }
                 default:
+                {
                     throw new IllegalStateException(state.toString());
+                }
             }
         }
 
@@ -741,8 +732,8 @@ public class HttpURI
         {
             // The RFC requires this to be canonical before decoding, but this can leave dot segments and dot dot segments
             // which are not canonicalized and could be used in an attempt to bypass security checks.
-            String decodeNonCanonical = URIUtil.decodePath(_path);
+            String decodedNonCanonical = URIUtil.decodePath(_path);
-            _decodedPath = URIUtil.canonicalPath(decodeNonCanonical);
+            _decodedPath = URIUtil.canonicalPath(decodedNonCanonical);
             if (_decodedPath == null)
                 throw new IllegalArgumentException("Bad URI");
         }
@@ -763,7 +754,8 @@ public class HttpURI
         // This method is called once for every segment parsed.
         // A URI like "/foo/" has two segments: "foo" and an empty segment.
         // Empty segments are only ambiguous if they are not the last segment
-        // So if this method is called for any segment and we have previously seen an empty segment, then it was ambiguous
+        // So if this method is called for any segment and we have previously
+        // seen an empty segment, then it was ambiguous.
         if (_emptySegment)
             _violations.add(Violation.EMPTY);
 
@@ -843,7 +835,8 @@ public class HttpURI
     }
 
     /**
-     * @return True if the URI has either an {@link #hasAmbiguousSegment()} or {@link #hasAmbiguousSeparator()}.
+     * @return True if the URI has either an {@link #hasAmbiguousSegment()} or {@link #hasAmbiguousEmptySegment()}
+     * or {@link #hasAmbiguousSeparator()} or {@link #hasAmbiguousParameter()}
      */
     public boolean isAmbiguous()
     {
@@ -858,7 +851,7 @@ public class HttpURI
         return !_violations.isEmpty();
     }
 
-    public boolean hasViolation(Violation violation)
+    boolean hasViolation(Violation violation)
     {
         return _violations.contains(violation);
     }

jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java

@@ -322,6 +322,7 @@ public class HttpURITest
                 {"/f%6f%6F/bar", "/foo/bar", EnumSet.noneOf(Violation.class)},
                 {"/f%u006f%u006F/bar", "/foo/bar", EnumSet.of(Violation.UTF16)},
                 {"/f%u0001%u0001/bar", "/f\001\001/bar", EnumSet.of(Violation.UTF16)},
+                {"/foo/%u20AC/bar", "/foo/\u20AC/bar", EnumSet.of(Violation.UTF16)},
 
                 // illegal paths
                 {"//host/../path/info", null, EnumSet.noneOf(Violation.class)},
@@ -333,6 +334,9 @@ public class HttpURITest
                 {"/path/%u000X/info", null, EnumSet.noneOf(Violation.class)},
                 {"/path/Fo%u0000/info", null, EnumSet.noneOf(Violation.class)},
                 {"/path/Fo%00/info", null, EnumSet.noneOf(Violation.class)},
+                {"/path/Foo/info%u0000", null, EnumSet.noneOf(Violation.class)},
+                {"/path/Foo/info%00", null, EnumSet.noneOf(Violation.class)},
+                {"/path/%U20AC", null, EnumSet.noneOf(Violation.class)},
                 {"%2e%2e/info", null, EnumSet.noneOf(Violation.class)},
                 {"%u002e%u002e/info", null, EnumSet.noneOf(Violation.class)},
                 {"%2e%2e;/info", null, EnumSet.noneOf(Violation.class)},
@@ -769,4 +773,20 @@ public class HttpURITest
         assertThat("[" + input + "] .fragment", httpUri.getFragment(), is(javaUri.getFragment()));
         assertThat("[" + input + "] .toString", httpUri.toString(), is(javaUri.toASCIIString()));
     }
+
+    public static Stream<Arguments> queryData()
+    {
+        return Stream.of(
+            new String[]{"/path?p=%U20AC", "p=%U20AC"},
+            new String[]{"/path?p=%u20AC", "p=%u20AC"}
+        ).map(Arguments::of);
+    }
+
+    @ParameterizedTest
+    @MethodSource("queryData")
+    public void testEncodedQuery(String input, String expectedQuery)
+    {
+        HttpURI httpURI = new HttpURI(input);
+        assertThat("[" + input + "] .query", httpURI.getQuery(), is(expectedQuery));
+    }
 }

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyWebAppContext.java

@@ -39,6 +39,7 @@ import org.eclipse.jetty.servlet.FilterMapping;
 import org.eclipse.jetty.servlet.ServletHolder;
 import org.eclipse.jetty.servlet.ServletMapping;
 import org.eclipse.jetty.util.StringUtil;
+import org.eclipse.jetty.util.URIUtil;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 import org.eclipse.jetty.util.resource.Resource;
@@ -450,12 +451,17 @@ public class JettyWebAppContext extends WebAppContext
         // If no regular resource exists check for access to /WEB-INF/lib or /WEB-INF/classes
         if ((resource == null || !resource.exists()) && uriInContext != null && _classes != null)
         {
+            // Canonicalize again to look for the resource inside /WEB-INF subdirectories.
+            String uri = URIUtil.canonicalPath(uriInContext);
+            if (uri == null)
+                return null;
+
             try
             {
                 // Replace /WEB-INF/classes with candidates for the classpath
-                if (uriInContext.startsWith(WEB_INF_CLASSES_PREFIX))
+                if (uri.startsWith(WEB_INF_CLASSES_PREFIX))
                 {
-                    if (uriInContext.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX) || uriInContext.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX + "/"))
+                    if (uri.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX) || uri.equalsIgnoreCase(WEB_INF_CLASSES_PREFIX + "/"))
                     {
                         //exact match for a WEB-INF/classes, so preferentially return the resource matching the web-inf classes
                         //rather than the test classes
@@ -471,7 +477,7 @@ public class JettyWebAppContext extends WebAppContext
                         int i = 0;
                         while (res == null && (i < _webInfClasses.size()))
                         {
-                            String newPath = StringUtil.replace(uriInContext, WEB_INF_CLASSES_PREFIX, _webInfClasses.get(i).getPath());
+                            String newPath = StringUtil.replace(uri, WEB_INF_CLASSES_PREFIX, _webInfClasses.get(i).getPath());
                             res = Resource.newResource(newPath);
                             if (!res.exists())
                             {
@@ -482,11 +488,11 @@ public class JettyWebAppContext extends WebAppContext
                         return res;
                     }
                 }
-                else if (uriInContext.startsWith(WEB_INF_LIB_PREFIX))
+                else if (uri.startsWith(WEB_INF_LIB_PREFIX))
                 {
                     // Return the real jar file for all accesses to
                     // /WEB-INF/lib/*.jar
-                    String jarName = StringUtil.strip(uriInContext, WEB_INF_LIB_PREFIX);
+                    String jarName = StringUtil.strip(uri, WEB_INF_LIB_PREFIX);
                     if (jarName.startsWith("/") || jarName.startsWith("\\"))
                         jarName = jarName.substring(1);
                     if (jarName.length() == 0)

jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java

@@ -53,12 +53,12 @@ public final class RedirectUtil
                 String path = request.getRequestURI();
                 String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path);
                 location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location));
-                if (!location.startsWith("/"))
+                if (location != null && !location.startsWith("/"))
                     url.append('/');
             }
 
             if (location == null)
-                throw new IllegalStateException("path cannot be above root");
+                throw new IllegalStateException("redirect path cannot be above root");
             url.append(location);
 
             location = url.toString();

jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java

@@ -24,6 +24,7 @@ import org.junit.jupiter.api.Test;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 @SuppressWarnings("unused")
@@ -75,6 +76,12 @@ public class ValidUrlRuleTest extends AbstractRuleTestCase
 
     @Test
     public void testInvalidJsp() throws Exception
+    {
+        assertThrows(IllegalArgumentException.class, () -> _request.setURIPathQuery("/jsp/bean1.jsp%00"));
+    }
+
+    @Test
+    public void testInvalidJspWithNullByte() throws Exception
     {
         _rule.setCode("405");
         _rule.setReason("foo");
@@ -87,6 +94,12 @@ public class ValidUrlRuleTest extends AbstractRuleTestCase
         assertEquals("foo", _response.getReason());
     }
 
+    @Test
+    public void testInvalidShamrock() throws Exception
+    {
+        assertThrows(IllegalArgumentException.class, () -> _request.setURIPathQuery("/jsp/shamrock-%00%E2%98%98.jsp"));
+    }
+
     @Test
     public void testValidShamrock() throws Exception
     {
@@ -110,4 +123,3 @@ public class ValidUrlRuleTest extends AbstractRuleTestCase
         //@checkstyle-enable-check : IllegalTokenText
     }
 }
-

jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java

@@ -1942,13 +1942,11 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
         if (_baseResource == null)
             return null;
 
-        // Does the path go above the current scope?
-        path = URIUtil.canonicalPath(path);
-        if (path == null)
-            return null;
-
         try
         {
+            // addPath with accept non-canonical paths that don't go above the root,
+            // but will treat them as aliases. So unless allowed by an AliasChecker
+            // they will be rejected below.
             Resource resource = _baseResource.addPath(path);
 
             if (checkAlias(path, resource))
@@ -2133,9 +2131,6 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
             return ContextHandler.this;
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getContext(java.lang.String)
-         */
         @Override
         public ServletContext getContext(String uripath)
         {
@@ -2224,9 +2219,6 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
             return null;
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getMimeType(java.lang.String)
-         */
         @Override
         public String getMimeType(String file)
         {
@@ -2235,9 +2227,6 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
             return _mimeTypes.getMimeByExtension(file);
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getRequestDispatcher(java.lang.String)
-         */
         @Override
         public RequestDispatcher getRequestDispatcher(String uriInContext)
         {
@@ -2250,6 +2239,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
 
             try
             {
+                // The uriInContext will be canonicalized by HttpURI.
                 HttpURI uri = new HttpURI(null, null, 0, uriInContext);
                 String pathInfo = uri.getDecodedPath();
                 String contextPath = getContextPath();
@@ -2265,12 +2255,13 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
             return null;
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getRealPath(java.lang.String)
-         */
         @Override
         public String getRealPath(String path)
         {
+            // This is an API call from the application which may have arbitrary non canonical paths passed
+            // Thus we canonicalize here, to avoid the enforcement of only canonical paths in
+            // ContextHandler.this.getResource(path).
+            path = URIUtil.canonicalPath(path);
             if (path == null)
                 return null;
             if (path.length() == 0)
@@ -2299,6 +2290,10 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
         @Override
         public URL getResource(String path) throws MalformedURLException
         {
+            // This is an API call from the application which may have arbitrary non canonical paths passed
+            // Thus we canonicalize here, to avoid the enforcement of only canonical paths in
+            // ContextHandler.this.getResource(path).
+            path = URIUtil.canonicalPath(path);
             if (path == null)
                 return null;
             Resource resource = ContextHandler.this.getResource(path);
@@ -2307,9 +2302,6 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
             return null;
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getResourceAsStream(java.lang.String)
-         */
         @Override
         public InputStream getResourceAsStream(String path)
         {
@@ -2331,65 +2323,48 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
             }
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getResourcePaths(java.lang.String)
-         */
         @Override
         public Set<String> getResourcePaths(String path)
         {
+            // This is an API call from the application which may have arbitrary non canonical paths passed
+            // Thus we canonicalize here, to avoid the enforcement of only canonical paths in
+            // ContextHandler.this.getResource(path).
+            path = URIUtil.canonicalPath(path);
             if (path == null)
                 return null;
             return ContextHandler.this.getResourcePaths(path);
         }
 
-        /*
-         * @see javax.servlet.ServletContext#log(java.lang.Exception, java.lang.String)
-         */
         @Override
         public void log(Exception exception, String msg)
         {
             _logger.warn(msg, exception);
         }
 
-        /*
-         * @see javax.servlet.ServletContext#log(java.lang.String)
-         */
         @Override
         public void log(String msg)
         {
             _logger.info(msg);
         }
 
-        /*
-         * @see javax.servlet.ServletContext#log(java.lang.String, java.lang.Throwable)
-         */
         @Override
         public void log(String message, Throwable throwable)
         {
             _logger.warn(message, throwable);
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getInitParameter(java.lang.String)
-         */
         @Override
         public String getInitParameter(String name)
         {
             return ContextHandler.this.getInitParameter(name);
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getInitParameterNames()
-         */
         @Override
         public Enumeration<String> getInitParameterNames()
         {
             return ContextHandler.this.getInitParameterNames();
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getAttribute(java.lang.String)
-         */
         @Override
         public Object getAttribute(String name)
         {
@@ -2399,9 +2374,6 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
             return o;
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getAttributeNames()
-         */
         @Override
         public Enumeration<String> getAttributeNames()
         {
@@ -2420,9 +2392,6 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
             return Collections.enumeration(set);
         }
 
-        /*
-         * @see javax.servlet.ServletContext#setAttribute(java.lang.String, java.lang.Object)
-         */
         @Override
         public void setAttribute(String name, Object value)
         {
@@ -2449,9 +2418,6 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
             }
         }
 
-        /*
-         * @see javax.servlet.ServletContext#removeAttribute(java.lang.String)
-         */
         @Override
         public void removeAttribute(String name)
         {
@@ -2468,9 +2434,6 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
             }
         }
 
-        /*
-         * @see javax.servlet.ServletContext#getServletContextName()
-         */
         @Override
         public String getServletContextName()
         {

jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java

@@ -187,7 +187,9 @@ public class ResourceHandler extends HandlerWrapper implements ResourceFactory,
                 }
             }
             else if (_context != null)
+            {
                 r = _context.getResource(path);
+            }
 
             if ((r == null || !r.exists()) && path.endsWith("/jetty-dir.css"))
                 r = getStylesheet();

jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java

@@ -829,6 +829,12 @@ public class HttpConnectionTest
         Log.getLogger(HttpParser.class).info("badMessage: bad encoding expected ...");
         String response;
 
+        response = connector.getResponse("GET /foo/bar%c0%00 HTTP/1.1\r\n" +
+            "Host: localhost\r\n" +
+            "Connection: close\r\n" +
+            "\r\n");
+        checkContains(response, 0, "HTTP/1.1 400");
+
         response = connector.getResponse("GET /bad/utf8%c1 HTTP/1.1\r\n" +
             "Host: localhost\r\n" +
             "Connection: close\r\n" +

jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java

@@ -235,16 +235,23 @@ public class ContextHandlerGetResourceTest
     @Test
     public void testAlias() throws Exception
     {
-        Resource resource = context.getResource("/./index.html");
+        String path = "/./index.html";
-        assertNotNull(resource);
+        Resource resource = context.getResource(path);
-        assertFalse(resource.isAlias());
-
-        resource = context.getResource("/down/../index.html");
-        assertNotNull(resource);
-        assertFalse(resource.isAlias());
-
-        resource = context.getResource("//index.html");
         assertNull(resource);
+        URL resourceURL = context.getServletContext().getResource(path);
+        assertFalse(resourceURL.getPath().contains("/./"));
+
+        path = "/down/../index.html";
+        resource = context.getResource(path);
+        assertNull(resource);
+        resourceURL = context.getServletContext().getResource(path);
+        assertFalse(resourceURL.getPath().contains("/../"));
+
+        path = "//index.html";
+        resource = context.getResource(path);
+        assertNull(resource);
+        resourceURL = context.getServletContext().getResource(path);
+        assertNull(resourceURL);
     }
 
     @ParameterizedTest

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java

@@ -74,7 +74,8 @@ public class RequestURITest
         ret.add(Arguments.of("/hello%u0025world", "/hello%u0025world", null));
         ret.add(Arguments.of("/hello-euro-%E2%82%AC", "/hello-euro-%E2%82%AC", null));
         ret.add(Arguments.of("/hello-euro?%E2%82%AC", "/hello-euro", "%E2%82%AC"));
-        // test the ascii control characters (just for completeness)
+        // Test the ascii control characters (just for completeness).
+        // Zero is not allowed in UTF-8 sequences so start from 1.
         for (int i = 0x1; i < 0x1f; i++)
         {
             String raw = String.format("/hello%%%02Xworld", i);
@@ -198,7 +199,6 @@ public class RequestURITest
             // Read the response.
             String response = readResponse(client);
 
-            // TODO: is HTTP/1.1 response appropriate for an HTTP/1.0 request?
             assertThat(response, Matchers.containsString("HTTP/1.1 200 OK"));
             assertThat(response, Matchers.containsString("RequestURI: " + expectedReqUri));
             assertThat(response, Matchers.containsString("QueryString: " + expectedQuery));
@@ -225,4 +225,45 @@ public class RequestURITest
             assertThat(response, Matchers.containsString("QueryString: " + expectedQuery));
         }
     }
+
+    public static Stream<Arguments> badData()
+    {
+        List<Arguments> ret = new ArrayList<>();
+        ret.add(Arguments.of("/hello\000"));
+        ret.add(Arguments.of("/hello%00"));
+        ret.add(Arguments.of("/hello%u0000"));
+        ret.add(Arguments.of("/hello\000/world"));
+        ret.add(Arguments.of("/hello%00world"));
+        ret.add(Arguments.of("/hello%u0000world"));
+        ret.add(Arguments.of("/hello%GG"));
+        ret.add(Arguments.of("/hello%;/world"));
+        ret.add(Arguments.of("/hello/../../world"));
+        ret.add(Arguments.of("/hello/..;/world"));
+        ret.add(Arguments.of("/hello/..;?/world"));
+        ret.add(Arguments.of("/hello/%#x/../world"));
+        ret.add(Arguments.of("/../hello/world"));
+        ret.add(Arguments.of("/hello%u00u00/world"));
+        ret.add(Arguments.of("hello"));
+
+        return ret.stream();
+    }
+
+    @ParameterizedTest
+    @MethodSource("badData")
+    public void testGetBadRequestsURIHTTP10(String rawpath) throws Exception
+    {
+        try (Socket client = newSocket(serverURI.getHost(), serverURI.getPort()))
+        {
+            OutputStream os = client.getOutputStream();
+
+            String request = String.format("GET %s HTTP/1.0\r\n\r\n", rawpath);
+            os.write(request.getBytes(StandardCharsets.ISO_8859_1));
+            os.flush();
+
+            // Read the response.
+            String response = readResponse(client);
+
+            assertThat(response, Matchers.containsString("HTTP/1.1 400 "));
+        }
+    }
 }

jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java

@@ -537,7 +537,6 @@ public class URIUtil
         {
             throw new IllegalArgumentException("cannot decode URI", e);
         }
-
     }
 
     /* Decode a URI path and strip parameters of ISO-8859-1 path
@@ -790,7 +789,6 @@ public class URIUtil
      * @param uri the encoded URI from the path onwards, which may contain query strings and/or fragments
      * @return the canonical path, or null if path traversal above root.
      * @see #canonicalPath(String)
-     * @see #canonicalURI(String)
      */
     public static String canonicalURI(String uri)
     {
@@ -890,6 +888,17 @@ public class URIUtil
         return canonical.toString();
     }
 
+    /**
+     * @param path the encoded URI from the path onwards, which may contain query strings and/or fragments
+     * @return the canonical path, or null if path traversal above root.
+     * @deprecated Use {@link #canonicalURI(String)}
+     */
+    @Deprecated
+    public static String canonicalEncodedPath(String path)
+    {
+        return canonicalURI(path);
+    }
+
     /**
      * Convert a decoded URI path to a canonical form.
      * <p>

jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java

@@ -270,8 +270,11 @@ public class FileResource extends Resource
     {
         assertValidPath(path);
 
-        if (path == null)
+        // Check that the path is within the root,
-            throw new MalformedURLException();
+        // but use the original path to create the
+        // resource, to preserve aliasing.
+        if (URIUtil.canonicalPath(path) == null)
+            throw new MalformedURLException(path);
 
         if ("/".equals(path))
             return this;

jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java

@@ -94,8 +94,11 @@ public class PathResource extends Resource
         if (!abs.isAbsolute())
             abs = path.toAbsolutePath();
 
+        // Any normalization difference means it's an alias,
+        // and we don't want to bother further to follow
+        // symlinks as it's an alias anyway.
         Path normal = path.normalize();
-        if (!abs.equals(normal))
+        if (!isSameName(abs, normal))
             return normal;
 
         try
@@ -105,11 +108,8 @@ public class PathResource extends Resource
             if (Files.exists(path))
             {
                 Path real = abs.toRealPath(FOLLOW_LINKS);
-
                 if (!isSameName(abs, real))
-                {
                     return real;
-                }
             }
         }
         catch (IOException e)
@@ -361,20 +361,23 @@ public class PathResource extends Resource
     }
 
     @Override
-    public Resource addPath(final String subpath) throws IOException
+    public Resource addPath(final String subPath) throws IOException
     {
-        if ((subpath == null) || (subpath.length() == 0))
+        // Check that the path is within the root,
-            throw new MalformedURLException(subpath);
+        // but use the original path to create the
+        // resource, to preserve aliasing.
+        if (URIUtil.canonicalPath(subPath) == null)
+            throw new MalformedURLException(subPath);
 
-        if ("/".equals(subpath))
+        if ("/".equals(subPath))
             return this;
 
-        // subpaths are always under PathResource
+        // Sub-paths are always under PathResource
-        // compensate for input subpaths like "/subdir"
+        // compensate for input sub-paths like "/subdir"
         // where default resolve behavior would be
         // to treat that like an absolute path
 
-        return new PathResource(this, subpath);
+        return new PathResource(this, subPath);
     }
 
     private void assertValidPath(Path path)

jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java

@@ -459,10 +459,12 @@ public abstract class Resource implements ResourceFactory, Closeable
      * Returns the resource contained inside the current resource with the
      * given name.
      *
-     * @param path The path segment to add, which is not encoded
+     * @param path The path segment to add, which is not encoded.  The path may be non canonical, but if so then
+     * the resulting Resource will return true from {@link #isAlias()}.
      * @return the Resource for the resolved path within this Resource.
      * @throws IOException if unable to resolve the path
-     * @throws MalformedURLException if the resolution of the path fails because the input path parameter is malformed.
+     * @throws MalformedURLException if the resolution of the path fails because the input path parameter is malformed, or
+     * a relative path attempts to access above the root resource.
      */
     public abstract Resource addPath(String path)
         throws IOException, MalformedURLException;
@@ -555,6 +557,8 @@ public abstract class Resource implements ResourceFactory, Closeable
      */
     public String getListHTML(String base, boolean parent, String query) throws IOException
     {
+        // This method doesn't check aliases, so it is OK to canonicalize here.
+        base = URIUtil.canonicalPath(base);
         if (base == null || !isDirectory())
             return null;
 

jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java

@@ -271,8 +271,11 @@ public class URLResource extends Resource
     public Resource addPath(String path)
         throws IOException
     {
-        if (path == null)
+        // Check that the path is within the root,
-            throw new MalformedURLException("null path");
+        // but use the original path to create the
+        // resource, to preserve aliasing.
+        if (URIUtil.canonicalPath(path) == null)
+            throw new MalformedURLException(path);
 
         return newResource(URIUtil.addEncodedPaths(_url.toExternalForm(), URIUtil.encodePath(path)), _useCaches);
     }

jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java

@@ -151,7 +151,9 @@ public class URIUtilCanonicalPathTest
 
         // Check canonicalURI
         if (expectedResult == null)
+        {
             assertThat(URIUtil.canonicalURI(input), nullValue());
+        }
         else
         {
             // mostly encodedURI will be the same
@@ -162,4 +164,22 @@ public class URIUtilCanonicalPathTest
         }
     }
 
+    public static Stream<Arguments> queries()
+    {
+        String[][] data =
+            {
+                {"/ctx/../dir?/../index.html", "/dir?/../index.html"},
+                {"/get-files?file=/etc/passwd", "/get-files?file=/etc/passwd"},
+                {"/get-files?file=../../../../../passwd", "/get-files?file=../../../../../passwd"}
+            };
+        return Stream.of(data).map(Arguments::of);
+    }
+
+    @ParameterizedTest
+    @MethodSource("queries")
+    public void testQuery(String input, String expectedPath)
+    {
+        String actual = URIUtil.canonicalURI(input);
+        assertThat(actual, is(expectedPath));
+    }
 }

jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceTest.java

@@ -21,6 +21,7 @@ package org.eclipse.jetty.util.resource;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
+import java.net.MalformedURLException;
 import java.net.URI;
 import java.net.URL;
 import java.nio.file.Path;
@@ -45,6 +46,8 @@ import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.startsWith;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class ResourceTest
 {
@@ -325,4 +328,19 @@ public class ResourceTest
 
         assertEquals(rb, ra);
     }
+
+    @Test
+    public void testClimbAboveBase() throws Exception
+    {
+        Resource resource = Resource.newResource("/foo/bar");
+        assertThrows(MalformedURLException.class, () -> resource.addPath(".."));
+
+        Resource same = resource.addPath(".");
+        assertNotNull(same);
+        assertTrue(same.isAlias());
+
+        assertThrows(MalformedURLException.class, () -> resource.addPath("./.."));
+
+        assertThrows(MalformedURLException.class, () -> resource.addPath("./../bar"));
+    }
 }