[SECURITY-3016]

src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptor.java

@@ -57,6 +57,7 @@
 import org.codehaus.groovy.runtime.XmlGroovyMethods;
 import org.codehaus.groovy.runtime.metaclass.ClosureMetaMethod;
 import org.codehaus.groovy.runtime.typehandling.NumberMathModificationInfo;
+import org.codehaus.groovy.syntax.Types;
 import org.codehaus.groovy.tools.DgmConverter;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist;
@@ -174,6 +175,24 @@ final class SandboxInterceptor extends GroovyInterceptor {
         } else if (StaticWhitelist.isPermanentlyBlacklistedConstructor(c)) {
             throw StaticWhitelist.rejectNew(c);
         } else if (whitelist.permitsConstructor(c, args)) {
+            if (c.getParameterCount() == 0 && args.length == 1 && args[0] instanceof Map) {
+                // c.f. https://github.com/apache/groovy/blob/41b990d0a20e442f29247f0e04cbed900f3dcad4/src/main/groovy/lang/MetaClassImpl.java#L1728-L1738
+                // We replace the arguments that the invoker will use to construct the object with the empty array to
+                // bypass Groovy's default handling for implicit map constructors.
+                Object newInstance = super.onNewInstance(invoker, receiver, new Object[0]);
+                if (newInstance == null) {
+                    // We were called by Checker.preCheckedCast. Our options here are limited, so we just reject everything.
+                    throw new UnsupportedOperationException("Groovy map constructors may only be invoked using the 'new' keyword in the sandbox (attempted to construct " + receiver + " via a Groovy cast)");
+                }
+                // The call to Map.entrySet below may be on a user-defined type, which could be a problem if we iterated
+                // over it here to pre-check the property assignments and then let Groovy iterate over it again to
+                // actually perform them, so we only iterate over it once and perform the property assignments
+                // ourselves using sandbox-aware methods.
+                for (Map.Entry<Object, Object> entry : ((Map<Object, Object>) args[0]).entrySet()) {
+                    Checker.checkedSetProperty(newInstance, entry.getKey(), false, false, Types.ASSIGN, entry.getValue());
+                }
+                return newInstance;
+            }
             return super.onNewInstance(invoker, receiver, args);
         } else {
             throw StaticWhitelist.rejectNew(c);

src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptorTest.java

@@ -384,6 +384,20 @@ public static final class Special {
     @Issue("JENKINS-34741")
     @Test public void structConstructor() throws Exception {
         assertEvaluate(new StaticWhitelist(), "ok", "class C {String f}; new C(f: 'ok').f");
+        // Map literals are equivalent to the named argument syntax.
+        assertEvaluate(new StaticWhitelist(), "ok", "class C {String f}; new C([f: 'ok']).f");
+        // The map does not have to be a literal.
+        assertEvaluate(new StaticWhitelist(), "ok", "class C {String f}; def map = [f: 'ok']; new C(map).f");
+        // Make sure that we do not assign properties more than once.
+        assertEvaluate(new StaticWhitelist(), 2,
+                "class Global { static int x = 0 }\n" +
+                "class C { def y; def setY(def y) { Global.x += y } }\n" +
+                "new C(y: 2); Global.x");
+        // Make sure that we do not instantiate the object more than once.
+        assertEvaluate(new StaticWhitelist(), 1,
+                "class Global { static int x = 0 }\n" +
+                "class C { def y; C() { Global.x += 1 } }\n" +
+                "new C(y: 2); Global.x");
     }
 
     @Test public void defSyntax() throws Exception {
@@ -669,7 +683,7 @@ public static abstract class SpecialScript extends Script {
 
     @Issue("kohsuke/groovy-sandbox #16")
     @Test public void infiniteLoop() throws Exception {
-        assertEvaluate(new BlanketWhitelist(), "abc", "def split = 'a b c'.split(' '); def b = new StringBuilder(); for (i = 0; i < split.length; i++) {println(i); b.append(split[i])}; b.toString()");
+        assertEvaluate(new BlanketWhitelist(), "abc", "def split = 'a b c'.split(' '); def b = new StringBuilder(); for (i = 0; i < split.length; i++) {b.append(split[i])}; b.toString()");
     }
 
     @Issue("JENKINS-25118")
@@ -1376,6 +1390,16 @@ public void blockIllegalAnnotationsInAnnotations() throws Exception {
                 "new Subclass().class.simpleName");
     }
 
+    @Test public void blockCallsToSyntheticConstructorsViaCasts() throws Exception {
+        // Not ok, the call to super() in the synthetic constructor for Subclass cannot be intercepted.
+        assertRejected(new GenericWhitelist(), "new Subclass org.kohsuke.groovy.sandbox.impl.Checker$SuperConstructorWrapper",
+                        "class Superclass { }\n" +
+                        "class Subclass extends Superclass {\n" +
+                        "  Subclass() { }\n" +
+                        "}\n" +
+                        "Subclass x = [null]");
+    }
+
     @Issue("SECURITY-1754")
     @Test public void groovyInterceptable() throws Throwable {
         assertRejected(new GenericWhitelist(), "method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object",
@@ -1703,6 +1727,27 @@ public static Whitelist allowlist(String... signatures) throws Exception {
         }
     }
 
+    @Issue("SECURITY-3016")
+    @Test public void blockUnsafeCastsPropertyAssignmentViaImplicitMapConstructor() throws Throwable {
+        // Map constructors are supported when using new, but these property assignments are unsafe.
+        assertRejected(new GenericWhitelist(), "new java.io.File java.lang.String",
+                "class Test { File f }; new Test(f: ['secret.key'])");
+        assertRejected(new GenericWhitelist(), "new java.io.File java.lang.String",
+                "class Test { File f; int x }; new Test(x: 1, f: ['secret.key'])");
+        // Map constructors are not supported when casting, regardless of whether the property assignments are safe or not.
+        errors.checkThrows(UnsupportedOperationException.class, () -> evaluate(new GenericWhitelist(),
+                "class Test { File f }; Test t = [f: ['secret.key']]"));
+        errors.checkThrows(UnsupportedOperationException.class, () -> evaluate(new GenericWhitelist(),
+                "class Test { File f; int x }; Test t = [x: 1, f: ['secret.key']]"));
+        errors.checkThrows(UnsupportedOperationException.class, () -> evaluate(new GenericWhitelist(),
+                "class Test { File f }; [f: ['secret.key']] as Test"));
+        errors.checkThrows(UnsupportedOperationException.class, () -> evaluate(new GenericWhitelist(),
+                "class Test { File f }; (Test)[f: ['secret.key']]"));
+        // Map constructors are not currently supported when constructing inner classes.
+        errors.checkThrows(SecurityException.class, () -> evaluate(new GenericWhitelist(),
+                "class Outer { class Inner { File f }; def makeInner() { new Inner(f: ['secret.key']) } }; new Outer().makeInner().f"));
+    }
+
     /**
      * Checks that the annotation is blocked from being used in the provided script whether it is imported or used via
      * fully-qualified class name.