diff --git a/changelog/unreleased/ghsa-7v9x-j5xj-57rq.toml b/changelog/unreleased/ghsa-7v9x-j5xj-57rq.toml new file mode 100644 index 000000000000..106b06683877 --- /dev/null +++ b/changelog/unreleased/ghsa-7v9x-j5xj-57rq.toml @@ -0,0 +1,4 @@ +type = "s" +message = "Fix path traversal in the web interface and preflight asset resources" +details.users = "See [GHSA-7v9x-j5xj-57rq](https://github.com/Graylog2/graylog2-server/security/advisories/GHSA-7v9x-j5xj-57rq) for details." +pulls = ["26942"] diff --git a/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/PreflightWebModule.java b/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/PreflightWebModule.java index c7785d7a32eb..bf102e8e4b44 100644 --- a/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/PreflightWebModule.java +++ b/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/PreflightWebModule.java @@ -48,6 +48,7 @@ import org.graylog2.shared.bindings.providers.EventBusProvider; import org.graylog2.shared.bindings.providers.ServiceManagerProvider; import org.graylog2.shared.initializers.PeriodicalsService; +import org.graylog2.web.resources.ResourceFileReader; import static java.util.Objects.requireNonNull; @@ -75,6 +76,7 @@ protected void configure() { bind(PreflightConfigService.class).to(PreflightConfigServiceImpl.class); bind(PreflightBoot.class).asEagerSingleton(); bind(NotificationService.class).to(NullNotificationService.class); + bind(ResourceFileReader.class).asEagerSingleton(); addPreflightRestResource(PreflightResource.class); addPreflightRestResource(CertificateRenewalPolicyResource.class); diff --git a/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/web/resources/PreflightAssetsResource.java b/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/web/resources/PreflightAssetsResource.java index ff6defefb112..f1587071b242 100644 --- a/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/web/resources/PreflightAssetsResource.java +++ b/graylog2-server/src/main/java/org/graylog2/bootstrap/preflight/web/resources/PreflightAssetsResource.java @@ -16,20 +16,7 @@ */ package org.graylog2.bootstrap.preflight.web.resources; -import com.google.common.cache.CacheBuilder; -import com.google.common.cache.CacheLoader; -import com.google.common.cache.LoadingCache; -import com.google.common.hash.HashCode; -import com.google.common.hash.Hashing; -import com.google.common.io.Resources; -import org.apache.shiro.authz.annotation.RequiresPermissions; -import org.graylog2.bootstrap.preflight.PreflightConstants; - -import javax.activation.MimetypesFileTypeMap; -import javax.annotation.Nonnull; - import jakarta.inject.Inject; - import jakarta.ws.rs.GET; import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.Path; @@ -41,21 +28,16 @@ import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Request; import jakarta.ws.rs.core.Response; +import org.apache.shiro.authz.annotation.RequiresPermissions; +import org.graylog2.bootstrap.preflight.PreflightConstants; import org.graylog2.bootstrap.preflight.PreflightWebModule; +import org.graylog2.web.resources.ResourceFileReader; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; -import java.io.FileNotFoundException; +import javax.activation.MimetypesFileTypeMap; import java.io.IOException; -import java.net.URI; import java.net.URISyntaxException; -import java.net.URL; -import java.nio.file.FileSystem; -import java.nio.file.FileSystemAlreadyExistsException; -import java.nio.file.FileSystemNotFoundException; -import java.nio.file.FileSystems; -import java.nio.file.Files; -import java.nio.file.Paths; -import java.nio.file.attribute.FileTime; -import java.util.Collections; import java.util.Date; import java.util.concurrent.TimeUnit; @@ -63,28 +45,15 @@ @Path("/") public class PreflightAssetsResource { + private static final Logger LOG = LoggerFactory.getLogger(PreflightAssetsResource.class); + private final MimetypesFileTypeMap mimeTypes; - private final LoadingCache fileSystemCache; + private final ResourceFileReader resourceFileReader; @Inject - public PreflightAssetsResource(MimetypesFileTypeMap mimeTypes) { + public PreflightAssetsResource(MimetypesFileTypeMap mimeTypes, ResourceFileReader resourceFileReader) { this.mimeTypes = mimeTypes; - this.fileSystemCache = CacheBuilder.newBuilder() - .maximumSize(1024) - .build(new CacheLoader() { - @Override - public FileSystem load(@Nonnull URI key) throws Exception { - try { - return FileSystems.getFileSystem(key); - } catch (FileSystemNotFoundException e) { - try { - return FileSystems.newFileSystem(key, Collections.emptyMap()); - } catch (FileSystemAlreadyExistsException f) { - return FileSystems.getFileSystem(key); - } - } - } - }); + this.resourceFileReader = resourceFileReader; } @Produces(MediaType.TEXT_HTML) @@ -99,43 +68,19 @@ public Response index(@Context Request request) { @RequiresPermissions(PreflightWebModule.PERMISSION_PREFLIGHT_ONLY) public Response get(@Context Request request, @PathParam("filename") String filename) { try { - final URL resourceUrl = getResourceUri(filename); - return getResponse(request, filename, resourceUrl); + final var resource = resourceFileReader.readFileFrom(PreflightConstants.ASSETS_RESOURCE_DIR, filename, getClass()); + return getResponse(request, filename, resource); } catch (IOException | URISyntaxException e) { - throw new NotFoundException("Couldn't find " + filename, e); - } - } - - private URL getResourceUri(String filename) throws FileNotFoundException { - final URL resourceUrl = this.getClass().getResource(PreflightConstants.ASSETS_RESOURCE_DIR + filename); - if (resourceUrl == null) { - throw new FileNotFoundException("Resource file " + filename + " not found."); + LOG.debug("Couldn't serve preflight asset <{}>.", filename, e); + // Don't reflect the requested file name back to the client. + throw new NotFoundException("Couldn't find the requested resource.", e); } - return resourceUrl; } - private Response getResponse(Request request, String filename, URL resourceUrl) throws IOException, URISyntaxException { - final URI uri = resourceUrl.toURI(); - - final java.nio.file.Path path; - final byte[] fileContents; - switch (resourceUrl.getProtocol()) { - case "file" -> { - path = Paths.get(uri); - fileContents = Files.readAllBytes(path); - } - case "jar" -> { - final FileSystem fileSystem = fileSystemCache.getUnchecked(uri); - path = fileSystem.getPath(PreflightConstants.ASSETS_RESOURCE_DIR + filename); - fileContents = Resources.toByteArray(resourceUrl); - } - default -> throw new IllegalArgumentException("Not a JAR or local file: " + resourceUrl); - } - - final FileTime lastModifiedTime = Files.getLastModifiedTime(path); - final Date lastModified = Date.from(lastModifiedTime.toInstant()); - final HashCode hashCode = Hashing.sha256().hashBytes(fileContents); - final EntityTag entityTag = new EntityTag(hashCode.toString()); + private Response getResponse(Request request, String filename, ResourceFileReader.ResourceFile resource) { + final byte[] fileContents = resource.contents().get(); + final Date lastModified = resource.lastModified().orElseGet(Date::new); + final EntityTag entityTag = resource.entityTag().get(); final Response.ResponseBuilder response = request.evaluatePreconditions(lastModified, entityTag); if (response != null) { diff --git a/graylog2-server/src/main/java/org/graylog2/web/resources/ResourceFileReader.java b/graylog2-server/src/main/java/org/graylog2/web/resources/ResourceFileReader.java index ac27a2f91107..4f6433f86232 100644 --- a/graylog2-server/src/main/java/org/graylog2/web/resources/ResourceFileReader.java +++ b/graylog2-server/src/main/java/org/graylog2/web/resources/ResourceFileReader.java @@ -16,6 +16,7 @@ */ package org.graylog2.web.resources; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Suppliers; import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; @@ -28,6 +29,7 @@ import org.graylog2.web.PluginAssets; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.io.FileNotFoundException; import java.io.IOException; import java.net.URI; @@ -38,8 +40,10 @@ import java.nio.file.FileSystemNotFoundException; import java.nio.file.FileSystems; import java.nio.file.Files; +import java.nio.file.InvalidPathException; import java.nio.file.Paths; import java.nio.file.attribute.FileTime; +import java.security.CodeSource; import java.util.Collections; import java.util.Date; import java.util.Optional; @@ -47,6 +51,12 @@ @Singleton public class ResourceFileReader { + /** + * Plugin assets are packaged at the root of the plugin JAR. + */ + public static final String PLUGIN_ASSETS_DIR = "/"; + public static final String WEB_INTERFACE_ASSETS_DIR = "/" + PluginAssets.pathPrefix + "/"; + private final LoadingCache fileSystemCache; @Inject @@ -93,17 +103,32 @@ public Optional lastModified() { } public ResourceFile readFile(String filename, Class aClass) throws URISyntaxException, IOException { - return readFile(false, filename, aClass); + return readFileFrom(WEB_INTERFACE_ASSETS_DIR, filename, aClass); } - public ResourceFile readFileFromPlugin(String filename, Class aClass) throws URISyntaxException, IOException { - return readFile(true, filename, aClass); + /** + * Reads an asset of the plugin that {@code pluginClass} belongs to. + *

+ * Plugin assets are packaged at the root of the plugin JAR, which is also where every other JAR on the + * classpath keeps its own root-level resources. A classpath lookup delegates beyond the plugin, and a + * traversal sequence normalizes away at the root ({@code "/../../log4j2.xml"} becomes + * {@code "/log4j2.xml"}), so {@link #resolveResourceName(String, String)} alone cannot keep this from + * addressing an unrelated JAR. We therefore additionally require the resource to originate from the + * plugin's own code source. + */ + public ResourceFile readFileFromPlugin(String filename, Class pluginClass) throws URISyntaxException, IOException { + final String resourceName = resolveResourceName(PLUGIN_ASSETS_DIR, filename); + if (!isFromCodeSourceOf(pluginClass.getResource(resourceName), pluginClass)) { + throw new FileNotFoundException("Resource file not found."); + } + return readFileFrom(PLUGIN_ASSETS_DIR, filename, pluginClass); } - private ResourceFile readFile(boolean fromPlugin, String filename, Class aClass) throws URISyntaxException, IOException { - final URL resourceUrl = aClass.getResource(pluginPrefixFilename(fromPlugin, filename)); + public ResourceFile readFileFrom(String resourceDir, String filename, Class aClass) throws URISyntaxException, IOException { + final String resourceName = resolveResourceName(resourceDir, filename); + final URL resourceUrl = aClass.getResource(resourceName); if (resourceUrl == null) { - throw new FileNotFoundException("Resource file " + filename + " not found."); + throw new FileNotFoundException("Resource file not found."); } final URI uri = resourceUrl.toURI(); @@ -115,7 +140,7 @@ private ResourceFile readFile(boolean fromPlugin, String filename, Class aCla } case "jar": { final FileSystem fileSystem = fileSystemCache.getUnchecked(uri); - final java.nio.file.Path path = fileSystem.getPath(pluginPrefixFilename(fromPlugin, filename)); + final java.nio.file.Path path = fileSystem.getPath(resourceName); final var contents = Resources.toByteArray(resourceUrl); return ResourceFile.create(path, contents); } @@ -124,13 +149,86 @@ private ResourceFile readFile(boolean fromPlugin, String filename, Class aCla } } - + /** + * Resolves {@code filename} inside {@code resourceDir} and verifies that the result is a direct child of + * it, so a path traversal sequence in the (already URL-decoded) filename cannot address resources outside + * of the directory. + *

+ * The check requires a direct child rather than mere containment below {@code resourceDir}, because all + * asset directories we serve from are flat, and because containment is meaningless for + * {@link #PLUGIN_ASSETS_DIR}: {@code "/"} contains every path, including a traversed one. + * + * @return the canonical classpath resource name of the requested file + * @throws FileNotFoundException if the filename does not address a direct child of {@code resourceDir} + */ + @VisibleForTesting @Nonnull - private String pluginPrefixFilename(boolean fromPlugin, String filename) { - if (fromPlugin) { - return "/" + filename; - } else { - return "/" + PluginAssets.pathPrefix + "/" + filename; + static String resolveResourceName(String resourceDir, String filename) throws FileNotFoundException { + final java.nio.file.Path base = Paths.get(resourceDir).normalize(); + final java.nio.file.Path resolved; + try { + resolved = base.resolve(filename).normalize(); + } catch (InvalidPathException e) { + throw new FileNotFoundException("Invalid resource file name."); + } + if (!base.equals(resolved.getParent())) { + throw new FileNotFoundException("Invalid resource file name."); + } + // Rebuild the name from the validated single path segment: classpath resource names always use "/" + // as a separator, whereas Path#toString() would use the platform-dependent one. + return resourceDir.endsWith("/") + ? resourceDir + resolved.getFileName() + : resourceDir + "/" + resolved.getFileName(); + } + + /** + * Checks whether {@code resourceUrl} was loaded from the same JAR or classes directory that + * {@code aClass} itself came from. + */ + @VisibleForTesting + static boolean isFromCodeSourceOf(@Nullable URL resourceUrl, Class aClass) { + if (resourceUrl == null) { + return false; + } + final CodeSource codeSource = aClass.getProtectionDomain().getCodeSource(); + if (codeSource == null || codeSource.getLocation() == null) { + // Without a code source there is no way to tell where the resource came from, so refuse to + // serve it rather than falling back to the whole classpath. + return false; } + + // Compare local paths rather than URL strings, so that the two sides cannot differ merely in how they + // percent-encode a location, for example an installation directory containing a space. + final var codeSourcePath = toLocalPath(codeSource.getLocation().toString()); + final var resourcePath = toLocalPath("jar".equals(resourceUrl.getProtocol()) + ? jarLocation(resourceUrl) + : resourceUrl.toString()); + if (codeSourcePath.isEmpty() || resourcePath.isEmpty()) { + return false; + } + + // A resource in a JAR resolves to the JAR itself, one on an exploded classpath to a file below the + // classes directory. Path#startsWith matches whole path segments, so a sibling directory with a + // common name prefix does not pass. + return resourcePath.get().startsWith(codeSourcePath.get()); + } + + private static Optional toLocalPath(String url) { + try { + return Optional.of(Paths.get(URI.create(url))); + } catch (IllegalArgumentException | FileSystemNotFoundException e) { + return Optional.empty(); + } + } + + /** + * Strips the entry from a {@code jar:} URL, turning + * {@code jar:file:/opt/graylog/plugin/example.jar!/abc.js} into + * {@code file:/opt/graylog/plugin/example.jar}. + */ + private static String jarLocation(URL resourceUrl) { + final String url = resourceUrl.toString(); + final int entrySeparator = url.indexOf("!/"); + return entrySeparator < 0 ? url : url.substring("jar:".length(), entrySeparator); } } diff --git a/graylog2-server/src/main/java/org/graylog2/web/resources/WebInterfaceAssetsResource.java b/graylog2-server/src/main/java/org/graylog2/web/resources/WebInterfaceAssetsResource.java index 19c164aa0b9d..bbfdce1772fa 100644 --- a/graylog2-server/src/main/java/org/graylog2/web/resources/WebInterfaceAssetsResource.java +++ b/graylog2-server/src/main/java/org/graylog2/web/resources/WebInterfaceAssetsResource.java @@ -37,6 +37,8 @@ import org.graylog2.shared.rest.resources.csp.CSPDynamicFeature; import org.graylog2.web.IndexHtmlGenerator; import org.graylog2.web.customization.CustomizationConfig; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import javax.activation.MimetypesFileTypeMap; import java.io.IOException; @@ -55,6 +57,8 @@ @CSP(group = CSP.DEFAULT) @NonApiResource public class WebInterfaceAssetsResource { + private static final Logger LOG = LoggerFactory.getLogger(WebInterfaceAssetsResource.class); + private static final String ASSETS_PREFIX = "assets"; private static final String FAVICON = "favicon.png"; private final MimetypesFileTypeMap mimeTypes; @@ -122,7 +126,9 @@ public Response get(@Context Request request, final var resource = resourceFileReader.readFileFromPlugin(filenameWithoutSuffix, plugin.metadata().getClass()); return getResponse(request, filenameWithoutSuffix, resource); } catch (URISyntaxException | IOException e) { - throw new NotFoundException("Couldn't find " + filenameWithoutSuffix + " in plugin " + pluginName, e); + LOG.debug("Couldn't serve asset <{}> of plugin <{}>.", filenameWithoutSuffix, pluginName, e); + // Don't reflect the requested file name back to the client. + throw new NotFoundException("Couldn't find the requested resource in plugin " + pluginName, e); } } diff --git a/graylog2-server/src/test/java/org/graylog2/bootstrap/preflight/web/resources/PreflightAssetsResourceTest.java b/graylog2-server/src/test/java/org/graylog2/bootstrap/preflight/web/resources/PreflightAssetsResourceTest.java new file mode 100644 index 000000000000..4ace458c6058 --- /dev/null +++ b/graylog2-server/src/test/java/org/graylog2/bootstrap/preflight/web/resources/PreflightAssetsResourceTest.java @@ -0,0 +1,83 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog2.bootstrap.preflight.web.resources; + +import jakarta.ws.rs.NotFoundException; +import jakarta.ws.rs.core.EntityTag; +import jakarta.ws.rs.core.Request; +import jakarta.ws.rs.core.Response; +import org.graylog2.web.resources.ResourceFileReader; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import javax.activation.MimetypesFileTypeMap; +import java.util.Date; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assumptions.assumeTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class PreflightAssetsResourceTest { + private final PreflightAssetsResource resource = + new PreflightAssetsResource(new MimetypesFileTypeMap(), new ResourceFileReader()); + + /** + * The JAX-RS path template does not match a literal slash, but percent-encoded traversal sequences pass + * the template and are URL-decoded before they reach the resource method, which is what we get here. + */ + @ParameterizedTest + @ValueSource(strings = { + "../PreflightConstants.class", + "../../log4j2.xml", + "../..", + "..", + "/etc/passwd", + "", + "sub/dir/app.js", + }) + void refusesToServeAnythingOutsideOfTheAssetsDirectory(String filename) { + assertThatThrownBy(() -> resource.get(mock(Request.class), filename)) + .isInstanceOf(NotFoundException.class); + } + + @Test + void doesNotReflectTheRequestedFileNameBackToTheClient() { + assertThatThrownBy(() -> resource.get(mock(Request.class), "../log4j2.xml")) + .isInstanceOf(NotFoundException.class) + .hasMessageNotContaining("log4j2.xml"); + } + + @Test + void servesAnAssetFromTheAssetsDirectory() { + assumeTrue(getClass().getResource("/preflight/assets/index.html") != null, + "preflight assets are not on the classpath, the frontend was not built into the server"); + + final Request request = mock(Request.class); + // No preconditions to evaluate, so the full response gets built. + when(request.evaluatePreconditions(any(Date.class), any(EntityTag.class))).thenReturn(null); + + final Response response = resource.index(request); + + assertThat(response.getStatus()).isEqualTo(Response.Status.OK.getStatusCode()); + assertThat((byte[]) response.getEntity()).isNotEmpty(); + assertThat(response.getEntityTag()).isNotNull(); + } +} diff --git a/graylog2-server/src/test/java/org/graylog2/web/resources/ResourceFileReaderTest.java b/graylog2-server/src/test/java/org/graylog2/web/resources/ResourceFileReaderTest.java new file mode 100644 index 000000000000..0856e1868c7e --- /dev/null +++ b/graylog2-server/src/test/java/org/graylog2/web/resources/ResourceFileReaderTest.java @@ -0,0 +1,193 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog2.web.resources; + +import org.graylog2.bootstrap.preflight.PreflightConstants; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import java.io.FileNotFoundException; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.stream.Stream; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +class ResourceFileReaderTest { + private static final String TEST_ASSETS_DIR = "/org/graylog2/web/resources/assets-test/"; + + private final ResourceFileReader reader = new ResourceFileReader(); + + @ParameterizedTest + @ValueSource(strings = {"index.html", "abc.js", "abc.js.map", "./abc.js", "abc.js/"}) + void acceptsAndCanonicalizesPlainFileNames(String filename) throws Exception { + assertThat(ResourceFileReader.resolveResourceName(ResourceFileReader.WEB_INTERFACE_ASSETS_DIR, filename)) + .isEqualTo("/web-interface/assets/" + Paths.get(filename).getFileName()); + } + + @Test + void acceptsPlainFileNamesInThePluginJarRoot() throws Exception { + assertThat(ResourceFileReader.resolveResourceName(ResourceFileReader.PLUGIN_ASSETS_DIR, "abc.js")) + .isEqualTo("/abc.js"); + } + + @Test + void appendsASeparatorToABaseDirectoryWithoutATrailingSlash() throws Exception { + assertThat(ResourceFileReader.resolveResourceName("/web-interface/assets", "abc.js")) + .isEqualTo("/web-interface/assets/abc.js"); + } + + @ParameterizedTest + @ValueSource(strings = { + "../../etc/passwd", + "..", + "../", + "./../abc.js", + "foo/../../..", + "/etc/passwd", + "", + ".", + "sub/dir/app.js", + }) + void rejectsAnythingButADirectChildOfTheAssetsDirectory(String filename) { + assertThatThrownBy(() -> ResourceFileReader.resolveResourceName(ResourceFileReader.WEB_INTERFACE_ASSETS_DIR, filename)) + .isInstanceOf(FileNotFoundException.class) + .hasMessage("Invalid resource file name."); + } + + /** + * Regression test for the plugin asset directory being the JAR root: a plain containment check + * ({@code resolved.startsWith(base)}) passes for every one of these, because {@code "/"} contains any + * path. Only the direct-child check rejects them. + */ + @ParameterizedTest + @ValueSource(strings = {"../etc/passwd", "..", "../..", "etc/passwd", "/etc/passwd", ""}) + void rejectsTraversalOutOfThePluginJarRoot(String filename) { + assertThatThrownBy(() -> ResourceFileReader.resolveResourceName(ResourceFileReader.PLUGIN_ASSETS_DIR, filename)) + .isInstanceOf(FileNotFoundException.class) + .hasMessage("Invalid resource file name."); + } + + @Test + void rejectsFileNamesThatAreNotValidPaths() { + // A NUL character makes Paths.get() throw an InvalidPathException. + final String filenameWithNulCharacter = "abc" + (char) 0 + ".js"; + + assertThatThrownBy(() -> ResourceFileReader.resolveResourceName(ResourceFileReader.WEB_INTERFACE_ASSETS_DIR, filenameWithNulCharacter)) + .isInstanceOf(FileNotFoundException.class) + .hasMessage("Invalid resource file name."); + } + + /** + * A backslash is a path separator on Windows but an ordinary filename character elsewhere, so the two + * platforms reject this by different routes (rejected outright vs. resolved to a nonexistent file inside the directory). + * Either way it must not address anything outside of the assets directory. + */ + @Test + void doesNotLetBackslashSeparatorsEscapeTheAssetsDirectory() { + try { + final String resourceName = + ResourceFileReader.resolveResourceName(ResourceFileReader.WEB_INTERFACE_ASSETS_DIR, "..\\abc.js"); + assertThat(resourceName).startsWith(ResourceFileReader.WEB_INTERFACE_ASSETS_DIR); + } catch (FileNotFoundException e) { + assertThat(e).hasMessage("Invalid resource file name."); + } + } + + @Test + void readsAFileFromTheGivenDirectory() throws Exception { + final var resource = reader.readFileFrom(TEST_ASSETS_DIR, "inside.txt", getClass()); + + assertThat(new String(resource.contents().get(), StandardCharsets.UTF_8)) + .isEqualTo("inside-the-assets-directory\n"); + } + + /** + * The traversal target exists on the classpath, so this fails only because the guard rejects it. + */ + @Test + void refusesToReadAnExistingFileOutsideOfTheGivenDirectory() { + assumeTrue(getClass().getResource("/org/graylog2/web/resources/outside.txt") != null, + "test fixture missing"); + + assertThatThrownBy(() -> reader.readFileFrom(TEST_ASSETS_DIR, "../outside.txt", getClass())) + .isInstanceOf(FileNotFoundException.class); + } + + @Test + void readsAPluginAssetFromThePluginsOwnCodeSource() throws Exception { + // This test class and the fixture both live in target/test-classes, i.e. the same code source. + final var resource = reader.readFileFromPlugin("plugin-asset-test.js", getClass()); + + assertThat(new String(resource.contents().get(), StandardCharsets.UTF_8)) + .startsWith("// Stands in for a plugin asset"); + } + + /** + * A traversal sequence normalizes away at the JAR root, so {@code ../../log4j2.xml} resolves to the + * perfectly root-level {@code /log4j2.xml}. Only the code source check stops it: {@code log4j2.xml} is + * packaged in the server's classes directory, not in this "plugin's" own. + */ + @ParameterizedTest + @ValueSource(strings = {"log4j2.xml", "../log4j2.xml", "../../log4j2.xml", "git.properties"}) + void refusesToServePluginAssetsFromAnotherCodeSource(String filename) { + assumeTrue(getClass().getResource("/log4j2.xml") != null, "test fixture missing"); + + assertThatThrownBy(() -> reader.readFileFromPlugin(filename, getClass())) + .isInstanceOf(FileNotFoundException.class); + } + + @Test + void treatsAResourceWithoutAResolvableCodeSourceAsNotFound() { + assertThat(ResourceFileReader.isFromCodeSourceOf(null, getClass())).isFalse(); + } + + @Test + void doesNotLeakTheRequestedFileNameInTheNotFoundMessage() { + assertThatThrownBy(() -> reader.readFileFrom(TEST_ASSETS_DIR, "does-not-exist.txt", getClass())) + .isInstanceOf(FileNotFoundException.class) + .hasMessageNotContaining("does-not-exist.txt"); + } + + /** + * The direct-child check in {@link ResourceFileReader#resolveResourceName(String, String)} means assets in + * a subdirectory could not be served. Both asset directories are flat today; this fails the build if a + * future frontend build starts emitting nested assets, rather than letting them 404 in production. + */ + @ParameterizedTest + @ValueSource(strings = {"/web-interface/assets", PreflightConstants.ASSETS_RESOURCE_DIR}) + void assetDirectoriesAreFlat(String assetsDir) throws Exception { + final String directoryName = assetsDir.endsWith("/") ? assetsDir.substring(0, assetsDir.length() - 1) : assetsDir; + final URL url = getClass().getResource(directoryName); + assumeTrue(url != null, "assets are not on the classpath, the frontend was not built into the server"); + assumeTrue("file".equals(url.getProtocol()), "assets are not served from a directory"); + + final Path root = Paths.get(url.toURI()); + try (Stream entries = Files.walk(root)) { + assertThat(entries.filter(path -> !path.equals(root))) + .allSatisfy(path -> assertThat(path.getParent()) + .as("asset %s is not a direct child of %s", path, assetsDir) + .isEqualTo(root)); + } + } +} diff --git a/graylog2-server/src/test/java/org/graylog2/web/resources/WebInterfaceAssetsResourceTest.java b/graylog2-server/src/test/java/org/graylog2/web/resources/WebInterfaceAssetsResourceTest.java new file mode 100644 index 000000000000..c909412d9f57 --- /dev/null +++ b/graylog2-server/src/test/java/org/graylog2/web/resources/WebInterfaceAssetsResourceTest.java @@ -0,0 +1,172 @@ +/* + * Copyright (C) 2020 Graylog, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + */ +package org.graylog2.web.resources; + +import jakarta.ws.rs.NotFoundException; +import jakarta.ws.rs.core.HttpHeaders; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.MultivaluedHashMap; +import jakarta.ws.rs.core.Response; +import org.glassfish.jersey.server.ContainerRequest; +import org.graylog2.configuration.HttpConfiguration; +import org.graylog2.plugin.Plugin; +import org.graylog2.plugin.PluginMetaData; +import org.graylog2.plugin.ServerStatus; +import org.graylog2.plugin.Version; +import org.graylog2.web.IndexHtmlGenerator; +import org.graylog2.web.customization.CustomizationConfig; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import javax.activation.MimetypesFileTypeMap; +import java.net.URI; +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +class WebInterfaceAssetsResourceTest { + private static final String PLUGIN_ID = "org.graylog.plugins.test.TestPlugin"; + + private IndexHtmlGenerator indexHtmlGenerator; + private HttpHeaders headers; + private WebInterfaceAssetsResource resource; + + /** + * A real class rather than a mock, so that it has a meaningful code source (this module's test classes) + * for {@link ResourceFileReader#readFileFromPlugin(String, Class)} to check assets against. + */ + private static class TestPluginMetaData implements PluginMetaData { + @Override + public String getUniqueId() { + return PLUGIN_ID; + } + + @Override + public String getName() { + return "Test"; + } + + @Override + public String getAuthor() { + return "Graylog, Inc."; + } + + @Override + public URI getURL() { + return URI.create("https://www.graylog.org/"); + } + + @Override + public Version getVersion() { + return Version.from(1, 0, 0); + } + + @Override + public String getDescription() { + return "Test plugin"; + } + + @Override + public Version getRequiredVersion() { + return Version.from(1, 0, 0); + } + + @Override + public Set getRequiredCapabilities() { + return Set.of(); + } + } + + @BeforeEach + void setUp() { + indexHtmlGenerator = mock(IndexHtmlGenerator.class); + when(indexHtmlGenerator.get(any(), any())).thenReturn("index"); + + headers = mock(HttpHeaders.class); + when(headers.getRequestHeaders()).thenReturn(new MultivaluedHashMap<>()); + + final HttpConfiguration httpConfiguration = mock(HttpConfiguration.class); + when(httpConfiguration.getHttpExternalUri()).thenReturn(URI.create("http://localhost:9000/")); + + final Plugin plugin = mock(Plugin.class); + when(plugin.metadata()).thenReturn(new TestPluginMetaData()); + + resource = new WebInterfaceAssetsResource(indexHtmlGenerator, + Set.of(plugin), + new MimetypesFileTypeMap(), + httpConfiguration, + mock(CustomizationConfig.class), + new ResourceFileReader()); + } + + @Test + void servesAnAssetOfTheRequestedPlugin() { + final Response response = resource.get(mock(ContainerRequest.class), headers, PLUGIN_ID, "plugin-asset-test.js"); + + assertThat(response.getStatus()).isEqualTo(Response.Status.OK.getStatusCode()); + assertThat((byte[]) response.getEntity()).isNotEmpty(); + } + + /** + * Plugin assets are looked up at the root of the plugin JAR, so a traversal sequence normalizes away and + * lands on another JAR's root-level resource. {@code log4j2.xml} ships in the server's classes directory, + * so it belongs to a different code source than this plugin. + */ + @ParameterizedTest + @ValueSource(strings = { + "../../log4j2.xml", + "../log4j2.xml", + "log4j2.xml", + "..", + "/etc/passwd", + "org/graylog2/web/resources/outside.txt", + }) + void refusesToServePluginAssetsThatAreNotThePluginsOwn(String filename) { + assertThatThrownBy(() -> resource.get(mock(ContainerRequest.class), headers, PLUGIN_ID, filename)) + .isInstanceOf(NotFoundException.class); + } + + @Test + void doesNotReflectTheRequestedPluginAssetNameBackToTheClient() { + assertThatThrownBy(() -> resource.get(mock(ContainerRequest.class), headers, PLUGIN_ID, "../log4j2.xml")) + .isInstanceOf(NotFoundException.class) + .hasMessageNotContaining("log4j2.xml"); + } + + /** + * The non-plugin asset route deliberately falls back to the single-page-application entry point for + * anything it cannot resolve, so a rejected traversal is indistinguishable from any other unknown route. + * This pins that behaviour so the fallback cannot turn into a file read. + */ + @ParameterizedTest + @ValueSource(strings = {"../../log4j2.xml", "..", "/etc/passwd", "../outside.txt"}) + void servesTheIndexPageInsteadOfTraversingOutOfTheAssetsDirectory(String filename) { + final Response response = resource.get(mock(ContainerRequest.class), headers, filename); + + assertThat(response.getStatus()).isEqualTo(Response.Status.OK.getStatusCode()); + assertThat(response.getHeaderString(HttpHeaders.CONTENT_TYPE)).isEqualTo(MediaType.TEXT_HTML); + assertThat(response.getEntity()).isEqualTo("index"); + verify(indexHtmlGenerator).get(any(), any()); + } +} diff --git a/graylog2-server/src/test/resources/org/graylog2/web/resources/assets-test/inside.txt b/graylog2-server/src/test/resources/org/graylog2/web/resources/assets-test/inside.txt new file mode 100644 index 000000000000..f6d93678d9b5 --- /dev/null +++ b/graylog2-server/src/test/resources/org/graylog2/web/resources/assets-test/inside.txt @@ -0,0 +1 @@ +inside-the-assets-directory diff --git a/graylog2-server/src/test/resources/org/graylog2/web/resources/outside.txt b/graylog2-server/src/test/resources/org/graylog2/web/resources/outside.txt new file mode 100644 index 000000000000..d37b9ac3a27a --- /dev/null +++ b/graylog2-server/src/test/resources/org/graylog2/web/resources/outside.txt @@ -0,0 +1 @@ +outside-the-assets-directory diff --git a/graylog2-server/src/test/resources/plugin-asset-test.js b/graylog2-server/src/test/resources/plugin-asset-test.js new file mode 100644 index 000000000000..f3c67d2475fa --- /dev/null +++ b/graylog2-server/src/test/resources/plugin-asset-test.js @@ -0,0 +1 @@ +// Stands in for a plugin asset packaged at the root of a plugin JAR.