[SECURITY-2824]

pom.xml

@@ -62,7 +62,7 @@
     <dependency>
       <groupId>org.kohsuke</groupId>
       <artifactId>groovy-sandbox</artifactId>
-      <version>1.27</version>
+      <version>1.30</version>
       <exclusions>
         <exclusion>
           <groupId>org.codehaus.groovy</groupId>

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

@@ -5,6 +5,7 @@
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 
 /**
  * {@link Whitelist} that allows everything defined from a specific classloader.
@@ -23,30 +24,81 @@ private boolean permits(Class<?> declaringClass) {
     }
 
     @Override public boolean permitsMethod(Method method, Object receiver, Object[] args) {
-        return permits(method.getDeclaringClass());
+        return permits(method.getDeclaringClass()) && !isIllegalSyntheticMethod(method);
     }
 
     @Override public boolean permitsConstructor(Constructor<?> constructor, Object[] args) {
-        return permits(constructor.getDeclaringClass());
+        return permits(constructor.getDeclaringClass()) && !isIllegalSyntheticConstructor(constructor);
     }
 
     @Override public boolean permitsStaticMethod(Method method, Object[] args) {
-        return permits(method.getDeclaringClass());
+        return permits(method.getDeclaringClass()) && !isIllegalSyntheticMethod(method);
     }
 
     @Override public boolean permitsFieldGet(Field field, Object receiver) {
-        return permits(field.getDeclaringClass());
+        return permits(field.getDeclaringClass()) && !isIllegalSyntheticField(field);
     }
 
     @Override public boolean permitsFieldSet(Field field, Object receiver, Object value) {
-        return permits(field.getDeclaringClass());
+        return permits(field.getDeclaringClass()) && !isIllegalSyntheticField(field);
     }
 
     @Override public boolean permitsStaticFieldGet(Field field) {
-        return permits(field.getDeclaringClass());
+        return permits(field.getDeclaringClass()) && !isIllegalSyntheticField(field);
     }
 
     @Override public boolean permitsStaticFieldSet(Field field, Object value) {
-        return permits(field.getDeclaringClass());
+        return permits(field.getDeclaringClass()) && !isIllegalSyntheticField(field);
+    }
+
+    /**
+     * Checks whether a given field was synthetically created by the Groovy compiler and should be inaccessible even if
+     * it is declared by a class defined by the specified class loader.
+     */
+    private static boolean isIllegalSyntheticField(Field field) {
+        if (!field.isSynthetic()) {
+            return false;
+        }
+        Class<?> declaringClass = field.getDeclaringClass();
+        Class<?> enclosingClass = declaringClass.getEnclosingClass();
+        if (field.getType() == enclosingClass && field.getName().startsWith("this$")) {
+            // Synthetic field added to inner classes to reference the outer class.
+            return false;
+        } else if (declaringClass.isEnum() && Modifier.isStatic(field.getModifiers()) && field.getName().equals("$VALUES")) {
+            // Synthetic field added by Groovy to enum classes to hold the enum constants.
+            return false;
+        }
+        return true;
+    }
+
+    /**
+     * Checks whether a given method was synthetically created by the Groovy compiler and should be inaccessible even
+     * if it is declared by a class defined by the specified class loader.
+     */
+    private static boolean isIllegalSyntheticMethod(Method method) {
+        if (!method.isSynthetic()) {
+            return false;
+        } else if (Modifier.isStatic(method.getModifiers()) && method.getDeclaringClass().isEnum() && method.getName().equals("$INIT")) {
+            // Synthetic method added by Groovy to enum classes used to initialize the enum constants.
+            return false;
+        }
+        return true;
+    }
+
+    /**
+     * Checks whether a given constructor was created by the Groovy compiler (or groovy-sandbox) and
+     * should be inaccessible even if it is declared by a class defined by the specified class loader.
+     */
+    private static boolean isIllegalSyntheticConstructor(Constructor constructor) {
+        if (!constructor.isSynthetic()) {
+            return false;
+        }
+        Class<?> declaringClass = constructor.getDeclaringClass();
+        Class<?> enclosingClass = declaringClass.getEnclosingClass();
+        if (enclosingClass != null && constructor.getParameters().length > 0 && constructor.getParameterTypes()[0] == enclosingClass) {
+            // Synthetic constructor added by Groovy to anonymous classes.
+            return false;
+        }
+        return true;
     }
 }

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

