Merge pull request #11279 from jetty/jetty-12.0.x-11271-AliasCheckCom…

…binedResource

Issue #11271 - fix use of AliasCheckers with CombinedResource

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java

@@ -44,9 +44,12 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
 
     private final ContextHandler _contextHandler;
     private final Supplier<Resource> _resourceBaseSupplier;
-    private final List<Path> _protected = new ArrayList<>();
+    private final List<String> _protected = new ArrayList<>();
     private final AllowedResourceAliasCheckListener _listener = new AllowedResourceAliasCheckListener();
     private boolean _initialized;
+    protected Resource _baseResource;
+
+    @Deprecated
     protected Path _base;
 
     /**
@@ -80,25 +83,20 @@ private String[] getProtectedTargets()
 
     private void extractBaseResourceFromContext()
     {
-        _base = getPath(_resourceBaseSupplier.get());
-        if (_base == null)
+        _baseResource = _resourceBaseSupplier.get();
+        if (_baseResource == null)
             return;
 
         try
         {
-            if (Files.exists(_base, NO_FOLLOW_LINKS))
-                _base = _base.toRealPath(FOLLOW_LINKS);
             String[] protectedTargets = getProtectedTargets();
             if (protectedTargets != null)
-            {
-                for (String s : protectedTargets)
-                    _protected.add(_base.getFileSystem().getPath(_base.toString(), s));
-            }
+                _protected.addAll(Arrays.asList(protectedTargets));
         }
-        catch (IOException e)
+        catch (Throwable t)
         {
-            LOG.warn("Base resource failure ({} is disabled): {}", this.getClass().getName(), _base, e);
-            _base = null;
+            LOG.warn("Base resource failure ({} is disabled): {}", this.getClass().getName(), _baseResource, t);
+            _baseResource = null;
         }
     }
 
@@ -123,7 +121,7 @@ protected void doStart() throws Exception
     protected void doStop() throws Exception
     {
         _contextHandler.removeEventListener(_listener);
-        _base = null;
+        _baseResource = null;
         _protected.clear();
     }
 
@@ -132,7 +130,7 @@ public boolean checkAlias(String pathInContext, Resource resource)
     {
         if (!_initialized)
             extractBaseResourceFromContext();
-        if (_base == null)
+        if (_baseResource == null)
             return false;
 
         try
@@ -141,11 +139,7 @@ public boolean checkAlias(String pathInContext, Resource resource)
             if (!resource.exists())
                 return false;
 
-            Path path = getPath(resource);
-            if (path == null)
-                return false;
-
-            return check(pathInContext, path);
+            return check(pathInContext, resource);
         }
         catch (Throwable t)
         {
@@ -162,6 +156,19 @@ protected boolean check(String pathInContext, Path path)
         return isAllowed(getRealPath(path));
     }
 
+    protected boolean check(String pathInContext, Resource resource)
+    {
+        // Allow any aliases (symlinks, 8.3, casing, etc.) so long as
+        // the resulting real file is allowed.
+        for (Resource r : resource)
+        {
+            if (!check(pathInContext, r.getPath()))
+                return false;
+        }
+
+        return true;
+    }
+
     protected boolean isAllowed(Path path)
     {
         // If the resource doesn't exist we cannot determine whether it is protected, so we assume it is.
@@ -172,14 +179,20 @@ protected boolean isAllowed(Path path)
             {
                 // If the path is the same file as the base, then it is contained in the base and
                 // is not protected.
-                if (isSameFile(path, _base))
+                if (_baseResource.isSameFile(path))
                     return true;
 
                 // If the path is the same file as any protected resources, then it is protected.
-                for (Path p : _protected)
+                for (String protectedTarget : _protected)
                 {
-                    if (isSameFile(path, p))
-                        return false;
+                    Resource p = _baseResource.resolve(protectedTarget);
+                    if (p == null)
+                        continue;
+                    for (Resource r : p)
+                    {
+                        if (r.isSameFile(path))
+                            return false;
+                    }
                 }
 
                 // Walks up the aliased path name, not the real path name.
@@ -192,6 +205,7 @@ protected boolean isAllowed(Path path)
         return false;
     }
 
+    @Deprecated
     protected boolean isSameFile(Path path1, Path path2)
     {
         if (Objects.equals(path1, path2))
@@ -227,17 +241,10 @@ private static Path getRealPath(Path path)
         return null;
     }
 
+    @Deprecated
     protected Path getPath(Resource resource)
     {
-        try
-        {
-            return (resource == null) ? null : resource.getPath();
-        }
-        catch (Throwable t)
-        {
-            LOG.trace("getPath() failed", t);
-            return null;
-        }
+        return null;
     }
 
     private class AllowedResourceAliasCheckListener implements LifeCycle.Listener
@@ -256,7 +263,7 @@ public String toString()
         return String.format("%s@%x{base=%s,protected=%s}",
             this.getClass().getSimpleName(),
             hashCode(),
-            _base,
+            _baseResource,
             (protectedTargets == null) ? null : Arrays.asList(protectedTargets));
     }
 }

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java

@@ -24,7 +24,8 @@
 
 /**
  * An extension of {@link AllowedResourceAliasChecker} which will allow symlinks alias to arbitrary
- * targets, so long as the symlink file itself is an allowed resource.
+ * targets, so long as the symlink file itself is an allowed resource. Non symlinked paths are never
+ * allowed by this {@link AliasCheck}.
  */
 public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChecker
 {
@@ -46,7 +47,7 @@ public SymlinkAllowedResourceAliasChecker(ContextHandler contextHandler, Resourc
     @Override
     protected boolean check(String pathInContext, Path path)
     {
-        if (_base == null)
+        if (_baseResource == null)
             return false;
 
         // do not allow any file separation characters in the URI, as we need to know exactly what are the segments
@@ -57,29 +58,32 @@ protected boolean check(String pathInContext, Path path)
         // We rebuild the realURI, segment by segment, getting the real name at each step, so that we can distinguish between
         // alias types.  Specifically, so we can allow a symbolic link so long as it's realpath is not protected.
         String[] segments = pathInContext.substring(1).split("/");
-        Path fromBase = _base;
-        StringBuilder realURI = new StringBuilder();
+        StringBuilder segmentPath = new StringBuilder();
 
         try
         {
             for (String segment : segments)
             {
                 // Add the segment to the path and realURI.
-                fromBase = fromBase.resolve(segment);
-                realURI.append("/").append(fromBase.toRealPath(NO_FOLLOW_LINKS).getFileName());
+                segmentPath.append("/").append(segment);
+                Resource fromBase = _baseResource.resolve(segmentPath.toString());
+                for (Resource r : fromBase)
+                {
+                    Path p = r.getPath();
 
-                // If the ancestor of the alias is a symlink, then check if the real URI is protected, otherwise allow.
-                // This allows symlinks like /other->/WEB-INF and /external->/var/lib/docroot
-                // This does not allow symlinks like /WeB-InF->/var/lib/other
-                if (Files.isSymbolicLink(fromBase))
-                    return !getContextHandler().isProtectedTarget(realURI.toString());
+                    // If the ancestor of the alias is a symlink, then check if the real URI is protected, otherwise allow.
+                    // This allows symlinks like /other->/WEB-INF and /external->/var/lib/docroot
+                    // This does not allow symlinks like /WeB-InF->/var/lib/other
+                    if (Files.isSymbolicLink(p))
+                        return !getContextHandler().isProtectedTarget(segmentPath.toString());
 
-                // If the ancestor is not allowed then do not allow.
-                if (!isAllowed(fromBase))
-                    return false;
+                    // If the ancestor is not allowed then do not allow.
+                    if (!isAllowed(p))
+                        return false;
 
-                // TODO as we are building the realURI of the resource, it would be possible to
-                //  re-check that against security constraints.
+                    // TODO as we are building the realURI of the resource, it would be possible to
+                    //  re-check that against security constraints.
+                }
             }
         }
         catch (Throwable t)
@@ -89,7 +93,7 @@ protected boolean check(String pathInContext, Path path)
             return false;
         }
 
-        // No symlink found, so must be allowed. Double check it is the right path we checked.
-        return isSameFile(fromBase, path);
+        // No symlink found, so must not be allowed.
+        return false;
     }
 }

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/TrailingSlashAliasChecker.java

