Merge pull request #13844 from rmuir/lock_down_classloader_and_reflection

Clean up scripting permissions.

core/src/main/java/org/elasticsearch/bootstrap/Security.java

@@ -166,6 +166,8 @@ final class Security {
         m.put("repository-s3",       "org.elasticsearch.plugin.repository.s3.S3RepositoryPlugin");
         m.put("discovery-ec2",       "org.elasticsearch.plugin.discovery.ec2.Ec2DiscoveryPlugin");
         m.put("cloud-gce",           "org.elasticsearch.plugin.cloud.gce.CloudGcePlugin");
+        m.put("lang-expression",     "org.elasticsearch.script.expression.ExpressionPlugin");
+        m.put("lang-groovy",         "org.elasticsearch.script.groovy.GroovyPlugin");
         m.put("lang-javascript",     "org.elasticsearch.plugin.javascript.JavaScriptPlugin");
         m.put("lang-python",         "org.elasticsearch.plugin.python.PythonPlugin");
         SPECIAL_PLUGINS = Collections.unmodifiableMap(m);

core/src/main/resources/org/elasticsearch/bootstrap/security.policy

@@ -57,6 +57,20 @@ grant codeBase "${es.security.plugin.cloud-gce}" {
   permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
 };
 
+grant codeBase "${es.security.plugin.lang-expression}" {
+  // needed to generate runtime classes
+  permission java.lang.RuntimePermission "createClassLoader";
+};
+
+grant codeBase "${es.security.plugin.lang-groovy}" {
+  // needed to generate runtime classes
+  permission java.lang.RuntimePermission "createClassLoader";
+  // needed by groovy engine
+  permission java.lang.RuntimePermission "accessClassInPackage.sun.reflect";
+  // needed by GroovyScriptEngineService to close its classloader (why?)
+  permission java.lang.RuntimePermission "closeClassLoader";
+};
+
 grant codeBase "${es.security.plugin.lang-javascript}" {
   // needed to generate runtime classes
   permission java.lang.RuntimePermission "createClassLoader";
@@ -106,6 +120,8 @@ grant codeBase "${es.security.jar.randomizedtesting.junit4}" {
 grant {
 
   // Allow executing groovy scripts with codesource of /groovy/script
+  // TODO: make our own general ScriptServicePermission we check instead and 
+  // check-before-createClassLoader for all scripting engines.
   permission groovy.security.GroovyCodeSourcePermission "/groovy/script";
 
   // Allow connecting to the internet anywhere
@@ -114,15 +130,9 @@ grant {
   // Allow read/write to all system properties
   permission java.util.PropertyPermission "*", "read,write";
 
-  // needed by scripting engines, etc
-  permission java.lang.RuntimePermission "createClassLoader";
-
   // needed by lucene SPI currently 
   permission java.lang.RuntimePermission "getClassLoader";
 
-  // needed by GroovyScriptEngineService
-  permission java.lang.RuntimePermission "closeClassLoader";
-
   // needed by Settings
   permission java.lang.RuntimePermission "getenv.*";
 
@@ -130,12 +140,10 @@ grant {
   // otherwise can be provided only to test libraries
   permission java.lang.RuntimePermission "modifyThread";
 
-  // needed by groovy scripting
+  // needed by ExceptionSerializationTests and RestTestCase for
+  // some hackish things they do. otherwise only needed by groovy 
+  // (TODO: clean this up?)
   permission java.lang.RuntimePermission "getProtectionDomain";
-
-  // reflection hacks:
-  // needed by groovy engine
-  permission java.lang.RuntimePermission "accessClassInPackage.sun.reflect";
   
   // likely not low hanging fruit...
   permission java.lang.RuntimePermission "accessDeclaredMembers";

plugins/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngineService.java

@@ -43,6 +43,8 @@ import org.elasticsearch.script.SearchScript;
 import org.elasticsearch.search.MultiValueMode;
 import org.elasticsearch.search.lookup.SearchLookup;
 
+import java.security.AccessController;
+import java.security.PrivilegedAction;
 import java.text.ParseException;
 import java.util.Calendar;
 import java.util.Map;
@@ -91,12 +93,18 @@ public class ExpressionScriptEngineService extends AbstractComponent implements
 
     @Override
     public Object compile(String script) {
-        try {
-            // NOTE: validation is delayed to allow runtime vars, and we don't have access to per index stuff here
-            return JavascriptCompiler.compile(script);
-        } catch (ParseException e) {
-            throw new ScriptException("Failed to parse expression: " + script, e);
-        }
+        // classloader created here
+        return AccessController.doPrivileged(new PrivilegedAction<Expression>() {
+            @Override
+            public Expression run() {
+                try {
+                    // NOTE: validation is delayed to allow runtime vars, and we don't have access to per index stuff here
+                    return JavascriptCompiler.compile(script);
+                } catch (ParseException e) {
+                    throw new ScriptException("Failed to parse expression: " + script, e);
+                }
+            }
+        });
     }
 
     @Override

plugins/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java

@@ -423,6 +423,7 @@ public class MoreExpressionTests extends ESIntegTestCase {
 
     // series of unit test for using expressions as executable scripts
     public void testExecutableScripts() throws Exception {
+        assumeTrue("test creates classes directly, cannot run with security manager", System.getSecurityManager() == null);
         Map<String, Object> vars = new HashMap<>();
         vars.put("a", 2.5);
         vars.put("b", 3);

plugins/lang-groovy/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java

@@ -20,10 +20,13 @@
 package org.elasticsearch.script.groovy;
 
 import java.nio.charset.StandardCharsets;
+
 import com.google.common.hash.Hashing;
+
 import groovy.lang.Binding;
 import groovy.lang.GroovyClassLoader;
 import groovy.lang.Script;
+
 import org.apache.lucene.index.LeafReaderContext;
 import org.apache.lucene.search.Scorer;
 import org.codehaus.groovy.ast.ClassCodeExpressionTransformer;
@@ -49,6 +52,8 @@ import org.elasticsearch.search.lookup.SearchLookup;
 
 import java.io.IOException;
 import java.math.BigDecimal;
+import java.security.AccessController;
+import java.security.PrivilegedAction;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -99,17 +104,30 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri
         }
 
         // Groovy class loader to isolate Groovy-land code
-        this.loader = new GroovyClassLoader(getClass().getClassLoader(), config);
+        // classloader created here
+        this.loader = AccessController.doPrivileged(new PrivilegedAction<GroovyClassLoader>() {
+            @Override
+            public GroovyClassLoader run() {
+                return new GroovyClassLoader(getClass().getClassLoader(), config);
+            }
+        });
     }
 
     @Override
     public void close() {
         loader.clearCache();
-        try {
-            loader.close();
-        } catch (IOException e) {
-            logger.warn("Unable to close Groovy loader", e);
-        }
+        // close classloader here (why do we do this?)
+        AccessController.doPrivileged(new PrivilegedAction<Void>() {
+            @Override
+            public Void run() {
+                try {
+                    loader.close();
+                } catch (IOException e) {
+                    logger.warn("Unable to close Groovy loader", e);
+                }
+                return null;
+            }
+        });
     }
 
     @Override