@@ -31,9 +31,12 @@
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
+import java.util.ArrayList;
 import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.Predicate;
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import org.apache.commons.lang.ClassUtils;
@@ -198,6 +201,41 @@ private static boolean isInstancePrimitive(@NonNull Class<?> type, @NonNull Obje
         return findMatchingMethod(receiver, method, args);
     }
 
+    /**
+     * Like {@link #method}, but returns all methods with the given name that match the given predicate.
+     */
+    public static List<Method> methods(@NonNull Object receiver, @NonNull String method, Predicate<Method> filter) {
+        Set<Class<?>> types = types(receiver);
+        if (types.contains(GroovyInterceptable.class) && !"invokeMethod".equals(method)) {
+            return methods(receiver, "invokeMethod", m -> true);
+        }
+        List<Method> candidates = new ArrayList<>();
+        for (Class<?> c : types) {
+            for (Method candidate : c.getDeclaredMethods()) {
+                if (candidate.getName().equals(method) && filter.test(candidate)) {
+                    candidates.add(candidate);
+                }
+            }
+        }
+        if (receiver instanceof GString) { // cf. GString.invokeMethod
+            candidates.addAll(methods(String.class, method, filter));
+        }
+        return candidates;
+    }
+
+    /**
+     * Like {@link #staticMethod}, but returns all methods with the given name that match the given predicate.
+     */
+    public static List<Method> staticMethods(@NonNull Class<?> receiver, @NonNull String method, Predicate<Method> filter) {
+        List<Method> candidates = new ArrayList<>();
+        for (Method candidate : receiver.getDeclaredMethods()) {
+            if (candidate.getName().equals(method) && filter.test(candidate)) {
+                candidates.add(candidate);
+            }
+        }
+        return candidates;
+    }
+
     private static Method findMatchingMethod(@NonNull Class<?> receiver, @NonNull String method, @NonNull Object[] args) {
         Method candidate = null;
 

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

@@ -26,20 +26,28 @@
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import groovy.grape.GrabAnnotationTransformation;
+import groovy.lang.Binding;
 import groovy.lang.GroovyClassLoader;
+import groovy.lang.GroovyCodeSource;
+import groovy.lang.GroovyObject;
+import groovy.lang.GroovyRuntimeException;
 import groovy.lang.GroovyShell;
 import static groovy.lang.GroovyShell.DEFAULT_CODE_BASE;
+import groovy.lang.MissingPropertyException;
 import groovy.lang.Script;
 import hudson.ExtensionList;
 import hudson.model.RootAction;
 import hudson.model.TaskListener;
 import hudson.util.FormValidation;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.security.CodeSource;
 import java.security.cert.Certificate;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.Map;
 import java.util.concurrent.Callable;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -49,6 +57,8 @@
 import org.codehaus.groovy.control.CompilationUnit;
 import org.codehaus.groovy.control.CompilerConfiguration;
 import org.codehaus.groovy.control.Phases;
+import org.codehaus.groovy.runtime.InvokerHelper;
+import org.codehaus.groovy.syntax.Types;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.ProxyWhitelist;
@@ -58,6 +68,7 @@
 import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApprovalNote;
 import org.kohsuke.groovy.sandbox.GroovyInterceptor;
 import org.kohsuke.groovy.sandbox.SandboxTransformer;
+import org.kohsuke.groovy.sandbox.impl.Checker;
 
 /**
  * Allows Groovy scripts (including Groovy Templates) to be run inside a sandbox.
@@ -143,24 +154,107 @@ public Scope enter() {
     @FunctionalInterface
     public interface Scope extends AutoCloseable {
 
+        /**
+         * Variant of {@link GroovyShell#parse(String)} that intercepts potentially unsafe calls when the script is created.
+         *
+         * <p>{@link GroovySandbox#runScript(GroovyShell, String)} should be used instead of this method in most cases.
+         */
+        default Script parse(GroovyShell shell, GroovyCodeSource codeSource) {
+            Class<?> scriptClass = shell.getClassLoader().parseClass(codeSource, false);
+            Script script = checkedCreateScript(scriptClass, shell.getContext());
+            return script;
+        }
+
         @Override void close();
 
     }
 
     /**
      * Compiles and runs a script within the sandbox.
      * @param shell the shell to be used; see {@link #createSecureCompilerConfiguration} and similar methods
-     * @param script the script to run
+     * @param scriptText the script to run
      * @return the return value of the script
      */
