From 068f60504080c680d9e582fe479acdbf10f9ae25 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 30 Sep 2020 00:37:41 +1000 Subject: [PATCH] Use compilation as validation for painless role template (#62845) (#63010) * Use compilation as validation for painless role template (#62845) Role template validation now performs only compilation if the script is painless. It no longer attempts to execute the script with empty input which is problematic. The compliation process will catch things like invalid syntax, undefined variables, which still provide certain level of protection against ill-defined role templates. Behaviour for Mustache script is unchanged. * Checkstyle --- .../support/mapper/TemplateRoleName.java | 10 +++- .../support/MustacheTemplateEvaluator.java | 7 ++- .../support/mapper/TemplateRoleNameTests.java | 51 +++++++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/mapper/TemplateRoleName.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/mapper/TemplateRoleName.java index e5a039b89a1..b88fd77987d 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/mapper/TemplateRoleName.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/mapper/TemplateRoleName.java @@ -24,7 +24,9 @@ import org.elasticsearch.common.xcontent.XContentParseException; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService; +import org.elasticsearch.script.TemplateScript; import org.elasticsearch.xpack.core.security.authc.support.mapper.expressiondsl.ExpressionModel; import org.elasticsearch.xpack.core.security.support.MustacheTemplateEvaluator; @@ -99,7 +101,13 @@ public class TemplateRoleName implements ToXContentObject, Writeable { public void validate(ScriptService scriptService) { try { - parseTemplate(scriptService, Collections.emptyMap()); + final XContentParser parser = XContentHelper.createParser( + NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, template, XContentType.JSON); + final Script script = MustacheTemplateEvaluator.parseForScript(parser, Collections.emptyMap()); + final TemplateScript compiledTemplate = scriptService.compile(script, TemplateScript.CONTEXT).newInstance(script.getParams()); + if ("mustache".equals(script.getLang())) { + compiledTemplate.execute(); + } } catch (IllegalArgumentException e) { throw e; } catch (IOException e) { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/MustacheTemplateEvaluator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/MustacheTemplateEvaluator.java index 02f730333de..1e1fea7137d 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/MustacheTemplateEvaluator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/support/MustacheTemplateEvaluator.java @@ -25,7 +25,7 @@ public final class MustacheTemplateEvaluator { throw new UnsupportedOperationException("Cannot construct " + MustacheTemplateEvaluator.class); } - public static String evaluate(ScriptService scriptService, XContentParser parser, Map extraParams) throws IOException { + public static Script parseForScript(XContentParser parser, Map extraParams) throws IOException { Script script = Script.parse(parser); // Add the user details to the params Map params = new HashMap<>(); @@ -36,6 +36,11 @@ public final class MustacheTemplateEvaluator { // Always enforce mustache script lang: script = new Script(script.getType(), script.getType() == ScriptType.STORED ? null : "mustache", script.getIdOrCode(), script.getOptions(), params); + return script; + } + + public static String evaluate(ScriptService scriptService, XContentParser parser, Map extraParams) throws IOException { + Script script = parseForScript(parser, extraParams); TemplateScript compiledTemplate = scriptService.compile(script, TemplateScript.CONTEXT).newInstance(script.getParams()); return compiledTemplate.execute(); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/support/mapper/TemplateRoleNameTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/support/mapper/TemplateRoleNameTests.java index 8dc14c603ab..d99ef97c39d 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/support/mapper/TemplateRoleNameTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/support/mapper/TemplateRoleNameTests.java @@ -20,11 +20,13 @@ import org.elasticsearch.common.xcontent.DeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.script.ScriptEngine; import org.elasticsearch.script.ScriptException; import org.elasticsearch.script.ScriptMetadata; import org.elasticsearch.script.ScriptModule; import org.elasticsearch.script.ScriptService; import org.elasticsearch.script.StoredScriptSource; +import org.elasticsearch.script.TemplateScript; import org.elasticsearch.script.mustache.MustacheScriptEngine; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.EqualsHashCodeTestUtils; @@ -40,7 +42,13 @@ import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.notNullValue; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class TemplateRoleNameTests extends ESTestCase { @@ -183,6 +191,49 @@ public class TemplateRoleNameTests extends ESTestCase { assertTrue(e.getCause() instanceof ScriptException); } + public void testValidateWillCompileButNotExecutePainlessScript() { + final TemplateScript compiledScript = mock(TemplateScript.class); + doThrow(new IllegalStateException("Validate should not execute painless script")).when(compiledScript).execute(); + final TemplateScript.Factory scriptFactory = mock(TemplateScript.Factory.class); + when(scriptFactory.newInstance(any())).thenReturn(compiledScript); + + final ScriptEngine scriptEngine = mock(ScriptEngine.class); + when(scriptEngine.getType()).thenReturn("painless"); + when(scriptEngine.compile(eq("valid"), eq("params.metedata.group"), any(), + eq(org.elasticsearch.common.collect.Map.of()))) + .thenReturn(scriptFactory); + final ScriptException scriptException = + new ScriptException("exception", new IllegalStateException(), org.elasticsearch.common.collect.List.of(), + "bad syntax", "painless"); + doThrow(scriptException) + .when(scriptEngine).compile(eq("invalid"), eq("bad syntax"), any(), + eq(org.elasticsearch.common.collect.Map.of())); + + final ScriptService scriptService = new ScriptService(Settings.EMPTY, + org.elasticsearch.common.collect.Map.of("painless", scriptEngine), ScriptModule.CORE_CONTEXTS) { + @Override + protected StoredScriptSource getScriptFromClusterState(String id) { + if ("valid".equals(id)) { + return new StoredScriptSource("painless", "params.metedata.group", + org.elasticsearch.common.collect.Map.of()); + } else { + return new StoredScriptSource("painless", "bad syntax", + org.elasticsearch.common.collect.Map.of()); + } + } + }; + // Validation succeeds if compilation is successful + new TemplateRoleName(new BytesArray("{ \"id\":\"valid\" }"), Format.STRING).validate(scriptService); + verify(scriptEngine, times(1)) + .compile(eq("valid"), eq("params.metedata.group"), any(), eq(org.elasticsearch.common.collect.Map.of())); + verify(compiledScript, never()).execute(); + + // Validation fails if compilation fails + final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, + () -> new TemplateRoleName(new BytesArray("{ \"id\":\"invalid\" }"), Format.STRING).validate(scriptService)); + assertSame(scriptException, e.getCause()); + } + public void testValidationWillFailWhenInlineScriptIsNotEnabled() { final Settings settings = Settings.builder().put("script.allowed_types", ScriptService.ALLOW_NONE).build(); final ScriptService scriptService = new ScriptService(settings,