SOLR-17543: Input validation in FSConfigSetService

See JIRA ticket for more details.

solr/CHANGES.txt

@@ -13,6 +13,8 @@ Bug Fixes
 
 * SOLR-17530: Metrics: The new Prometheus response writer wasn't detecting TLOG or PULL replicas properly. (Matthew Biscocho)
 
+* SOLR-17543: Adds input validation to zip entries used by FileSystemConfigSetService (Jason Gerlowski)
+
 ==================  9.7.0 ==================
 New Features
 ---------------------

solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java

@@ -38,6 +38,7 @@
 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 void uploadFileToConfig(
       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);
     }
   }
 

solr/core/src/java/org/apache/solr/util/FileUtils.java

@@ -103,4 +103,28 @@ public static Path createDirectories(Path path) throws IOException {
     }
     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);
+  }
 }

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 static void afterClass() throws Exception {
     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 void testUploadAndDeleteConfig() throws IOException {
     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());

solr/core/src/test/org/apache/solr/util/FileUtilsTest.java

@@ -17,15 +17,65 @@
 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"));
     assertEquals(
         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();
+  }
 }