SOLR-4882: Restrict SolrResourceLoader to only allow access to resource files below the instance dir

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1525246 13f79535-47bb-0310-9956-ffa450edef68

solr/CHANGES.txt

@@ -71,6 +71,17 @@ New Features
 * SOLR-5167: Add support for AnalyzingInfixSuggester (AnalyzingInfixLookupFactory).
   (Areek Zillur, Varun Thacker via Robert Muir)
 
+Security
+----------------------
+
+* SOLR-4882: SolrResourceLoader was restricted to only allow access to resource
+  files below the instance dir. The reason for this is security related: Some
+  Solr components allow to pass in resource paths via REST parameters
+  (e.g. XSL stylesheets, velocity templates,...) and load them via resource
+  loader. For backwards compatibility, this security feature can be disabled
+  by a new system property: solr.allow.unsafe.resourceloading=true
+  (Uwe Schindler)
+
 Other Changes
 ----------------------
 

solr/contrib/velocity/src/java/org/apache/solr/response/VelocityResponseWriter.java

@@ -67,7 +67,6 @@ public class VelocityResponseWriter implements QueryResponseWriter {
     } catch (ClassCastException e) {
       // known edge case where QueryResponse's extraction assumes "response" is a SolrDocumentList
       // (AnalysisRequestHandler emits a "response")
-      e.printStackTrace();
       rsp = new SolrResponseBase();
       rsp.setResponse(parsedResponse);
     }
