Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -293,10 +293,19 @@ public Object run() {
// NOTE: we truncate the stack because IndyInterface has security issue (needs getClassLoader)
// we don't do a security check just as a tradeoff, it cannot really escalate to anything.
return AccessController.doPrivileged((PrivilegedAction<Object>) script::run);
} catch (Exception e) {
if (logger.isTraceEnabled()) {
logger.trace("failed to run {}", e, compiledScript);
} catch (AssertionError ae) {
// Groovy asserts are not java asserts, and cannot be disabled, so we do a best-effort trying to determine if this is a
// Groovy assert (in which case we wrap it and throw), or a real Java assert, in which case we rethrow it as-is, likely
// resulting in the uncaughtExceptionHandler handling it.
final StackTraceElement[] elements = ae.getStackTrace();
if (elements.length > 0 && "org.codehaus.groovy.runtime.InvokerHelper".equals(elements[0].getClassName())) {
logger.trace("failed to run {}", ae, compiledScript);
throw new ScriptException("Error evaluating " + compiledScript.name(),
ae, emptyList(), "", compiledScript.lang());
}
throw ae;
} catch (Exception | NoClassDefFoundError e) {
logger.trace("failed to run {}", e, compiledScript);
throw new ScriptException("Error evaluating " + compiledScript.name(), e, emptyList(), "", compiledScript.lang());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,13 @@ public void testEvilGroovyScripts() throws Exception {
}
}

public void testGroovyScriptsThatThrowErrors() throws Exception {
assertFailure("assert false, \"msg\";", AssertionError.class);
assertFailure("def foo=false; assert foo;", AssertionError.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For extra super paranoia's sake can you add a test that throws NoClassDefFoundError so we have something independent that checks that user scripts that do that don't halt the jvm?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to do that, we have to add permissions for the java.lang.NoClassDefFoundError in ClassPermission.java. Are we sure we want to add permissions for that class only to add a test case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me.

// Groovy's asserts require org.codehaus.groovy.runtime.InvokerHelper, so they are denied
assertFailure("def foo=false; assert foo, \"msg2\";", NoClassDefFoundError.class);
}

/** runs a script */
private void doTest(String script) {
Map<String, Object> vars = new HashMap<String, Object>();
Expand All @@ -146,7 +153,7 @@ private void assertSuccess(String script) {
doTest(script);
}

/** asserts that a script triggers securityexception */
/** asserts that a script triggers the given exceptionclass */
private void assertFailure(String script, Class<? extends Throwable> exceptionClass) {
try {
doTest(script);
Expand Down