From e345ee28a56a9a5bd5e2eb2cf339ab2cfcf6ea19 Mon Sep 17 00:00:00 2001 From: Lachlan Date: Tue, 7 Dec 2021 09:37:11 +1100 Subject: [PATCH] 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 --- .../server/AllowedResourceAliasChecker.java | 59 +++++-- .../SymlinkAllowedResourceAliasChecker.java | 3 + .../jetty/server/handler/ContextHandler.java | 17 +- .../jetty/servlet/DefaultServletTest.java | 7 - .../jetty/servlet/ServletLifeCycleTest.java | 3 +- .../test/AllowedResourceAliasCheckerTest.java | 150 ++++++++++++++++++ 6 files changed, 209 insertions(+), 30 deletions(-) create mode 100644 tests/test-integration/src/test/java/org/eclipse/jetty/test/AllowedResourceAliasCheckerTest.java 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 a0debdcbc90..4170057631a 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 @@ -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 _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)); } } 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 8130195f972..172eeedc4b6 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 @@ -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; diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 3094af56edd..72e97df62af 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -228,7 +228,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu private final List _contextListeners = new CopyOnWriteArrayList<>(); private final Set _durableListeners = new HashSet<>(); private Index _protectedTargets = Index.empty(false); - private final CopyOnWriteArrayList _aliasChecks = new CopyOnWriteArrayList<>(); + private final List _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 getAliasChecks() { - return _aliasChecks; + return Collections.unmodifiableList(_aliasChecks); } /** @@ -2093,7 +2095,7 @@ public class ContextHandler extends ScopedHandler implements Attributes, Gracefu public void setAliasChecks(List 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 aliasChecks = getAliasChecks(); - aliasChecks.forEach(this::removeBean); - aliasChecks.clear(); + _aliasChecks.forEach(this::removeBean); + _aliasChecks.clear(); } /** diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java index 3229bcbff6d..e319a447ba8 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java @@ -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, "

Hello World

"); diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java index edee222d814..9c99a1142d0 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletLifeCycleTest.java @@ -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(); diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/AllowedResourceAliasCheckerTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/AllowedResourceAliasCheckerTest.java new file mode 100644 index 00000000000..3e1c91471c2 --- /dev/null +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/AllowedResourceAliasCheckerTest.java @@ -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")); + } +} \ No newline at end of file