fix alias checkers to allow use of CombinedResource
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
This commit is contained in:
parent
02f583ef3d
commit
aaaf7aa67b
|
@ -26,6 +26,7 @@ import java.util.function.Supplier;
|
|||
import org.eclipse.jetty.server.handler.ContextHandler;
|
||||
import org.eclipse.jetty.util.component.AbstractLifeCycle;
|
||||
import org.eclipse.jetty.util.component.LifeCycle;
|
||||
import org.eclipse.jetty.util.resource.CombinedResource;
|
||||
import org.eclipse.jetty.util.resource.Resource;
|
||||
import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
|
@ -44,9 +45,12 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
|
|||
|
||||
private final ContextHandler _contextHandler;
|
||||
private final Supplier<Resource> _resourceBaseSupplier;
|
||||
private final List<Path> _protected = new ArrayList<>();
|
||||
private final List<Resource> _protected = new ArrayList<>();
|
||||
private final AllowedResourceAliasCheckListener _listener = new AllowedResourceAliasCheckListener();
|
||||
private boolean _initialized;
|
||||
protected Resource _baseResource;
|
||||
|
||||
@Deprecated
|
||||
protected Path _base;
|
||||
|
||||
/**
|
||||
|
@ -80,25 +84,29 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
|
|||
|
||||
private void extractBaseResourceFromContext()
|
||||
{
|
||||
_base = getPath(_resourceBaseSupplier.get());
|
||||
if (_base == null)
|
||||
_baseResource = _resourceBaseSupplier.get();
|
||||
if (_baseResource == null)
|
||||
return;
|
||||
|
||||
try
|
||||
{
|
||||
if (Files.exists(_base, NO_FOLLOW_LINKS))
|
||||
_base = _base.toRealPath(FOLLOW_LINKS);
|
||||
String[] protectedTargets = getProtectedTargets();
|
||||
if (protectedTargets != null)
|
||||
{
|
||||
for (String s : protectedTargets)
|
||||
_protected.add(_base.getFileSystem().getPath(_base.toString(), s));
|
||||
{
|
||||
Resource p = _baseResource.resolve(s);
|
||||
// TODO: we still want to include the protected target if it does not exist.
|
||||
if (p == null)
|
||||
continue;
|
||||
_protected.add(p);
|
||||
}
|
||||
}
|
||||
}
|
||||
catch (IOException e)
|
||||
catch (Throwable t)
|
||||
{
|
||||
LOG.warn("Base resource failure ({} is disabled): {}", this.getClass().getName(), _base, e);
|
||||
_base = null;
|
||||
LOG.warn("Base resource failure ({} is disabled): {}", this.getClass().getName(), _baseResource, t);
|
||||
_baseResource = null;
|
||||
}
|
||||
}
|
||||
|
||||
|
@ -123,7 +131,7 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
|
|||
protected void doStop() throws Exception
|
||||
{
|
||||
_contextHandler.removeEventListener(_listener);
|
||||
_base = null;
|
||||
_baseResource = null;
|
||||
_protected.clear();
|
||||
}
|
||||
|
||||
|
@ -132,7 +140,7 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
|
|||
{
|
||||
if (!_initialized)
|
||||
extractBaseResourceFromContext();
|
||||
if (_base == null)
|
||||
if (_baseResource == null)
|
||||
return false;
|
||||
|
||||
try
|
||||
|
@ -141,11 +149,7 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
|
|||
if (!resource.exists())
|
||||
return false;
|
||||
|
||||
Path path = getPath(resource);
|
||||
if (path == null)
|
||||
return false;
|
||||
|
||||
return check(pathInContext, path);
|
||||
return check(pathInContext, resource);
|
||||
}
|
||||
catch (Throwable t)
|
||||
{
|
||||
|
@ -162,6 +166,19 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
|
|||
return isAllowed(getRealPath(path));
|
||||
}
|
||||
|
||||
protected boolean check(String pathInContext, Resource resource)
|
||||
{
|
||||
// Allow any aliases (symlinks, 8.3, casing, etc.) so long as
|
||||
// the resulting real file is allowed.
|
||||
for (Resource r : resource)
|
||||
{
|
||||
if (!check(pathInContext, r.getPath()))
|
||||
return false;
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
protected boolean isAllowed(Path path)
|
||||
{
|
||||
// If the resource doesn't exist we cannot determine whether it is protected, so we assume it is.
|
||||
|
@ -172,14 +189,17 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
|
|||
{
|
||||
// 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))
|
||||
if (isSameFile(path, _baseResource))
|
||||
return true;
|
||||
|
||||
// If the path is the same file as any protected resources, then it is protected.
|
||||
for (Path p : _protected)
|
||||
for (Resource p : _protected)
|
||||
{
|
||||
if (isSameFile(path, p))
|
||||
return false;
|
||||
for (Resource r : p)
|
||||
{
|
||||
if (isSameFile(path, r.getPath()))
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
// Walks up the aliased path name, not the real path name.
|
||||
|
@ -192,6 +212,26 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
|
|||
return false;
|
||||
}
|
||||
|
||||
protected boolean isSameFile(Path path, Resource resource)
|
||||
{
|
||||
if (resource == null)
|
||||
return false;
|
||||
|
||||
if (resource instanceof CombinedResource combinedResource)
|
||||
{
|
||||
for (Resource r : combinedResource)
|
||||
{
|
||||
if (isSameFile(path, r.getPath()))
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
else
|
||||
{
|
||||
return isSameFile(path, resource.getPath());
|
||||
}
|
||||
}
|
||||
|
||||
protected boolean isSameFile(Path path1, Path path2)
|
||||
{
|
||||
if (Objects.equals(path1, path2))
|
||||
|
@ -227,17 +267,10 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
|
|||
return null;
|
||||
}
|
||||
|
||||
@Deprecated
|
||||
protected Path getPath(Resource resource)
|
||||
{
|
||||
try
|
||||
{
|
||||
return (resource == null) ? null : resource.getPath();
|
||||
}
|
||||
catch (Throwable t)
|
||||
{
|
||||
LOG.trace("getPath() failed", t);
|
||||
return null;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
private class AllowedResourceAliasCheckListener implements LifeCycle.Listener
|
||||
|
@ -256,7 +289,7 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Al
|
|||
return String.format("%s@%x{base=%s,protected=%s}",
|
||||
this.getClass().getSimpleName(),
|
||||
hashCode(),
|
||||
_base,
|
||||
_baseResource,
|
||||
(protectedTargets == null) ? null : Arrays.asList(protectedTargets));
|
||||
}
|
||||
}
|
||||
|
|
|
@ -46,7 +46,7 @@ public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChec
|
|||
@Override
|
||||
protected boolean check(String pathInContext, Path path)
|
||||
{
|
||||
if (_base == null)
|
||||
if (_baseResource == null)
|
||||
return false;
|
||||
|
||||
// do not allow any file separation characters in the URI, as we need to know exactly what are the segments
|
||||
|
@ -57,29 +57,33 @@ public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChec
|
|||
// 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();
|
||||
StringBuilder segmentPath = 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());
|
||||
segmentPath.append("/").append(segment);
|
||||
Resource fromBase = _baseResource.resolve(segmentPath.toString());
|
||||
for (Resource r : fromBase)
|
||||
{
|
||||
Path p = r.getPath();
|
||||
String realURI = p.toRealPath(NO_FOLLOW_LINKS).getFileName().toString();
|
||||
|
||||
// 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 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(p))
|
||||
return !getContextHandler().isProtectedTarget(realURI);
|
||||
|
||||
// If the ancestor is not allowed then do not allow.
|
||||
if (!isAllowed(fromBase))
|
||||
return false;
|
||||
// If the ancestor is not allowed then do not allow.
|
||||
if (!isAllowed(p))
|
||||
return false;
|
||||
|
||||
// TODO as we are building the realURI of the resource, it would be possible to
|
||||
// re-check that against security constraints.
|
||||
// TODO as we are building the realURI of the resource, it would be possible to
|
||||
// re-check that against security constraints.
|
||||
}
|
||||
}
|
||||
}
|
||||
catch (Throwable t)
|
||||
|
@ -90,6 +94,12 @@ public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChec
|
|||
}
|
||||
|
||||
// No symlink found, so must be allowed. Double check it is the right path we checked.
|
||||
return isSameFile(fromBase, path);
|
||||
Resource fromBase = _baseResource.resolve(segmentPath.toString());
|
||||
for (Resource r : fromBase)
|
||||
{
|
||||
if (isSameFile(r.getPath(), path))
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
|
|
@ -281,11 +281,11 @@ public class AliasCheckerSymlinkTest
|
|||
Arguments.of(allowedResource, "/files/file1", HttpStatus.OK_200, "file1 from combined dir"),
|
||||
Arguments.of(allowedResource, "/files/file2", HttpStatus.OK_200, "file1 from webroot"),
|
||||
|
||||
Arguments.of(allowedResource, "/combinedSymlinkFile", HttpStatus.OK_200, "file1 from webroot"),
|
||||
Arguments.of(allowedResource, "/combinedSymlinkFile", HttpStatus.OK_200, "This file is inside webroot."),
|
||||
Arguments.of(allowedResource, "/externalCombinedSymlinkFile/file", HttpStatus.NOT_FOUND_404, null),
|
||||
Arguments.of(allowedResource, "/combinedWebInfSymlink/web.xml", HttpStatus.NOT_FOUND_404, null),
|
||||
|
||||
Arguments.of(symlinkAllowedResource, "/combinedSymlinkFile", HttpStatus.OK_200, "file1 from webroot"),
|
||||
Arguments.of(symlinkAllowedResource, "/combinedSymlinkFile", HttpStatus.OK_200, "This file is inside webroot."),
|
||||
Arguments.of(symlinkAllowedResource, "/externalCombinedSymlinkFile/file", HttpStatus.OK_200, "This file is inside a sibling dir to webroot."),
|
||||
Arguments.of(symlinkAllowedResource, "/combinedWebInfSymlink/web.xml", HttpStatus.OK_200, "This is the web.xml file.")
|
||||
);
|
||||
|
|
Loading…
Reference in New Issue