Issue #6277 - add replacement Symlink AliasChecker

Signed-off-by: Lachlan Roberts <[email protected]>

jetty-gcloud/jetty-gcloud-session-manager/src/test/java/org/eclipse/jetty/gcloud/session/GCloudSessionTester.java

@@ -20,7 +20,7 @@
 
 import org.eclipse.jetty.security.HashLoginService;
 import org.eclipse.jetty.server.Server;
-import org.eclipse.jetty.server.handler.AllowSymLinkAliasChecker;
+import org.eclipse.jetty.server.SymlinkAllowedResourceAliasChecker;
 import org.eclipse.jetty.server.session.DefaultSessionCache;
 import org.eclipse.jetty.server.session.DefaultSessionIdManager;
 import org.eclipse.jetty.webapp.WebAppContext;
@@ -47,7 +47,7 @@ public static void main(String[] args) throws Exception
         WebAppContext webapp = new WebAppContext();
         webapp.setContextPath("/");
         webapp.setWar("../../jetty-distribution/target/distribution/demo-base/webapps/test.war");
-        webapp.addAliasCheck(new AllowSymLinkAliasChecker());
+        webapp.addAliasCheck(new SymlinkAllowedResourceAliasChecker(webapp));
         GCloudSessionDataStore ds = new GCloudSessionDataStore();
 
         DefaultSessionCache ss = new DefaultSessionCache(webapp.getSessionHandler());

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

@@ -23,28 +23,36 @@
 import java.nio.file.Files;
 import java.nio.file.LinkOption;
 import java.nio.file.Path;
+import java.util.Objects;
 
 import org.eclipse.jetty.server.handler.ContextHandler;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