@@ -0,0 +1,41 @@
+//
+// ========================================================================
+// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Eclipse Public License v. 2.0 which is available at
+// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
+// which is available at https://www.apache.org/licenses/LICENSE-2.0.
+//
+// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
+// ========================================================================
+//
+
+package org.eclipse.jetty.server;
+
+import java.util.Objects;
+
+import org.eclipse.jetty.util.component.AbstractLifeCycle;
+import org.eclipse.jetty.util.resource.Resource;
+
+/**
+ * <p>This will approve an alias where the only difference is a trailing slash.</p>
+ * <p>For example, a request for a file containing a trailing slash like <code>/context/dir/index.html/</code>,
+ * can be approved as an alias to the file <code>/context/dir/index.html</code> which exists.</p>
+ */
+public class TrailingSlashAliasChecker extends AbstractLifeCycle implements AliasCheck
+{
+    @Override
+    public boolean checkAlias(String pathInContext, Resource resource)
+    {
+        String uri = resource.getURI().toString();
+        if (uri.isEmpty())
+            return false;
+
+        String realUri = resource.getRealURI().toString();
+        if (uri.endsWith("/") && !realUri.endsWith("/"))
+            return Objects.equals(uri.substring(0, uri.length() - 1), realUri);
+
+        return false;
+    }
+}

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