@@ -121,25 +120,7 @@ public class VelocityResponseWriter implements QueryResponseWriter {
     SolrVelocityResourceLoader resourceLoader =
         new SolrVelocityResourceLoader(request.getCore().getSolrConfig().getResourceLoader());
     engine.setProperty("solr.resource.loader.instance", resourceLoader);
-
+    engine.setProperty(RuntimeConstants.RESOURCE_LOADER, "params,solr");
-    File fileResourceLoaderBaseDir = null;
-    try {
-      String template_root = request.getParams().get("v.base_dir");
-      fileResourceLoaderBaseDir = new File(request.getCore().getResourceLoader().getConfigDir(), "velocity");
-      if (template_root != null) {
-        fileResourceLoaderBaseDir = new File(template_root);
-      }
-    } catch (SolrException e) {
-      // no worries... probably in ZooKeeper mode and getConfigDir() isn't available, so we'll just ignore omit
-      // the file system resource loader
-    }
-
-    if (fileResourceLoaderBaseDir != null) {
-      engine.setProperty(RuntimeConstants.FILE_RESOURCE_LOADER_PATH, fileResourceLoaderBaseDir.getAbsolutePath());
-      engine.setProperty(RuntimeConstants.RESOURCE_LOADER, "params,file,solr");
-    } else {
-      engine.setProperty(RuntimeConstants.RESOURCE_LOADER, "params,solr");
-    }
 
     // TODO: Externalize Velocity properties
     String propFile = request.getParams().get("v.properties");

solr/core/src/java/org/apache/solr/cloud/ZkSolrResourceLoader.java

@@ -18,6 +18,7 @@ package org.apache.solr.cloud;
  */
 
 import java.io.ByteArrayInputStream;
+import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.List;
@@ -75,7 +76,7 @@ public class ZkSolrResourceLoader extends SolrResourceLoader {
     String file = collectionZkPath + "/" + resource;
     try {
       if (zkController.pathExists(file)) {
-        byte[] bytes = zkController.getZkClient().getData(collectionZkPath + "/" + resource, null, null, true);
+        byte[] bytes = zkController.getZkClient().getData(file, null, null, true);
         return new ByteArrayInputStream(bytes);
       }
     } catch (Exception e) {
@@ -83,7 +84,7 @@ public class ZkSolrResourceLoader extends SolrResourceLoader {
     }
     try {
       // delegate to the class loader (looking into $INSTANCE_DIR/lib jars)
-      is = classLoader.getResourceAsStream(resource);
+      is = classLoader.getResourceAsStream(resource.replace(File.separatorChar, '/'));
     } catch (Exception e) {
       throw new IOException("Error opening " + resource, e);
     }

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

@@ -55,6 +55,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.lang.reflect.Constructor;
 import java.net.MalformedURLException;
+import java.net.URI;
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.nio.charset.CharacterCodingException;
@@ -250,7 +251,7 @@ public class SolrResourceLoader implements ResourceLoader,Closeable
   }
 
   public String getConfigDir() {
-    return instanceDir + "conf/";
+    return instanceDir + "conf" + File.separator;
   }
   
   public String getDataDir()    {
@@ -299,27 +300,46 @@ public class SolrResourceLoader implements ResourceLoader,Closeable
   public InputStream openResource(String resource) throws IOException {
     InputStream is=null;
     try {
-      File f0 = new File(resource);
+      File f0 = new File(resource), f = f0;
-      File f = f0;
       if (!f.isAbsolute()) {
         // try $CWD/$configDir/$resource
-        f = new File(getConfigDir() + resource);
+        f = new File(getConfigDir() + resource).getAbsoluteFile();
       }
-      if (f.isFile() && f.canRead()) {
+      boolean found = f.isFile() && f.canRead();
+      if (!found) { // no success with $CWD/$configDir/$resource
+        f = f0.getAbsoluteFile();
+        found = f.isFile() && f.canRead();
+      }
+      // check that we don't escape instance dir
+      if (found) {
+        if (!Boolean.parseBoolean(System.getProperty("solr.allow.unsafe.resourceloading", "false"))) {
+          final URI instanceURI = new File(getInstanceDir()).getAbsoluteFile().toURI().normalize();
+          final URI fileURI = f.toURI().normalize();
+          if (instanceURI.relativize(fileURI) == fileURI) {
+            // no URI relativize possible, so they don't share same base folder
+            throw new IOException("For security reasons, SolrResourceLoader cannot load files from outside the instance's directory: " + f +
+                "; if you want to override this safety feature and you are sure about the consequences, you can pass the system property "+
+                "-Dsolr.allow.unsafe.resourceloading=true to your JVM");
+          }
+        }
+        // relativize() returned a relative, new URI, so we are fine!
         return new FileInputStream(f);
-      } else if (f != f0) { // no success with $CWD/$configDir/$resource
-        if (f0.isFile() && f0.canRead())
-          return new FileInputStream(f0);
       }
-      // delegate to the class loader (looking into $INSTANCE_DIR/lib jars)
+      // Delegate to the class loader (looking into $INSTANCE_DIR/lib jars).
-      is = classLoader.getResourceAsStream(resource);
+      // We need a ClassLoader-compatible (forward-slashes) path here!
-      if (is == null)
+      is = classLoader.getResourceAsStream(resource.replace(File.separatorChar, '/'));
-        is = classLoader.getResourceAsStream(getConfigDir() + resource);
+      // This is a hack just for tests (it is not done in ZKResourceLoader)!
+      // -> the getConfigDir's path must not be absolute!
+      if (is == null && System.getProperty("jetty.testMode") != null && !new File(getConfigDir()).isAbsolute()) {
+        is = classLoader.getResourceAsStream((getConfigDir() + resource).replace(File.separatorChar, '/'));
+      }
+    } catch (IOException ioe) {
+      throw ioe;
     } catch (Exception e) {
       throw new IOException("Error opening " + resource, e);
     }
     if (is==null) {
-      throw new IOException("Can't find resource '" + resource + "' in classpath or '" + getConfigDir() + "', cwd="+System.getProperty("user.dir"));
+      throw new IOException("Can't find resource '" + resource + "' in classpath or '" + new File(getConfigDir()).getAbsolutePath() + "'");
     }
     return is;
   }

solr/core/src/test/org/apache/solr/core/ResourceLoaderTest.java

@@ -19,10 +19,10 @@ package org.apache.solr.core;
 
 import junit.framework.Assert;
 
-import org.apache.lucene.util.LuceneTestCase;
 import org.apache.lucene.analysis.core.KeywordTokenizerFactory;
 import org.apache.lucene.analysis.ngram.NGramFilterFactory;
 import org.apache.lucene.util._TestUtil;
+import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.handler.admin.LukeRequestHandler;
 import org.apache.solr.handler.component.FacetComponent;
@@ -33,6 +33,7 @@ import org.apache.solr.util.plugin.SolrCoreAware;
 import java.io.File;
 import java.io.FileFilter;
 import java.io.FileOutputStream;
+import java.io.IOException;
 import java.io.InputStream;
 import java.nio.charset.CharacterCodingException;
 import java.util.Arrays;
@@ -41,23 +42,46 @@ import java.util.List;
 import java.util.jar.JarEntry;
 import java.util.jar.JarOutputStream;
 
-public class ResourceLoaderTest extends LuceneTestCase 
+public class ResourceLoaderTest extends SolrTestCaseJ4 
 {
   public void testInstanceDir() throws Exception {
     SolrResourceLoader loader = new SolrResourceLoader(null);
     String instDir = loader.getInstanceDir();
     assertTrue(instDir + " is not equal to " + "solr/", instDir.equals("solr/") == true);
+    loader.close();
 
     loader = new SolrResourceLoader("solr");
     instDir = loader.getInstanceDir();
     assertTrue(instDir + " is not equal to " + "solr/", instDir.equals("solr" + File.separator) == true);
+    loader.close();
   }
 
-  public void testAwareCompatibility() 
+  public void testEscapeInstanceDir() throws Exception {
+    File temp = _TestUtil.getTempDir("testEscapeInstanceDir");
+    try {
+      temp.mkdirs();
+      new File(temp, "dummy.txt").createNewFile();
+      File instanceDir = new File(temp, "instance");
+      instanceDir.mkdir();
+      new File(instanceDir, "conf").mkdir();
+      SolrResourceLoader loader = new SolrResourceLoader(instanceDir.getAbsolutePath());
+      try {
+        loader.openResource("../../dummy.txt").close();
+        fail();
+      } catch (IOException ioe) {
+        assertTrue(ioe.getMessage().startsWith("For security reasons, SolrResourceLoader"));
+      }
+      loader.close();
+    } finally {
+      _TestUtil.rmDir(temp);
+    }
+  }
+
+  public void testAwareCompatibility() throws Exception
   {
     SolrResourceLoader loader = new SolrResourceLoader( "." );
     
-    Class clazz = ResourceLoaderAware.class;
+    Class<?> clazz = ResourceLoaderAware.class;
     // Check ResourceLoaderAware valid objects
     loader.assertAwareCompatibility( clazz, new NGramFilterFactory(new HashMap<String,String>()) );
     loader.assertAwareCompatibility( clazz, new KeywordTokenizerFactory(new HashMap<String,String>()) );
@@ -97,6 +121,8 @@ public class ResourceLoaderTest extends LuceneTestCase
       }
       catch( SolrException ex ) { } // OK
     }
+    
+    loader.close();
   }
   
   public void testBOMMarkers() throws Exception {
@@ -123,6 +149,8 @@ public class ResourceLoaderTest extends LuceneTestCase
     List<String> lines = loader.getLines(fileWithBom);
     assertEquals(1, lines.size());
     assertEquals("BOMsAreEvil", lines.get(0));
+    
+    loader.close();
   }
   
   public void testWrongEncoding() throws Exception {
@@ -130,11 +158,12 @@ public class ResourceLoaderTest extends LuceneTestCase
     SolrResourceLoader loader = new SolrResourceLoader("solr/collection1");
     // ensure we get our exception
     try {
-      List<String> lines = loader.getLines(wrongEncoding);
+      loader.getLines(wrongEncoding);
       fail();
     } catch (SolrException expected) {
       assertTrue(expected.getCause() instanceof CharacterCodingException);
     }
+    loader.close();
   }
 
   public void testClassLoaderLibs() throws Exception {

solr/core/src/test/org/apache/solr/schema/PrimitiveFieldTypeTest.java

@@ -43,11 +43,18 @@ public class PrimitiveFieldTypeTest extends SolrTestCaseJ4 {
     System.setProperty("enable.update.log", "false"); // schema12 doesn't support _version_
     System.setProperty("solr.test.sys.prop1", "propone");
     System.setProperty("solr.test.sys.prop2", "proptwo");
+    System.setProperty("solr.allow.unsafe.resourceloading", "true");
 
     initMap = new HashMap<String,String>();
     config = new SolrConfig(new SolrResourceLoader("solr/collection1"), testConfHome + "solrconfig.xml", null);
   }
   
+  @Override
+  public void tearDown() throws Exception {
+    System.clearProperty("solr.allow.unsafe.resourceloading");
+    super.tearDown();
+  }
+
   @SuppressWarnings("deprecation")
   @Test
   public void testDefaultOmitNorms() throws Exception {

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

@@ -28,6 +28,16 @@ import org.apache.commons.io.IOUtils;
 
 public class TestSystemIdResolver extends LuceneTestCase {
   
+  public void setUp() throws Exception {
+    super.setUp();
+    System.setProperty("solr.allow.unsafe.resourceloading", "true");
+  }
+
+  public void tearDown() throws Exception {
+    System.clearProperty("solr.allow.unsafe.resourceloading");
+    super.tearDown();
+  }
+
   private void assertEntityResolving(SystemIdResolver resolver, String expectedSystemId, String base, String systemId) throws Exception {
     final InputSource is = resolver.resolveEntity(null, null, base, systemId);
     try {