Merge branch 'master' of https://github.com/jenkinsci/script-security…

…-plugin into nullability_annotations

pom.xml

@@ -51,7 +51,7 @@
       <dependency>
         <groupId>io.jenkins.tools.bom</groupId>
         <artifactId>bom-2.346.x</artifactId>
-        <version>1382.v7d694476f340</version>
+        <version>1742.vb_70478c1b_25f</version>
         <scope>import</scope>
         <type>pom</type>
       </dependency>
@@ -62,7 +62,7 @@
     <dependency>
       <groupId>org.kohsuke</groupId>
       <artifactId>groovy-sandbox</artifactId>
-      <version>1.31</version>
+      <version>1.32</version>
       <exclusions>
         <exclusion>
           <groupId>org.codehaus.groovy</groupId>

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/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java

@@ -352,6 +352,13 @@ private static void cleanUpClassHelperCache(@NonNull Class<?> clazz) throws Exce
     }
 
     private static void cleanUpObjectStreamClassCaches(@NonNull Class<?> clazz) throws Exception {
+      int releaseVersion = JavaSpecificationVersion.forCurrentJVM().toReleaseVersion();
+      VersionNumber javaVersion = new VersionNumber(System.getProperty("java.version"));
+      if ((releaseVersion < 11)
+              || (releaseVersion == 11 && javaVersion.isOlderThan(new VersionNumber("11.0.16")))
+              || (releaseVersion > 11 && releaseVersion < 17)
+              || (releaseVersion == 17 && javaVersion.isOlderThan(new VersionNumber("17.0.4")))
+              || (releaseVersion == 18 && javaVersion.isOlderThan(new VersionNumber("18.0.2")))) {
         Class<?> cachesC = Class.forName("java.io.ObjectStreamClass$Caches");
         for (String cacheFName : new String[] {"localDescs", "reflectors"}) {
             Field cacheF = cachesC.getDeclaredField(cacheFName);
@@ -371,6 +378,7 @@ private static void cleanUpObjectStreamClassCaches(@NonNull Class<?> clazz) thro
                 }
             }
         }
+      }
     }
 
     /** @deprecated use {@link #evaluate(ClassLoader, Binding, TaskListener)} */

src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/AclAwareWhitelist.java

@@ -33,10 +33,10 @@
 import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist;
 
 /**
- * Delegating whitelist which allows certain calls to be made only when a non-{@link ACL#SYSTEM} user is making them.
+ * Delegating whitelist which allows certain calls to be made only when a non-{@link ACL#SYSTEM2} user is making them.
  * <p>First there is a list of unrestricted signatures; these can always be run.
  * <p>Then there is a (probably much smaller) list of restricted signatures.
- * These can be run only when the {@linkplain Jenkins#getAuthentication current user} is a real user or even {@linkplain Jenkins#ANONYMOUS}, but not when {@link ACL#SYSTEM}.
+ * These can be run only when the {@linkplain Jenkins#getAuthentication2 current user} is a real user or even {@linkplain Jenkins#ANONYMOUS2}, but not when {@link ACL#SYSTEM2}.
  * Restricted methods should be limited to those which actually perform a permissions check, typically using {@link ACL#checkPermission}.
  * Allowing the system pseudo-user to run these would be dangerous, since we do not know “on whose behalf” a script is running, and this “user” is permitted to do anything.
  */
