From 68d93fbb194b6fc216b9c930f0d4beb0bec72919 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 27 Jul 2021 14:41:31 +1000 Subject: [PATCH] Issue #6497 - Improve comments, re-implement the hasSymbolicLink method. Signed-off-by: Lachlan Roberts --- .../server/AllowedResourceAliasChecker.java | 26 +++++++++++++------ .../SymlinkAllowedResourceAliasChecker.java | 7 +++-- .../jetty/test/AliasCheckerSymlinkTest.java | 21 +++++---------- 3 files changed, 28 insertions(+), 26 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 00c07e8aa19b..a646d7d74496 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 @@ -90,6 +90,18 @@ public boolean check(String uri, Resource resource) 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 { String basePath = Objects.requireNonNull(getPath(_contextHandler.getBaseResource())).toRealPath(linkOptions).toString(); @@ -125,16 +137,14 @@ protected boolean isProtectedPath(Path resourcePath, LinkOption[] linkOptions) t protected boolean hasSymbolicLink(Path path) { - // Is file itself a symlink? - if (Files.isSymbolicLink(path)) - return true; + return hasSymbolicLink(path.getRoot(), path); + } - // Lets try each path segment - Path base = path.getRoot(); - for (Path segment : path) + protected boolean hasSymbolicLink(Path base, Path path) + { + for (Path p = path; (p != null) && !p.equals(base); p = p.getParent()) { - base = base.resolve(segment); - if (Files.isSymbolicLink(base)) + if (Files.isSymbolicLink(p)) return true; } 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 d9442b903a4a..1a874b92d118 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 @@ -28,7 +28,7 @@ import org.eclipse.jetty.util.resource.Resource; /** - * An extension of {@link AllowedResourceAliasChecker} which only allows resources if they are symlinks. + * An extension of {@link AllowedResourceAliasChecker} which only allows aliased resources if they are symlinks. */ public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChecker { @@ -51,13 +51,12 @@ public boolean check(String uri, Resource resource) if (!super.check(uri, resource)) return false; - // Only approve resource if it is accessed by a symbolic link. + // Approve if path is a symbolic link. Path resourcePath = resource.getFile().toPath(); if (Files.isSymbolicLink(resourcePath)) return true; - // TODO: If base resource contains symlink then this will always return true. - // But we don't want to deny all paths if the resource base is symbolically linked. + // Approve if path has symlink in under its resource base. if (super.hasSymbolicLink(resourcePath)) return true; } 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 05cb240bd1fd..dc96c25f8a34 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 @@ -40,7 +40,6 @@ import org.eclipse.jetty.util.resource.PathResource; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -142,12 +141,6 @@ public static void afterAll() throws Exception _server.stop(); } - @Test - public void test() throws Exception - { - _server.join(); - } - public static Stream testCases() { AllowedResourceAliasChecker allowedResource = new AllowedResourceAliasChecker(_context, false); @@ -176,13 +169,13 @@ public static Stream testCases() 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(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."), + 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."),