SOLR-14561 CoreAdminAPI's parameters instanceDir and dataDir are now validated

solr/CHANGES.txt

@@ -147,6 +147,10 @@ Improvements
   Also fixes SolrIndexSearcher.warm which should have re-instated previous SRI.
   (Nazerke Seidan, David Smiley)
 
+* SOLR-14561: CoreAdminAPI's parameters instanceDir and dataDir are now validated, and must be relative to either
+  SOLR_HOME, SOLR_DATA_HOME or coreRootDir. Added new solr.xml config 'allowPaths', controlled by system property
+  'solr.allowPaths' that allows you to add other allowed paths when needed.
+
 Optimizations
 ---------------------
 * SOLR-8306: Do not collect expand documents when expand.rows=0 (Marshall Sanders, Amelia Henderson)

solr/bin/solr.in.cmd

@@ -209,3 +209,7 @@ REM change the value to true. The option is configured as a system property as d
 REM scripts.
 REM set SOLR_ADMIN_UI_DISABLED=false
 
+REM Solr is by default allowed to read and write data from/to SOLR_HOME and a few other well defined locations
+REM Sometimes it may be necessary to place a core or a backup on a different location or a different disk
+REM This parameter lets you specify file system path(s) to explicitly allow. The special value of '*' will allow any path
+REM SOLR_OPTS="%SOLR_OPTS% -Dsolr.allowPaths=D:\,E:\other\path"

solr/bin/solr.in.sh

@@ -239,3 +239,8 @@
 # change the value to true. The option is configured as a system property as defined in SOLR_START_OPTS in the start
 # scripts.
 # SOLR_ADMIN_UI_DISABLED=false
+
+# Solr is by default allowed to read and write data from/to SOLR_HOME and a few other well defined locations
+# Sometimes it may be necessary to place a core or a backup on a different location or a different disk
+# This parameter lets you specify file system path(s) to explicitly allow. The special value of '*' will allow any path
+#SOLR_OPTS="$SOLR_OPTS -Dsolr.allowPaths=/mnt/bigdisk,/other/path"

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

@@ -42,6 +42,7 @@
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import org.apache.commons.lang3.StringUtils;
@@ -238,6 +239,7 @@ public CoreLoadFailure(CoreDescriptor cd, Exception loadFailure) {
   private PackageStoreAPI packageStoreAPI;
   private PackageLoader packageLoader;
 
+  private Set<Path> allowPaths;
 
   // Bits for the state variable.
   public final static long LOAD_COMPLETE = 0x1L;
@@ -338,6 +340,19 @@ public CoreContainer(NodeConfig config, CoresLocator locator, boolean asyncSolrC
         ExecutorUtil.newMDCAwareCachedThreadPool(
             cfg.getReplayUpdatesThreads(),
             new SolrNamedThreadFactory("replayUpdatesExecutor")));
+
+    this.allowPaths = new java.util.HashSet<>();
+    this.allowPaths.add(cfg.getSolrHome());
+    this.allowPaths.add(cfg.getCoreRootDirectory());
+    if (cfg.getSolrDataHome() != null) {
+      this.allowPaths.add(cfg.getSolrDataHome());
+    }
+    if (!cfg.getAllowPaths().isEmpty()) {
+      this.allowPaths.addAll(cfg.getAllowPaths());
+      if (log.isInfoEnabled()) {
+        log.info("Allowing use of paths: {}", cfg.getAllowPaths());
+      }
+    }
   }
 
   @SuppressWarnings({"unchecked"})
@@ -1200,6 +1215,10 @@ public SolrCore create(String coreName, Path instancePath, Map<String, String> p
       throw new SolrException(ErrorCode.SERVER_ERROR, "Core with name '" + coreName + "' already exists.");
     }
 