@@ -631,6 +631,8 @@ protected void doStart() throws Exception
         {
             if (!Resources.isReadable(baseResource))
                 throw new IllegalArgumentException("Base Resource is not valid: " + baseResource);
+            if (baseResource.isAlias())
+                LOG.warn("Base Resource should not be an alias");
         }
 
         _availability.set(Availability.STARTING);

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/HotSwapHandler.java

@@ -71,9 +71,7 @@ public void setHandler(Handler handler)
             if (handler != null)
             {
                 handler.setServer(server);
-                addBean(handler, true);
-                if (oldHandler != null && oldHandler.isStarted())
-                    handler.start();
+                addManaged(handler);
             }
             _handler = handler;
             if (oldHandler != null)

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

@@ -38,6 +38,8 @@
 import org.eclipse.jetty.util.resource.Resource;
 import org.eclipse.jetty.util.resource.ResourceFactory;
 import org.eclipse.jetty.util.resource.Resources;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Resource Handler.
@@ -56,6 +58,8 @@
  */
 public class ResourceHandler extends Handler.Wrapper
 {
+    private static final Logger LOG = LoggerFactory.getLogger(ResourceHandler.class);
+
     private final ResourceService _resourceService = newResourceService();
     private ByteBufferPool _byteBufferPool;
     private Resource _baseResource;
@@ -93,6 +97,10 @@ public void doStart() throws Exception
             if (context != null)
                 _baseResource = context.getBaseResource();
         }
+        else if (_baseResource.isAlias())
+        {
+            LOG.warn("Base Resource should not be an alias");
+        }
 
         setMimeTypes(context == null ? MimeTypes.DEFAULTS : context.getMimeTypes());
 

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ResourceHandlerTest.java

@@ -55,6 +55,7 @@
 import org.eclipse.jetty.server.LocalConnector;
 import org.eclipse.jetty.server.ResourceService;
 import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.TrailingSlashAliasChecker;
 import org.eclipse.jetty.toolchain.test.FS;
 import org.eclipse.jetty.toolchain.test.MavenPaths;
 import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
@@ -3132,6 +3133,7 @@ public void testRangeRequests(ResourceHandlerTest.Scenario scenario) throws Exce
     @Test
     public void testRelativeRedirect() throws Exception
     {
+        _contextHandler.addAliasCheck(new TrailingSlashAliasChecker());
         Path dir = docRoot.resolve("dir");
         FS.ensureDirExists(dir);
         Path index = dir.resolve("index.html");
@@ -3180,6 +3182,7 @@ public void testRelativeRedirect() throws Exception
     @Test
     public void testResourceRedirect() throws Exception
     {
+        _contextHandler.addAliasCheck(new TrailingSlashAliasChecker());
         setupSimpleText(docRoot);
 
         HttpTester.Response response = HttpTester.parseResponse(

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/CombinedResource.java

@@ -403,4 +403,15 @@ else if (!relative.equals(rel))
         }
         return relative;
     }
+
+    @Override
+    public boolean isSameFile(Path path)
+    {
+        for (Resource r : this)
+        {
+            if (r.isSameFile(path))
+                return true;
+        }
+        return false;
+    }
 }

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

@@ -423,4 +423,22 @@ public Collection<Resource> getAllResources()
             throw new IllegalStateException(e);
         }
     }
+
+    public boolean isSameFile(Path path)
+    {
+        Path resourcePath = getPath();
+        if (Objects.equals(path, resourcePath))
+            return true;
+        try
+        {
+            if (Files.isSameFile(path, resourcePath))
+                return true;
+        }
+        catch (Throwable t)
+        {
+            if (LOG.isDebugEnabled())
+                LOG.debug("ignored", t);
+        }
+        return false;
+    }
 }