From f20a04809cb81cc6c29b9523d2282a8dcabf4fa6 Mon Sep 17 00:00:00 2001 From: Aurora Lahtela <24460436+AuroraLS3@users.noreply.github.com> Date: Sun, 15 Jan 2023 09:30:30 +0200 Subject: [PATCH] Test against and identify path traversal vulnerability in other methods --- .../plan/delivery/web/ResourceSvc.java | 22 ++++--- .../delivery/webserver/ResponseFactory.java | 4 +- .../settings/config/ResourceSettings.java | 5 +- .../plan/storage/file/FileResource.java | 6 +- .../plan/storage/file/PlanFiles.java | 14 ++-- .../plan/storage/file/Resource.java | 2 + .../plan/storage/file/ResourceCache.java | 3 +- .../plan/storage/file/PlanFilesTest.java | 65 +++++++++++++++++++ 8 files changed, 102 insertions(+), 19 deletions(-) create mode 100644 Plan/common/src/test/java/com/djrapitops/plan/storage/file/PlanFilesTest.java diff --git a/Plan/common/src/main/java/com/djrapitops/plan/delivery/web/ResourceSvc.java b/Plan/common/src/main/java/com/djrapitops/plan/delivery/web/ResourceSvc.java index 54fb14e0c..9efa6f387 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/delivery/web/ResourceSvc.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/delivery/web/ResourceSvc.java @@ -23,6 +23,7 @@ import com.djrapitops.plan.settings.locale.Locale; import com.djrapitops.plan.settings.locale.lang.PluginLang; import com.djrapitops.plan.storage.file.PlanFiles; import com.djrapitops.plan.storage.file.Resource; +import com.djrapitops.plan.utilities.dev.Untrusted; import com.djrapitops.plan.utilities.logging.ErrorContext; import com.djrapitops.plan.utilities.logging.ErrorLogger; import net.playeranalytics.plugin.server.PluginLogger; @@ -76,12 +77,12 @@ public class ResourceSvc implements ResourceService { } @Override - public WebResource getResource(String pluginName, String fileName, Supplier source) { + public WebResource getResource(String pluginName, @Untrusted String fileName, Supplier source) { checkParams(pluginName, fileName, source); return applySnippets(pluginName, fileName, getTheResource(pluginName, fileName, source)); } - public void checkParams(String pluginName, String fileName, Supplier source) { + public void checkParams(String pluginName, @Untrusted String fileName, Supplier source) { if (pluginName == null || pluginName.isEmpty()) { throw new IllegalArgumentException("'pluginName' can't be '" + pluginName + "'!"); } @@ -93,7 +94,7 @@ public class ResourceSvc implements ResourceService { } } - private WebResource applySnippets(String pluginName, String fileName, WebResource resource) { + private WebResource applySnippets(String pluginName, @Untrusted String fileName, WebResource resource) { Map byPosition = calculateSnippets(fileName); if (byPosition.isEmpty()) return resource; @@ -129,7 +130,7 @@ public class ResourceSvc implements ResourceService { return html; } - private Map calculateSnippets(String fileName) { + private Map calculateSnippets(@Untrusted String fileName) { Map byPosition = new EnumMap<>(Position.class); for (Snippet snippet : snippets) { if (snippet.matches(fileName)) { @@ -139,7 +140,7 @@ public class ResourceSvc implements ResourceService { return byPosition; } - public WebResource getTheResource(String pluginName, String fileName, Supplier source) { + public WebResource getTheResource(String pluginName, @Untrusted String fileName, Supplier source) { try { if (resourceSettings.shouldBeCustomized(pluginName, fileName)) { return getOrWriteCustomized(fileName, source); @@ -153,7 +154,7 @@ public class ResourceSvc implements ResourceService { return source.get(); } - public WebResource getOrWriteCustomized(String fileName, Supplier source) throws IOException { + public WebResource getOrWriteCustomized(@Untrusted String fileName, Supplier source) throws IOException { Optional customizedResource = files.getCustomizableResource(fileName); if (customizedResource.isPresent()) { return readCustomized(customizedResource.get()); @@ -170,11 +171,16 @@ public class ResourceSvc implements ResourceService { } } - public WebResource writeCustomized(String fileName, Supplier source) throws IOException { + public WebResource writeCustomized(@Untrusted String fileName, Supplier source) throws IOException { WebResource original = source.get(); byte[] bytes = original.asBytes(); OpenOption[] overwrite = {StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE}; - Path to = resourceSettings.getCustomizationDirectory().resolve(fileName); + @Untrusted Path to = resourceSettings.getCustomizationDirectory().resolve(fileName); + if (!to.startsWith(resourceSettings.getCustomizationDirectory())) { + throw new IllegalArgumentException( + "Absolute path was given for writing a customized file, " + + "writing outside customization directory is prevented for security reasons."); + } Path dir = to.getParent(); if (!Files.isSymbolicLink(dir)) Files.createDirectories(dir); Files.write(to, bytes, overwrite); diff --git a/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/ResponseFactory.java b/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/ResponseFactory.java index a1ea83b93..4325ea6e3 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/ResponseFactory.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/delivery/webserver/ResponseFactory.java @@ -84,7 +84,7 @@ public class ResponseFactory { this.addresses = addresses; } - public WebResource getResource(String resourceName) { + public WebResource getResource(@Untrusted String resourceName) { return ResourceService.getInstance().getResource("Plan", resourceName, () -> files.getResourceFromJar("web/" + resourceName).asWebResource()); } @@ -205,7 +205,7 @@ public class ResponseFactory { return StringUtils.replace(resource, "PLAN_BASE_ADDRESS", address); } - public Response cssResponse(String fileName) { + public Response cssResponse(@Untrusted String fileName) { try { String content = UnaryChain.of(getResource(fileName).asString()) .chain(theme::replaceThemeColors) diff --git a/Plan/common/src/main/java/com/djrapitops/plan/settings/config/ResourceSettings.java b/Plan/common/src/main/java/com/djrapitops/plan/settings/config/ResourceSettings.java index dbe55d47a..dedf4d6e2 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/settings/config/ResourceSettings.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/settings/config/ResourceSettings.java @@ -19,6 +19,7 @@ package com.djrapitops.plan.settings.config; import com.djrapitops.plan.settings.config.paths.CustomizedFileSettings; import com.djrapitops.plan.settings.config.paths.PluginSettings; import com.djrapitops.plan.storage.file.PlanFiles; +import com.djrapitops.plan.utilities.dev.Untrusted; import org.apache.commons.lang3.StringUtils; import java.io.IOException; @@ -39,7 +40,7 @@ public class ResourceSettings { this.config = config; } - public boolean shouldBeCustomized(String plugin, String fileName) { + public boolean shouldBeCustomized(String plugin, @Untrusted String fileName) { if (config.isTrue(CustomizedFileSettings.WEB_DEV_MODE) && config.isFalse(PluginSettings.FRONTEND_BETA)) { return true; } @@ -48,6 +49,8 @@ public class ResourceSettings { fileCustomization.setComment(Collections.singletonList("The files are placed in /Plan/web/ if the setting is 'true' when accessed.")); ConfigNode pluginCustomization = fileCustomization.getNode(plugin).orElseGet(() -> fileCustomization.addNode(plugin)); + + // No longer untrusted in configuration context, but may contain untrusted data in other context. String fileNameNonPath = StringUtils.replaceChars(fileName, '.', ','); if (pluginCustomization.contains(fileNameNonPath)) { diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/FileResource.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/FileResource.java index 964337d5f..0ebd760cc 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/FileResource.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/FileResource.java @@ -16,6 +16,8 @@ */ package com.djrapitops.plan.storage.file; +import com.djrapitops.plan.utilities.dev.Untrusted; + import java.io.File; import java.io.FileInputStream; import java.io.IOException; @@ -35,10 +37,11 @@ import java.util.stream.Stream; */ public class FileResource implements Resource { + @Untrusted private final String resourceName; private final File file; - public FileResource(String resourceName, File file) { + public FileResource(@Untrusted String resourceName, File file) { this.resourceName = resourceName; this.file = file; } @@ -54,6 +57,7 @@ public class FileResource implements Resource { } @Override + @Untrusted public String getResourceName() { return resourceName; } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java index a3ce8185f..963247e92 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/PlanFiles.java @@ -119,7 +119,7 @@ public class PlanFiles implements SubSystem { * @param resourceName Path to the file inside jar/assets/plan/ folder. * @return a {@link Resource} for accessing the resource. */ - public Resource getResourceFromJar(String resourceName) { + public Resource getResourceFromJar(@Untrusted String resourceName) { return new JarResource("assets/plan/" + resourceName, getResourceStream); } @@ -134,11 +134,11 @@ public class PlanFiles implements SubSystem { } // TODO Customized file logic should be moved to another class so the circular dependency on config can be removed. - public Optional getCustomizableResource(String resourceName) { + public Optional getCustomizableResource(@Untrusted String resourceName) { return Optional.ofNullable(findCustomized(resourceName)); } - private Resource findCustomized(String resourceName) { + private Resource findCustomized(@Untrusted String resourceName) { if (config.get().isTrue(CustomizedFileSettings.WEB_DEV_MODE)) { // Bypass cache in web developer mode. return getFileResource(resourceName); @@ -147,19 +147,21 @@ public class PlanFiles implements SubSystem { } } - private FileResource getFileResource(String resourceName) { + private FileResource getFileResource(@Untrusted String resourceName) { return attemptToFind(resourceName) .map(found -> new FileResource(resourceName, found)) .orElse(null); } - public Optional attemptToFind(String resourceName) { + public Optional attemptToFind(@Untrusted String resourceName) { Path dir = config.get().getResourceSettings().getCustomizationDirectory(); if (dir.toFile().exists() && dir.toFile().isDirectory()) { - Path asPath = dir.resolve(resourceName); + // Path may be absolute due to resolving untrusted path + @Untrusted Path asPath = dir.resolve(resourceName); if (!asPath.startsWith(dir)) { return Optional.empty(); } + // Now it should be trustworthy File found = asPath.toFile(); if (found.exists()) { return Optional.of(found); diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/Resource.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/Resource.java index 6e46a8513..219dd7bef 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/Resource.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/Resource.java @@ -17,6 +17,7 @@ package com.djrapitops.plan.storage.file; import com.djrapitops.plan.delivery.web.resource.WebResource; +import com.djrapitops.plan.utilities.dev.Untrusted; import com.google.gson.Gson; import com.google.gson.reflect.TypeToken; import org.apache.commons.lang3.StringUtils; @@ -38,6 +39,7 @@ public interface Resource { * * @return Relative file path given to {@link PlanFiles}. */ + @Untrusted String getResourceName(); byte[] asBytes() throws IOException; diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/ResourceCache.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/ResourceCache.java index 974a68170..d37e5c1c6 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/file/ResourceCache.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/file/ResourceCache.java @@ -16,6 +16,7 @@ */ package com.djrapitops.plan.storage.file; +import com.djrapitops.plan.utilities.dev.Untrusted; import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; @@ -40,7 +41,7 @@ public class ResourceCache { // Static class } - public static Resource getOrCache(String resourceName, Supplier resourceSupplier) { + public static Resource getOrCache(@Untrusted String resourceName, Supplier resourceSupplier) { String found = cache.getIfPresent(resourceName); if (found == null) { Resource created = resourceSupplier.get(); diff --git a/Plan/common/src/test/java/com/djrapitops/plan/storage/file/PlanFilesTest.java b/Plan/common/src/test/java/com/djrapitops/plan/storage/file/PlanFilesTest.java new file mode 100644 index 000000000..72f49d350 --- /dev/null +++ b/Plan/common/src/test/java/com/djrapitops/plan/storage/file/PlanFilesTest.java @@ -0,0 +1,65 @@ +/* + * This file is part of Player Analytics (Plan). + * + * Plan is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License v3 as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Plan 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 + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Plan. If not, see . + */ +package com.djrapitops.plan.storage.file; + +import com.djrapitops.plan.settings.config.PlanConfig; +import com.djrapitops.plan.settings.config.paths.CustomizedFileSettings; +import extension.FullSystemExtension; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.io.TempDir; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * @author AuroraLS3 + */ +@ExtendWith(FullSystemExtension.class) +class PlanFilesTest { + + @Test + @DisplayName("getFileFromPluginFolder has no Path Traversal vulnerability") + void getFileFromPluginFolderDoesNotAllowAbsolutePathTraversal(@TempDir Path tempDir, PlanFiles files) throws IOException { + Path testFile = tempDir.resolve("file.db"); + Files.createDirectories(tempDir.getParent()); + Files.createFile(testFile); + + File file = files.getFileFromPluginFolder(testFile.toFile().getAbsolutePath()); + assertNotEquals(testFile.toFile().getAbsolutePath(), file.getAbsolutePath()); + } + + @Test + @DisplayName("getCustomizableResource has no Path Traversal vulnerability") + void getCustomizableResourceDoesNotAllowAbsolutePathTraversal(@TempDir Path tempDir, PlanConfig config, PlanFiles files) throws IOException { + config.set(CustomizedFileSettings.PATH, tempDir.resolve("customized").toFile().getAbsolutePath()); + + Path testFile = tempDir.resolve("file.db"); + Files.createDirectories(tempDir.getParent()); + Files.createFile(testFile); + + Optional resource = files.getCustomizableResource(testFile.toFile().getAbsolutePath()); + assertTrue(resource.isEmpty()); + } +} \ No newline at end of file