From 8cb802f6b41fded557a9c52ff6f9b1d9373570c6 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 30 Aug 2021 12:48:05 +1000 Subject: [PATCH] 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."),