From 69bf08f6c6b12ccc22028f7b9a06f5d1673fac88 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 12 Sep 2016 12:06:29 -0400 Subject: [PATCH] Disable regexes by default in painless Adds a new node level, non-dynamic setting, `script.painless.regex.enabled` can be used to enable regexes. Closes #20397 --- docs/build.gradle | 3 ++ .../modules/scripting/painless.asciidoc | 9 +++++ .../painless/CompilerSettings.java | 30 +++++++++++++++++ .../painless/PainlessPlugin.java | 13 ++++++-- .../painless/PainlessScriptEngineService.java | 29 ++++++++++------ .../elasticsearch/painless/antlr/Walker.java | 5 +++ .../elasticsearch/painless/RegexTests.java | 11 ++++++- .../painless/ScriptTestCase.java | 10 +++++- .../painless/WhenThingsGoWrongTests.java | 15 +++++++-- .../test/plan_a/40_disabled.yaml | 33 +++++++++++++++++++ 10 files changed, 141 insertions(+), 17 deletions(-) create mode 100644 modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/40_disabled.yaml diff --git a/docs/build.gradle b/docs/build.gradle index d930dfb5b60..6e2b02b0263 100644 --- a/docs/build.gradle +++ b/docs/build.gradle @@ -24,6 +24,9 @@ integTest { setting 'script.inline', 'true' setting 'script.stored', 'true' setting 'script.max_compilations_per_minute', '1000' + /* Enable regexes in painless so our tests don't complain about example + * snippets that use them. */ + setting 'script.painless.regex.enabled', 'true' Closure configFile = { extraConfigFile it, "src/test/cluster/config/$it" } diff --git a/docs/reference/modules/scripting/painless.asciidoc b/docs/reference/modules/scripting/painless.asciidoc index 308f5564f24..7995e45e7b9 100644 --- a/docs/reference/modules/scripting/painless.asciidoc +++ b/docs/reference/modules/scripting/painless.asciidoc @@ -196,6 +196,15 @@ POST hockey/player/1/_update [[modules-scripting-painless-regex]] === Regular expressions +NOTE: Regexes are disabled by default because they circumvent Painless's +protection against long running and memory hungry scripts. To make matters +worse even innocuous looking regexes can have staggering performance and stack +depth behavior. They remain an amazing powerful tool but are too scary to enable +by default. To enable them yourself set `script.painless.regex.enabled: true` in +`elasticsearch.yml`. We'd like very much to have a safe alternative +implementation that can be enabled by default so check this space for later +developments! + Painless's native support for regular expressions has syntax constructs: * `/pattern/`: Pattern literals create patterns. This is the only way to create diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java index f0e1bde74d0..9ef1b2ccf12 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java @@ -19,10 +19,18 @@ package org.elasticsearch.painless; +import org.elasticsearch.common.settings.Setting; +import org.elasticsearch.common.settings.Setting.Property; + /** * Settings to use when compiling a script. */ public final class CompilerSettings { + /** + * Are regexes enabled? This is a node level setting because regexes break out of painless's lovely sandbox and can cause stack + * overflows and we can't analyze the regex to be sure it won't. + */ + public static final Setting REGEX_ENABLED = Setting.boolSetting("script.painless.regex.enabled", false, Property.NodeScope); /** * Constant to be used when specifying the maximum loop counter when compiling a script. @@ -55,6 +63,12 @@ public final class CompilerSettings { */ private int initialCallSiteDepth = 0; + /** + * Are regexes enabled? They are currently disabled by default because they break out of the loop counter and even fairly simple + * looking regexes can cause stack overflows. + */ + private boolean regexesEnabled = false; + /** * Returns the value for the cumulative total number of statements that can be made in all loops * in a script before an exception is thrown. This attempts to prevent infinite loops. Note if @@ -104,4 +118,20 @@ public final class CompilerSettings { public void setInitialCallSiteDepth(int depth) { this.initialCallSiteDepth = depth; } + + /** + * Are regexes enabled? They are currently disabled by default because they break out of the loop counter and even fairly simple + * looking regexes can cause stack overflows. + */ + public boolean areRegexesEnabled() { + return regexesEnabled; + } + + /** + * Are regexes enabled? They are currently disabled by default because they break out of the loop counter and even fairly simple + * looking regexes can cause stack overflows. + */ + public void setRegexesEnabled(boolean regexesEnabled) { + this.regexesEnabled = regexesEnabled; + } } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessPlugin.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessPlugin.java index 954314286bc..c00dc643102 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessPlugin.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessPlugin.java @@ -20,19 +20,21 @@ package org.elasticsearch.painless; +import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.plugins.ScriptPlugin; -import org.elasticsearch.script.ScriptEngineRegistry; import org.elasticsearch.script.ScriptEngineService; -import org.elasticsearch.script.ScriptModule; + +import java.util.Arrays; +import java.util.List; /** * Registers Painless as a plugin. */ public final class PainlessPlugin extends Plugin implements ScriptPlugin { - // force to pare our definition at startup (not on the user's first script) + // force to parse our definition at startup (not on the user's first script) static { Definition.VOID_TYPE.hashCode(); } @@ -41,4 +43,9 @@ public final class PainlessPlugin extends Plugin implements ScriptPlugin { public ScriptEngineService getScriptEngineService(Settings settings) { return new PainlessScriptEngineService(settings); } + + @Override + public List> getSettings() { + return Arrays.asList(CompilerSettings.REGEX_ENABLED); + } } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngineService.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngineService.java index 834593aeb99..cc165343994 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngineService.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngineService.java @@ -53,11 +53,6 @@ public final class PainlessScriptEngineService extends AbstractComponent impleme */ public static final String NAME = "painless"; - /** - * Default compiler settings to be used. - */ - private static final CompilerSettings DEFAULT_COMPILER_SETTINGS = new CompilerSettings(); - /** * Permissions context used during compilation. */ @@ -74,12 +69,19 @@ public final class PainlessScriptEngineService extends AbstractComponent impleme }); } + /** + * Default compiler settings to be used. Note that {@link CompilerSettings} is mutable but this instance shouldn't be mutated outside + * of {@link PainlessScriptEngineService#PainlessScriptEngineService(Settings)}. + */ + private final CompilerSettings defaultCompilerSettings = new CompilerSettings(); + /** * Constructor. * @param settings The settings to initialize the engine with. */ public PainlessScriptEngineService(final Settings settings) { super(settings); + defaultCompilerSettings.setRegexesEnabled(CompilerSettings.REGEX_ENABLED.get(settings)); } /** @@ -111,29 +113,36 @@ public final class PainlessScriptEngineService extends AbstractComponent impleme if (params.isEmpty()) { // Use the default settings. - compilerSettings = DEFAULT_COMPILER_SETTINGS; + compilerSettings = defaultCompilerSettings; } else { // Use custom settings specified by params. compilerSettings = new CompilerSettings(); - Map copy = new HashMap<>(params); - String value = copy.remove(CompilerSettings.MAX_LOOP_COUNTER); + // Except regexes enabled - this is a node level setting and can't be changed in the request. + compilerSettings.setRegexesEnabled(defaultCompilerSettings.areRegexesEnabled()); + + Map copy = new HashMap<>(params); + + String value = copy.remove(CompilerSettings.MAX_LOOP_COUNTER); if (value != null) { compilerSettings.setMaxLoopCounter(Integer.parseInt(value)); } value = copy.remove(CompilerSettings.PICKY); - if (value != null) { compilerSettings.setPicky(Boolean.parseBoolean(value)); } value = copy.remove(CompilerSettings.INITIAL_CALL_SITE_DEPTH); - if (value != null) { compilerSettings.setInitialCallSiteDepth(Integer.parseInt(value)); } + value = copy.remove(CompilerSettings.REGEX_ENABLED.getKey()); + if (value != null) { + throw new IllegalArgumentException("[painless.regex.enabled] can only be set on node startup."); + } + if (!copy.isEmpty()) { throw new IllegalArgumentException("Unrecognized compile-time parameter(s): " + copy); } diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java index 61269419fdf..da430f4280a 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/antlr/Walker.java @@ -796,6 +796,11 @@ public final class Walker extends PainlessParserBaseVisitor { @Override public ANode visitRegex(RegexContext ctx) { + if (false == settings.areRegexesEnabled()) { + throw location(ctx).createError(new IllegalStateException("Regexes are disabled. Set [script.painless.regex.enabled] to [true] " + + "in elasticsearch.yaml to allow them. Be careful though, regexes break out of Painless's protection against deep " + + "recursion and long loops.")); + } String text = ctx.REGEX().getText(); int lastSlash = text.lastIndexOf('/'); String pattern = text.substring(1, lastSlash); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/RegexTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/RegexTests.java index dbbb9958d71..1c53692ad74 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/RegexTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/RegexTests.java @@ -19,17 +19,26 @@ package org.elasticsearch.painless; +import org.elasticsearch.common.settings.Settings; + import java.nio.CharBuffer; import java.util.Arrays; import java.util.HashSet; import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; -import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.hamcrest.Matchers.containsString; public class RegexTests extends ScriptTestCase { + @Override + protected Settings scriptEngineSettings() { + // Enable regexes just for this test. They are disabled by default. + return Settings.builder() + .put(CompilerSettings.REGEX_ENABLED.getKey(), true) + .build(); + } + public void testPatternAfterReturn() { assertEquals(true, exec("return 'foo' ==~ /foo/")); assertEquals(false, exec("return 'bar' ==~ /foo/")); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java index 63c929a69a7..672204cbc25 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java @@ -45,7 +45,14 @@ public abstract class ScriptTestCase extends ESTestCase { @Before public void setup() { - scriptEngine = new PainlessScriptEngineService(Settings.EMPTY); + scriptEngine = new PainlessScriptEngineService(scriptEngineSettings()); + } + + /** + * Settings used to build the script engine. Override to customize settings like {@link RegexTests} does to enable regexes. + */ + protected Settings scriptEngineSettings() { + return Settings.EMPTY; } /** Compiles and returns the result of {@code script} */ @@ -71,6 +78,7 @@ public abstract class ScriptTestCase extends ESTestCase { if (picky) { CompilerSettings pickySettings = new CompilerSettings(); pickySettings.setPicky(true); + pickySettings.setRegexesEnabled(CompilerSettings.REGEX_ENABLED.get(scriptEngineSettings())); Walker.buildPainlessTree(getTestName(), script, pickySettings, null); } // test actual script execution diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java index 1d60eb9c29a..7e4311f24ec 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java @@ -20,14 +20,13 @@ package org.elasticsearch.painless; import org.apache.lucene.util.Constants; -import org.elasticsearch.script.ScriptException; import java.lang.invoke.WrongMethodTypeException; import java.util.Arrays; import java.util.Collections; import static java.util.Collections.emptyMap; -import static org.hamcrest.Matchers.containsString; +import static java.util.Collections.singletonMap; public class WhenThingsGoWrongTests extends ScriptTestCase { public void testNullPointer() { @@ -234,4 +233,16 @@ public class WhenThingsGoWrongTests extends ScriptTestCase { exec("void recurse(int x, int y) {recurse(x, y)} recurse(1, 2);"); }); } + + public void testRegexDisabledByDefault() { + IllegalStateException e = expectThrows(IllegalStateException.class, () -> exec("return 'foo' ==~ /foo/")); + assertEquals("Regexes are disabled. Set [script.painless.regex.enabled] to [true] in elasticsearch.yaml to allow them. " + + "Be careful though, regexes break out of Painless's protection against deep recursion and long loops.", e.getMessage()); + } + + public void testCanNotOverrideRegexEnabled() { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> exec("", null, singletonMap(CompilerSettings.REGEX_ENABLED.getKey(), "true"), null, false)); + assertEquals("[painless.regex.enabled] can only be set on node startup.", e.getMessage()); + } } diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/40_disabled.yaml b/modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/40_disabled.yaml new file mode 100644 index 00000000000..bcf02f657b0 --- /dev/null +++ b/modules/lang-painless/src/test/resources/rest-api-spec/test/plan_a/40_disabled.yaml @@ -0,0 +1,33 @@ +--- +"Regex in update fails": + + - do: + index: + index: test_1 + type: test + id: 1 + body: + foo: bar + count: 1 + + - do: + catch: /Regexes are disabled. Set \[script.painless.regex.enabled\] to \[true\] in elasticsearch.yaml to allow them. Be careful though, regexes break out of Painless's protection against deep recursion and long loops./ + update: + index: test_1 + type: test + id: 1 + body: + script: + lang: painless + inline: "ctx._source.foo = params.bar ==~ /cat/" + params: { bar: 'xxx' } + +--- +"Regex enabled is not a dynamic setting": + + - do: + catch: /setting \[script.painless.regex.enabled\], not dynamically updateable/ + cluster.put_settings: + body: + transient: + script.painless.regex.enabled: true