[lucene-solr] branch branch_8x updated: SOLR-13993: sandbox velocity template render (if security manager is enabled)

[rm...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rmuir pushed a commit to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git


The following commit(s) were added to refs/heads/branch_8x by this push:
     new e6728cd  SOLR-13993: sandbox velocity template render (if security manager is enabled)
e6728cd is described below

commit e6728cdf64472e91186b3cc080d2d1c5de774196
Author: Robert Muir <rm...@apache.org>
AuthorDate: Thu Dec 5 01:06:38 2019 -0500

    SOLR-13993: sandbox velocity template render (if security manager is enabled)
    
    The solr permissions are weak sauce due to the huge number of features, third-party dependencies, etc.
    
    Hence they have access to do many things. For "scripting" such as velocity we have to look at a more aggressive stance:
    
    Step 1: Can we wrap a sandbox around the whole goddamn thing and call it a day?
    Step 2: Let's separate the "engine" from "untrusted code" and only be an asshole to the latter.
    Step 3: Java's security is shit, Lets contain that classloader and whitelist access.
---
 lucene/tools/junit4/solr-tests.policy              |  2 ++
 .../solr/response/VelocityResponseWriter.java      | 41 +++++++++++++++++++++
 .../collection1/conf/velocity/outside_the_box.vm   |  4 +++
 .../conf/velocity/sandbox_intersection.vm          |  5 +++
 .../solr/velocity/VelocityResponseWriterTest.java  | 42 ++++++++++++++++++++++
 5 files changed, 94 insertions(+)

diff --git a/lucene/tools/junit4/solr-tests.policy b/lucene/tools/junit4/solr-tests.policy
index c3cd0e8..4d60f13 100644
--- a/lucene/tools/junit4/solr-tests.policy
+++ b/lucene/tools/junit4/solr-tests.policy
@@ -166,4 +166,6 @@ grant {
   // java 8 accessibility requires this perm - should not after 8 I believe (rrd4j is the root reason we hit an accessibility code path)
   permission java.awt.AWTPermission "*";
 
+  // used by solr to create sandboxes (e.g. script execution)
+  permission java.security.SecurityPermission "createAccessControlContext";
 };
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 b3f022a..39b0a69 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
@@ -17,6 +17,7 @@
 package org.apache.solr.response;
 
 import java.io.File;
+import java.io.FilePermission;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
@@ -24,11 +25,18 @@ import java.io.StringWriter;
 import java.io.Writer;
 import java.lang.invoke.MethodHandles;
 import java.nio.charset.StandardCharsets;
+import java.security.AccessControlContext;
+import java.security.AccessController;
+import java.security.Permissions;
+import java.security.PrivilegedActionException;
+import java.security.PrivilegedExceptionAction;
+import java.security.ProtectionDomain;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Properties;
+import java.util.PropertyPermission;
 import java.util.ResourceBundle;
 
 import org.apache.solr.client.solrj.SolrResponse;
@@ -147,6 +155,39 @@ public class VelocityResponseWriter implements QueryResponseWriter, SolrCoreAwar
 
   @Override
   public void write(Writer writer, SolrQueryRequest request, SolrQueryResponse response) throws IOException {
+    // run doWrite() with the velocity sandbox
+    try {
+      AccessController.doPrivileged(new PrivilegedExceptionAction<Void>() {
+        @Override
+        public Void run() throws IOException {
+          doWrite(writer, request, response);
+          return null;
+        }
+      }, VELOCITY_SANDBOX);
+    } catch (PrivilegedActionException e) {
+      throw (IOException) e.getException();
+    }
+  }
+
+  // sandbox for velocity code
+  // TODO: we could read in a policy file instead, in case someone needs to tweak it?
+  private static final AccessControlContext VELOCITY_SANDBOX;
+  static {
+    Permissions permissions = new Permissions();
+    // TODO: restrict the scope of this! we probably only need access to classpath
+    permissions.add(new FilePermission("<<ALL FILES>>", "read,readlink"));
+    // properties needed by SolrResourceLoader (called from velocity code)
+    permissions.add(new PropertyPermission("jetty.testMode", "read"));
+    permissions.add(new PropertyPermission("solr.allow.unsafe.resourceloading", "read"));
+    // properties needed by log4j (called from velocity code)
+    permissions.add(new PropertyPermission("java.version", "read"));
+    // needed by velocity duck-typing
+    permissions.add(new RuntimePermission("accessDeclaredMembers"));
+    permissions.setReadOnly();
+    VELOCITY_SANDBOX = new AccessControlContext(new ProtectionDomain[] { new ProtectionDomain(null, permissions) });
+  }
+
+  private void doWrite(Writer writer, SolrQueryRequest request, SolrQueryResponse response) throws IOException {
     VelocityEngine engine = createEngine(request);  // TODO: have HTTP headers available for configuring engine
 
     Template template = getTemplate(engine, request);
diff --git a/solr/contrib/velocity/src/test-files/velocity/solr/collection1/conf/velocity/outside_the_box.vm b/solr/contrib/velocity/src/test-files/velocity/solr/collection1/conf/velocity/outside_the_box.vm
new file mode 100644
index 0000000..2842823
--- /dev/null
+++ b/solr/contrib/velocity/src/test-files/velocity/solr/collection1/conf/velocity/outside_the_box.vm
@@ -0,0 +1,4 @@
+#set($x='')
+#set($sys=$x.class.forName('java.lang.System'))
+#set($ex=$sys.getProperty('os.name'))
+$ex
diff --git a/solr/contrib/velocity/src/test-files/velocity/solr/collection1/conf/velocity/sandbox_intersection.vm b/solr/contrib/velocity/src/test-files/velocity/solr/collection1/conf/velocity/sandbox_intersection.vm
new file mode 100644
index 0000000..75d97a18
--- /dev/null
+++ b/solr/contrib/velocity/src/test-files/velocity/solr/collection1/conf/velocity/sandbox_intersection.vm
@@ -0,0 +1,5 @@
+#set($x='')
+#set($sys=$x.class.forName('java.nio.file.Paths'))
+#set($path=$sys.get('/dumbass/denied_location'))
+#set($ex=$path.resolve($path).toRealPath())
+$ex
diff --git a/solr/contrib/velocity/src/test/org/apache/solr/velocity/VelocityResponseWriterTest.java b/solr/contrib/velocity/src/test/org/apache/solr/velocity/VelocityResponseWriterTest.java
index 33f1b02..c139379 100644
--- a/solr/contrib/velocity/src/test/org/apache/solr/velocity/VelocityResponseWriterTest.java
+++ b/solr/contrib/velocity/src/test/org/apache/solr/velocity/VelocityResponseWriterTest.java
@@ -18,6 +18,7 @@ package org.apache.solr.velocity;
 
 import java.io.IOException;
 import java.io.StringWriter;
+import java.security.AccessControlException;
 
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.common.SolrException;
@@ -27,6 +28,7 @@ import org.apache.solr.response.QueryResponseWriter;
 import org.apache.solr.response.SolrParamResourceLoader;
 import org.apache.solr.response.SolrQueryResponse;
 import org.apache.solr.response.VelocityResponseWriter;
+import org.apache.velocity.exception.MethodInvocationException;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
 import org.junit.Test;
@@ -53,6 +55,46 @@ public class VelocityResponseWriterTest extends SolrTestCaseJ4 {
   }
 
   @Test
+  public void testTemplateSandbox() throws Exception {
+    assumeTrue("This test only works with security manager", System.getSecurityManager() != null);
+    VelocityResponseWriter vrw = new VelocityResponseWriter();
+    NamedList<String> nl = new NamedList<>();
+    nl.add("template.base.dir", getFile("velocity").getAbsolutePath());
+    vrw.init(nl);
+    SolrQueryRequest req = req(VelocityResponseWriter.TEMPLATE,"outside_the_box");
+    SolrQueryResponse rsp = new SolrQueryResponse();
+    StringWriter buf = new StringWriter();
+    try {
+      vrw.write(buf, req, rsp);
+      fail("template broke outside the box, retrieved OS: " + buf);
+    } catch (MethodInvocationException e) {
+      assertNotNull(e.getCause());
+      assertEquals(AccessControlException.class, e.getCause().getClass());
+      // expected failure, can't get outside the box
+    }
+  }
+
+  @Test
+  public void testSandboxIntersection() throws Exception {
+    assumeTrue("This test only works with security manager", System.getSecurityManager() != null);
+    VelocityResponseWriter vrw = new VelocityResponseWriter();
+    NamedList<String> nl = new NamedList<>();
+    nl.add("template.base.dir", getFile("velocity").getAbsolutePath());
+    vrw.init(nl);
+    SolrQueryRequest req = req(VelocityResponseWriter.TEMPLATE,"sandbox_intersection");
+    SolrQueryResponse rsp = new SolrQueryResponse();
+    StringWriter buf = new StringWriter();
+    try {
+      vrw.write(buf, req, rsp);
+      fail("template broke outside the box, retrieved OS: " + buf);
+    } catch (MethodInvocationException e) {
+      assertNotNull(e.getCause());
+      assertEquals(AccessControlException.class, e.getCause().getClass());
+      // expected failure, can't get outside the box
+    }
+  }
+
+  @Test
   public void testCustomParamTemplate() throws Exception {
     org.apache.solr.response.VelocityResponseWriter vrw = new VelocityResponseWriter();
     NamedList<String> nl = new NamedList<>();