From aa793eeafe9669abad81582b94b1f485a34aa930 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 21 Sep 2021 18:35:51 +1000 Subject: [PATCH] Jetty 10.0.x 6497 alias checkers alt (#6681) * Issue #6497 - Replace the Alias checkers with new implementation. Signed-off-by: Lachlan Roberts Signed-off-by: Greg Wilkins Co-authored-by: Lachlan Roberts --- .../java/org/eclipse/jetty/io/IOTest.java | 57 +++++ .../maven/plugin/MavenWebAppContext.java | 8 +- .../jetty/osgi/boot/OSGiWebappConstants.java | 2 +- .../server/AllowedResourceAliasChecker.java | 205 ++++++++++++++++ .../jetty/server/SameFileAliasChecker.java | 4 +- .../SymlinkAllowedResourceAliasChecker.java | 86 +++++++ .../handler/AllowSymLinkAliasChecker.java | 9 +- .../jetty/server/handler/ContextHandler.java | 43 ++-- .../handler/AllowSymLinkAliasCheckerTest.java | 3 +- .../ContextHandlerGetResourceTest.java | 11 +- .../server/handler/ContextHandlerTest.java | 2 +- .../jetty/servlet/DefaultServletTest.java | 11 +- .../eclipse/jetty/webapp/WebAppContext.java | 14 +- .../jetty/test/AliasCheckerSymlinkTest.java | 222 ++++++++++++++++++ .../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 + 20 files changed, 644 insertions(+), 42 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-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-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 new file mode 100644 index 000000000000..a0debdcbc90d --- /dev/null +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java @@ -0,0 +1,205 @@ +// +// ======================================================================== +// 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.LinkOption; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Objects; + +import org.eclipse.jetty.server.handler.ContextHandler; +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 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.

+ */ +public class AllowedResourceAliasChecker extends AbstractLifeCycle implements ContextHandler.AliasCheck +{ + private static final Logger LOG = LoggerFactory.getLogger(AllowedResourceAliasChecker.class); + 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 List _protected = new ArrayList<>(); + protected Path _base; + + /** + * @param contextHandler the context handler to use. + */ + public AllowedResourceAliasChecker(ContextHandler contextHandler) + { + _contextHandler = contextHandler; + } + + protected ContextHandler getContextHandler() + { + return _contextHandler; + } + + @Override + protected void doStart() throws Exception + { + _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) + _protected.add(_base.getFileSystem().getPath(_base.toString(), s)); + } + } + + @Override + protected void doStop() throws Exception + { + _base = null; + _protected.clear(); + } + + @Override + public boolean check(String pathInContext, Resource resource) + { + try + { + // The existence check resolves the symlinks. + if (!resource.exists()) + return false; + + Path path = getPath(resource); + if (path == null) + return false; + + return check(pathInContext, path); + } + catch (Throwable t) + { + if (LOG.isDebugEnabled()) + LOG.debug("Failed to check alias", t); + return false; + } + } + + 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(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 (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) + { + // 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; + + // If the path is the same file as any protected resources, then it is protected. + for (Path p : _protected) + { + if (isSameFile(path, p)) + return false; + } + + // Walks up the aliased path name, not the real path name. + // If WEB-INF is a link to /var/lib/webmeta then after checking + // a URI of /WEB-INF/file.xml the parent is /WEB-INF and not /var/lib/webmeta + path = path.getParent(); + } + } + + return false; + } + + protected boolean isSameFile(Path path1, Path path2) + { + if (Objects.equals(path1, path2)) + return true; + try + { + if (Files.isSameFile(path1, path2)) + return true; + } + catch (Throwable t) + { + if (LOG.isDebugEnabled()) + LOG.debug("ignored", t); + } + return false; + } + + private static Path getRealPath(Path path) + { + if (path == null || !Files.exists(path)) + 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 + { + 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{base=%s,protected=%s}", + 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 1a50e5a2ffdd..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 @@ -39,13 +39,15 @@ * 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); @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 new file mode 100644 index 000000000000..8130195f9726 --- /dev/null +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java @@ -0,0 +1,86 @@ +// +// ======================================================================== +// 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.nio.file.Files; +import java.nio.file.Path; + +import org.eclipse.jetty.server.handler.ContextHandler; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * 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 +{ + private static final Logger LOG = LoggerFactory.getLogger(SymlinkAllowedResourceAliasChecker.class); + + /** + * @param contextHandler the context handler to use. + */ + public SymlinkAllowedResourceAliasChecker(ContextHandler contextHandler) + { + super(contextHandler); + } + + @Override + protected boolean check(String pathInContext, Path path) + { + // 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(); + + try + { + for (String segment : segments) + { + // Add the segment to the path and realURI. + fromBase = fromBase.resolve(segment); + realURI.append("/").append(fromBase.toRealPath(NO_FOLLOW_LINKS).getFileName()); + + // If the ancestor of the alias is a symlink, then check if the real URI is protected, otherwise allow. + // This allows symlinks like /other->/WEB-INF and /external->/var/lib/docroot + // This does not allow symlinks like /WeB-InF->/var/lib/other + if (Files.isSymbolicLink(fromBase)) + return !getContextHandler().isProtectedTarget(realURI.toString()); + + // If the ancestor 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) + { + if (LOG.isDebugEnabled()) + LOG.debug("Failed to check alias", t); + return false; + } + + // No symlink found, so must be allowed. Double check it is the right path we checked. + return isSameFile(fromBase, path); + } +} 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..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 @@ -28,13 +28,20 @@ * 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) + 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 414ff29d510a..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 @@ -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); @@ -1918,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; @@ -1935,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; } @@ -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(); } /** @@ -3019,20 +3027,22 @@ 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); } /** * Approve all aliases. + * @deprecated use {@link org.eclipse.jetty.server.AllowedResourceAliasChecker} instead. */ + @Deprecated public static class ApproveAliases implements AliasCheck { @Override - public boolean check(String path, Resource resource) + public boolean check(String pathInContext, Resource resource) { return true; } @@ -3041,10 +3051,11 @@ 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 - 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/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..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 @@ -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,19 +105,21 @@ 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) + 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-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-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/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..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 @@ -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 @@ -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) { 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..165238eb6df9 --- /dev/null +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java @@ -0,0 +1,222 @@ +// +// ======================================================================== +// 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); + SymlinkAllowedResourceAliasChecker symlinkAllowedResource = new SymlinkAllowedResourceAliasChecker(_context); + AllowSymLinkAliasChecker allowSymlinks = new AllowSymLinkAliasChecker(); + ContextHandler.ApproveAliases approveAliases = new ContextHandler.ApproveAliases(); + + return Stream.of( + // AllowedResourceAliasChecker that checks the target of symlinks. + Arguments.of(allowedResource, "/symlinkFile", HttpStatus.OK_200, "This file is inside 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.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."), + 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