+import org.eclipse.jetty.util.resource.PathResource;
 import org.eclipse.jetty.util.resource.Resource;
 
 /**
- * This will approve an alias to any resource which is not a protected target.
- * Except symlinks...
+ * <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>
+ * <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>
  */
 public class AllowedResourceAliasChecker implements ContextHandler.AliasCheck
 {
     private static final Logger LOG = Log.getLogger(AllowedResourceAliasChecker.class);
+    private static final LinkOption[] FOLLOW_LINKS = new LinkOption[0];
+    private static final LinkOption[] NO_FOLLOW_LINKS = new LinkOption[]{LinkOption.NOFOLLOW_LINKS};
+
     private final ContextHandler _contextHandler;
     private final boolean _checkSymlinkTargets;
 
-    public AllowedResourceAliasChecker(ContextHandler contextHandler)
-    {
-        this(contextHandler, false);
-    }
-
+    /**
+     * @param contextHandler the context handler to use.
+     * @param checkSymlinkTargets true to check that the target of the symlink is an allowed resource.
+     */
     public AllowedResourceAliasChecker(ContextHandler contextHandler, boolean checkSymlinkTargets)
     {
         _contextHandler = contextHandler;
@@ -54,20 +62,22 @@ public AllowedResourceAliasChecker(ContextHandler contextHandler, boolean checkS
     @Override
     public boolean check(String uri, Resource resource)
     {
+        // The existence check resolves the symlinks.
+        if (!resource.exists())
+            return false;
+
+        Path resourcePath = getPath(resource);
+        if (resourcePath == null)
+            return false;
+
         try
         {
-            if (!resource.exists())
-                return false;
-
-            Path resourcePath = resource.getFile().toPath();
-            Path realPath = resourcePath.toRealPath(LinkOption.NOFOLLOW_LINKS);
-            if (isProtectedPath(realPath, false))
+            if (isProtectedPath(resourcePath, NO_FOLLOW_LINKS))
                 return false;
 
             if (_checkSymlinkTargets && hasSymbolicLink(resourcePath))
             {
-                realPath = resourcePath.toRealPath();
-                if (isProtectedPath(realPath, true))
+                if (isProtectedPath(resourcePath, FOLLOW_LINKS))
                     return false;
             }
         }
@@ -80,34 +90,40 @@ public boolean check(String uri, Resource resource)
         return true;
     }
 
-    private boolean isProtectedPath(Path path, boolean followLinks) throws IOException
+    protected boolean isProtectedPath(Path resourcePath, LinkOption[] linkOptions) throws IOException
     {
-        String basePath = followLinks ? _contextHandler.getBaseResource().getFile().toPath().toRealPath().toString()
-            : _contextHandler.getBaseResource().getFile().toPath().toRealPath(LinkOption.NOFOLLOW_LINKS).toString();
-        String targetPath = path.toString();
+        String basePath = Objects.requireNonNull(getPath(_contextHandler.getBaseResource())).toRealPath(linkOptions).toString();
+        String targetPath = resourcePath.toRealPath(linkOptions).toString();
 
         if (!targetPath.startsWith(basePath))
             return true;
 
-        for (String s : _contextHandler.getProtectedTargets())
+        String[] protectedTargets = _contextHandler.getProtectedTargets();
+        if (protectedTargets != null)
         {
-            String protectedTarget = new File(basePath, s).getCanonicalPath();
-            if (StringUtil.startsWithIgnoreCase(targetPath, protectedTarget))
+            for (String s : protectedTargets)
             {
-                if (targetPath.length() == protectedTarget.length())
-                    return true;
-
-                // Check that the target prefix really is a path segment.
-                char c = targetPath.charAt(protectedTarget.length());
-                if (c == File.separatorChar)
-                    return true;
+                // TODO: we are always following links for the base resource.
+                //  We cannot use toRealPath(linkOptions) here as it throws if file does not exist,
+                //  and the protected targets do not have to always exist.
+                String protectedTarget = new File(basePath, s).getCanonicalPath();
+                if (StringUtil.startsWithIgnoreCase(targetPath, protectedTarget))
+                {
+                    if (targetPath.length() == protectedTarget.length())
+                        return true;
+
+                    // Check that the target prefix really is a path segment.
+                    char c = targetPath.charAt(protectedTarget.length());
+                    if (c == File.separatorChar)
+                        return true;
+                }
             }
         }
 
         return false;
     }
 
-    private boolean hasSymbolicLink(Path path)
+    protected boolean hasSymbolicLink(Path path)
     {
         // Is file itself a symlink?
         if (Files.isSymbolicLink(path))
@@ -125,6 +141,21 @@ private boolean hasSymbolicLink(Path path)
         return false;
     }
 
+    private Path getPath(Resource resource)
+    {
+        try
+        {
+            if (resource instanceof PathResource)
+                return ((PathResource)resource).getPath();
+            return resource.getFile().toPath();
+        }
+        catch (Throwable t)
+        {
+            LOG.ignore(t);
+            return null;
+        }
+    }
+
     @Override
     public String toString()
     {

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

@@ -44,7 +44,9 @@
  * or Linux on XFS) the the actual file could be stored using UTF-16,
  * but be accessed using NFD UTF-8 or NFC UTF-8 for the same file.
  * </p>
+ * @deprecated use {@link org.eclipse.jetty.server.AllowedResourceAliasChecker} instead.
  */
+@Deprecated
 public class SameFileAliasChecker implements AliasCheck
 {
     private static final Logger LOG = Log.getLogger(SameFileAliasChecker.class);

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

@@ -0,0 +1,74 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.server;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.eclipse.jetty.server.handler.ContextHandler;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
+import org.eclipse.jetty.util.resource.Resource;
+
+/**
+ * An extension of {@link AllowedResourceAliasChecker} which only allows resources if they are symlinks.
+ */
+public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChecker
+{
+    private static final Logger LOG = Log.getLogger(SymlinkAllowedResourceAliasChecker.class);
+    private final ContextHandler _contextHandler;
+
+    /**
+     * @param contextHandler the context handler to use.
+     */
+    public SymlinkAllowedResourceAliasChecker(ContextHandler contextHandler)
+    {
+        super(contextHandler, false);
+        _contextHandler = contextHandler;
+    }
+
+    @Override
+    public boolean check(String uri, Resource resource)
+    {
+        try
+        {
+            // Check the resource is allowed to be accessed.
+            if (!super.check(uri, resource))
+                return false;
+
+            // Only approve resource if it is accessed by a symbolic link.
+            Path resourcePath = resource.getFile().toPath();
+            if (Files.isSymbolicLink(resourcePath))
+                return true;
+
+            // TODO: If base resource contains symlink then this will always return true.
+            //  But we don't want to deny all paths if the resource base is symbolically linked.
+            if (super.hasSymbolicLink(resourcePath))
+                return true;
+        }
+        catch (IOException e)
+        {
+            LOG.ignore(e);
+            return false;
+        }
+
+        return false;
+    }
+}

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

@@ -68,13 +68,15 @@
 
 import org.eclipse.jetty.http.HttpURI;
 import org.eclipse.jetty.http.MimeTypes;
+import org.eclipse.jetty.server.AllowedResourceAliasChecker;
 import org.eclipse.jetty.server.ClassLoaderDump;
 import org.eclipse.jetty.server.Connector;
 import org.eclipse.jetty.server.Dispatcher;
 import org.eclipse.jetty.server.Handler;
 import org.eclipse.jetty.server.HandlerContainer;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.SymlinkAllowedResourceAliasChecker;
 import org.eclipse.jetty.util.Attributes;
 import org.eclipse.jetty.util.AttributesMap;
 import org.eclipse.jetty.util.FutureCallback;
@@ -108,8 +110,8 @@
  * The executor is made available via a context attributed {@code org.eclipse.jetty.server.Executor}.
  * </p>
  * <p>
- * By default, the context is created with alias checkers for {@link AllowSymLinkAliasChecker} (unix only) and {@link ApproveNonExistentDirectoryAliases}. If
- * these alias checkers are not required, then {@link #clearAliasChecks()} or {@link #setAliasChecks(List)} should be called.
+ * By default, the context is created with the {@link AllowedResourceAliasChecker} which is configured to allow symlinks. If
+ * this alias checker is not required, then {@link #clearAliasChecks()} or {@link #setAliasChecks(List)} should be called.
  * </p>
  */
 @ManagedObject("URI Context")
@@ -264,9 +266,8 @@ private ContextHandler(Context context, HandlerContainer parent, String contextP
         _scontext = context == null ? new Context() : context;
         _attributes = new AttributesMap();
         _initParams = new HashMap<>();
-        addAliasCheck(new ApproveNonExistentDirectoryAliases());
         if (File.separatorChar == '/')
-            addAliasCheck(new AllowSymLinkAliasChecker());
+            addAliasCheck(new SymlinkAllowedResourceAliasChecker(this));
 
         if (contextPath != null)
             setContextPath(contextPath);
@@ -2970,7 +2971,9 @@ public interface AliasCheck
 
     /**
      * Approve all aliases.
+     * @deprecated use {@link org.eclipse.jetty.server.AllowedResourceAliasChecker} instead.
      */
+    @Deprecated
     public static class ApproveAliases implements AliasCheck
     {
         @Override
@@ -2983,6 +2986,7 @@ public boolean check(String path, Resource resource)
     /**
      * Approve Aliases of a non existent directory. If a directory "/foobar/" does not exist, then the resource is aliased to "/foobar". Accept such aliases.
      */
+    @Deprecated
     public static class ApproveNonExistentDirectoryAliases implements AliasCheck
     {
         @Override

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

@@ -32,6 +32,7 @@
 import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.server.LocalConnector;
 import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.SymlinkAllowedResourceAliasChecker;
 import org.eclipse.jetty.toolchain.test.FS;
 import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
 import org.eclipse.jetty.util.BufferUtil;
@@ -184,7 +185,7 @@ private void setupServer() throws Exception
         fileResourceContext.setBaseResource(new PathResource(rootPath));
 
         fileResourceContext.clearAliasChecks();
-        fileResourceContext.addAliasCheck(new AllowSymLinkAliasChecker());
+        fileResourceContext.addAliasCheck(new SymlinkAllowedResourceAliasChecker(fileResourceContext));
 
         server.setHandler(fileResourceContext);
         server.start();

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

@@ -24,6 +24,7 @@
 import java.nio.file.Files;
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import org.eclipse.jetty.server.AllowedResourceAliasChecker;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.toolchain.test.FS;
 import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
@@ -109,11 +110,10 @@ public static void beforeClass() throws Exception
         server = new Server();
         context = new ContextHandler("/");
         context.clearAliasChecks();
-        context.addAliasCheck(new ContextHandler.ApproveNonExistentDirectoryAliases());
         context.setBaseResource(Resource.newResource(docroot));
         context.addAliasCheck(new ContextHandler.AliasCheck()
         {
-            final AllowSymLinkAliasChecker symlinkcheck = new AllowSymLinkAliasChecker();
+            final AllowedResourceAliasChecker symlinkcheck = new AllowedResourceAliasChecker(context, false);
 
             @Override
             public boolean check(String path, Resource resource)

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

@@ -52,13 +52,12 @@
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpTester;
+import org.eclipse.jetty.server.AllowedResourceAliasChecker;
 import org.eclipse.jetty.server.HttpConfiguration;
 import org.eclipse.jetty.server.LocalConnector;
 import org.eclipse.jetty.server.ResourceContentFactory;
 import org.eclipse.jetty.server.ResourceService;
-import org.eclipse.jetty.server.SameFileAliasChecker;
 import org.eclipse.jetty.server.Server;
-import org.eclipse.jetty.server.handler.AllowSymLinkAliasChecker;
 import org.eclipse.jetty.toolchain.test.FS;
 import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
 import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
@@ -1097,8 +1096,6 @@ public void testSymLinks() throws Exception
             response = HttpTester.parseResponse(rawResponse);
             assertThat(response.toString(), response.getStatus(), is(HttpStatus.NOT_FOUND_404));
 
-            context.addAliasCheck(new AllowSymLinkAliasChecker());
-
             rawResponse = connector.getResponse("GET /context/dir/link.txt HTTP/1.0\r\n\r\n");
             response = HttpTester.parseResponse(rawResponse);
             assertThat(response.toString(), response.getStatus(), is(HttpStatus.OK_200));
@@ -2070,7 +2067,7 @@ public void testGetUtf8NfcFile() throws Exception
         FS.ensureEmpty(docRoot);
 
         context.addServlet(DefaultServlet.class, "/");
-        context.addAliasCheck(new SameFileAliasChecker());
+        context.addAliasCheck(new AllowedResourceAliasChecker(context, true));
 
         // Create file with UTF-8 NFC format
         String filename = "swedish-" + new String(TypeUtil.fromHexString("C3A5"), UTF_8) + ".txt";
@@ -2110,7 +2107,7 @@ public void testGetUtf8NfdFile() throws Exception
         FS.ensureEmpty(docRoot);
 
         context.addServlet(DefaultServlet.class, "/");
-        context.addAliasCheck(new SameFileAliasChecker());
+        context.addAliasCheck(new AllowedResourceAliasChecker(context, true));
 
         // Create file with UTF-8 NFD format
         String filename = "swedish-a" + new String(TypeUtil.fromHexString("CC8A"), UTF_8) + ".txt";