Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Issue #7059 - prevent an internal NPE in AllowedResourceAliasChecker doStart (#7076) 

- prevent an internal NPE in AllowedResourceAliasChecker doStart
- Fix LifeCycle issues with AllowedResourceAliasChecker
- add null check for protected targets in toString.
- improve warning message for AllowedResourceAliasChecker
- add AllowedResourceAliasCheckerTest
This commit is contained in:
Lachlan 2021-12-07 09:37:11 +11:00 committed by GitHub
parent 04ae66777d
commit e345ee28a5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 209 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

59
jetty-server/src/main/java/org/eclipse/jetty/server/AllowedResourceAliasChecker.java
View File

@ -17,7 +17,6 @@ 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.ArrayList;
import java.util.Arrays;
import java.util.List;
@ -25,6 +24,7 @@ import java.util.Objects;
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.PathResource;
import org.eclipse.jetty.util.resource.Resource;
import org.slf4j.Logger;
@ -44,6 +44,7 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Co
private final ContextHandler _contextHandler;
private final List<Path> _protected = new ArrayList<>();
private final AllowedResourceAliasCheckListener _listener = new AllowedResourceAliasCheckListener();
protected Path _base;
/**
@ -51,7 +52,7 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Co
*/
public AllowedResourceAliasChecker(ContextHandler contextHandler)
{
_contextHandler = contextHandler;
_contextHandler = Objects.requireNonNull(contextHandler);
}
protected ContextHandler getContextHandler()
@ -59,26 +60,45 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Co
return _contextHandler;
}
@Override
protected void doStart() throws Exception
protected void initialize()
{
_base = getPath(_contextHandler.getBaseResource());
if (_base == null)
_base = Paths.get("/").toAbsolutePath();
if (Files.exists(_base, NO_FOLLOW_LINKS))
_base = _base.toRealPath(FOLLOW_LINKS);
return;
String[] protectedTargets = _contextHandler.getProtectedTargets();
if (protectedTargets != null)
try
{
for (String s : protectedTargets)
_protected.add(_base.getFileSystem().getPath(_base.toString(), s));
if (Files.exists(_base, NO_FOLLOW_LINKS))
_base = _base.toRealPath(FOLLOW_LINKS);
String[] protectedTargets = _contextHandler.getProtectedTargets();
if (protectedTargets != null)
{
for (String s : protectedTargets)
_protected.add(_base.getFileSystem().getPath(_base.toString(), s));
}
}
catch (IOException e)
{
LOG.warn("Base resource failure ({} is disabled): {}", this.getClass().getName(), _base, e);
_base = null;
}
}
@Override
protected void doStart() throws Exception
{
// We can only initialize if ContextHandler in started state, the baseResource can be changed even in starting state.
// If the ContextHandler is not started add a listener to delay initialization until fully started.
if (_contextHandler.isStarted())
initialize();
else
_contextHandler.addEventListener(_listener);
}
@Override
protected void doStop() throws Exception
{
_contextHandler.removeEventListener(_listener);
_base = null;
_protected.clear();
}
@ -86,6 +106,9 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Co
@Override
public boolean check(String pathInContext, Resource resource)
{
if (_base == null)
return false;
try
{
// The existence check resolves the symlinks.
@ -184,7 +207,7 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Co
{
if (resource instanceof PathResource)
return ((PathResource)resource).getPath();
return resource.getFile().toPath();
return (resource == null) ? null : resource.getFile().toPath();
}
catch (Throwable t)
{
@ -193,13 +216,23 @@ public class AllowedResourceAliasChecker extends AbstractLifeCycle implements Co
}
}
private class AllowedResourceAliasCheckListener implements LifeCycle.Listener
{
@Override
public void lifeCycleStarted(LifeCycle event)
{
initialize();
}
}
@Override
public String toString()
{
String[] protectedTargets = _contextHandler.getProtectedTargets();
return String.format("%s@%x{base=%s,protected=%s}",
this.getClass().getSimpleName(),
hashCode(),
_base,
Arrays.asList(_contextHandler.getProtectedTargets()));
(protectedTargets == null) ? null : Arrays.asList(protectedTargets));
}
}

