This is an automated email from the ASF dual-hosted git repository. houston pushed a commit to branch add-pki-caching in repository https://gitbox.apache.org/repos/asf/solr.git
commit ea9fa7c13a7c6fb454a40340c9b63a9b83ad9496 Author: Jason Gerlowski <[email protected]> AuthorDate: Mon Nov 11 10:34:20 2024 -0500 SOLR-17543: Input validation in FSConfigSetService See JIRA ticket for more details. --- solr/CHANGES.txt | 2 + .../solr/core/FileSystemConfigSetService.java | 38 ++++++++++------ .../src/java/org/apache/solr/util/FileUtils.java | 24 +++++++++++ .../solr/core/TestFileSystemConfigSetService.java | 38 ++++++++++++++-- .../test/org/apache/solr/util/FileUtilsTest.java | 50 ++++++++++++++++++++++ 5 files changed, 136 insertions(+), 16 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index bfde8a52c99..10c3ff5c965 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -148,6 +148,8 @@ Bug Fixes * SOLR-17515: Http2SolrClient now sets its 'AuthenticationStore' in all circumstances, preventing NPE's when used with a HttpClientBuilderFactory (Jason Gerlowski) +* SOLR-17543: Adds input validation to zip entries used by FileSystemConfigSetService (Jason Gerlowski) + Dependency Upgrades --------------------- * PR#2512: Update dependency com.carrotsearch:hppc to v0.10.0 (solrbot) diff --git a/solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java b/solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java index da01cc57620..56f17c395d1 100644 --- a/solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java +++ b/solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java @@ -38,6 +38,7 @@ import org.apache.solr.common.SolrException; import org.apache.solr.common.cloud.ZkMaintenanceUtils; import org.apache.solr.common.util.Utils; import org.apache.solr.util.FileTypeMagicUtil; +import org.apache.solr.util.FileUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -150,19 +151,30 @@ public class FileSystemConfigSetService extends ConfigSetService { throws IOException { if (ZkMaintenanceUtils.isFileForbiddenInConfigSets(fileName)) { log.warn("Not including uploading file to config, as it is a forbidden type: {}", fileName); - } else { - if (!FileTypeMagicUtil.isFileForbiddenInConfigset(data)) { - Path filePath = getConfigDir(configName).resolve(normalizePathToOsSeparator(fileName)); - if (!Files.exists(filePath) || overwriteOnExists) { - Files.write(filePath, data); - } - } else { - String mimeType = FileTypeMagicUtil.INSTANCE.guessMimeType(data); - log.warn( - "Not including uploading file {}, as it matched the MAGIC signature of a forbidden mime type {}", - fileName, - mimeType); - } + return; + } + if (FileTypeMagicUtil.isFileForbiddenInConfigset(data)) { + String mimeType = FileTypeMagicUtil.INSTANCE.guessMimeType(data); + log.warn( + "Not including uploading file {}, as it matched the MAGIC signature of a forbidden mime type {}", + fileName, + mimeType); + return; + } + final var configsetBasePath = getConfigDir(configName); + final var configsetFilePath = configsetBasePath.resolve(normalizePathToOsSeparator(fileName)); + if (!FileUtils.isPathAChildOfParent( + configsetBasePath, configsetFilePath)) { // See SOLR-17543 for context + log.warn( + "Not uploading file [{}], as it resolves to a location [{}] outside of the configset root directory [{}]", + fileName, + configsetFilePath, + configsetBasePath); + return; + } + + if (overwriteOnExists || !Files.exists(configsetFilePath)) { + Files.write(configsetFilePath, data); } } diff --git a/solr/core/src/java/org/apache/solr/util/FileUtils.java b/solr/core/src/java/org/apache/solr/util/FileUtils.java index f90f8a658aa..b027d1e4d77 100644 --- a/solr/core/src/java/org/apache/solr/util/FileUtils.java +++ b/solr/core/src/java/org/apache/solr/util/FileUtils.java @@ -103,4 +103,28 @@ public class FileUtils { } return Files.createDirectories(path); } + + /** + * Checks whether a child path falls under a particular parent + * + * <p>Useful for validating user-provided relative paths, which generally aren't expected to + * "escape" a given parent/root directory. Parent and child paths are "normalized" by {@link + * Path#normalize()}. This removes explicit backtracking (e.g. "../") though it will not resolve + * symlinks if any are present in the provided Paths, so some forms of parent "escape" remain + * undetected. Paths needn't exist as a file or directory for comparison purposes. + * + * <p>Note, this method does not consult the file system + * + * @param parent the path of a 'parent' node. Path must be syntactically valid but needn't exist. + * @param potentialChild the path of a potential child. Typically obtained via: + * parent.resolve(relativeChildPath). Path must be syntactically valid but needn't exist. + * @return true if 'potentialChild' nests under the provided 'parent', false otherwise. + */ + public static boolean isPathAChildOfParent(Path parent, Path potentialChild) { + final var normalizedParent = parent.toAbsolutePath().normalize(); + final var normalizedChild = potentialChild.toAbsolutePath().normalize(); + + return normalizedChild.startsWith(normalizedParent) + && !normalizedChild.equals(normalizedParent); + } } diff --git a/solr/core/src/test/org/apache/solr/core/TestFileSystemConfigSetService.java b/solr/core/src/test/org/apache/solr/core/TestFileSystemConfigSetService.java index f9695a990e5..e4ceb1b831c 100644 --- a/solr/core/src/test/org/apache/solr/core/TestFileSystemConfigSetService.java +++ b/solr/core/src/test/org/apache/solr/core/TestFileSystemConfigSetService.java @@ -17,7 +17,9 @@ package org.apache.solr.core; import static org.apache.solr.core.FileSystemConfigSetService.METADATA_FILE; +import static org.hamcrest.Matchers.hasItem; +import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; @@ -49,13 +51,43 @@ public class TestFileSystemConfigSetService extends SolrTestCaseJ4 { fileSystemConfigSetService = null; } + @Test + public void testIgnoresFileUploadsOutsideOfConfigSetDirectory() throws IOException { + final var initialNumConfigs = fileSystemConfigSetService.listConfigs().size(); + final String configName = "fileEscapeTestConfig"; + final var specificConfigSetBase = configSetBase.resolve(configName); + + fileSystemConfigSetService.uploadConfig(configName, configset("cloud-minimal")); + assertEquals(fileSystemConfigSetService.listConfigs().size(), initialNumConfigs + 1); + assertTrue(fileSystemConfigSetService.checkConfigExists(configName)); + + // This succeeds, as the file is an allowed type and the path doesn't attempt to escape the + // configset's root + byte[] testdata = "test data".getBytes(StandardCharsets.UTF_8); + fileSystemConfigSetService.uploadFileToConfig(configName, "validPath", testdata, true); + final var knownFiles = fileSystemConfigSetService.getAllConfigFiles(configName); + assertThat(knownFiles, hasItem("validPath")); + assertTrue(Files.exists(specificConfigSetBase.resolve("validPath"))); + + // Each of these will fail "quietly" as ConfigSetService opts to log warnings but otherwise not + // surface validation errors to enable bulk uploading + final var invalidFilePaths = + List.of( + ".." + File.separator + "escapePath", + "foo" + File.separator + ".." + File.separator + ".." + File.separator + "bar"); + for (String invalidFilePath : invalidFilePaths) { + fileSystemConfigSetService.uploadFileToConfig(configName, invalidFilePath, testdata, true); + assertFalse(Files.exists(specificConfigSetBase.resolve(invalidFilePath))); + } + } + @Test public void testUploadAndDeleteConfig() throws IOException { + final var initialNumConfigs = fileSystemConfigSetService.listConfigs().size(); String configName = "testconfig"; fileSystemConfigSetService.uploadConfig(configName, configset("cloud-minimal")); - - assertEquals(fileSystemConfigSetService.listConfigs().size(), 1); + assertEquals(fileSystemConfigSetService.listConfigs().size(), initialNumConfigs + 1); assertTrue(fileSystemConfigSetService.checkConfigExists(configName)); byte[] testdata = "test data".getBytes(StandardCharsets.UTF_8); @@ -79,7 +111,7 @@ public class TestFileSystemConfigSetService extends SolrTestCaseJ4 { assertEquals("[schema.xml, solrconfig.xml]", allConfigFiles.toString()); fileSystemConfigSetService.copyConfig(configName, "copytestconfig"); - assertEquals(fileSystemConfigSetService.listConfigs().size(), 2); + assertEquals(fileSystemConfigSetService.listConfigs().size(), initialNumConfigs + 2); allConfigFiles = fileSystemConfigSetService.getAllConfigFiles("copytestconfig"); assertEquals("[schema.xml, solrconfig.xml]", allConfigFiles.toString()); diff --git a/solr/core/src/test/org/apache/solr/util/FileUtilsTest.java b/solr/core/src/test/org/apache/solr/util/FileUtilsTest.java index 671157c9e72..0dfeff109a3 100644 --- a/solr/core/src/test/org/apache/solr/util/FileUtilsTest.java +++ b/solr/core/src/test/org/apache/solr/util/FileUtilsTest.java @@ -17,10 +17,13 @@ package org.apache.solr.util; import java.io.File; +import java.nio.file.Path; import org.apache.solr.SolrTestCase; +import org.junit.Test; public class FileUtilsTest extends SolrTestCase { + @Test public void testResolve() { String cwd = new File(".").getAbsolutePath(); assertEquals(new File("conf/data"), FileUtils.resolvePath(new File("conf"), "data")); @@ -28,4 +31,51 @@ public class FileUtilsTest extends SolrTestCase { new File(cwd + "/conf/data"), FileUtils.resolvePath(new File(cwd + "/conf"), "data")); assertEquals(new File(cwd + "/data"), FileUtils.resolvePath(new File("conf"), cwd + "/data")); } + + @Test + public void testDetectsPathEscape() { + final var parent = Path.of("."); + + // Allows simple child + assertTrue(FileUtils.isPathAChildOfParent(parent, parent.resolve("child"))); + + // Allows "./" prefixed child + assertTrue(FileUtils.isPathAChildOfParent(parent, parent.resolve(buildPath(".", "child")))); + + // Allows nested child + assertTrue( + FileUtils.isPathAChildOfParent(parent, parent.resolve(buildPath("nested", "child")))); + + // Allows backtracking, provided it stays "under" parent + assertTrue( + FileUtils.isPathAChildOfParent( + parent, parent.resolve(buildPath("child1", "..", "child2")))); + assertTrue( + FileUtils.isPathAChildOfParent( + parent, parent.resolve(buildPath("child", "grandchild1", "..", "grandchild2")))); + + // Prevents identical path + assertFalse(FileUtils.isPathAChildOfParent(parent, parent)); + + // Detects sibling of parent + assertFalse(FileUtils.isPathAChildOfParent(parent, parent.resolve(buildPath("..", "sibling")))); + + // Detects "grandparent" of parent + assertFalse(FileUtils.isPathAChildOfParent(parent, parent.resolve(".."))); + + // Detects many-layered backtracking + assertFalse( + FileUtils.isPathAChildOfParent(parent, parent.resolve(buildPath("..", "..", "..", "..")))); + } + + private static String buildPath(String... pathSegments) { + final var sb = new StringBuilder(); + for (int i = 0; i < pathSegments.length; i++) { + sb.append(pathSegments[i]); + if (i < pathSegments.length - 1) { + sb.append(File.separator); + } + } + return sb.toString(); + } }
