Make groovy sandbox method blacklist dynamically additive

src/main/java/org/elasticsearch/cluster/settings/ClusterDynamicSettingsModule.java

@@ -33,6 +33,7 @@
 import org.elasticsearch.indices.recovery.RecoverySettings;
 import org.elasticsearch.indices.store.IndicesStore;
 import org.elasticsearch.indices.ttl.IndicesTTLService;
+import org.elasticsearch.script.groovy.GroovyScriptEngineService;
 import org.elasticsearch.threadpool.ThreadPool;
 
 /**
@@ -100,6 +101,7 @@ public ClusterDynamicSettingsModule() {
         clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.FIELDDATA_CIRCUIT_BREAKER_OVERHEAD_SETTING, Validator.NON_NEGATIVE_DOUBLE);
         clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_LIMIT_SETTING, Validator.MEMORY_SIZE);
         clusterDynamicSettings.addDynamicSetting(HierarchyCircuitBreakerService.REQUEST_CIRCUIT_BREAKER_OVERHEAD_SETTING, Validator.NON_NEGATIVE_DOUBLE);
+        clusterDynamicSettings.addDynamicSetting(GroovyScriptEngineService.GROOVY_SCRIPT_BLACKLIST_PATCH);
     }
 
     public void addDynamicSettings(String... settings) {

src/main/java/org/elasticsearch/script/ScriptModule.java

@@ -63,7 +63,7 @@ protected void configure() {
         MapBinder<String, NativeScriptFactory> scriptsBinder
                 = MapBinder.newMapBinder(binder(), String.class, NativeScriptFactory.class);
         for (Map.Entry<String, Class<? extends NativeScriptFactory>> entry : scripts.entrySet()) {
-            scriptsBinder.addBinding(entry.getKey()).to(entry.getValue());
+            scriptsBinder.addBinding(entry.getKey()).to(entry.getValue()).asEagerSingleton();
         }
 
         // now, check for config based ones
@@ -74,35 +74,35 @@ protected void configure() {
             if (type == NativeScriptFactory.class) {
                 throw new ElasticsearchIllegalArgumentException("type is missing for native script [" + name + "]");
             }
-            scriptsBinder.addBinding(name).to(type);
+            scriptsBinder.addBinding(name).to(type).asEagerSingleton();
         }
 
         Multibinder<ScriptEngineService> multibinder = Multibinder.newSetBinder(binder(), ScriptEngineService.class);
         multibinder.addBinding().to(NativeScriptEngineService.class);
 
         try {
             settings.getClassLoader().loadClass("groovy.lang.GroovyClassLoader");
-            multibinder.addBinding().to(GroovyScriptEngineService.class);
+            multibinder.addBinding().to(GroovyScriptEngineService.class).asEagerSingleton();
         } catch (Throwable t) {
             Loggers.getLogger(ScriptService.class, settings).debug("failed to load groovy", t);
         }
 
         try {
             settings.getClassLoader().loadClass("com.github.mustachejava.Mustache");
-            multibinder.addBinding().to(MustacheScriptEngineService.class);
+            multibinder.addBinding().to(MustacheScriptEngineService.class).asEagerSingleton();
         } catch (Throwable t) {
             Loggers.getLogger(ScriptService.class, settings).debug("failed to load mustache", t);
         }
 
         try {
             settings.getClassLoader().loadClass("org.apache.lucene.expressions.Expression");
-            multibinder.addBinding().to(ExpressionScriptEngineService.class);
+            multibinder.addBinding().to(ExpressionScriptEngineService.class).asEagerSingleton();
         } catch (Throwable t) {
             Loggers.getLogger(ScriptService.class, settings).debug("failed to load lucene expressions", t);
         }
 
         for (Class<? extends ScriptEngineService> scriptEngine : scriptEngines) {
-            multibinder.addBinding().to(scriptEngine);
+            multibinder.addBinding().to(scriptEngine).asEagerSingleton();
         }
 
         bind(ScriptService.class).asEagerSingleton();

src/main/java/org/elasticsearch/script/ScriptService.java

@@ -57,18 +57,18 @@
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.index.query.TemplateQueryParser;
+import org.elasticsearch.node.settings.NodeSettingsService;
+import org.elasticsearch.script.groovy.GroovyScriptEngineService;
 import org.elasticsearch.search.lookup.SearchLookup;
 import org.elasticsearch.watcher.FileChangesListener;
 import org.elasticsearch.watcher.FileWatcher;
 import org.elasticsearch.watcher.ResourceWatcherService;
 
-import java.io.File;
-import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStreamReader;
-import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Arrays;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
@@ -96,6 +96,7 @@ public class ScriptService extends AbstractComponent {
 
     private final Cache<CacheKey, CompiledScript> cache;
     private final Path scriptsDirectory;
+    private final FileWatcher fileWatcher;
 
     private final DynamicScriptDisabling dynamicScriptingDisabled;
 
@@ -206,9 +207,26 @@ static class IndexedScript {
         }
     }
 
+    class ApplySettings implements NodeSettingsService.Listener {
+        @Override
+        public void onRefreshSettings(Settings settings) {
+            GroovyScriptEngineService engine = (GroovyScriptEngineService) ScriptService.this.scriptEngines.get("groovy");
+            String[] patches = settings.getAsArray(GroovyScriptEngineService.GROOVY_SCRIPT_BLACKLIST_PATCH, Strings.EMPTY_ARRAY);
+            if (Arrays.equals(patches, engine.blacklistAdditions()) == false) {
+                logger.info("updating [{}] from {} to {}", GroovyScriptEngineService.GROOVY_SCRIPT_BLACKLIST_PATCH,
+                        engine.blacklistAdditions(), patches);
+                engine.blacklistAdditions(patches);
+                engine.reloadConfig();
+                // Because the GroovyScriptEngineService knows nothing about the
+                // cache, we need to clear it here if the setting changes
+                ScriptService.this.clearCache();
+            }
+        }
+    }
+
     @Inject
     public ScriptService(Settings settings, Environment env, Set<ScriptEngineService> scriptEngines,
-                         ResourceWatcherService resourceWatcherService) throws IOException {
+                         ResourceWatcherService resourceWatcherService, NodeSettingsService nodeSettingsService) throws IOException {
         super(settings);
 
         int cacheMaxSize = settings.getAsInt(SCRIPT_CACHE_SIZE_SETTING, 100);
@@ -241,7 +259,7 @@ public ScriptService(Settings settings, Environment env, Set<ScriptEngineService
         if (logger.isTraceEnabled()) {
             logger.trace("Using scripts directory [{}] ", scriptsDirectory);
         }
-        FileWatcher fileWatcher = new FileWatcher(scriptsDirectory);
+        this.fileWatcher = new FileWatcher(scriptsDirectory);
         fileWatcher.addListener(new ScriptChangesListener());
 
         if (componentSettings.getAsBoolean("auto_reload_enabled", true)) {
@@ -251,6 +269,7 @@ public ScriptService(Settings settings, Environment env, Set<ScriptEngineService
             // automatic reload is disable just load scripts once
             fileWatcher.init();
         }
+        nodeSettingsService.addListener(new ApplySettings());
     }
 
     //This isn't set in the ctor because doing so creates a guice circular
@@ -273,6 +292,21 @@ public CompiledScript compile(String lang, String script) {
         return compile(lang, script, ScriptType.INLINE);
     }
 
+    /**
+     * Clear both the in memory and on disk compiled script caches. Files on
+     * disk will be treated as if they are new and recompiled.
+     * */
+    public void clearCache() {
+        logger.debug("clearing script cache");
+        // Clear the in-memory script caches
+        this.cache.invalidateAll();
+        this.cache.cleanUp();
+        // Clear the cache of on-disk scripts
+        this.staticCache.clear();
+        // Clear the file watcher's state so it re-compiles on-disk scripts
+        this.fileWatcher.clearState();
+    }
+
     public CompiledScript compile(String lang,  String script, ScriptType scriptType) {
         if (logger.isTraceEnabled()) {
             logger.trace("Compiling lang: [{}] type: [{}] script: {}", lang, scriptType, script);

src/main/java/org/elasticsearch/script/groovy/GroovySandboxExpressionChecker.java

@@ -49,18 +49,22 @@ public class GroovySandboxExpressionChecker implements SecureASTCustomizer.Expre
     public static String GROOVY_SCRIPT_SANDBOX_RECEIVER_WHITELIST = "script.groovy.sandbox.receiver_whitelist";
 
     private final Set<String> methodBlacklist;
+    private final Set<String> additionalMethodBlacklist;
     private final Set<String> packageWhitelist;
     private final Set<String> classWhitelist;
 
-    public GroovySandboxExpressionChecker(Settings settings) {
+    public GroovySandboxExpressionChecker(Settings settings, String[] blacklistAdditions) {
         this.methodBlacklist = ImmutableSet.copyOf(settings.getAsArray(GROOVY_SANDBOX_METHOD_BLACKLIST, defaultMethodBlacklist, true));
+        this.additionalMethodBlacklist = ImmutableSet.copyOf(blacklistAdditions);
         this.packageWhitelist = ImmutableSet.copyOf(settings.getAsArray(GROOVY_SANDBOX_PACKAGE_WHITELIST, defaultPackageWhitelist, true));
         this.classWhitelist = ImmutableSet.copyOf(settings.getAsArray(GROOVY_SANDBOX_CLASS_WHITELIST, defaultClassConstructionWhitelist, true));
     }
 
     // Never allow calling these methods, regardless of the object type
     public static String[] defaultMethodBlacklist = new String[]{
             "getClass",
+            "class",
+            "forName",
             "wait",
             "notify",
             "notifyAll",
@@ -121,6 +125,8 @@ public boolean isAuthorized(Expression expression) {
             String methodName = mce.getMethodAsString();
             if (methodBlacklist.contains(methodName)) {
                 return false;
+            } else if (additionalMethodBlacklist.contains(methodName)) {
+                return false;
             } else if (methodName == null && mce.getMethod() instanceof GStringExpression) {
                 // We do not allow GStrings for method invocation, they are a security risk
                 return false;
@@ -142,7 +148,7 @@ public boolean isAuthorized(Expression expression) {
      * Returns a customized ASTCustomizer that includes the whitelists and
      * expression checker.
      */
-    public static SecureASTCustomizer getSecureASTCustomizer(Settings settings) {
+    public static SecureASTCustomizer getSecureASTCustomizer(Settings settings, String[] blacklistAdditions) {
         SecureASTCustomizer scz = new SecureASTCustomizer();
         // Closures are allowed
         scz.setClosuresAllowed(true);
@@ -158,7 +164,7 @@ public static SecureASTCustomizer getSecureASTCustomizer(Settings settings) {
         String[] receiverWhitelist = settings.getAsArray(GROOVY_SCRIPT_SANDBOX_RECEIVER_WHITELIST, defaultReceiverWhitelist, true);
         scz.setReceiversWhiteList(newArrayList(receiverWhitelist));
         // Add the customized expression checker for finer-grained checking
-        scz.addExpressionCheckers(new GroovySandboxExpressionChecker(settings));
+        scz.addExpressionCheckers(new GroovySandboxExpressionChecker(settings, blacklistAdditions));
         return scz;
     }
 }

src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java

@@ -37,6 +37,7 @@
 import org.codehaus.groovy.control.customizers.ImportCustomizer;
 import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.common.Nullable;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.component.AbstractComponent;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.logging.ESLogger;
@@ -56,22 +57,36 @@
 public class GroovyScriptEngineService extends AbstractComponent implements ScriptEngineService {
 
     public static String GROOVY_SCRIPT_SANDBOX_ENABLED = "script.groovy.sandbox.enabled";
+    public static String GROOVY_SCRIPT_BLACKLIST_PATCH = "script.groovy.sandbox.method_blacklist_patch";
 
     private final AtomicLong counter = new AtomicLong();
-    private final GroovyClassLoader loader;
     private final boolean sandboxed;
+    private volatile GroovyClassLoader loader;
+    private volatile String[] blacklistAdditions = Strings.EMPTY_ARRAY;
 
     @Inject
     public GroovyScriptEngineService(Settings settings) {
         super(settings);
+        this.sandboxed = settings.getAsBoolean(GROOVY_SCRIPT_SANDBOX_ENABLED, true);
+        reloadConfig();
+    }
+
+    public String[] blacklistAdditions() {
+        return this.blacklistAdditions;
+    }
+
+    public void blacklistAdditions(String[] additions) {
+        this.blacklistAdditions = additions;
+    }
+
+    public void reloadConfig() {
         ImportCustomizer imports = new ImportCustomizer();
         imports.addStarImports("org.joda.time");
         imports.addStaticStars("java.lang.Math");
         CompilerConfiguration config = new CompilerConfiguration();
         config.addCompilationCustomizers(imports);
-        this.sandboxed = settings.getAsBoolean(GROOVY_SCRIPT_SANDBOX_ENABLED, true);
         if (this.sandboxed) {
-            config.addCompilationCustomizers(GroovySandboxExpressionChecker.getSecureASTCustomizer(settings));
+            config.addCompilationCustomizers(GroovySandboxExpressionChecker.getSecureASTCustomizer(settings, this.blacklistAdditions));
         }
         // Add BigDecimal -> Double transformer
         config.addCompilationCustomizers(new GroovyBigDecimalTransformer(CompilePhase.CONVERSION));

src/main/java/org/elasticsearch/watcher/FileWatcher.java

@@ -18,14 +18,11 @@
  */
 package org.elasticsearch.watcher;
 
-import com.google.common.collect.Iterators;
 import org.elasticsearch.common.io.FileSystemUtils;
 import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.common.logging.Loggers;
 
-import java.io.File;
 import java.io.IOException;
-import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Arrays;
@@ -38,16 +35,30 @@
 public class FileWatcher extends AbstractResourceWatcher<FileChangesListener> {
 
     private FileObserver rootFileObserver;
+    private Path file;
 
     private static final ESLogger logger = Loggers.getLogger(FileWatcher.class);
 
     /**
      * Creates new file watcher on the given directory
      */
     public FileWatcher(Path file) {
+        this.file = file;
         rootFileObserver = new FileObserver(file);
     }
 
+    /**
+     * Clears any state with the FileWatcher, making all files show up as new
+     */
+    public void clearState() {
+        rootFileObserver = new FileObserver(file);
+        try {
+            rootFileObserver.init(false);
+        } catch (IOException e) {
+            // ignore IOException
+        }
+    }
+
     @Override
     protected void doInit() throws IOException {
         rootFileObserver.init(true);

src/test/java/org/elasticsearch/script/GroovySandboxScriptTests.java

@@ -22,6 +22,9 @@
 import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.search.SearchPhaseExecutionException;
 import org.elasticsearch.action.search.SearchResponse;
+import org.elasticsearch.common.settings.ImmutableSettings;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.script.groovy.GroovyScriptEngineService;
 import org.elasticsearch.test.ElasticsearchIntegrationTest;
 import org.junit.Test;
 
@@ -31,6 +34,7 @@
 /**
  * Tests for the Groovy scripting sandbox
  */
+@ElasticsearchIntegrationTest.ClusterScope(scope = ElasticsearchIntegrationTest.Scope.TEST)
 public class GroovySandboxScriptTests extends ElasticsearchIntegrationTest {
 
     @Test
@@ -62,7 +66,7 @@ public void testSandboxedGroovyScript() {
                 "Expression [MethodCallExpression] is not allowed: d.$(get + Class)().$(getDeclared + Method)(now).$(set + Accessible)(false)");
 
         testFailure("Class.forName(\\\"DateTime\\\").getDeclaredMethod(\\\"plus\\\").setAccessible(true)",
-                "Method calls not allowed on [java.lang.Class]");
+                "Expression [MethodCallExpression] is not allowed: java.lang.Class.forName(DateTime)");
 
         testFailure("Eval.me('2 + 2')", "Method calls not allowed on [groovy.util.Eval]");
 
@@ -90,6 +94,37 @@ public void testSandboxedGroovyScript() {
                 "Expression [MethodCallExpression] is not allowed: java.lang.Runtime.$(get + Runtime)().$methodNameec(touch /tmp/gotcha2)");
     }
 
+    @Test
+    public void testDynamicBlacklist() throws Exception {
+        client().prepareIndex("test", "doc", "1").setSource("foo", 5).setRefresh(true).get();
+
+        testSuccess("[doc['foo'].value, 3, 4].isEmpty()");
+        testSuccess("[doc['foo'].value, 3, 4].size()");
+
+        // Now we blacklist two methods, .isEmpty() and .size()
+        Settings blacklistSettings = ImmutableSettings.builder()
+                .put(GroovyScriptEngineService.GROOVY_SCRIPT_BLACKLIST_PATCH, "isEmpty,size")
+                .build();
+
+        client().admin().cluster().prepareUpdateSettings().setTransientSettings(blacklistSettings).get();
+
+        testFailure("[doc['foo'].value, 3, 4].isEmpty()",
+                "Expression [MethodCallExpression] is not allowed: [doc[foo].value, 3, 4].isEmpty()");
+        testFailure("[doc['foo'].value, 3, 4].size()",
+                "Expression [MethodCallExpression] is not allowed: [doc[foo].value, 3, 4].size()");
+
+        // Undo the blacklist addition and make sure the scripts still work
+        Settings emptyBlacklistSettings = ImmutableSettings.builder()
+                .put(GroovyScriptEngineService.GROOVY_SCRIPT_BLACKLIST_PATCH, "")
+                .build();
+
+        client().admin().cluster().prepareUpdateSettings().setTransientSettings(emptyBlacklistSettings).get();
+
+        testSuccess("[doc['foo'].value, 3, 4].isEmpty()");
+        testSuccess("[doc['foo'].value, 3, 4].size()");
+
+    }
+
     public void testSuccess(String script) {
         logger.info("--> script: " + script);
         SearchResponse resp = client().prepareSearch("test")

src/test/java/org/elasticsearch/script/ScriptServiceTests.java

@@ -24,6 +24,7 @@
 import org.elasticsearch.common.io.Streams;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
+import org.elasticsearch.node.settings.NodeSettingsService;
 import org.elasticsearch.search.lookup.SearchLookup;
 import org.elasticsearch.test.ElasticsearchTestCase;
 import org.elasticsearch.watcher.ResourceWatcherService;
@@ -57,7 +58,8 @@ public void testScriptsWithoutExtensions() throws IOException {
         ResourceWatcherService resourceWatcherService = new ResourceWatcherService(settings, null);
 
         logger.info("--> setup script service");
-        ScriptService scriptService = new ScriptService(settings, environment, ImmutableSet.of(new TestEngineService()), resourceWatcherService);
+        ScriptService scriptService = new ScriptService(settings, environment,
+                ImmutableSet.of(new TestEngineService()), resourceWatcherService, new NodeSettingsService(settings));
         Path scriptsFile = genericConfigFolder.resolve("scripts");
         Files.createDirectories(scriptsFile);
         resourceWatcherService.notifyNow();