3
jetty-server/src/main/java/org/eclipse/jetty/server/SymlinkAllowedResourceAliasChecker.java
View File

@ -40,6 +40,9 @@ public class SymlinkAllowedResourceAliasChecker extends AllowedResourceAliasChec
@Override
protected boolean check(String pathInContext, Path path)
{
if (_base == null)
return false;
// do not allow any file separation characters in the URI, as we need to know exactly what are the segments
if (File.separatorChar != '/' && pathInContext.indexOf(File.separatorChar) >= 0)
return false;

17
jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java
View File

@ -228,7 +228,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
private final List<ContextScopeListener> _contextListeners = new CopyOnWriteArrayList<>();
private final Set<EventListener> _durableListeners = new HashSet<>();
private Index<ProtectedTargetType> _protectedTargets = Index.empty(false);
private final CopyOnWriteArrayList<AliasCheck> _aliasChecks = new CopyOnWriteArrayList<>();
private final List<AliasCheck> _aliasChecks = new CopyOnWriteArrayList<>();
public enum Availability
{
@ -1700,6 +1700,8 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
*/
public void setBaseResource(Resource base)
{
if (isStarted())
throw new IllegalStateException("Cannot call setBaseResource after starting");
_baseResource = base;
}
@ -2072,7 +2074,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
*/
public void addAliasCheck(AliasCheck check)
{
getAliasChecks().add(check);
_aliasChecks.add(check);
if (check instanceof LifeCycle)
addManaged((LifeCycle)check);
else
@ -2080,11 +2082,11 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
}
/**
* @return Mutable list of Alias checks
* @return Immutable list of Alias checks
*/
public List<AliasCheck> getAliasChecks()
{
return _aliasChecks;
return Collections.unmodifiableList(_aliasChecks);
}
/**
@ -2093,7 +2095,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
public void setAliasChecks(List<AliasCheck> checks)
{
clearAliasChecks();
getAliasChecks().addAll(checks);
checks.forEach(this::addAliasCheck);
}
/**
@ -2101,9 +2103,8 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu
*/
public void clearAliasChecks()
{
List<AliasCheck> aliasChecks = getAliasChecks();
aliasChecks.forEach(this::removeBean);
aliasChecks.clear();
_aliasChecks.forEach(this::removeBean);
_aliasChecks.clear();
}
/**

7
jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java
View File

@ -60,7 +60,6 @@ import org.eclipse.jetty.toolchain.test.jupiter.WorkDir;
import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension;
import org.eclipse.jetty.util.TypeUtil;
import org.eclipse.jetty.util.resource.PathResource;
import org.eclipse.jetty.util.resource.Resource;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@ -958,8 +957,6 @@ public class DefaultServletTest
public void testWelcomeRedirectDirWithQuestion() throws Exception
{
FS.ensureDirExists(docRoot);
context.setBaseResource(new PathResource(docRoot));
Path dir = assumeMkDirSupported(docRoot, "dir?");
Path index = dir.resolve("index.html");
@ -992,8 +989,6 @@ public class DefaultServletTest
public void testWelcomeRedirectDirWithSemicolon() throws Exception
{
FS.ensureDirExists(docRoot);
context.setBaseResource(new PathResource(docRoot));
Path dir = assumeMkDirSupported(docRoot, "dir;");
Path index = dir.resolve("index.html");
@ -1215,8 +1210,6 @@ public class DefaultServletTest
public void testDirectFromResourceHttpContent() throws Exception
{
FS.ensureDirExists(docRoot);
context.setBaseResource(Resource.newResource(docRoot));
Path index = docRoot.resolve("index.html");
createFile(index, "<h1>Hello World</h1>");

3
jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java
View File

@ -130,8 +130,7 @@ public class ServletLifeCycleTest
server.start();
context.addEventListener(new EventListener() {});
listeners = context.getEventListeners();
listeners = context.getEventListeners();
assertThat(listeners.size(), is(3));
assertThat(listeners.size(), is(4));
server.stop();
listeners = context.getEventListeners();

150
tests/test-integration/src/test/java/org/eclipse/jetty/test/AllowedResourceAliasCheckerTest.java Normal file
View File

@ -0,0 +1,150 @@
//
// ========================================================================
// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
//
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
// ========================================================================
//
package org.eclipse.jetty.test;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.net.URI;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.Path;
import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.client.api.ContentResponse;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.server.AllowedResourceAliasChecker;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.servlet.DefaultServlet;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.util.IO;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
public class AllowedResourceAliasCheckerTest
{
private static Server _server;
private static ServerConnector _connector;
private static HttpClient _client;
private static ServletContextHandler _context;
private static File _baseDir;
private static Path getResourceDir() throws Exception
{
URL url = AllowedResourceAliasCheckerTest.class.getClassLoader().getResource(".");
assertNotNull(url);
return new File(url.toURI()).toPath();
}
public void start() throws Exception
{
_server.start();
_client.start();
}
@BeforeAll
public static void beforeAll() throws Exception
{
_client = new HttpClient();
_server = new Server();
_connector = new ServerConnector(_server);
_server.addConnector(_connector);
_context = new ServletContextHandler();
_context.setContextPath("/");
_context.addServlet(DefaultServlet.class, "/");
_server.setHandler(_context);
_baseDir = getResourceDir().resolve("baseDir").toFile();
_baseDir.deleteOnExit();
assertFalse(_baseDir.exists());
_context.setResourceBase(_baseDir.getAbsolutePath());
}
@AfterAll
public static void afterAll() throws Exception
{
_client.stop();
_server.stop();
}
@AfterEach
public void afterEach()
{
IO.delete(_baseDir);
}
public void createBaseDir() throws IOException
{
assertFalse(_baseDir.exists());
assertTrue(_baseDir.mkdir());
// Create a file in the baseDir.
File file = _baseDir.toPath().resolve("file.txt").toFile();
file.deleteOnExit();
assertTrue(file.createNewFile());
try (FileWriter fileWriter = new FileWriter(file))
{
fileWriter.write("this is a file in the baseDir");
}
// Create a symlink to that file.
// Symlink to a directory inside of the webroot.
File symlink = _baseDir.toPath().resolve("symlink").toFile();
symlink.deleteOnExit();
Files.createSymbolicLink(symlink.toPath(), file.toPath());
assertTrue(symlink.exists());
}
@Test
public void testCreateBaseDirBeforeStart() throws Exception
{
_context.clearAliasChecks();
_context.addAliasCheck(new AllowedResourceAliasChecker(_context));
createBaseDir();
start();
assertThat(_context.getAliasChecks().size(), equalTo(1));
URI uri = URI.create("http://localhost:" + _connector.getLocalPort() + "/symlink");
ContentResponse response = _client.GET(uri);
assertThat(response.getStatus(), is(HttpStatus.OK_200));
assertThat(response.getContentAsString(), is("this is a file in the baseDir"));
}
@Test
public void testCreateBaseDirAfterStart() throws Exception
{
_context.clearAliasChecks();
_context.addAliasCheck(new AllowedResourceAliasChecker(_context));
start();
createBaseDir();
assertThat(_context.getAliasChecks().size(), equalTo(1));
URI uri = URI.create("http://localhost:" + _connector.getLocalPort() + "/symlink");
ContentResponse response = _client.GET(uri);
assertThat(response.getStatus(), is(HttpStatus.OK_200));
assertThat(response.getContentAsString(), is("this is a file in the baseDir"));
}
}