This is an automated email from the ASF dual-hosted git repository.
gerlowskija pushed a commit to branch branch_9x
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/branch_9x by this push:
new f1d81443e07 SOLR-17543: Input validation in FSConfigSetService
f1d81443e07 is described below
commit f1d81443e07e782ad753217d58b78ec3d67a9c95
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/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 ++++++++++++++++++++++
4 files changed, 134 insertions(+), 16 deletions(-)
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();
+ }
}