-    public Object runScript(@NonNull GroovyShell shell, @NonNull String script) {
+    public Object runScript(@NonNull GroovyShell shell, @NonNull String scriptText) {
         GroovySandbox derived = new GroovySandbox().
             withApprovalContext(context).
             withTaskListener(listener).
             withWhitelist(new ProxyWhitelist(new ClassLoaderWhitelist(shell.getClassLoader()), whitelist()));
         try (Scope scope = derived.enter()) {
-            return shell.parse(script).run();
+            // GroovyShell does not expose any public APIs that allow us to access the generated Script class before InvokerHelper.createScript is called.
+            String scriptFileName = "Script0.groovy";
+            try {
+                Method generateScriptName = shell.getClass().getDeclaredMethod("generateScriptName");
+                generateScriptName.setAccessible(true); // Method is protected.
+                scriptFileName = (String) generateScriptName.invoke(shell);
+            } catch (ReflectiveOperationException e) {
+                LOGGER.log(Level.WARNING, null, e);
+            }
+            GroovyCodeSource codeSource = new GroovyCodeSource(scriptText, scriptFileName, DEFAULT_CODE_BASE);
+            Script script = scope.parse(shell, codeSource);
+            return script.run();
+        }
+    }
+
+    /**
+     * Variant of {@link InvokerHelper#createScript} that intercepts potentially unsafe reflective behaviors.
+     *
+     * <p>{@link GroovySandbox#runScript(GroovyShell, String)} or {@link Scope#parse} should be used instead of this method in most cases.
+     *
+     * @see InvokerHelper#createScript
+     */
+    private static Script checkedCreateScript(Class<?> scriptClass, Binding context) {
+        Script script;
+        try {
+            if (Script.class.isAssignableFrom(scriptClass)) {
+                try {
+                    script = InvokerHelper.newScript(scriptClass, context);
+                } catch (InvocationTargetException e) {
+                    throw e.getCause(); // Typically RejectedAccessException.
+                }
+            } else {
+                // TODO: Could we just throw a SecurityException instead and tell users to extend Script or not have a
+                // class definition as the only top-level content in their script?
+                final GroovyObject object = (GroovyObject) scriptClass.newInstance();
+                // it could just be a class, so let's wrap it in a Script
+                // wrapper; though the bindings will be ignored
+                script = new Script(context) {
+                    public Object run() {
+                        Object[] argsToPass = new Object[0];
+                        try {
+                            Object args = getProperty("args");
+                            if (args instanceof String[]) {
+                                argsToPass = (String[])args;
+                            }
+                        } catch (MissingPropertyException e) {
+                            // They'll get empty args since none exist in the context.
+                        }
+                        try {
+                            Checker.checkedCall(object, false, false, "main", argsToPass);
+                        } catch (Throwable t) {
+                            throw new GroovyRuntimeException(t);
+                        }
+                        return null;
+                    }
+                };
+                Map variables = context.getVariables();
+                for (Object o : variables.entrySet()) {
+                    Map.Entry entry = (Map.Entry) o;
+                    String key = entry.getKey().toString();
+                    // assume underscore variables are for the wrapper script
+                    try {
+                        Checker.checkedSetProperty(key.startsWith("_") ? script : object, key, true, false, Types.ASSIGN, entry.getValue());
+                    } catch (MissingPropertyException e) {
+                        // Swallow MissingPropertyException to match Groovy behavior in this case.
+                    }
+                }
+            }
+        } catch (Throwable e) {
+            throw new GroovyRuntimeException(
+                    "Failed to create Script instance for class: "
+                            + scriptClass + ". Reason: " + e, e);
         }
+        return script;
     }
 
     /**