@@ -58,7 +58,7 @@ public AclAwareWhitelist(Whitelist unrestricted, Whitelist restricted) {
     }
 
     private static boolean authenticated() {
-        return !ACL.SYSTEM.equals(Jenkins.getAuthentication());
+        return !ACL.SYSTEM2.equals(Jenkins.getAuthentication2());
     }
 
     @Override public boolean permitsMethod(@NonNull Method method, @NonNull Object receiver, @NonNull Object[] args) {

src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ApprovalContext.java

@@ -58,19 +58,19 @@ public static ApprovalContext create() {
 
     /**
      * Creates a context with a specified user ID.
-     * ({@link ACL#SYSTEM} is automatically ignored.)
+     * ({@link ACL#SYSTEM2} is automatically ignored.)
      */
     public ApprovalContext withUser(@CheckForNull String user) {
-        return new ApprovalContext(ACL.SYSTEM.getName().equals(user) ? null : user, item, key);
+        return new ApprovalContext(ACL.SYSTEM2.getName().equals(user) ? null : user, item, key);
     }
 
     /**
      * Creates a context with the user associated with the current thread.
-     * ({@link ACL#SYSTEM} is automatically ignored, but the user might be {@link Jenkins#ANONYMOUS}.)
+     * ({@link ACL#SYSTEM2} is automatically ignored, but the user might be {@link Jenkins#ANONYMOUS2}.)
      */
     public ApprovalContext withCurrentUser() {
         User u = User.current();
-        return withUser(u != null ? u.getId() : Jenkins.ANONYMOUS.getName());
+        return withUser(u != null ? u.getId() : Jenkins.ANONYMOUS2.getName());
     }
 
     /**

src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java

@@ -26,6 +26,7 @@
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.model.BallColor;
+import hudson.security.ACLContext;
 import jenkins.model.GlobalConfiguration;
 import jenkins.model.GlobalConfigurationCategory;
 import jenkins.util.SystemProperties;
@@ -77,8 +78,6 @@
 import edu.umd.cs.findbugs.annotations.NonNull;
 import jenkins.model.Jenkins;
 import net.sf.json.JSON;
-import org.acegisecurity.context.SecurityContext;
-import org.acegisecurity.context.SecurityContextHolder;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
@@ -581,7 +580,7 @@ boolean isEmpty() {
      * It should also be called from a {@code readResolve} method (which may then simply return {@code this}),
      * so that administrators can for example POST to {@code config.xml} and have their scripts be considered approved.
      * <p>If the script has already been approved, this does nothing.
-     * Otherwise, if this user has the {@link Jenkins#ADMINISTER} permission (and is not {@link ACL#SYSTEM})
+     * Otherwise, if this user has the {@link Jenkins#ADMINISTER} permission (and is not {@link ACL#SYSTEM2})
      * and a corresponding flag is set to {@code true}, or Jenkins is running without security, it is added to the approved list.
      * Otherwise, it is added to the pending list.
      * @param script the text of a possibly novel script
@@ -594,7 +593,7 @@ public synchronized String configuring(@NonNull String script, @NonNull Language
         final ConversionCheckResult result = checkAndConvertApprovedScript(script, language);
         if (!result.approved) {
             if (!Jenkins.get().isUseSecurity() || 
-                    ((Jenkins.getAuthentication() != ACL.SYSTEM && Jenkins.get().hasPermission(Jenkins.ADMINISTER)) 
+                    ((Jenkins.getAuthentication2() != ACL.SYSTEM2 && Jenkins.get().hasPermission(Jenkins.ADMINISTER))
                             && (ADMIN_AUTO_APPROVAL_ENABLED || approveIfAdmin))) {
                 approvedScriptHashes.add(result.newHash);
                 //Pending scripts are not stored with a precalculated hash, so no need to remove any old hashes
@@ -677,7 +676,7 @@ public synchronized void configuring(@NonNull ClasspathEntry entry, @NonNull App
             boolean shouldSave = false;
             PendingClasspathEntry pcp = new PendingClasspathEntry(result.newHash, url, context);
             if (!Jenkins.get().isUseSecurity() ||
-                    ((Jenkins.getAuthentication() != ACL.SYSTEM && Jenkins.get().hasPermission(Jenkins.ADMINISTER))
+                    ((Jenkins.getAuthentication2() != ACL.SYSTEM2 && Jenkins.get().hasPermission(Jenkins.ADMINISTER))
                             && (ADMIN_AUTO_APPROVAL_ENABLED || entry.isShouldBeApproved() || !StringUtils.equals(entry.getOldPath(), entry.getPath())))) {
                 LOG.log(Level.FINE, "Classpath entry {0} ({1}) is approved as configured with ADMINISTER permission.", new Object[] {url, result.newHash});
                 ApprovedClasspathEntry acp = new ApprovedClasspathEntry(result.newHash, url);
@@ -899,7 +898,7 @@ public synchronized String[] getAclApprovedSignatures() {
 
     @DataBoundSetter
     public synchronized void setApprovedScriptHashes(String[] scriptHashes) throws IOException {
-        Jenkins.get().checkPermission(Jenkins.RUN_SCRIPTS);
+        Jenkins.get().checkPermission(Jenkins.ADMINISTER);
         approvedScriptHashes.clear();
         for (String scriptHash : scriptHashes) {
             if (StringUtils.isNotEmpty(scriptHash)) {
@@ -971,13 +970,11 @@ public Set<PendingScript> getPendingScripts() {
             removePendingScript(hash);
             save();
         }
-        SecurityContext orig = ACL.impersonate(ACL.SYSTEM);
-        try {
+
+        try (ACLContext ctx = ACL.as2(ACL.SYSTEM2)) {
             for (ApprovalListener listener : ExtensionList.lookup(ApprovalListener.class)) {
                 listener.onApproved(hash);
             }
-        } finally {
-            SecurityContextHolder.setContext(orig);
         }
     }
 
@@ -1198,13 +1195,10 @@ public JSON approveClasspathEntry(String hash) throws IOException {
             }
         }
         if (url != null) {
-            SecurityContext orig = ACL.impersonate(ACL.SYSTEM);
-            try {
+            try (ACLContext ctx = ACL.as2(ACL.SYSTEM2)) {
                 for (ApprovalListener listener : ExtensionList.lookup(ApprovalListener.class)) {
                     listener.onApprovedClasspathEntry(hash, url);
                 }
-            } finally {
-                SecurityContextHolder.setContext(orig);
             }
         }
         return getClasspathRenderInfo();

src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalLink.java

@@ -77,10 +77,9 @@
         return Jenkins.ADMINISTER;
     }
 
-    // TODO: Override `getCategory` instead using `Category.SECURITY` when minimum core version is 2.226+, see https://github.com/jenkinsci/jenkins/commit/6de7e5fc7f6fb2e2e4cb342461788f97e3dfd8f6.
     @NonNull
-    protected String getCategoryName() {
-        return "SECURITY";
+    @Override
+    public Category getCategory() {
+        return Category.SECURITY;
     }
-
 }

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

@@ -385,6 +385,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 {
@@ -670,7 +684,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")
@@ -1377,6 +1391,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",
@@ -1704,6 +1728,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.

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

@@ -28,6 +28,7 @@
 import com.gargoylesoftware.htmlunit.html.HtmlInput;
 import groovy.lang.Binding;
 import hudson.remoting.Which;
+import hudson.security.ACLContext;
 import org.apache.tools.ant.AntClassLoader;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
 import org.jenkinsci.plugins.scriptsecurity.scripts.ClasspathEntry;
@@ -52,7 +53,6 @@
 import java.util.List;
 import java.util.Set;
 import jenkins.model.Jenkins;
-import jenkins.security.NotReallyRoleSensitiveCallable;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.lang.StringUtils;
 import org.apache.tools.ant.DirectoryScanner;
@@ -1011,24 +1011,18 @@ public void testSandboxClassResolution() throws Exception {
         p.getPublishersList().add(new TestGroovyRecorder(
                 new SecureGroovyScript("class Test { public void finalize() { } }; null", false, null)));
 
-        ACL.impersonate(User.getById("dev", true).impersonate(), new NotReallyRoleSensitiveCallable<Void, Exception>() {
-            public Void call() throws Exception {
-                FreeStyleBuild b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0));
-                r.assertLogContains("UnapprovedUsageException", b);
-                return null;
-            }
-        });
+        try (ACLContext ctx = ACL.as(User.getById("dev", true))) {
+            FreeStyleBuild b = r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0));
+            r.assertLogContains("UnapprovedUsageException", b);
+        }
 
         Set<ScriptApproval.PendingScript> ps = ScriptApproval.get().getPendingScripts();
         assertEquals(1, ps.size());
         ScriptApproval.get().approveScript(ps.iterator().next().getHash());
 
-        ACL.impersonate(User.getById("dev", true).impersonate(), new NotReallyRoleSensitiveCallable<Void, Exception>() {
-            public Void call() throws Exception {
-                r.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0));
-                return null;
-            }
-        });
+        try (ACLContext ctx = ACL.as(User.getById("dev", true))) {
+            r.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0));
+        }
     }
 
     @Issue("SECURITY-1292")