[SECURITY-1754] Prevent unsandboxed constructor invocation and handle…

… GroovyInterceptable correctly

pom.xml

@@ -63,7 +63,7 @@
     <dependency>
       <groupId>org.kohsuke</groupId>
       <artifactId>groovy-sandbox</artifactId>
-      <version>1.25</version>
+      <version>1.26</version>
       <exclusions>
         <exclusion>
           <groupId>org.codehaus.groovy</groupId>

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<Class<?>> 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<Class<?>> types(@Nonnull Object o) {
+    private static Set<Class<?>> types(@Nonnull Object o) {
         Set<Class<?>> types = new LinkedHashSet<Class<?>>();
         visitTypes(types, o.getClass());
         return types;

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<MethodSignature> methodSignatures = new ArrayList<MethodSignature>();
     final List<NewSignature> newSignatures = new ArrayList<NewSignature>();

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[]

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.