From 497687d4151c8ec8eb01fcdf69ab68c420c5252c Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 31 Aug 2021 08:54:17 +1000 Subject: [PATCH] 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); } }