diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptor.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptor.java index 95770e101..4a46bf5b1 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxInterceptor.java +++ b/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 entry : ((Map) 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); 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 088a98d26..24c4dc24e 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 @@ -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.