From da34b18cb3092df4972e2b6fa5178d1059923910 Mon Sep 17 00:00:00 2001 From: Uwe Schindler Date: Sat, 21 Sep 2013 14:51:51 +0000 Subject: [PATCH] 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 | 11 +++++ .../solr/response/VelocityResponseWriter.java | 21 +-------- .../solr/cloud/ZkSolrResourceLoader.java | 5 +- .../apache/solr/core/SolrResourceLoader.java | 46 +++++++++++++------ .../apache/solr/core/ResourceLoaderTest.java | 39 ++++++++++++++-- .../solr/schema/PrimitiveFieldTypeTest.java | 7 +++ .../solr/util/TestSystemIdResolver.java | 10 ++++ 7 files changed, 99 insertions(+), 40 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 90fd5f01caa..abc943b2ea4 100644 --- a/solr/CHANGES.txt +++ b/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 ---------------------- diff --git a/solr/contrib/velocity/src/java/org/apache/solr/response/VelocityResponseWriter.java b/solr/contrib/velocity/src/java/org/apache/solr/response/VelocityResponseWriter.java index 1a9861de2be..413d708f079 100644 --- a/solr/contrib/velocity/src/java/org/apache/solr/response/VelocityResponseWriter.java +++ b/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); - - 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"); - } + engine.setProperty(RuntimeConstants.RESOURCE_LOADER, "params,solr"); // TODO: Externalize Velocity properties String propFile = request.getParams().get("v.properties"); diff --git a/solr/core/src/java/org/apache/solr/cloud/ZkSolrResourceLoader.java b/solr/core/src/java/org/apache/solr/cloud/ZkSolrResourceLoader.java index 8a52852279e..21081bdb7db 100644 --- a/solr/core/src/java/org/apache/solr/cloud/ZkSolrResourceLoader.java +++ b/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); } diff --git a/solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java b/solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java index a3dd5acbe1d..ca544750b8e 100644 --- a/solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java +++ b/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 f = f0; + File f0 = new File(resource), 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) - is = classLoader.getResourceAsStream(resource); - if (is == null) - is = classLoader.getResourceAsStream(getConfigDir() + resource); + // Delegate to the class loader (looking into $INSTANCE_DIR/lib jars). + // We need a ClassLoader-compatible (forward-slashes) path here! + is = classLoader.getResourceAsStream(resource.replace(File.separatorChar, '/')); + // 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; } diff --git a/solr/core/src/test/org/apache/solr/core/ResourceLoaderTest.java b/solr/core/src/test/org/apache/solr/core/ResourceLoaderTest.java index d5966b6bda1..34a5abdd9f6 100644 --- a/solr/core/src/test/org/apache/solr/core/ResourceLoaderTest.java +++ b/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()) ); loader.assertAwareCompatibility( clazz, new KeywordTokenizerFactory(new HashMap()) ); @@ -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 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 lines = loader.getLines(wrongEncoding); + loader.getLines(wrongEncoding); fail(); } catch (SolrException expected) { assertTrue(expected.getCause() instanceof CharacterCodingException); } + loader.close(); } public void testClassLoaderLibs() throws Exception { diff --git a/solr/core/src/test/org/apache/solr/schema/PrimitiveFieldTypeTest.java b/solr/core/src/test/org/apache/solr/schema/PrimitiveFieldTypeTest.java index be283608319..4649103abed 100644 --- a/solr/core/src/test/org/apache/solr/schema/PrimitiveFieldTypeTest.java +++ b/solr/core/src/test/org/apache/solr/schema/PrimitiveFieldTypeTest.java @@ -43,10 +43,17 @@ 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(); 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 diff --git a/solr/core/src/test/org/apache/solr/util/TestSystemIdResolver.java b/solr/core/src/test/org/apache/solr/util/TestSystemIdResolver.java index 98cce4edf23..10a7ffb06a8 100644 --- a/solr/core/src/test/org/apache/solr/util/TestSystemIdResolver.java +++ b/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 {