Alternate fix for #6497

Signed-off-by: Greg Wilkins <gregw@webtide.com>

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

@@ -13,6 +13,7 @@
 
 package org.eclipse.jetty.server;
 
+import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.LinkOption;
@@ -22,7 +23,6 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Objects;
-import java.util.stream.Collectors;
 
 import org.eclipse.jetty.server.handler.ContextHandler;
 import org.eclipse.jetty.util.component.AbstractLifeCycle;
@@ -33,9 +33,7 @@
 
 /**
  * <p>This will approve any alias to anything inside of the {@link ContextHandler}s resource base which
- * is not protected by {@link ContextHandler#isProtectedTarget(String)}.</p>
- * <p>This will approve symlinks to outside of the resource base. This can be optionally configured to check that the
- * target of the symlinks is also inside of the resource base and is not a protected target.</p>
+ * is not protected by a protected target as defined by {@link ContextHandler#getProtectedTargets()} at start.</p>
  * <p>Aliases approved by this may still be able to bypass SecurityConstraints, so this class would need to be extended
  * to enforce any additional security constraints that are required.</p>
  */
@@ -89,59 +87,84 @@ protected void doStop() throws Exception
     @Override
     public boolean check(String pathInContext, Resource resource)
     {
-        // The existence check resolves the symlinks.
-        if (!resource.exists())
-            return false;
-
-        Path path = getPath(resource);
-        if (path == null)
-            return false;
-
         try
         {
-            Path link = path.toRealPath(NO_FOLLOW_LINKS);
-            Path real = path.toRealPath(FOLLOW_LINKS);
+            // do not allow any file separation characters in the URI
+            if (File.separatorChar != '/' && pathInContext.indexOf(File.separatorChar) >= 0)
+                return false;
+
+            // The existence check resolves the symlinks.
+            if (!resource.exists())
+                return false;
+
+            Path path = getPath(resource);
+            if (path == null)
+                return false;
 
-            // Allowed IFF the real file is allowed and either it is not linked or the link file itself is also allowed.
-            return isAllowed(real) && (link.equals(real) || isAllowed(link));
+            return check(pathInContext, path);
         }
         catch (Throwable t)
         {
-            LOG.warn("Failed to check alias", t);
+            if (LOG.isDebugEnabled())
+                LOG.debug("Failed to check alias", t);
             return false;
         }
     }
 
-    protected boolean isAllowed(Path path) throws IOException
+    protected boolean check(String pathInContext, Path path) throws Exception
+    {
+        // Allow any aliases (symlinks, 8.3, casing, etc.) so long as
+        // the resulting real file is allowed.
+        return isAllowed(path.toRealPath(FOLLOW_LINKS));
+    }
+
+    protected boolean isAllowed(Path path)
     {
         // If the resource doesn't exist we cannot determine whether it is protected so we assume it is.
         if (Files.exists(path))
         {
-            // resolve the protected paths now, as they may have changed since start
-            List<Path> protect = _protected.stream()
-                .map(AllowedResourceAliasChecker::getRealPath)
-                .filter(Objects::nonNull)
-                .collect(Collectors.toList());
-
             // Walk the path parent links looking for the base resource, but failing if any steps are protected
             while (path != null)
             {
-                if (Files.isSameFile(path, _base))
+                // 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))
                     return true;
 
-                for (Path p : protect)
+                // If the path is the same file as any protected resources, then it is protected.
+                for (Path p : _protected)
                 {
-                    if (Files.isSameFile(path, p))
+                    if (isSameFile(path, p))
                         return false;
                 }
 
+                // Walks up the aliased path name, not the real path name.
+                // If WEB-INF is a link to /var/lib/webmeta then after checking
+                // a URI of /WEB-INF/file.xml the parent is /WEB-INF and not /var/lib/webmeta
                 path = path.getParent();
             }
         }
 
         return false;
     }
 
+    protected boolean isSameFile(Path path1, Path path2)
+    {
+        if (Objects.equals(path1, path2))
+            return true;
+        try
+        {
+            if (Files.isSameFile(path1, path2))
+                return true;
+        }
+        catch (Throwable t)
+        {
+            if (LOG.isDebugEnabled())
+                LOG.debug("ignored", t);
+        }
+        return false;
+    }
+
     private static Path getRealPath(Path path)
     {
         if (path == null || !Files.exists(path))

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

@@ -17,12 +17,12 @@
 import java.nio.file.Path;
 
 import org.eclipse.jetty.server.handler.ContextHandler;
-import org.eclipse.jetty.util.resource.Resource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
- * An extension of {@link AllowedResourceAliasChecker} which only allows aliased resources if they are symlinks.
+ * An extension of {@link AllowedResourceAliasChecker} which will allow symlinks alias to arbitrary
+ * targets, so long as the symlink file itself is an allowed resource.
  */
 public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChecker
 {
@@ -37,38 +37,39 @@ public SymlinkAllowedResourceAliasChecker(ContextHandler contextHandler)
     }
 
     @Override
-    public boolean check(String pathInContext, Resource resource)
+    protected boolean check(String pathInContext, Path path)
     {
-        // The existence check resolves the symlinks.
-        if (!resource.exists())
-            return false;
-
-        Path path = getPath(resource);
-        if (path == null)
-            return false;
-
-        String[] segments = pathInContext.split("/");
+        // Split the URI path so we can walk down the resource tree and build the realURI of any symlink found
+        String[] segments = pathInContext.substring(1).split("/");
         Path fromBase = _base;
+        StringBuilder realURI = new StringBuilder();
 
         try
         {
             for (String segment : segments)
             {
+                // Add the segment to the path and realURI.
                 fromBase = fromBase.resolve(segment);
-                if (!Files.exists(fromBase))
-                    return false;
+                realURI.append("/").append(fromBase.toRealPath(NO_FOLLOW_LINKS).getFileName());
+
+                // If the ancestor of the alias is a symlink, then check if the real URI is protected, otherwise allow.
+                // This will allow a symlink like /other->/WEB-INF, but not /WeB-InF->/var/lib/other
                 if (Files.isSymbolicLink(fromBase))
-                    return true;
+                    return !getContextHandler().isProtectedTarget(realURI.toString());
+
+                // If the ancestor is not allowed then do not allow.
                 if (!isAllowed(fromBase))
                     return false;
             }
         }
         catch (Throwable t)
         {
-            LOG.warn("Failed to check alias", t);
+            if (LOG.isDebugEnabled())
+                LOG.debug("Failed to check alias", t);
             return false;
         }
-
-        return true;
+
+        // No symlink found, so must be allowed. Double check it is the right path we checked.
+        return isSameFile(fromBase, path);
     }
 }