From c1106482ed10db5fbaca84e41fded52cc2788ff6 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 26 Aug 2021 16:01:35 +1000 Subject: [PATCH 1/7] Issue #6497 - Replace the Alias checkers with new implementation. Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/io/IOTest.java | 57 +++++ .../server/AllowedResourceAliasChecker.java | 228 +++++++++++++++++ .../jetty/server/SameFileAliasChecker.java | 2 + .../SymlinkAllowedResourceAliasChecker.java | 66 +++++ .../handler/AllowSymLinkAliasChecker.java | 7 + .../jetty/server/handler/ContextHandler.java | 23 +- .../handler/AllowSymLinkAliasCheckerTest.java | 3 +- .../ContextHandlerGetResourceTest.java | 7 +- .../jetty/servlet/DefaultServletTest.java | 11 +- .../jetty/test/AliasCheckerSymlinkTest.java | 232 ++++++++++++++++++ .../test-integration/src/test/resources/file | 1 + .../src/test/resources/sibling/file | 1 + .../test/resources/webroot/WEB-INF/web.xml | 1 + .../src/test/resources/webroot/documents/file | 1 + .../src/test/resources/webroot/file | 1 + .../src/test/resources/webroot/index.html | 4 + 16 files changed, 630 insertions(+), 15 deletions(-) create mode 100644 jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java create mode 100644 jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java create mode 100644 tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java create mode 100644 tests/test-integration/src/test/resources/file create mode 100644 tests/test-integration/src/test/resources/sibling/file create mode 100644 tests/test-integration/src/test/resources/webroot/WEB-INF/web.xml create mode 100644 tests/test-integration/src/test/resources/webroot/documents/file create mode 100644 tests/test-integration/src/test/resources/webroot/file create mode 100644 tests/test-integration/src/test/resources/webroot/index.html diff --git a/jetty-io/src/test/java/org/eclipse/jetty/io/IOTest.java b/jetty-io/src/test/java/org/eclipse/jetty/io/IOTest.java index 972e491c6a57..2139fa1c7ad4 100644 --- a/jetty-io/src/test/java/org/eclipse/jetty/io/IOTest.java +++ b/jetty-io/src/test/java/org/eclipse/jetty/io/IOTest.java @@ -43,6 +43,8 @@ import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.resource.PathResource; +import org.eclipse.jetty.util.resource.Resource; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInfo; @@ -545,4 +547,59 @@ public void testSelectorWakeup() throws Exception } } } + + @Test + public void testSymbolicLink(TestInfo testInfo) throws Exception + { + File dir = MavenTestingUtils.getTargetTestingDir(testInfo.getDisplayName()); + FS.ensureEmpty(dir); + File realFile = new File(dir, "real"); + Path realPath = realFile.toPath(); + FS.touch(realFile); + + File linkFile = new File(dir, "link"); + Path linkPath = linkFile.toPath(); + Files.createSymbolicLink(linkPath, realPath); + Path targPath = linkPath.toRealPath(); + + System.err.printf("realPath = %s%n", realPath); + System.err.printf("linkPath = %s%n", linkPath); + System.err.printf("targPath = %s%n", targPath); + + assertFalse(Files.isSymbolicLink(realPath)); + assertTrue(Files.isSymbolicLink(linkPath)); + + Resource link = new PathResource(dir).addPath("link"); + assertThat(link.isAlias(), is(true)); + } + + @Test + public void testSymbolicLinkDir(TestInfo testInfo) throws Exception + { + File dir = MavenTestingUtils.getTargetTestingDir(testInfo.getDisplayName()); + FS.ensureEmpty(dir); + + File realDirFile = new File(dir, "real"); + Path realDirPath = realDirFile.toPath(); + Files.createDirectories(realDirPath); + + File linkDirFile = new File(dir, "link"); + Path linkDirPath = linkDirFile.toPath(); + Files.createSymbolicLink(linkDirPath, realDirPath); + + File realFile = new File(realDirFile, "file"); + Path realPath = realFile.toPath(); + FS.touch(realFile); + + File linkFile = new File(linkDirFile, "file"); + Path linkPath = linkFile.toPath(); + Path targPath = linkPath.toRealPath(); + + System.err.printf("realPath = %s%n", realPath); + System.err.printf("linkPath = %s%n", linkPath); + System.err.printf("targPath = %s%n", targPath); + + assertFalse(Files.isSymbolicLink(realPath)); + assertFalse(Files.isSymbolicLink(linkPath)); + } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java new file mode 100644 index 000000000000..07ddd95dac90 --- /dev/null +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java @@ -0,0 +1,228 @@ +// +// ======================================================================== +// Copyright (c) 1995-2021 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.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.HashSet; + +import org.eclipse.jetty.server.handler.ContextHandler; +import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.util.component.AbstractLifeCycle; +import org.eclipse.jetty.util.resource.PathResource; +import org.eclipse.jetty.util.resource.Resource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + *

