From bbada97d89a45708d1fad86313544df153e2040d Mon Sep 17 00:00:00 2001 From: Tiago Fernandez Date: Mon, 7 Nov 2011 14:45:46 +0100 Subject: [PATCH 1/2] Add GroovyClassLoader-aware constructor to GroovyScriptEngineImpl. --- .../groovy/jsr223/GroovyScriptEngineImpl.java | 16 ++-- .../groovy/jsr223/JSR223SecurityTest.java | 86 +++++++++++++------ 2 files changed, 67 insertions(+), 35 deletions(-) diff --git a/src/main/org/codehaus/groovy/jsr223/GroovyScriptEngineImpl.java b/src/main/org/codehaus/groovy/jsr223/GroovyScriptEngineImpl.java index 22581ca185..3ac4fee59d 100644 --- a/src/main/org/codehaus/groovy/jsr223/GroovyScriptEngineImpl.java +++ b/src/main/org/codehaus/groovy/jsr223/GroovyScriptEngineImpl.java @@ -75,10 +75,10 @@ public class GroovyScriptEngineImpl private static boolean debug = false; // script-string-to-generated Class map - private Map classMap; + private Map classMap = Collections.synchronizedMap(new HashMap()); // global closures map - this is used to simulate a single // global functions namespace - private Map globalClosures; + private Map globalClosures = Collections.synchronizedMap(new HashMap()); // class loader for Groovy generated classes private GroovyClassLoader loader; // lazily initialized factory @@ -92,10 +92,12 @@ public class GroovyScriptEngineImpl } public GroovyScriptEngineImpl() { - classMap = Collections.synchronizedMap(new HashMap()); - globalClosures = Collections.synchronizedMap(new HashMap()); - loader = new GroovyClassLoader(getParentLoader(), - new CompilerConfiguration()); + this.loader = new GroovyClassLoader(getParentLoader(), new CompilerConfiguration()); + } + + public GroovyScriptEngineImpl(GroovyClassLoader classLoader) { + if (classLoader == null) throw new IllegalArgumentException("GroovyClassLoader is null"); + this.loader = classLoader; } public Object eval(Reader reader, ScriptContext ctx) @@ -339,7 +341,7 @@ Class getScriptClass(String script) return clazz; } - public void setClassLoader(final GroovyClassLoader classLoader) { + public void setClassLoader(GroovyClassLoader classLoader) { this.loader = classLoader; } diff --git a/src/test/org/codehaus/groovy/jsr223/JSR223SecurityTest.java b/src/test/org/codehaus/groovy/jsr223/JSR223SecurityTest.java index 853d884197..1c7596857a 100644 --- a/src/test/org/codehaus/groovy/jsr223/JSR223SecurityTest.java +++ b/src/test/org/codehaus/groovy/jsr223/JSR223SecurityTest.java @@ -11,6 +11,7 @@ import org.codehaus.groovy.control.CompilerConfiguration; import org.codehaus.groovy.control.Phases; import org.codehaus.groovy.control.SourceUnit; +import org.junit.Before; import org.junit.Test; import javax.script.ScriptEngine; @@ -26,32 +27,61 @@ */ public class JSR223SecurityTest { + class TestFixture { + String script = "System.exit 2"; + String forbiddenInstruction = "java.lang.System"; + } + + TestFixture testFixture; + + @Before + public void resetTestFixture() { + testFixture = new TestFixture(); + } + @Test(expected = ScriptException.class) public void should_forbid_an_instruction_when_overriding_GroovyClassLoader_using_reflection() throws Exception { - secureEval("System.exit 2", "java.lang.System", true); + secureEval(ClassLoaderDefinitionType.REFLECTION); } @Test(expected = ScriptException.class) public void should_forbid_an_instruction_when_overriding_GroovyClassLoader_using_injection() throws Exception { - secureEval("System.exit 2", "java.lang.System", false); + secureEval(ClassLoaderDefinitionType.INJECTION); } - private void secureEval(final String script, final String forbiddenInstruction, final boolean useReflection) throws Exception { - final ScriptEngine groovyEngine = new ScriptEngineManager().getEngineByName("groovy"); + @Test(expected = ScriptException.class) + public void should_forbid_an_instruction_when_overriding_GroovyClassLoader_using_constructor() throws Exception { + secureEval(ClassLoaderDefinitionType.CONSTRUCTOR); + } - final GroovySecurityManager groovySecurityManager = GroovySecurityManager.instance(); - groovySecurityManager.overrideGroovyClassLoader(groovyEngine, useReflection); - groovySecurityManager.forbid(forbiddenInstruction); + private void secureEval(ClassLoaderDefinitionType classLoaderDefType) throws Exception { + ScriptEngine engine = createScriptEngine(classLoaderDefType); - groovyEngine.eval(script); + GroovySecurityManager securityMgr = GroovySecurityManager.instance(); + securityMgr.overrideGroovyClassLoader(engine, classLoaderDefType); + securityMgr.forbid(testFixture.forbiddenInstruction); + + engine.eval(testFixture.script); + } + + private ScriptEngine createScriptEngine(ClassLoaderDefinitionType classLoaderDefType) { + return (classLoaderDefType == ClassLoaderDefinitionType.CONSTRUCTOR) + ? new GroovyScriptEngineImpl(new CustomGroovyClassLoader()) + : new ScriptEngineManager().getEngineByName("groovy"); } } +enum ClassLoaderDefinitionType { + CONSTRUCTOR, + INJECTION, + REFLECTION +} + class GroovySecurityManager { - private final static GroovySecurityManager instance = new GroovySecurityManager(); + private static GroovySecurityManager instance = new GroovySecurityManager(); - private final Set blacklist = new HashSet(); + private Set blacklist = new HashSet(); private GroovySecurityManager() { } @@ -59,22 +89,25 @@ public synchronized static GroovySecurityManager instance() { return instance; } - public void overrideGroovyClassLoader(final ScriptEngine engine, final boolean useReflection) { + public void overrideGroovyClassLoader(ScriptEngine engine, ClassLoaderDefinitionType classLoaderDefType) { try { - if (useReflection) + if (classLoaderDefType == ClassLoaderDefinitionType.REFLECTION) { overrideDefaultGroovyClassLoaderUsingReflection(engine); - else + } + else if (classLoaderDefType == ClassLoaderDefinitionType.INJECTION) { overrideDefaultGroovyClassLoaderUsingInjection(engine); - } catch (Throwable ex) { + } + } + catch (Throwable ex) { throw new RuntimeException("Could not initialize the security manager", ex); } } - public void forbid(final String instruction) { + public void forbid(String instruction) { blacklist.add(instruction); } - public boolean isForbidden(final String instruction) { + public boolean isForbidden(String instruction) { for (String forbidden : blacklist) if (instruction.startsWith(forbidden)) return true; @@ -82,13 +115,13 @@ public boolean isForbidden(final String instruction) { return false; } - private void overrideDefaultGroovyClassLoaderUsingReflection(final ScriptEngine engine) throws Exception { - final Field classLoader = engine.getClass().getDeclaredField("loader"); + private void overrideDefaultGroovyClassLoaderUsingReflection(ScriptEngine engine) throws Exception { + Field classLoader = engine.getClass().getDeclaredField("loader"); classLoader.setAccessible(true); classLoader.set(engine, new CustomGroovyClassLoader()); } - private void overrideDefaultGroovyClassLoaderUsingInjection(final ScriptEngine engine) throws Exception { + private void overrideDefaultGroovyClassLoaderUsingInjection(ScriptEngine engine) throws Exception { GroovyScriptEngineImpl concreteEngine = (GroovyScriptEngineImpl) engine; concreteEngine.setClassLoader(new CustomGroovyClassLoader()); } @@ -96,16 +129,15 @@ private void overrideDefaultGroovyClassLoaderUsingInjection(final ScriptEngine e class GroovySecurityException extends RuntimeException { - public GroovySecurityException(final String message) { + public GroovySecurityException(String message) { super(message); } } class CustomGroovyClassLoader extends GroovyClassLoader { - @Override - protected CompilationUnit createCompilationUnit(final CompilerConfiguration config, final CodeSource source) { - final CompilationUnit unit = super.createCompilationUnit(config, source); + protected CompilationUnit createCompilationUnit(CompilerConfiguration config, CodeSource source) { + CompilationUnit unit = super.createCompilationUnit(config, source); unit.addPhaseOperation(new CustomPrimaryClassNodeOperation(), Phases.SEMANTIC_ANALYSIS); return unit; } @@ -113,8 +145,7 @@ protected CompilationUnit createCompilationUnit(final CompilerConfiguration conf class CustomPrimaryClassNodeOperation extends PrimaryClassNodeOperation { - @Override - public void call(final SourceUnit source, final GeneratorContext context, final ClassNode classNode) { + public void call(SourceUnit source, GeneratorContext context, ClassNode classNode) { for (Object statement : source.getAST().getStatementBlock().getStatements()) ((ExpressionStatement) statement).visit(new CustomCodeVisitorSupport()); } @@ -122,10 +153,9 @@ public void call(final SourceUnit source, final GeneratorContext context, final class CustomCodeVisitorSupport extends CodeVisitorSupport { - private final GroovySecurityManager groovySecurityManager = GroovySecurityManager.instance(); + private GroovySecurityManager groovySecurityManager = GroovySecurityManager.instance(); - @Override - public void visitMethodCallExpression(final MethodCallExpression call) { + public void visitMethodCallExpression(MethodCallExpression call) { if (groovySecurityManager.isForbidden(call.getText())) throw new GroovySecurityException("The following code is forbidden in the script: " + call.getText()); } From fb2face9643c28a87bdb30fd32befd31c4d03b44 Mon Sep 17 00:00:00 2001 From: Tiago Fernandez Date: Mon, 30 Jan 2012 15:15:57 +0100 Subject: [PATCH 2/2] Add a test suite to JSR-223 module. [junit] Running UberTestCaseJSR223 [junit] Tests run: 3, Failures: 0, Errors: 0, Time elapsed: 0.281 sec ... Class UberTestCaseJSR223 Name Tests Errors Failures Time(s) UberTestCaseJSR223 3 0 0 0.250 should_forbid_an_instruction_when_overriding_GroovyClassLoader_using_reflection Success 0.203 should_forbid_an_instruction_when_overriding_GroovyClassLoader_using_injection Success 0.000 should_forbid_an_instruction_when_overriding_GroovyClassLoader_using_constructor Success 0.000 --- .../groovy-jsr223/src/test/java/UberTestCaseJSR223.java | 9 +++++++++ .../org/codehaus/groovy/jsr223/JSR223SecurityTest.java | 4 +++- 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 subprojects/groovy-jsr223/src/test/java/UberTestCaseJSR223.java diff --git a/subprojects/groovy-jsr223/src/test/java/UberTestCaseJSR223.java b/subprojects/groovy-jsr223/src/test/java/UberTestCaseJSR223.java new file mode 100644 index 0000000000..d8b3effad9 --- /dev/null +++ b/subprojects/groovy-jsr223/src/test/java/UberTestCaseJSR223.java @@ -0,0 +1,9 @@ +import org.codehaus.groovy.jsr223.*; +import org.junit.runner.RunWith; +import org.junit.runners.Suite; + +@RunWith(Suite.class) +@Suite.SuiteClasses({ + JSR223SecurityTest.class +}) +public class UberTestCaseJSR223 {} diff --git a/subprojects/groovy-jsr223/src/test/java/org/codehaus/groovy/jsr223/JSR223SecurityTest.java b/subprojects/groovy-jsr223/src/test/java/org/codehaus/groovy/jsr223/JSR223SecurityTest.java index 1c7596857a..11217ea96c 100644 --- a/subprojects/groovy-jsr223/src/test/java/org/codehaus/groovy/jsr223/JSR223SecurityTest.java +++ b/subprojects/groovy-jsr223/src/test/java/org/codehaus/groovy/jsr223/JSR223SecurityTest.java @@ -23,7 +23,9 @@ import java.util.Set; /** - * Test contributed by Tiago Fernandez, for GROOVY-3946 + * Test for GROOVY-3946 and GROOVY-5255. + * + * @author Tiago Fernandez */ public class JSR223SecurityTest {