From 5b1969e0bdf5cde04a165b847144756b28495788 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Tue, 3 Mar 2020 14:59:14 -0500 Subject: [PATCH] [SECURITY-1754] Prevent unsandboxed constructor invocation and handle GroovyInterceptable correctly --- pom.xml | 2 +- .../groovy/GroovyCallSiteSelector.java | 9 +- .../sandbox/whitelists/StaticWhitelist.java | 5 +- .../sandbox/whitelists/blacklist | 4 + .../groovy/SandboxInterceptorTest.java | 101 ++++++++++++++++++ 5 files changed, 117 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index 574ef4995..698de817c 100644 --- a/pom.xml +++ b/pom.xml @@ -63,7 +63,7 @@ org.kohsuke groovy-sandbox - 1.25 + 1.26 org.codehaus.groovy diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java index 70c97519a..271afcb27 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/GroovyCallSiteSelector.java @@ -26,6 +26,7 @@ import com.google.common.primitives.Primitives; import groovy.lang.GString; +import groovy.lang.GroovyInterceptable; import java.lang.reflect.AccessibleObject; import java.lang.reflect.Constructor; import java.lang.reflect.Field; @@ -146,7 +147,11 @@ private static boolean isInstancePrimitive(@Nonnull Class type, @Nonnull Obje * @param args a set of actual arguments */ public static @CheckForNull Method method(@Nonnull Object receiver, @Nonnull String method, @Nonnull Object[] args) { - for (Class c : types(receiver)) { + Set> types = types(receiver); + if (types.contains(GroovyInterceptable.class) && !"invokeMethod".equals(method)) { + return method(receiver, "invokeMethod", new Object[]{ method, args }); + } + for (Class c : types) { Method candidate = findMatchingMethod(c, method, args); if (candidate != null) { return candidate; @@ -262,7 +267,7 @@ private static boolean isVarArgsMethod(@Nonnull Method m, @Nonnull Object[] args return null; } - private static Iterable> types(@Nonnull Object o) { + private static Set> types(@Nonnull Object o) { Set> types = new LinkedHashSet>(); visitTypes(types, o.getClass()); return types; diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelist.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelist.java index 78d633e45..970c9842b 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelist.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelist.java @@ -63,7 +63,10 @@ public final class StaticWhitelist extends EnumeratingWhitelist { "staticMethod java.lang.System exit int", }; - private static final String[] PERMANENTLY_BLACKLISTED_CONSTRUCTORS = new String[0]; + private static final String[] PERMANENTLY_BLACKLISTED_CONSTRUCTORS = { + "new org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper java.lang.Object[]", + "new org.kohsuke.groovy.sandbox.impl.Checker$ThisConstructorWrapper java.lang.Object[]" + }; final List methodSignatures = new ArrayList(); final List newSignatures = new ArrayList(); diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/blacklist b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/blacklist index 2fd03799d..f792394ad 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/blacklist +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/blacklist @@ -110,3 +110,7 @@ staticMethod org.codehaus.groovy.runtime.ScriptBytecodeAdapter castToType java.l # TODO do we need a @Blacklisted annotation? method org.jenkinsci.plugins.workflow.support.steps.build.RunWrapper getRawBuild + +# SECURITY-1754 +new org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper java.lang.Object[] +new org.kohsuke.groovy.sandbox.impl.Checker$ThisConstructorWrapper java.lang.Object[] diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java index 871412e66..f2178b9da 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java @@ -79,6 +79,8 @@ import org.junit.Test; import org.junit.rules.ErrorCollector; import org.jvnet.hudson.test.Issue; +import org.kohsuke.groovy.sandbox.impl.Checker.SuperConstructorWrapper; +import org.kohsuke.groovy.sandbox.impl.Checker.ThisConstructorWrapper; public class SandboxInterceptorTest { @@ -1296,6 +1298,105 @@ public void blockIllegalAnnotationsInAnnotations() throws Exception { "class Foo { }\n"); } + @Issue("SECURITY-1754") + @Test public void blockDirectCallsToSyntheticConstructors() throws Exception { + try { + // Not ok, the call to super() in the synthetic constructor for Subclass cannot be intercepted. + evaluate(new GenericWhitelist(), + "class Superclass { }\n" + + "class Subclass extends Superclass { }\n" + + "new Subclass(null)"); + fail("Script should have failed"); + } catch (SecurityException e) { + assertThat(e.getMessage(), equalTo( + "Rejecting illegal call to synthetic constructor: private Subclass(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper). " + + "Perhaps you meant to use one of these constructors instead: public Subclass()")); + } + } + + @Issue("SECURITY-1754") + @Test public void blockMisinterceptedCallsToSyntheticConstructors() throws Exception { + try { + // Not ok, the call to super() in the synthetic constructor for Subclass cannot be intercepted. + evaluate(new GenericWhitelist(), + "class Superclass { }\n" + + "class Subclass extends Superclass {\n" + + " Subclass() { def x = 1 }\n" + + " Subclass(Subclass s) { def x = 1 }\n" + + "}\n" + + "new Subclass(null)"); // Intercepted as a call to the second constructor before SECURITY-1754, but actually calls synthetic constructor. + fail("Script should have failed"); + } catch (SecurityException e) { + assertThat(e.getMessage(), equalTo( + "Rejecting illegal call to synthetic constructor: private Subclass(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper). " + + "Perhaps you meant to use one of these constructors instead: public Subclass(), public Subclass(Subclass)")); + } + } + + @Issue("SECURITY-1754") + @Test public void blockCallsToSyntheticConstructorsViaOtherConstructors() throws Exception { + try { + // Not ok, the call to super() in the synthetic constructor for Subclass cannot be intercepted. + evaluate(new GenericWhitelist(), + "class Superclass { }\n" + + "class Subclass extends Superclass {\n" + + " Subclass() { }\n" + + " Subclass(int x, int y) { this(null) }\n" + // Calls synthetic constructor + "}\n" + + "new Subclass(1, 2)"); + fail("Script should have failed"); + } catch (SecurityException e) { + assertThat(e.getMessage(), equalTo( + "Rejecting illegal call to synthetic constructor: private Subclass(org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper). " + + "Perhaps you meant to use one of these constructors instead: public Subclass(), public Subclass(int,int)")); + } + } + + @Issue("SECURITY-1754") + @Test public void blockConstructorWrappersFromBeingUsedDirectly() throws Exception { + for (Class syntheticParamType : new Class[] { SuperConstructorWrapper.class, ThisConstructorWrapper.class }) { + // Not ok, instantiating any of the wrappers would allow attackers to bypass the fix. + assertRejected(new GenericWhitelist(), "new " + syntheticParamType.getName() + " java.lang.Object[]", + "new " + syntheticParamType.getCanonicalName() + "(null)"); + // The wrapper's constructors are permanently blacklisted + assertRejected(new BlanketWhitelist(), "new " + syntheticParamType.getName() + " java.lang.Object[]", + "new " + syntheticParamType.getCanonicalName() + "(null)"); + } + } + + @Issue("SECURITY-1754") + @Test public void allowCheckedCallsToSyntheticConstructors() throws Exception { + // Ok, super call is intercepted via Checker.checkedSuperConstructor. + assertEvaluate(new GenericWhitelist(), "Subclass", + "class Superclass { }\n" + + "class Subclass extends Superclass { }\n" + + "new Subclass().class.simpleName"); + // Ok, this call is intercepted via Checker.checkedThisConstructor. + assertEvaluate(new GenericWhitelist(), "Subclass", + "class Subclass {\n" + + " Subclass() { this(1) }\n" + + " Subclass(int x) { }\n" + + "}\n" + + "new Subclass().class.simpleName"); + } + + @Issue("SECURITY-1754") + @Test public void groovyInterceptable() throws Throwable { + assertRejected(new GenericWhitelist(), "method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object", + "class Test implements GroovyInterceptable {\n" + + " def hello() { 'world' }\n" + + " def invokeMethod(String name, Object args) { 'goodbye' }\n" + + "}\n" + + "new Test().hello()\n"); + // Property access is not affected by GroovyInterceptable. + assertEvaluate(new GenericWhitelist(), "world", + "class Test implements GroovyInterceptable {\n" + + " def hello = 'world'\n" + + " def invokeMethod(String name, Object args) { 'goodbye' }\n" + + "}\n" + + "new Test().hello\n"); + } + /** * Checks that the annotation is blocked from being used in the provided script whether it is imported or used via * fully-qualified class name.