+    // Validate paths are relative to known locations to avoid path traversal
+    assertPathAllowed(cd.getInstanceDir());
+    assertPathAllowed(Paths.get(cd.getDataDir()));
+
     boolean preExisitingZkEntry = false;
     try {
       if (getZkController() != null) {
@@ -1259,6 +1278,29 @@ public SolrCore create(String coreName, Path instancePath, Map<String, String> p
     }
   }
 
+  /**
+   * Checks that the given path is relative to SOLR_HOME, SOLR_DATA_HOME, coreRootDirectory or one of the paths
+   * specified in solr.xml's allowPaths element. Delegates to {@link SolrPaths#assertPathAllowed(Path, Set)}
+   * @param pathToAssert path to check
+   * @throws SolrException if path is outside allowed paths
+   */
+  public void assertPathAllowed(Path pathToAssert) throws SolrException {
+    SolrPaths.assertPathAllowed(pathToAssert, allowPaths);
+  }
+
+  /**
+   * <p>Return the file system paths that should be allowed for various API requests.
+   * This list is compiled at startup from SOLR_HOME, SOLR_DATA_HOME and the
+   * <code>allowPaths</code> configuration of solr.xml.
+   * These paths are used by the {@link #assertPathAllowed(Path)} method call.</p>
+   * <p><b>NOTE:</b></p> This method is currently only in use in tests in order to
+   * modify the mutable Set directly. Please treat this as a private method.
+   */
+  @VisibleForTesting
+  public Set<Path> getAllowPaths() {
+    return allowPaths;
+  }
+
   /**
    * Creates a new core based on a CoreDescriptor.
    *
@@ -1502,7 +1544,6 @@ public Map<String, CoreLoadFailure> getCoreInitFailures() {
     return ImmutableMap.copyOf(coreInitFailures);
   }
 
-
   // ---------------- Core name related methods ---------------
 
   private CoreDescriptor reloadCoreDescriptor(CoreDescriptor oldDesc) {

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

@@ -18,6 +18,7 @@
 
 import java.nio.file.Path;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Properties;
 import java.util.Set;
@@ -41,6 +42,8 @@ public class NodeConfig {
 
   private final Path configSetBaseDirectory;
 
+  private final Set<Path> allowPaths;
+
   private final String sharedLibDirectory;
 
   private final PluginInfo shardHandlerFactoryConfig;
@@ -95,7 +98,7 @@ private NodeConfig(String nodeName, Path coreRootDirectory, Path solrDataHome, I
                      Path solrHome, SolrResourceLoader loader,
                      Properties solrProperties, PluginInfo[] backupRepositoryPlugins,
                      MetricsConfig metricsConfig, PluginInfo transientCacheConfig, PluginInfo tracerConfig,
-                     boolean fromZookeeper) {
+                     boolean fromZookeeper, Set<Path> allowPaths) {
     // all Path params here are absolute and normalized.
     this.nodeName = nodeName;
     this.coreRootDirectory = coreRootDirectory;
@@ -125,6 +128,7 @@ private NodeConfig(String nodeName, Path coreRootDirectory, Path solrDataHome, I
     this.transientCacheConfig = transientCacheConfig;
     this.tracerConfig = tracerConfig;
     this.fromZookeeper = fromZookeeper;
+    this.allowPaths = allowPaths;
 
     if (this.cloudConfig != null && this.getCoreLoadThreadCount(false) < 2) {
       throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
@@ -263,6 +267,12 @@ public boolean isFromZookeeper() {
     return fromZookeeper;
   }
 
+  /**
+   * Extra file paths that will be allowed for core creation, in addition to
+   * SOLR_HOME, SOLR_DATA_HOME and coreRootDir
+   */
+  public Set<Path> getAllowPaths() { return allowPaths; }
+
   public static class NodeConfigBuilder {
     // all Path fields here are absolute and normalized.
     private SolrResourceLoader loader;
@@ -293,6 +303,7 @@ public static class NodeConfigBuilder {
     private PluginInfo transientCacheConfig;
     private PluginInfo tracerConfig;
     private boolean fromZookeeper = false;
+    private Set<Path> allowPaths = Collections.emptySet();
 
     private final Path solrHome;
     private final String nodeName;
@@ -457,6 +468,11 @@ public NodeConfigBuilder setFromZookeeper(boolean fromZookeeper) {
       return this;
     }
 
+    public NodeConfigBuilder setAllowPaths(Set<Path> paths) {
+      this.allowPaths = paths;
+      return this;
+    }
+
     public NodeConfig build() {
       // if some things weren't set then set them now.  Simple primitives are set on the field declaration
       if (loader == null) {
@@ -467,7 +483,7 @@ public NodeConfig build() {
                             updateShardHandlerConfig, coreAdminHandlerClass, collectionsAdminHandlerClass, healthCheckHandlerClass, infoHandlerClass, configSetsHandlerClass,
                             logWatcherConfig, cloudConfig, coreLoadThreads, replayUpdatesThreads, transientCacheSize, useSchemaCache, managementPath,
                             solrHome, loader, solrProperties,
-                            backupRepositoryPlugins, metricsConfig, transientCacheConfig, tracerConfig, fromZookeeper);
+                            backupRepositoryPlugins, metricsConfig, transientCacheConfig, tracerConfig, fromZookeeper, allowPaths);
     }
 
     public NodeConfigBuilder setSolrResourceLoader(SolrResourceLoader resourceLoader) {

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

@@ -28,6 +28,8 @@
 import java.util.Set;
 import java.util.concurrent.ConcurrentSkipListSet;
 
+import org.apache.commons.exec.OS;
+import org.apache.solr.common.SolrException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -128,4 +130,37 @@ private static void logOnceInfo(String key, String msg) {
       log.info(msg);
     }
   }
+
+  /**
+   * Checks that the given path is relative to one of the allowPaths supplied. Typically this will be
+   * called from {@link CoreContainer#assertPathAllowed(Path)} and allowPaths pre-filled with the node's
+   * SOLR_HOME, SOLR_DATA_HOME and coreRootDirectory folders, as well as any paths specified in
+   * solr.xml's allowPaths element. The following paths will always fail validation:
+   * <ul>
+   *   <li>Relative paths starting with <code>..</code></li>
+   *   <li>Windows UNC paths (such as <code>\\host\share\path</code>)</li>
+   *   <li>Paths which are not relative to any of allowPaths</li>
+   * </ul>
+   * @param pathToAssert path to check
+   * @param allowPaths list of paths that should be allowed prefixes for pathToAssert
+   * @throws SolrException if path is outside allowed paths
+   */
+  public static void assertPathAllowed(Path pathToAssert, Set<Path> allowPaths) throws SolrException {
+    if (OS.isFamilyWindows() && pathToAssert.toString().startsWith("\\\\")) {
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+          "Path " + pathToAssert + " disallowed. UNC paths not supported. Please use drive letter instead.");
+    }
+    // Conversion Path -> String -> Path is to be able to compare against org.apache.lucene.mockfile.FilterPath instances
+    final Path path = Path.of(pathToAssert.toString()).normalize();
+    if (path.startsWith("..")) {
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+          "Path " + pathToAssert + " disallowed due to path traversal..");
+    }
+    if (!path.isAbsolute()) return; // All relative paths are accepted
+    if (allowPaths.contains(Paths.get("_ALL_"))) return; // Catch-all path "*"/"_ALL_" will allow all other paths
+    if (allowPaths.stream().noneMatch(p -> path.startsWith(Path.of(p.toString())))) {
+      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+          "Path " + path + " must be relative to SOLR_HOME, SOLR_DATA_HOME coreRootDirectory. Set system property 'solr.allowPaths' to add other allowed paths.");
+    }
+  }
 }

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