This will approve any alias to anything inside of the {@link ContextHandler}s resource base which + * is not protected by {@link ContextHandler#isProtectedTarget(String)}.

+ *

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.

+ *

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.

+ */ +public class AllowedResourceAliasChecker extends AbstractLifeCycle implements ContextHandler.AliasCheck +{ + private static final Logger LOG = LoggerFactory.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; + private final HashSet _protectedPaths = new HashSet<>(); + private Path _basePath; + + /** + * @param contextHandler the context handler to use. + */ + 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; + _checkSymlinkTargets = checkSymlinkTargets; + } + + protected ContextHandler getContextHandler() + { + return _contextHandler; + } + + protected Path getBasePath() + { + return _basePath; + } + + @Override + protected void doStart() throws Exception + { + _basePath = getPath(_contextHandler.getBaseResource()); + if (_basePath == null) + _basePath = Paths.get("/").toAbsolutePath(); + + String[] protectedTargets = _contextHandler.getProtectedTargets(); + if (protectedTargets != null) + { + for (String s : protectedTargets) + { + Path path = _basePath.getFileSystem().getPath(_basePath.toString(), s); + _protectedPaths.add(path); + } + } + } + + @Override + protected void doStop() throws Exception + { + _basePath = null; + _protectedPaths.clear(); + } + + @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 (isProtectedPath(resourcePath, NO_FOLLOW_LINKS)) + return false; + + if (_checkSymlinkTargets && hasSymbolicLink(resourcePath)) + { + if (isProtectedPath(resourcePath, FOLLOW_LINKS)) + return false; + } + } + catch (Throwable t) + { + LOG.warn("Failed to check alias", t); + return false; + } + + return true; + } + + /** + *

Determines whether the provided resource path is protected.

+ * + *

The resource path is protected if it is under one of the protected targets defined by + * {@link ContextHandler#isProtectedTarget(String)} in which case the alias should not be allowed. + * The resource path may also attempt to traverse above the root path and should be denied.

+ * + * @param resourcePath the resource {@link Path} to be tested. + * @param linkOptions an array of {@link LinkOption} to be provided to the {@link Path#toRealPath(LinkOption...)} method. + * @return true if the resource path is protected and the alias should not be allowed. + * @throws IOException if an I/O error occurs. + */ + protected boolean isProtectedPath(Path resourcePath, LinkOption[] linkOptions) throws IOException + { + // If the resource doesn't exist we cannot determine whether it is protected so we assume it is. + if (!Files.exists(resourcePath, linkOptions)) + return true; + + Path basePath = _basePath.toRealPath(linkOptions); + Path targetPath = resourcePath.toRealPath(linkOptions); + String target = targetPath.toString(); + + // The target path must be under the base resource directory. + if (!targetPath.startsWith(basePath)) + return true; + + for (Path protectedPath : _protectedPaths) + { + String protect; + if (Files.exists(protectedPath, linkOptions)) + protect = protectedPath.toRealPath(linkOptions).toString(); + else if (linkOptions == NO_FOLLOW_LINKS) + protect = protectedPath.normalize().toAbsolutePath().toString(); + else + protect = protectedPath.toFile().getCanonicalPath(); + + // If the target path is protected then we will not allow it. + if (StringUtil.startsWithIgnoreCase(target, protect)) + { + if (target.length() == protect.length()) + return true; + + // Check that the target prefix really is a path segment. + if (target.charAt(protect.length()) == File.separatorChar) + return true; + } + } + + return false; + } + + protected boolean hasSymbolicLink(Path path) + { + return hasSymbolicLink(path.getRoot(), path); + } + + protected boolean hasSymbolicLink(Path base, Path path) + { + Path p = path; + while (!base.equals(p)) + { + if (p == null) + throw new IllegalArgumentException("path was not child of base"); + + if (Files.isSymbolicLink(p)) + return true; + + p = p.getParent(); + } + + return false; + } + + private Path getPath(Resource resource) + { + try + { + if (resource instanceof PathResource) + return ((PathResource)resource).getPath(); + return resource.getFile().toPath(); + } + catch (Throwable t) + { + LOG.trace("getPath() failed", t); + return null; + } + } + + @Override + public String toString() + { + return String.format("%s@%x{checkSymlinkTargets=%s}", AllowedResourceAliasChecker.class.getSimpleName(), hashCode(), _checkSymlinkTargets); + } +} \ No newline at end of file diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/SameFileAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/SameFileAliasChecker.java index 1a50e5a2ffdd..406b6a4148ee 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/SameFileAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/SameFileAliasChecker.java @@ -39,7 +39,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. *

+ * @deprecated use {@link org.eclipse.jetty.server.AllowedResourceAliasChecker} instead. */ +@Deprecated public class SameFileAliasChecker implements AliasCheck { private static final Logger LOG = LoggerFactory.getLogger(SameFileAliasChecker.class); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java new file mode 100644 index 000000000000..ae7560728cbf --- /dev/null +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java @@ -0,0 +1,66 @@ +// +// ======================================================================== +// Copyright (c) 1995-2021 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.io.IOException; +import java.nio.file.Files; +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. + */ +public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChecker +{ + private static final Logger LOG = LoggerFactory.getLogger(SymlinkAllowedResourceAliasChecker.class); + + /** + * @param contextHandler the context handler to use. + */ + public SymlinkAllowedResourceAliasChecker(ContextHandler contextHandler) + { + super(contextHandler, false); + } + + @Override + public boolean check(String uri, Resource resource) + { + try + { + // Check the resource is allowed to be accessed. + if (!super.check(uri, resource)) + return false; + + // Approve if path is a symbolic link. + Path resourcePath = resource.getFile().toPath(); + if (Files.isSymbolicLink(resourcePath)) + return true; + + // Approve if path has symlink in under its resource base. + if (super.hasSymbolicLink(getBasePath(), resourcePath)) + return true; + } + catch (IOException e) + { + LOG.trace("Failed to check alias", e); + return false; + } + + return false; + } +} diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasChecker.java index 4b65536b31ea..bf807bd14b73 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasChecker.java @@ -28,11 +28,18 @@ * to check resources that are aliased to other locations. The checker uses the * Java {@link Files#readSymbolicLink(Path)} and {@link Path#toRealPath(java.nio.file.LinkOption...)} * APIs to check if a file is aliased with symbolic links.

+ * @deprecated use {@link org.eclipse.jetty.server.SymlinkAllowedResourceAliasChecker} instead. */ +@Deprecated public class AllowSymLinkAliasChecker implements AliasCheck { private static final Logger LOG = LoggerFactory.getLogger(AllowSymLinkAliasChecker.class); + public AllowSymLinkAliasChecker() + { + LOG.warn("Deprecated, use SymlinkAllowedResourceAliasChecker instead."); + } + @Override public boolean check(String uri, Resource resource) { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 414ff29d510a..5fe5f65f7c86 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -63,6 +63,7 @@ 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; @@ -70,6 +71,7 @@ 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.Index; @@ -81,6 +83,7 @@ import org.eclipse.jetty.util.annotation.ManagedObject; import org.eclipse.jetty.util.component.DumpableCollection; import org.eclipse.jetty.util.component.Graceful; +import org.eclipse.jetty.util.component.LifeCycle; import org.eclipse.jetty.util.resource.Resource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -103,8 +106,8 @@ * The executor is made available via a context attributed {@code org.eclipse.jetty.server.Executor}. *

*

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

*/ @ManagedObject("URI Context") @@ -263,9 +266,8 @@ protected ContextHandler(Context context, HandlerContainer parent, String contex _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); @@ -2071,6 +2073,10 @@ private String normalizeHostname(String host) public void addAliasCheck(AliasCheck check) { getAliasChecks().add(check); + if (check instanceof LifeCycle) + addManaged((LifeCycle)check); + else + addBean(check); } /** @@ -2086,7 +2092,7 @@ public List getAliasChecks() */ public void setAliasChecks(List checks) { - getAliasChecks().clear(); + clearAliasChecks(); getAliasChecks().addAll(checks); } @@ -2095,7 +2101,9 @@ public void setAliasChecks(List checks) */ public void clearAliasChecks() { - getAliasChecks().clear(); + List aliasChecks = getAliasChecks(); + aliasChecks.forEach(this::removeBean); + aliasChecks.clear(); } /** @@ -3028,7 +3036,9 @@ public interface AliasCheck /** * Approve all aliases. + * @deprecated use {@link org.eclipse.jetty.server.AllowedResourceAliasChecker} instead. */ + @Deprecated public static class ApproveAliases implements AliasCheck { @Override @@ -3041,6 +3051,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 diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasCheckerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasCheckerTest.java index 94aee97a9ffc..70ba419bc4cb 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasCheckerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasCheckerTest.java @@ -27,6 +27,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; @@ -179,7 +180,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(); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java index d304c0bb2160..4d055bf4609d 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java @@ -20,6 +20,7 @@ import java.util.concurrent.atomic.AtomicBoolean; 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.resource.Resource; @@ -104,11 +105,13 @@ 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 SymlinkAllowedResourceAliasChecker symlinkcheck = new SymlinkAllowedResourceAliasChecker(context); + { + context.addBean(symlinkcheck); + } @Override public boolean check(String path, Resource resource) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java index 66957e4d79ef..61a2e34c62da 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java @@ -47,13 +47,13 @@ import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.logging.StacklessLogging; +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.server.SymlinkAllowedResourceAliasChecker; import org.eclipse.jetty.toolchain.test.FS; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; import org.eclipse.jetty.toolchain.test.jupiter.WorkDir; @@ -1081,8 +1081,7 @@ public void testSymLinks() throws Exception response = HttpTester.parseResponse(rawResponse); assertThat(response.toString(), response.getStatus(), is(HttpStatus.NOT_FOUND_404)); - context.addAliasCheck(new AllowSymLinkAliasChecker()); - + context.addAliasCheck(new SymlinkAllowedResourceAliasChecker(context)); 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)); @@ -2056,7 +2055,7 @@ public void testGetUtf8NfcFile() throws Exception FS.ensureEmpty(docRoot); context.addServlet(DefaultServlet.class, "/"); - context.addAliasCheck(new SameFileAliasChecker()); + context.addAliasCheck(new AllowedResourceAliasChecker(context)); // Create file with UTF-8 NFC format String filename = "swedish-" + new String(TypeUtil.fromHexString("C3A5"), UTF_8) + ".txt"; @@ -2096,7 +2095,7 @@ public void testGetUtf8NfdFile() throws Exception FS.ensureEmpty(docRoot); context.addServlet(DefaultServlet.class, "/"); - context.addAliasCheck(new SameFileAliasChecker()); + context.addAliasCheck(new AllowedResourceAliasChecker(context)); // Create file with UTF-8 NFD format String filename = "swedish-a" + new String(TypeUtil.fromHexString("CC8A"), UTF_8) + ".txt"; diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java new file mode 100644 index 000000000000..64f7ab5cfc2f --- /dev/null +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java @@ -0,0 +1,232 @@ +// +// ======================================================================== +// Copyright (c) 1995-2021 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.test; + +import java.io.File; +import java.net.URI; +import java.net.URL; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.stream.Stream; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.server.AllowedResourceAliasChecker; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.server.SymlinkAllowedResourceAliasChecker; +import org.eclipse.jetty.server.handler.AllowSymLinkAliasChecker; +import org.eclipse.jetty.server.handler.ContextHandler; +import org.eclipse.jetty.servlet.DefaultServlet; +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.resource.PathResource; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +public class AliasCheckerSymlinkTest +{ + private static Server _server; + private static ServerConnector _connector; + private static HttpClient _client; + private static ServletContextHandler _context; + + private static Path _symlinkFile; + private static Path _symlinkExternalFile; + private static Path _symlinkDir; + private static Path _symlinkParentDir; + private static Path _symlinkSiblingDir; + private static Path _webInfSymlink; + private static Path _webrootSymlink; + + private static Path getResource(String path) throws Exception + { + URL url = AliasCheckerSymlinkTest.class.getClassLoader().getResource(path); + assertNotNull(url); + return new File(url.toURI()).toPath(); + } + + private static void delete(Path path) + { + IO.delete(path.toFile()); + } + + private static void setAliasChecker(ContextHandler.AliasCheck aliasChecker) + { + _context.clearAliasChecks(); + if (aliasChecker != null) + _context.addAliasCheck(aliasChecker); + } + + @BeforeAll + public static void beforeAll() throws Exception + { + Path webRootPath = getResource("webroot"); + Path fileInWebroot = webRootPath.resolve("file"); + + // Create symlink file that targets inside the webroot directory. + _symlinkFile = webRootPath.resolve("symlinkFile"); + delete(_symlinkFile); + Files.createSymbolicLink(_symlinkFile, fileInWebroot).toFile().deleteOnExit(); + + // Create symlink file that targets outside the webroot directory. + _symlinkExternalFile = webRootPath.resolve("symlinkExternalFile"); + delete(_symlinkExternalFile); + Files.createSymbolicLink(_symlinkExternalFile, getResource("file")).toFile().deleteOnExit(); + + // Symlink to a directory inside of the webroot. + _symlinkDir = webRootPath.resolve("symlinkDir"); + delete(_symlinkDir); + Files.createSymbolicLink(_symlinkDir, webRootPath.resolve("documents")).toFile().deleteOnExit(); + + // Symlink to a directory parent of the webroot. + _symlinkParentDir = webRootPath.resolve("symlinkParentDir"); + delete(_symlinkParentDir); + Files.createSymbolicLink(_symlinkParentDir, webRootPath.resolve("..")).toFile().deleteOnExit(); + + // Symlink to a directory outside of the webroot. + _symlinkSiblingDir = webRootPath.resolve("symlinkSiblingDir"); + delete(_symlinkSiblingDir); + Files.createSymbolicLink(_symlinkSiblingDir, webRootPath.resolve("../sibling")).toFile().deleteOnExit(); + + // Symlink to the WEB-INF directory. + _webInfSymlink = webRootPath.resolve("webInfSymlink"); + delete(_webInfSymlink); + Files.createSymbolicLink(_webInfSymlink, webRootPath.resolve("WEB-INF")).toFile().deleteOnExit(); + + // External symlink to webroot. + _webrootSymlink = webRootPath.resolve("../webrootSymlink"); + delete(_webrootSymlink); + Files.createSymbolicLink(_webrootSymlink, webRootPath).toFile().deleteOnExit(); + + // Create and start Server and Client. + _server = new Server(); + _connector = new ServerConnector(_server); + _server.addConnector(_connector); + _context = new ServletContextHandler(); + _context.setContextPath("/"); + _context.setBaseResource(new PathResource(webRootPath)); + _context.setWelcomeFiles(new String[]{"index.html"}); + _context.setProtectedTargets(new String[]{"/web-inf", "/meta-inf"}); + _context.getMimeTypes().addMimeMapping("txt", "text/plain;charset=utf-8"); + _server.setHandler(_context); + _context.addServlet(DefaultServlet.class, "/"); + _context.clearAliasChecks(); + _server.start(); + + _client = new HttpClient(); + _client.start(); + } + + @AfterAll + public static void afterAll() throws Exception + { + // Try to delete all files now so that the symlinks do not confuse other tests. + Files.delete(_symlinkFile); + Files.delete(_symlinkExternalFile); + Files.delete(_symlinkDir); + Files.delete(_symlinkParentDir); + Files.delete(_symlinkSiblingDir); + Files.delete(_webInfSymlink); + Files.delete(_webrootSymlink); + + _client.stop(); + _server.stop(); + } + + public static Stream testCases() + { + AllowedResourceAliasChecker allowedResource = new AllowedResourceAliasChecker(_context, false); + AllowedResourceAliasChecker allowedResourceTarget = new AllowedResourceAliasChecker(_context, true); + SymlinkAllowedResourceAliasChecker symlinkAllowedResource = new SymlinkAllowedResourceAliasChecker(_context); + AllowSymLinkAliasChecker allowSymlinks = new AllowSymLinkAliasChecker(); + ContextHandler.ApproveAliases approveAliases = new ContextHandler.ApproveAliases(); + + return Stream.of( + // AllowedResourceAliasChecker that does not check the target of symlinks. + Arguments.of(allowedResource, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."), + Arguments.of(allowedResource, "/symlinkExternalFile", HttpStatus.OK_200, "This file is outside webroot."), + Arguments.of(allowedResource, "/symlinkDir/file", HttpStatus.OK_200, "This file is inside webroot/documents."), + Arguments.of(allowedResource, "/symlinkParentDir/webroot/file", HttpStatus.OK_200, "This file is inside webroot."), + Arguments.of(allowedResource, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.OK_200, "This is the web.xml file."), + Arguments.of(allowedResource, "/symlinkSiblingDir/file", HttpStatus.OK_200, "This file is inside a sibling dir to webroot."), + Arguments.of(allowedResource, "/webInfSymlink/web.xml", HttpStatus.OK_200, "This is the web.xml file."), + + // AllowedResourceAliasChecker that checks the target of symlinks. + Arguments.of(allowedResourceTarget, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."), + Arguments.of(allowedResourceTarget, "/symlinkExternalFile", HttpStatus.NOT_FOUND_404, null), + Arguments.of(allowedResourceTarget, "/symlinkDir/file", HttpStatus.OK_200, "This file is inside webroot/documents."), + Arguments.of(allowedResourceTarget, "/symlinkParentDir/webroot/file", HttpStatus.OK_200, "This file is inside webroot."), + Arguments.of(allowedResourceTarget, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.NOT_FOUND_404, null), + Arguments.of(allowedResourceTarget, "/symlinkSiblingDir/file", HttpStatus.NOT_FOUND_404, null), + Arguments.of(allowedResourceTarget, "/webInfSymlink/web.xml", HttpStatus.NOT_FOUND_404, null), + + // SymlinkAllowedResourceAliasChecker that does not check the target of symlinks, but only approves files obtained through a symlink. + Arguments.of(symlinkAllowedResource, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."), + Arguments.of(symlinkAllowedResource, "/symlinkExternalFile", HttpStatus.OK_200, "This file is outside webroot."), + Arguments.of(symlinkAllowedResource, "/symlinkDir/file", HttpStatus.OK_200, "This file is inside webroot/documents."), + Arguments.of(symlinkAllowedResource, "/symlinkParentDir/webroot/file", HttpStatus.OK_200, "This file is inside webroot."), + Arguments.of(symlinkAllowedResource, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.OK_200, "This is the web.xml file."), + Arguments.of(symlinkAllowedResource, "/symlinkSiblingDir/file", HttpStatus.OK_200, "This file is inside a sibling dir to webroot."), + Arguments.of(symlinkAllowedResource, "/webInfSymlink/web.xml", HttpStatus.OK_200, "This is the web.xml file."), + + // The AllowSymLinkAliasChecker. + Arguments.of(allowSymlinks, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."), + Arguments.of(allowSymlinks, "/symlinkExternalFile", HttpStatus.OK_200, "This file is outside webroot."), + Arguments.of(allowSymlinks, "/symlinkDir/file", HttpStatus.OK_200, "This file is inside webroot/documents."), + Arguments.of(allowSymlinks, "/symlinkParentDir/webroot/file", HttpStatus.OK_200, "This file is inside webroot."), + Arguments.of(allowSymlinks, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.OK_200, "This is the web.xml file."), + Arguments.of(allowSymlinks, "/symlinkSiblingDir/file", HttpStatus.OK_200, "This file is inside a sibling dir to webroot."), + Arguments.of(allowSymlinks, "/webInfSymlink/web.xml", HttpStatus.OK_200, "This is the web.xml file."), + + // The ApproveAliases (approves everything regardless). + Arguments.of(approveAliases, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."), + Arguments.of(approveAliases, "/symlinkExternalFile", HttpStatus.OK_200, "This file is outside webroot."), + Arguments.of(approveAliases, "/symlinkDir/file", HttpStatus.OK_200, "This file is inside webroot/documents."), + Arguments.of(approveAliases, "/symlinkParentDir/webroot/file", HttpStatus.OK_200, "This file is inside webroot."), + Arguments.of(approveAliases, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.OK_200, "This is the web.xml file."), + Arguments.of(approveAliases, "/symlinkSiblingDir/file", HttpStatus.OK_200, "This file is inside a sibling dir to webroot."), + Arguments.of(approveAliases, "/webInfSymlink/web.xml", HttpStatus.OK_200, "This is the web.xml file."), + + // No alias checker (any symlink should be an alias). + Arguments.of(null, "/symlinkFile", HttpStatus.NOT_FOUND_404, null), + Arguments.of(null, "/symlinkExternalFile", HttpStatus.NOT_FOUND_404, null), + Arguments.of(null, "/symlinkDir/file", HttpStatus.NOT_FOUND_404, null), + Arguments.of(null, "/symlinkParentDir/webroot/file", HttpStatus.NOT_FOUND_404, null), + Arguments.of(null, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.NOT_FOUND_404, null), + Arguments.of(null, "/symlinkSiblingDir/file", HttpStatus.NOT_FOUND_404, null), + Arguments.of(null, "/webInfSymlink/web.xml", HttpStatus.NOT_FOUND_404, null) + ); + } + + @ParameterizedTest + @MethodSource("testCases") + public void test(ContextHandler.AliasCheck aliasChecker, String path, int httpStatus, String responseContent) throws Exception + { + setAliasChecker(aliasChecker); + URI uri = URI.create("http://localhost:" + _connector.getLocalPort() + path); + ContentResponse response = _client.GET(uri); + assertThat(response.getStatus(), is(httpStatus)); + if (responseContent != null) + assertThat(response.getContentAsString(), is(responseContent)); + } +} diff --git a/tests/test-integration/src/test/resources/file b/tests/test-integration/src/test/resources/file new file mode 100644 index 000000000000..4b745d8b20e7 --- /dev/null +++ b/tests/test-integration/src/test/resources/file @@ -0,0 +1 @@ +This file is outside webroot. \ No newline at end of file diff --git a/tests/test-integration/src/test/resources/sibling/file b/tests/test-integration/src/test/resources/sibling/file new file mode 100644 index 000000000000..f57b0b89d7ae --- /dev/null +++ b/tests/test-integration/src/test/resources/sibling/file @@ -0,0 +1 @@ +This file is inside a sibling dir to webroot. \ No newline at end of file diff --git a/tests/test-integration/src/test/resources/webroot/WEB-INF/web.xml b/tests/test-integration/src/test/resources/webroot/WEB-INF/web.xml new file mode 100644 index 000000000000..47d791449100 --- /dev/null +++ b/tests/test-integration/src/test/resources/webroot/WEB-INF/web.xml @@ -0,0 +1 @@ +This is the web.xml file. \ No newline at end of file diff --git a/tests/test-integration/src/test/resources/webroot/documents/file b/tests/test-integration/src/test/resources/webroot/documents/file new file mode 100644 index 000000000000..b45088053628 --- /dev/null +++ b/tests/test-integration/src/test/resources/webroot/documents/file @@ -0,0 +1 @@ +This file is inside webroot/documents. \ No newline at end of file diff --git a/tests/test-integration/src/test/resources/webroot/file b/tests/test-integration/src/test/resources/webroot/file new file mode 100644 index 000000000000..7a16a16aa081 --- /dev/null +++ b/tests/test-integration/src/test/resources/webroot/file @@ -0,0 +1 @@ +This file is inside webroot. \ No newline at end of file diff --git a/tests/test-integration/src/test/resources/webroot/index.html b/tests/test-integration/src/test/resources/webroot/index.html new file mode 100644 index 000000000000..f7dc59cdc6b6 --- /dev/null +++ b/tests/test-integration/src/test/resources/webroot/index.html @@ -0,0 +1,4 @@ + +

hello world

+

body of index.html

+ \ No newline at end of file From 8cb802f6b41fded557a9c52ff6f9b1d9373570c6 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 30 Aug 2021 12:48:05 +1000 Subject: [PATCH 2/7] Alternate fix for #6497 Signed-off-by: Greg Wilkins --- .../jetty/osgi/boot/OSGiWebappConstants.java | 2 +- .../server/AllowedResourceAliasChecker.java | 143 +++++------------- .../SymlinkAllowedResourceAliasChecker.java | 32 ++-- .../server/handler/ContextHandlerTest.java | 2 +- .../eclipse/jetty/webapp/WebAppContext.java | 2 +- .../jetty/test/AliasCheckerSymlinkTest.java | 24 +-- 6 files changed, 60 insertions(+), 145 deletions(-) diff --git a/jetty-osgi/jetty-osgi-boot/src/main/java/org/eclipse/jetty/osgi/boot/OSGiWebappConstants.java b/jetty-osgi/jetty-osgi-boot/src/main/java/org/eclipse/jetty/osgi/boot/OSGiWebappConstants.java index 6304fd6b2b45..4af831072e5c 100644 --- a/jetty-osgi/jetty-osgi-boot/src/main/java/org/eclipse/jetty/osgi/boot/OSGiWebappConstants.java +++ b/jetty-osgi/jetty-osgi-boot/src/main/java/org/eclipse/jetty/osgi/boot/OSGiWebappConstants.java @@ -134,5 +134,5 @@ public class OSGiWebappConstants /** * Set of extra dirs that must not be served by osgi webapps */ - public static final String[] DEFAULT_PROTECTED_OSGI_TARGETS = {"/osgi-inf", "/osgi-opts"}; + public static final String[] DEFAULT_PROTECTED_OSGI_TARGETS = {"/OSGI-INF", "/OSGI-OPTS"}; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java index 07ddd95dac90..4a5377730630 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java @@ -13,16 +13,16 @@ package org.eclipse.jetty.server; -import java.io.File; import java.io.IOException; import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.Path; import java.nio.file.Paths; -import java.util.HashSet; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; import org.eclipse.jetty.server.handler.ContextHandler; -import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.component.AbstractLifeCycle; import org.eclipse.jetty.util.resource.PathResource; import org.eclipse.jetty.util.resource.Resource; @@ -40,30 +40,19 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements ContextHandler.AliasCheck { private static final Logger LOG = LoggerFactory.getLogger(AllowedResourceAliasChecker.class); - private static final LinkOption[] FOLLOW_LINKS = new LinkOption[0]; - private static final LinkOption[] NO_FOLLOW_LINKS = new LinkOption[]{LinkOption.NOFOLLOW_LINKS}; + protected static final LinkOption[] FOLLOW_LINKS = new LinkOption[0]; + protected static final LinkOption[] NO_FOLLOW_LINKS = new LinkOption[]{LinkOption.NOFOLLOW_LINKS}; private final ContextHandler _contextHandler; - private final boolean _checkSymlinkTargets; - private final HashSet _protectedPaths = new HashSet<>(); - private Path _basePath; + private final List _protected = new ArrayList<>(); + protected Path _base; /** * @param contextHandler the context handler to use. */ 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; - _checkSymlinkTargets = checkSymlinkTargets; } protected ContextHandler getContextHandler() @@ -71,34 +60,28 @@ protected ContextHandler getContextHandler() return _contextHandler; } - protected Path getBasePath() - { - return _basePath; - } - @Override protected void doStart() throws Exception { - _basePath = getPath(_contextHandler.getBaseResource()); - if (_basePath == null) - _basePath = Paths.get("/").toAbsolutePath(); + _base = getPath(_contextHandler.getBaseResource()); + if (_base == null) + _base = Paths.get("/").toAbsolutePath(); + if (Files.exists(_base, NO_FOLLOW_LINKS)) + _base = _base.toRealPath(FOLLOW_LINKS); String[] protectedTargets = _contextHandler.getProtectedTargets(); if (protectedTargets != null) { for (String s : protectedTargets) - { - Path path = _basePath.getFileSystem().getPath(_basePath.toString(), s); - _protectedPaths.add(path); - } + _protected.add(_base.getFileSystem().getPath(_base.toString(), s)); } } @Override protected void doStop() throws Exception { - _basePath = null; - _protectedPaths.clear(); + _base = null; + _protected.clear(); } @Override @@ -108,104 +91,48 @@ public boolean check(String uri, Resource resource) if (!resource.exists()) return false; - Path resourcePath = getPath(resource); - if (resourcePath == null) + Path path = getPath(resource); + if (path == null) return false; try { - if (isProtectedPath(resourcePath, NO_FOLLOW_LINKS)) - return false; + Path link = path.toRealPath(NO_FOLLOW_LINKS); + Path real = path.toRealPath(FOLLOW_LINKS); - if (_checkSymlinkTargets && hasSymbolicLink(resourcePath)) - { - if (isProtectedPath(resourcePath, FOLLOW_LINKS)) - return false; - } + return isAllowed(real) && (link.equals(real) || isAllowed(link)); } catch (Throwable t) { LOG.warn("Failed to check alias", t); return false; } - - return true; } - /** - *

Determines whether the provided resource path is protected.

- * - *

The resource path is protected if it is under one of the protected targets defined by - * {@link ContextHandler#isProtectedTarget(String)} in which case the alias should not be allowed. - * The resource path may also attempt to traverse above the root path and should be denied.

- * - * @param resourcePath the resource {@link Path} to be tested. - * @param linkOptions an array of {@link LinkOption} to be provided to the {@link Path#toRealPath(LinkOption...)} method. - * @return true if the resource path is protected and the alias should not be allowed. - * @throws IOException if an I/O error occurs. - */ - protected boolean isProtectedPath(Path resourcePath, LinkOption[] linkOptions) throws IOException + protected boolean isAllowed(Path path) throws IOException { // If the resource doesn't exist we cannot determine whether it is protected so we assume it is. - if (!Files.exists(resourcePath, linkOptions)) - return true; - - Path basePath = _basePath.toRealPath(linkOptions); - Path targetPath = resourcePath.toRealPath(linkOptions); - String target = targetPath.toString(); - - // The target path must be under the base resource directory. - if (!targetPath.startsWith(basePath)) - return true; - - for (Path protectedPath : _protectedPaths) + if (Files.exists(path)) { - String protect; - if (Files.exists(protectedPath, linkOptions)) - protect = protectedPath.toRealPath(linkOptions).toString(); - else if (linkOptions == NO_FOLLOW_LINKS) - protect = protectedPath.normalize().toAbsolutePath().toString(); - else - protect = protectedPath.toFile().getCanonicalPath(); - - // If the target path is protected then we will not allow it. - if (StringUtil.startsWithIgnoreCase(target, protect)) + while (path != null) { - if (target.length() == protect.length()) - return true; - - // Check that the target prefix really is a path segment. - if (target.charAt(protect.length()) == File.separatorChar) + if (Files.isSameFile(path, _base)) return true; - } - } - - return false; - } - - protected boolean hasSymbolicLink(Path path) - { - return hasSymbolicLink(path.getRoot(), path); - } - protected boolean hasSymbolicLink(Path base, Path path) - { - Path p = path; - while (!base.equals(p)) - { - if (p == null) - throw new IllegalArgumentException("path was not child of base"); + for (Path protectedPath : _protected) + { + if (Files.exists(protectedPath, FOLLOW_LINKS) && Files.isSameFile(path, protectedPath)) + return false; + } - if (Files.isSymbolicLink(p)) - return true; - - p = p.getParent(); + path = path.getParent(); + } } return false; } - private Path getPath(Resource resource) + protected Path getPath(Resource resource) { try { @@ -223,6 +150,10 @@ private Path getPath(Resource resource) @Override public String toString() { - return String.format("%s@%x{checkSymlinkTargets=%s}", AllowedResourceAliasChecker.class.getSimpleName(), hashCode(), _checkSymlinkTargets); + return String.format("%s@%x{base=%s,protected=%s}", + AllowedResourceAliasChecker.class.getSimpleName(), + hashCode(), + _base, + Arrays.asList(_contextHandler.getProtectedTargets())); } } \ No newline at end of file diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java index ae7560728cbf..abce7d052cf1 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java @@ -13,8 +13,6 @@ 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; @@ -34,33 +32,29 @@ public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChec */ public SymlinkAllowedResourceAliasChecker(ContextHandler contextHandler) { - super(contextHandler, false); + super(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; + // The existence check resolves the symlinks. + if (!resource.exists()) + return false; - // Approve if path is a symbolic link. - Path resourcePath = resource.getFile().toPath(); - if (Files.isSymbolicLink(resourcePath)) - return true; + Path path = getPath(resource); + if (path == null) + return false; - // Approve if path has symlink in under its resource base. - if (super.hasSymbolicLink(getBasePath(), resourcePath)) - return true; + try + { + Path link = path.toRealPath(NO_FOLLOW_LINKS); + return path.equals(link) ? isAllowed(path) : isAllowed(link); } - catch (IOException e) + catch (Throwable t) { - LOG.trace("Failed to check alias", e); + LOG.warn("Failed to check alias", t); return false; } - - return false; } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java index 31b4f575bfa1..6f1a9a886701 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java @@ -643,7 +643,7 @@ public void testAttributes() throws Exception public void testProtected() throws Exception { ContextHandler handler = new ContextHandler(); - String[] protectedTargets = {"/foo-inf", "/bar-inf"}; + String[] protectedTargets = {"/FOO-INF", "/BAR-INF"}; handler.setProtectedTargets(protectedTargets); assertTrue(handler.isProtectedTarget("/foo-inf")); diff --git a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java index 1a2af27da4c7..a85ef9354c04 100644 --- a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java +++ b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java @@ -158,7 +158,7 @@ public class WebAppContext extends ServletContextHandler implements WebAppClassL public static final String SERVER_SYS_CLASSES = "org.eclipse.jetty.webapp.systemClasses"; public static final String SERVER_SRV_CLASSES = "org.eclipse.jetty.webapp.serverClasses"; - private static String[] __dftProtectedTargets = {"/web-inf", "/meta-inf"}; + private static String[] __dftProtectedTargets = {"/WEB-INF", "/META-INF"}; // System classes are classes that cannot be replaced by // the web application, and they are *always* loaded via diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java index 64f7ab5cfc2f..165238eb6df9 100644 --- a/tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java @@ -126,7 +126,7 @@ public static void beforeAll() throws Exception _context.setContextPath("/"); _context.setBaseResource(new PathResource(webRootPath)); _context.setWelcomeFiles(new String[]{"index.html"}); - _context.setProtectedTargets(new String[]{"/web-inf", "/meta-inf"}); + _context.setProtectedTargets(new String[]{"/WEB-INF", "/META-INF"}); _context.getMimeTypes().addMimeMapping("txt", "text/plain;charset=utf-8"); _server.setHandler(_context); _context.addServlet(DefaultServlet.class, "/"); @@ -155,30 +155,20 @@ public static void afterAll() throws Exception public static Stream testCases() { - AllowedResourceAliasChecker allowedResource = new AllowedResourceAliasChecker(_context, false); - AllowedResourceAliasChecker allowedResourceTarget = new AllowedResourceAliasChecker(_context, true); + AllowedResourceAliasChecker allowedResource = new AllowedResourceAliasChecker(_context); SymlinkAllowedResourceAliasChecker symlinkAllowedResource = new SymlinkAllowedResourceAliasChecker(_context); AllowSymLinkAliasChecker allowSymlinks = new AllowSymLinkAliasChecker(); ContextHandler.ApproveAliases approveAliases = new ContextHandler.ApproveAliases(); return Stream.of( - // AllowedResourceAliasChecker that does not check the target of symlinks. + // AllowedResourceAliasChecker that checks the target of symlinks. Arguments.of(allowedResource, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."), - Arguments.of(allowedResource, "/symlinkExternalFile", HttpStatus.OK_200, "This file is outside webroot."), + Arguments.of(allowedResource, "/symlinkExternalFile", HttpStatus.NOT_FOUND_404, null), Arguments.of(allowedResource, "/symlinkDir/file", HttpStatus.OK_200, "This file is inside webroot/documents."), Arguments.of(allowedResource, "/symlinkParentDir/webroot/file", HttpStatus.OK_200, "This file is inside webroot."), - Arguments.of(allowedResource, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.OK_200, "This is the web.xml file."), - Arguments.of(allowedResource, "/symlinkSiblingDir/file", HttpStatus.OK_200, "This file is inside a sibling dir to webroot."), - Arguments.of(allowedResource, "/webInfSymlink/web.xml", HttpStatus.OK_200, "This is the web.xml file."), - - // AllowedResourceAliasChecker that checks the target of symlinks. - Arguments.of(allowedResourceTarget, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."), - Arguments.of(allowedResourceTarget, "/symlinkExternalFile", HttpStatus.NOT_FOUND_404, null), - Arguments.of(allowedResourceTarget, "/symlinkDir/file", HttpStatus.OK_200, "This file is inside webroot/documents."), - Arguments.of(allowedResourceTarget, "/symlinkParentDir/webroot/file", HttpStatus.OK_200, "This file is inside webroot."), - Arguments.of(allowedResourceTarget, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.NOT_FOUND_404, null), - Arguments.of(allowedResourceTarget, "/symlinkSiblingDir/file", HttpStatus.NOT_FOUND_404, null), - Arguments.of(allowedResourceTarget, "/webInfSymlink/web.xml", HttpStatus.NOT_FOUND_404, null), + Arguments.of(allowedResource, "/symlinkParentDir/webroot/WEB-INF/web.xml", HttpStatus.NOT_FOUND_404, null), + Arguments.of(allowedResource, "/symlinkSiblingDir/file", HttpStatus.NOT_FOUND_404, null), + Arguments.of(allowedResource, "/webInfSymlink/web.xml", HttpStatus.NOT_FOUND_404, null), // SymlinkAllowedResourceAliasChecker that does not check the target of symlinks, but only approves files obtained through a symlink. Arguments.of(symlinkAllowedResource, "/symlinkFile", HttpStatus.OK_200, "This file is inside webroot."), From a51e570e0c7c7e73b2be0315a95535c87d2c30de Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 30 Aug 2021 14:46:54 +1000 Subject: [PATCH 3/7] Alternate fix for #6497 Signed-off-by: Greg Wilkins --- .../maven/plugin/MavenWebAppContext.java | 8 ++++---- .../server/AllowedResourceAliasChecker.java | 12 +++++++---- .../jetty/server/SameFileAliasChecker.java | 2 +- .../SymlinkAllowedResourceAliasChecker.java | 20 ++++++++++++++++--- .../handler/AllowSymLinkAliasChecker.java | 2 +- .../jetty/server/handler/ContextHandler.java | 20 +++++++++---------- .../ContextHandlerGetResourceTest.java | 4 ++-- .../eclipse/jetty/webapp/WebAppContext.java | 12 +++++------ 8 files changed, 49 insertions(+), 31 deletions(-) diff --git a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java index 74a60614ba0a..ee81ee465b59 100644 --- a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java +++ b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java @@ -351,18 +351,18 @@ public void doStop() throws Exception } @Override - public Resource getResource(String uriInContext) throws MalformedURLException + public Resource getResource(String pathInContext) throws MalformedURLException { Resource resource = null; // Try to get regular resource - resource = super.getResource(uriInContext); + resource = super.getResource(pathInContext); // 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) + if ((resource == null || !resource.exists()) && pathInContext != null && _classes != null) { // Canonicalize again to look for the resource inside /WEB-INF subdirectories. - String uri = URIUtil.canonicalPath(uriInContext); + String uri = URIUtil.canonicalPath(pathInContext); if (uri == null) return null; diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java index 4a5377730630..5132c94ba048 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java @@ -85,7 +85,7 @@ protected void doStop() throws Exception } @Override - public boolean check(String uri, Resource resource) + public boolean check(String pathInContext, Resource resource) { // The existence check resolves the symlinks. if (!resource.exists()) @@ -121,8 +121,12 @@ protected boolean isAllowed(Path path) throws IOException for (Path protectedPath : _protected) { - if (Files.exists(protectedPath, FOLLOW_LINKS) && Files.isSameFile(path, protectedPath)) - return false; + if (Files.exists(protectedPath)) + { + protectedPath = protectedPath.toRealPath(FOLLOW_LINKS); + if (Files.exists(protectedPath) && Files.isSameFile(path, protectedPath)) + return false; + } } path = path.getParent(); @@ -151,7 +155,7 @@ protected Path getPath(Resource resource) public String toString() { return String.format("%s@%x{base=%s,protected=%s}", - AllowedResourceAliasChecker.class.getSimpleName(), + this.getClass().getSimpleName(), hashCode(), _base, Arrays.asList(_contextHandler.getProtectedTargets())); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/SameFileAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/SameFileAliasChecker.java index 406b6a4148ee..8185766d46ee 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/SameFileAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/SameFileAliasChecker.java @@ -47,7 +47,7 @@ public class SameFileAliasChecker implements AliasCheck private static final Logger LOG = LoggerFactory.getLogger(SameFileAliasChecker.class); @Override - public boolean check(String uri, Resource resource) + public boolean check(String pathInContext, Resource resource) { // Only support PathResource alias checking if (!(resource instanceof PathResource)) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java index abce7d052cf1..960c6345824f 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java @@ -13,6 +13,7 @@ package org.eclipse.jetty.server; +import java.nio.file.Files; import java.nio.file.Path; import org.eclipse.jetty.server.handler.ContextHandler; @@ -36,7 +37,7 @@ public SymlinkAllowedResourceAliasChecker(ContextHandler contextHandler) } @Override - public boolean check(String uri, Resource resource) + public boolean check(String pathInContext, Resource resource) { // The existence check resolves the symlinks. if (!resource.exists()) @@ -46,15 +47,28 @@ public boolean check(String uri, Resource resource) if (path == null) return false; + String[] segments = pathInContext.split("/"); + Path fromBase = _base; + try { - Path link = path.toRealPath(NO_FOLLOW_LINKS); - return path.equals(link) ? isAllowed(path) : isAllowed(link); + for (String segment : segments) + { + fromBase = fromBase.resolve(segment); + if (!Files.exists(fromBase)) + return false; + if (Files.isSymbolicLink(fromBase)) + return true; + if (!isAllowed(fromBase)) + return false; + } } catch (Throwable t) { LOG.warn("Failed to check alias", t); return false; } + + return true; } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasChecker.java index bf807bd14b73..dd81bbb8f605 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/AllowSymLinkAliasChecker.java @@ -41,7 +41,7 @@ public AllowSymLinkAliasChecker() } @Override - public boolean check(String uri, Resource resource) + public boolean check(String pathInContext, Resource resource) { // Only support PathResource alias checking if (!(resource instanceof PathResource)) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 5fe5f65f7c86..b95ad58cfeee 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -1920,14 +1920,14 @@ public Map getLocaleEncodings() /** * Attempt to get a Resource from the Context. * - * @param path the path within the resource to attempt to get + * @param pathInContext the path within the base resource to attempt to get * @return the resource, or null if not available. * @throws MalformedURLException if unable to form a Resource from the provided path */ - public Resource getResource(String path) throws MalformedURLException + public Resource getResource(String pathInContext) throws MalformedURLException { - if (path == null || !path.startsWith(URIUtil.SLASH)) - throw new MalformedURLException(path); + if (pathInContext == null || !pathInContext.startsWith(URIUtil.SLASH)) + throw new MalformedURLException(pathInContext); if (_baseResource == null) return null; @@ -1937,9 +1937,9 @@ public Resource getResource(String path) throws MalformedURLException // 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); + Resource resource = _baseResource.addPath(pathInContext); - if (checkAlias(path, resource)) + if (checkAlias(pathInContext, resource)) return resource; return null; } @@ -3027,11 +3027,11 @@ public interface AliasCheck /** * Check an alias * - * @param path The path the aliased resource was created for + * @param pathInContext The path the aliased resource was created for * @param resource The aliased resourced * @return True if the resource is OK to be served. */ - boolean check(String path, Resource resource); + boolean check(String pathInContext, Resource resource); } /** @@ -3042,7 +3042,7 @@ public interface AliasCheck public static class ApproveAliases implements AliasCheck { @Override - public boolean check(String path, Resource resource) + public boolean check(String pathInContext, Resource resource) { return true; } @@ -3055,7 +3055,7 @@ public boolean check(String path, Resource resource) public static class ApproveNonExistentDirectoryAliases implements AliasCheck { @Override - public boolean check(String path, Resource resource) + public boolean check(String pathInContext, Resource resource) { if (resource.exists()) return false; diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java index 4d055bf4609d..51c952a6dd25 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java @@ -114,12 +114,12 @@ public static void beforeClass() throws Exception } @Override - public boolean check(String path, Resource resource) + public boolean check(String pathInContext, Resource resource) { if (allowAliases.get()) return true; if (allowSymlinks.get()) - return symlinkcheck.check(path, resource); + return symlinkcheck.check(pathInContext, resource); return allowAliases.get(); } }); diff --git a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java index a85ef9354c04..14c1118335c8 100644 --- a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java +++ b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java @@ -395,23 +395,23 @@ public void setClassLoader(ClassLoader classLoader) } @Override - public Resource getResource(String uriInContext) throws MalformedURLException + public Resource getResource(String pathInContext) throws MalformedURLException { - if (uriInContext == null || !uriInContext.startsWith(URIUtil.SLASH)) - throw new MalformedURLException(uriInContext); + if (pathInContext == null || !pathInContext.startsWith(URIUtil.SLASH)) + throw new MalformedURLException(pathInContext); MalformedURLException mue = null; Resource resource = null; int loop = 0; - while (uriInContext != null && loop++ < 100) + while (pathInContext != null && loop++ < 100) { try { - resource = super.getResource(uriInContext); + resource = super.getResource(pathInContext); if (resource != null && resource.exists()) return resource; - uriInContext = getResourceAlias(uriInContext); + pathInContext = getResourceAlias(pathInContext); } catch (MalformedURLException e) { From c29ba3e5eeb51ee0a3f2fbda3f2261605babb18d Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 30 Aug 2021 16:33:29 +1000 Subject: [PATCH 4/7] Alternate fix for #6497 Signed-off-by: Greg Wilkins --- .../server/AllowedResourceAliasChecker.java | 40 +++++++++++++++---- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java index 5132c94ba048..734a1b357724 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java @@ -21,6 +21,8 @@ import java.util.ArrayList; 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; @@ -100,6 +102,7 @@ public boolean check(String pathInContext, Resource resource) Path link = path.toRealPath(NO_FOLLOW_LINKS); Path real = path.toRealPath(FOLLOW_LINKS); + // 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)); } catch (Throwable t) @@ -114,19 +117,22 @@ protected boolean isAllowed(Path path) throws IOException // 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 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)) return true; - for (Path protectedPath : _protected) + for (Path p : protect) { - if (Files.exists(protectedPath)) - { - protectedPath = protectedPath.toRealPath(FOLLOW_LINKS); - if (Files.exists(protectedPath) && Files.isSameFile(path, protectedPath)) - return false; - } + if (Files.isSameFile(path, p)) + return false; } path = path.getParent(); @@ -136,6 +142,24 @@ protected boolean isAllowed(Path path) throws IOException return false; } + private static Path getRealPath(Path path) + { + if (path == null || !Files.exists(path)) + return null; + try + { + path = path.toRealPath(FOLLOW_LINKS); + if (Files.exists(path)) + return path; + } + catch (IOException e) + { + if (LOG.isDebugEnabled()) + LOG.debug("No real path for {}", path, e); + } + return null; + } + protected Path getPath(Resource resource) { try @@ -160,4 +184,4 @@ public String toString() _base, Arrays.asList(_contextHandler.getProtectedTargets())); } -} \ No newline at end of file +} From 497687d4151c8ec8eb01fcdf69ab68c420c5252c Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 31 Aug 2021 08:54:17 +1000 Subject: [PATCH 5/7] Alternate fix for #6497 Signed-off-by: Greg Wilkins --- .../server/AllowedResourceAliasChecker.java | 77 ++++++++++++------- .../SymlinkAllowedResourceAliasChecker.java | 37 ++++----- 2 files changed, 69 insertions(+), 45 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java index 734a1b357724..0882eb03e09b 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java +++ b/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 @@ /** *

This will approve any alias to anything inside of the {@link ContextHandler}s resource base which - * is not protected by {@link ContextHandler#isProtectedTarget(String)}.

- *

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.

+ * is not protected by a protected target as defined by {@link ContextHandler#getProtectedTargets()} at start.

*

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.

*/ @@ -89,52 +87,60 @@ 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 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(); } } @@ -142,6 +148,23 @@ protected boolean isAllowed(Path path) throws IOException 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)) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java index 960c6345824f..dea79aba1e29 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java +++ b/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); } } From 48880c65d1e1f4711e5e5cee328bd8fe876af5dd Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 31 Aug 2021 08:56:27 +1000 Subject: [PATCH 6/7] Alternate fix for #6497 Signed-off-by: Greg Wilkins --- .../eclipse/jetty/server/AllowedResourceAliasChecker.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java index 0882eb03e09b..d10fb622fb55 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java @@ -111,17 +111,17 @@ public boolean check(String pathInContext, Resource resource) } } - protected boolean check(String pathInContext, Path path) throws Exception + protected boolean check(String pathInContext, Path path) { // Allow any aliases (symlinks, 8.3, casing, etc.) so long as // the resulting real file is allowed. - return isAllowed(path.toRealPath(FOLLOW_LINKS)); + return isAllowed(getRealPath(path)); } 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)) + if (path != null && Files.exists(path)) { // Walk the path parent links looking for the base resource, but failing if any steps are protected while (path != null) From 87c0f102ce23368558bc1660257315ea367665b2 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 31 Aug 2021 13:59:56 +1000 Subject: [PATCH 7/7] Fixed test for changed dump format Signed-off-by: Greg Wilkins --- .../jetty/server/AllowedResourceAliasChecker.java | 5 ----- .../SymlinkAllowedResourceAliasChecker.java | 15 +++++++++++++-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java index d10fb622fb55..a0debdcbc90d 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java @@ -13,7 +13,6 @@ package org.eclipse.jetty.server; -import java.io.File; import java.io.IOException; import java.nio.file.Files; import java.nio.file.LinkOption; @@ -89,10 +88,6 @@ public boolean check(String pathInContext, Resource resource) { try { - // 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; diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java b/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java index dea79aba1e29..8130195f9726 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java @@ -13,6 +13,7 @@ package org.eclipse.jetty.server; +import java.io.File; import java.nio.file.Files; import java.nio.file.Path; @@ -39,7 +40,13 @@ public SymlinkAllowedResourceAliasChecker(ContextHandler contextHandler) @Override protected boolean check(String pathInContext, Path path) { - // Split the URI path so we can walk down the resource tree and build the realURI of any symlink found + // do not allow any file separation characters in the URI, as we need to know exactly what are the segments + if (File.separatorChar != '/' && pathInContext.indexOf(File.separatorChar) >= 0) + return false; + + // Split the URI path into segments, to walk down the resource tree and build the realURI of any symlink found + // 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(); @@ -53,13 +60,17 @@ protected boolean check(String pathInContext, Path path) 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 + // 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 is not allowed then do not allow. if (!isAllowed(fromBase)) return false; + + // TODO as we are building the realURI of the resource, it would be possible to + // re-check that against security constraints. } } catch (Throwable t)