Skip to content

Commit

Permalink
Alternate fix for #6497
Browse files Browse the repository at this point in the history 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
  • Loading branch information
@gregw
gregw committed Aug 30, 2021
1 parent c110648 commit 8cb802f
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 145 deletions.
2 changes: 1 addition & 1 deletion ...y-osgi/jetty-osgi-boot/src/main/java/org/eclipse/jetty/osgi/boot/OSGiWebappConstants.java
View file
Expand Up @@ -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"};
}
143 changes: 37 additions & 106 deletions jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java
View file
Expand Up @@ -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;
Expand All @@ -40,65 +40,48 @@
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<Path> _protectedPaths = new HashSet<>();
private Path _basePath;
private final List<Path> _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()
{
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
Expand All @@ -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;
}

/**
* <p>Determines whether the provided resource path is protected.</p>
*
* <p>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.</p>
*
* @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
{
Expand All @@ -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()));
}
}
32 changes: 13 additions & 19 deletions jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java
View file
Expand Up @@ -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;
Expand All @@ -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;
}
}
2 changes: 1 addition & 1 deletion jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java
View file
Expand Up @@ -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"));
Expand Down
2 changes: 1 addition & 1 deletion jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java
View file
Expand Up @@ -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
Expand Down
24 changes: 7 additions & 17 deletions tests/test-integration/src/test/java/org/eclipse/jetty/test/AliasCheckerSymlinkTest.java
View file
Expand Up @@ -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, "/");
Expand Down Expand Up @@ -155,30 +155,20 @@ public static void afterAll() throws Exception

public static Stream<Arguments> 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."),
Expand Down

0 comments on commit 8cb802f

Please sign in to comment.