@@ -26,13 +26,17 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import com.google.common.base.Strings;
 import org.apache.commons.io.IOUtils;
@@ -277,6 +281,9 @@ private static NodeConfig fillSolrSection(NodeConfig.NodeConfigBuilder builder,
         case "sharedLib":
           builder.setSharedLibDirectory(value);
           break;
+        case "allowPaths":
+          builder.setAllowPaths(stringToPaths(value));
+          break;
         case "configSetBaseDir":
           builder.setConfigSetBaseDirectory(value);
           break;
@@ -300,6 +307,15 @@ private static NodeConfig fillSolrSection(NodeConfig.NodeConfigBuilder builder,
     return builder.build();
   }
 
+  private static Set<Path> stringToPaths(String commaSeparatedString) {
+    if (Strings.isNullOrEmpty(commaSeparatedString)) {
+      return Collections.emptySet();
+    }
+    // Parse list of paths. The special value '*' is mapped to _ALL_ to mean all paths
+    return Arrays.stream(commaSeparatedString.split(",\\s?"))
+        .map(p -> Paths.get("*".equals(p) ? "_ALL_" : p)).collect(Collectors.toSet());
+  }
+
   private static UpdateShardHandlerConfig loadUpdateConfig(NamedList<Object> nl, boolean alwaysDefine) {
 
     if (nl == null && !alwaysDefine)

solr/core/src/test-files/solr/solr-50-all.xml

@@ -24,6 +24,7 @@
   <str name="configSetsHandler">testConfigSetsHandler</str>
   <str name="managementPath">testManagementPath</str>
   <str name="sharedLib">testSharedLib</str>
+  <str name="allowPaths">${solr.allowPaths:}</str>
   <str name="shareSchema">${shareSchema:true}</str>
   <int name="transientCacheSize">66</int>
   <int name="replayUpdatesThreads">100</int>

solr/core/src/test-files/solr/solr-solrreporter.xml

@@ -17,6 +17,8 @@
 -->
 
 <solr>
+  <str name="allowPaths">${solr.allowPaths:}</str>
+
   <shardHandlerFactory name="shardHandlerFactory" class="HttpShardHandlerFactory">
     <str name="urlScheme">${urlScheme:}</str>
     <int name="socketTimeout">${socketTimeout:90000}</int>

solr/core/src/test-files/solr/solr.xml

@@ -24,6 +24,7 @@
   <str name="shareSchema">${shareSchema:false}</str>
   <str name="configSetBaseDir">${configSetBaseDir:configsets}</str>
   <str name="coreRootDirectory">${coreRootDirectory:.}</str>
+  <str name="allowPaths">${solr.allowPaths:}</str>
 
   <shardHandlerFactory name="shardHandlerFactory" class="HttpShardHandlerFactory">
     <str name="urlScheme">${urlScheme:}</str>

solr/core/src/test/org/apache/solr/cloud/CollectionsAPISolrJTest.java

@@ -38,6 +38,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 
+import com.google.common.collect.ImmutableList;
 import org.apache.lucene.util.LuceneTestCase;
 import org.apache.lucene.util.TestUtil;
 import org.apache.solr.client.solrj.SolrClient;
@@ -75,8 +76,6 @@
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
-
-import com.google.common.collect.ImmutableList;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -475,6 +474,7 @@ public void testCreateCollectionWithPropertyParam() throws Exception {
     Path tmpDir = createTempDir("testPropertyParamsForCreate");
     Path dataDir = tmpDir.resolve("dataDir-" + TestUtil.randomSimpleString(random(), 1, 5));
     Path ulogDir = tmpDir.resolve("ulogDir-" + TestUtil.randomSimpleString(random(), 1, 5));
+    cluster.getJettySolrRunners().forEach(j -> j.getCoreContainer().getAllowPaths().add(tmpDir));
 
     CollectionAdminResponse response = CollectionAdminRequest.createCollection(collectionName, "conf", 1, 1)
         .withProperty(CoreAdminParams.DATA_DIR, dataDir.toString())

solr/core/src/test/org/apache/solr/cloud/UnloadDistributedZkTest.java

@@ -40,6 +40,7 @@
 import org.junit.Test;
 
 import java.io.IOException;
+import java.nio.file.Path;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Random;
@@ -64,6 +65,7 @@ protected String getSolrXml() {
 
   @Test
   public void test() throws Exception {
+    jettys.forEach(j -> j.getCoreContainer().getAllowPaths().add(Path.of("_ALL_"))); // Allow non-standard core instance path
     testCoreUnloadAndLeaders(); // long
     testUnloadLotsOfCores(); // long
 

solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionsAPIDistributedZkTest.java

@@ -100,6 +100,7 @@ public void setupCluster() throws Exception {
     System.setProperty("createCollectionWaitTimeTillActive", "5");
     TestInjection.randomDelayInCoreCreation = "true:5";
     System.setProperty("validateAfterInactivity", "200");
+    System.setProperty("solr.allowPaths", "*");
 
     configureCluster(4)
         .addConfig("conf", configset(getConfigSet()))
@@ -114,6 +115,7 @@ public void tearDownCluster() throws Exception {
       shutdownCluster();
     } finally {
       System.clearProperty("createCollectionWaitTimeTillActive");
+      System.clearProperty("solr.allowPaths");
       super.tearDown();
     }
   }