Catch and wrap AssertionError and NoClassDefFoundError in groovy scripts #19958

dakrone · 2016-08-11T16:02:05Z

Previously, we only caught subclasses of Exception, however, there are
some cases when Errors are thrown instead of Exceptions. These two cases
are assert and when a class cannot be found.

Without this change, the exception would bubble up to the
uncaughtExceptionHandler, which would in turn, exit the JVM
(related: #19923).

A note of difference between regular Java asserts and Groovy asserts,
from http://docs.groovy-lang.org/docs/latest/html/documentation/core-testing-guide.html

"Another important difference from Java is that in Groovy assertions are
enabled by default. It has been a language design decision to remove the
possibility to deactivate assertions."

In the event that a user uses an assert such as:

def bar=false; assert bar, "message";

The GroovyScriptEngineService throws a NoClassDefFoundError being unable
to find the java.lang.StringBuffer class. It is highly recommended
that any Groovy scripting user switch to using regular exceptions rather
than unconfigurable Groovy assertions.

Resolves #19806

nik9000 · 2016-08-11T16:09:21Z

As much as I don't like catching java's AssertionError to catch a groovy feature, it looks like that is required or we'll exit the JVM on fairly innocuous errors in user scripts.

LGTM

nik9000 · 2016-08-11T16:09:54Z

Do we need the same protection in the other language plugins? Probably not painless, but lang-javascript and lang-python might need it.

jasontedor · 2016-08-11T16:16:40Z

...les/lang-groovy/src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java

@@ -293,7 +293,7 @@ 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) {
+            } catch (Exception | AssertionError | NoClassDefFoundError e) { // Groovy asserts are not java asserts, and cannot be disabled


I think we should inspect the AssertionError and make sure that it comes from Groovy (by inspecting the stack trace and looking for assertFailed and other relevant Groovy calls).

Okay, I added a check for this, and a comment explaining why

rmuir · 2016-08-11T16:33:08Z

Also why not simply add StringBuffer for groovy if it makes its assert work?

https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/script/ClassPermission.java#L79

The list is very simple: based on the old groovy sandbox, but that sandbox was not really "working" as well, for example java.lang.String was never whitelisted, yet it worked, because that mechanism was swiss-cheese.

So we may have to add classes like stringbuilder/buffer or other things.

nik9000 · 2016-08-11T16:40:14Z

why not simply add StringBuffer for groovy if it makes its assert work?

We may want to add those classes too, but we need something like that so that other asserts floating around groovy don't shoot the JVM. We also don't want asserts users write to shoot the jvm, or any time they call Class.forName somehow.

rmuir · 2016-08-11T16:47:50Z

Yes: I agree the two things are ... somewhat unrelated. But this is tied to the issue there, and adds a test that groovy's assert throws NoClassDefFoundError, which i really don't think needs to happen.

jasontedor · 2016-08-11T16:49:35Z

modules/lang-groovy/src/test/java/org/elasticsearch/script/groovy/GroovySecurityTests.java

+    public void testGroovyScriptsThatThrowErrors() throws Exception {
+        assertFailure("assert false, \"msg\";", AssertionError.class);
+        assertFailure("def foo=false; assert foo;", AssertionError.class);
+        assertFailure("def bar=false; assert bar, \"msg2\"", NoClassDefFoundError.class);


I guess this is because we are not granting the class permission for StringBuffer but I think we should?

Sure, I added the permissions for that (StringBuffer)

Sure, but that was before we knew that we would need to add a permission for InvokeHelper too? So maybe we should just not add it since it will not help with getting asserts to work? Sorry.

dakrone · 2016-08-11T17:08:10Z

Also why not simply add StringBuffer for groovy if it makes its assert work?

I can certainly add StringBuffer, however, that isn't the only class required to get groovy asserts to work, it also needs org.codehaus.groovy.runtime.InvokerHelper, which I think looks dangerous to add. I wish I could disable groovy's asserts entirely :-/

rmuir · 2016-08-11T17:12:22Z

yes, lets leave that one out of it, serialization/reflection/etc, its not a good guy. Thanks for checking.

nik9000 · 2016-08-11T19:47:23Z

modules/lang-groovy/src/test/java/org/elasticsearch/script/groovy/GroovySecurityTests.java

@@ -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);


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?

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?

Makes sense to me.

jasontedor · 2016-08-12T17:45:02Z

LGTM.

Previously, we only caught subclasses of Exception, however, there are some cases when Errors are thrown instead of Exceptions. These two cases are `assert` and when a class cannot be found. Without this change, the exception would bubble up to the `uncaughtExceptionHandler`, which would in turn, exit the JVM (related: elastic#19923). A note of difference between regular Java asserts and Groovy asserts, from http://docs.groovy-lang.org/docs/latest/html/documentation/core-testing-guide.html "Another important difference from Java is that in Groovy assertions are enabled by default. It has been a language design decision to remove the possibility to deactivate assertions." In the event that a user uses an assert such as: ```groovy def bar=false; assert bar, "message"; ``` The GroovyScriptEngineService throws a NoClassDefFoundError being unable to find the `java.lang.StringBuffer` class. It is *highly* recommended that any Groovy scripting user switch to using regular exceptions rather than unconfiguration Groovy assertions. Resolves elastic#19806

dakrone added >bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v5.0.0-beta1 labels Aug 11, 2016

dakrone changed the title ~~Catch AssertionError and NoClassDefFoundError in groovy scripts~~ Catch and wrap AssertionError and NoClassDefFoundError in groovy scripts Aug 11, 2016

jasontedor reviewed Aug 11, 2016
View reviewed changes

nik9000 reviewed Aug 11, 2016
View reviewed changes

dakrone force-pushed the handle-groovy-errors-better branch from f9bee1c to aab8c1f Compare August 12, 2016 19:06

dakrone merged commit aab8c1f into elastic:master Aug 12, 2016

dakrone deleted the handle-groovy-errors-better branch August 16, 2016 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch and wrap AssertionError and NoClassDefFoundError in groovy scripts #19958

Catch and wrap AssertionError and NoClassDefFoundError in groovy scripts #19958

dakrone commented Aug 11, 2016

nik9000 commented Aug 11, 2016

nik9000 commented Aug 11, 2016

jasontedor Aug 11, 2016

dakrone Aug 11, 2016

rmuir commented Aug 11, 2016

nik9000 commented Aug 11, 2016

rmuir commented Aug 11, 2016

jasontedor Aug 11, 2016

dakrone Aug 11, 2016

jasontedor Aug 11, 2016

dakrone commented Aug 11, 2016

rmuir commented Aug 11, 2016

nik9000 Aug 11, 2016

dakrone Aug 11, 2016

nik9000 Aug 11, 2016

jasontedor commented Aug 12, 2016

Catch and wrap AssertionError and NoClassDefFoundError in groovy scripts #19958

Catch and wrap AssertionError and NoClassDefFoundError in groovy scripts #19958

Conversation

dakrone commented Aug 11, 2016

nik9000 commented Aug 11, 2016

nik9000 commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmuir commented Aug 11, 2016

nik9000 commented Aug 11, 2016

rmuir commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakrone commented Aug 11, 2016

rmuir commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor commented Aug 12, 2016