From efeee2b5a86ca2a5f03efd48fc2c1e596c57e2a2 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 6 Jun 2016 09:49:07 -0400 Subject: [PATCH 1/5] [JENKINS-26481] Fail the script meaningfully when it attempts to pass a CPS-transformed closure to a binary method. --- .../cps/GroovyClassLoaderWhitelist.java | 52 +++++++++++++++---- .../workflow/cps/SandboxContinuable.java | 3 +- .../plugins/workflow/SerializationTest.java | 25 +++++++++ 3 files changed, 68 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java index 02feb3825..ee6b6ffb3 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java @@ -1,24 +1,38 @@ package org.jenkinsci.plugins.workflow.cps; +import com.cloudbees.groovy.cps.impl.CpsClosure; import groovy.lang.GroovyClassLoader; import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist; import java.lang.reflect.Constructor; +import java.lang.reflect.Executable; import java.lang.reflect.Field; import java.lang.reflect.Method; +import org.codehaus.groovy.runtime.DefaultGroovyMethods; +import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException; +import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.EnumeratingWhitelist; +import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.ProxyWhitelist; /** * Allow any calls into the scripted compiled in the sandbox. * * IOW, allow the user to call his own methods. - * - * @author Kohsuke Kawaguchi */ class GroovyClassLoaderWhitelist extends Whitelist { + private final ClassLoader scriptLoader; - public GroovyClassLoaderWhitelist(GroovyClassLoader scriptLoader) { + /** + * {@link ProxyWhitelist} has an optimization which bypasses {@code permits*} calls + * when it detects {@link EnumeratingWhitelist} delegates, + * so we must do delegation manually. + * For the same reason, doing this check inside {@link CpsWhitelist} is tricky. + */ + private final Whitelist delegate; + + GroovyClassLoaderWhitelist(GroovyClassLoader scriptLoader, Whitelist delegate) { this.scriptLoader = scriptLoader; + this.delegate = delegate; } private boolean permits(Class declaringClass) { @@ -30,30 +44,48 @@ private boolean permits(Class declaringClass) { } @Override public boolean permitsMethod(Method method, Object receiver, Object[] args) { - return permits(method.getDeclaringClass()); + return checkJenkins26481(args, method) || delegate.permitsMethod(method, receiver, args); } @Override public boolean permitsConstructor(Constructor constructor, Object[] args) { - return permits(constructor.getDeclaringClass()); + return checkJenkins26481(args, constructor) || delegate.permitsConstructor(constructor, args); } @Override public boolean permitsStaticMethod(Method method, Object[] args) { - return permits(method.getDeclaringClass()); + return checkJenkins26481(args, method) || delegate.permitsStaticMethod(method, args); } @Override public boolean permitsFieldGet(Field field, Object receiver) { - return permits(field.getDeclaringClass()); + return permits(field.getDeclaringClass()) || delegate.permitsFieldGet(field, receiver); } @Override public boolean permitsFieldSet(Field field, Object receiver, Object value) { - return permits(field.getDeclaringClass()); + return permits(field.getDeclaringClass()) || delegate.permitsFieldSet(field, receiver, value); } @Override public boolean permitsStaticFieldGet(Field field) { - return permits(field.getDeclaringClass()); + return permits(field.getDeclaringClass()) || delegate.permitsStaticFieldGet(field); } @Override public boolean permitsStaticFieldSet(Field field, Object value) { - return permits(field.getDeclaringClass()); + return permits(field.getDeclaringClass()) || delegate.permitsStaticFieldSet(field, value); + } + + /** + * Blocks accesses to some {@link DefaultGroovyMethods}, or any other binary method taking a closure, until JENKINS-26481 is fixed. + * Only improves diagnostics for scripts using the sandbox. + * Note that we do not simply return false for these cases, as that would throw {@link RejectedAccessException} without a meaningful explanation. + */ + private boolean checkJenkins26481(Object[] args, Executable method) throws UnsupportedOperationException { + if (permits(method.getDeclaringClass())) { // fine for source-defined methods to take closures + return true; + } + for (Object arg : args) { + if (arg instanceof CpsClosure) { + throw new UnsupportedOperationException("Calling " + method + " on a CPS-transformed closure is not yet supported (JENKINS-26481); encapsulate in a @NonCPS method, or use Java-style loops"); + } + } + return false; } + } diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java index 3d6713e82..e60a7375f 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java @@ -4,7 +4,6 @@ import com.cloudbees.groovy.cps.Outcome; import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException; import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox; -import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.ProxyWhitelist; import org.jenkinsci.plugins.scriptsecurity.scripts.ApprovalContext; import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval; @@ -37,7 +36,7 @@ public Outcome call() { } return outcome; } - }, new ProxyWhitelist(new GroovyClassLoaderWhitelist(thread.group.getExecution().getShell().getClassLoader()),CpsWhitelist.get())); + }, new GroovyClassLoaderWhitelist(thread.group.getExecution().getShell().getClassLoader(), CpsWhitelist.get())); } catch (RuntimeException e) { throw e; } catch (Exception e) { diff --git a/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java b/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java index 3d91df21e..3ce7ae6fd 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java @@ -233,6 +233,31 @@ public void stop(Throwable cause) throws Exception { }); } + @Issue("JENKINS-26481") + @Test public void eachClosureNonCps() { + story.addStep(new Statement() { + @Override public void evaluate() throws Throwable { + p = jenkins().createProject(WorkflowJob.class, "demo"); + p.setDefinition(new CpsFlowDefinition( + "@NonCPS def fine() {\n" + + " def text = ''\n" + + " ['a', 'b', 'c'].each {it -> text += it}\n" + + " text\n" + + "}\n" + + "def takesMyOwnClosure(body) {\n" + + " node {\n" + + " echo body()\n" + + " }\n" + + "}\n" + + "takesMyOwnClosure {\n" + + " fine()\n" + + "}\n", true)); + b = story.j.assertBuildStatusSuccess(p.scheduleBuild2(0)); + story.j.assertLogContains("abc", b); + } + }); + } + @Ignore("TODO JENKINS-31314: calls writeFile just once, echoes null (i.e., return value of writeFile), then succeeds") @Test public void nonCpsContinuable() { story.addStep(new Statement() { From 6ad4d04e3df379836cfa8e32072007ab8e59c3a7 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 6 Jun 2016 09:56:57 -0400 Subject: [PATCH 2/5] [JENKINS-26481] Limit the error to cases where the binary method actually requested a Closure, and is thus likely to try to call it. --- .../plugins/workflow/cps/GroovyClassLoaderWhitelist.java | 6 ++++-- .../org/jenkinsci/plugins/workflow/SerializationTest.java | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java index ee6b6ffb3..dc55b4304 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java @@ -1,6 +1,7 @@ package org.jenkinsci.plugins.workflow.cps; import com.cloudbees.groovy.cps.impl.CpsClosure; +import groovy.lang.Closure; import groovy.lang.GroovyClassLoader; import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist; @@ -80,8 +81,9 @@ private boolean checkJenkins26481(Object[] args, Executable method) throws Unsup if (permits(method.getDeclaringClass())) { // fine for source-defined methods to take closures return true; } - for (Object arg : args) { - if (arg instanceof CpsClosure) { + Class[] parameterTypes = method.getParameterTypes(); + for (int i = 0; i < args.length; i++) { + if (args[i] instanceof CpsClosure && parameterTypes.length > i && parameterTypes[i] == Closure.class) { throw new UnsupportedOperationException("Calling " + method + " on a CPS-transformed closure is not yet supported (JENKINS-26481); encapsulate in a @NonCPS method, or use Java-style loops"); } } diff --git a/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java b/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java index 3ce7ae6fd..e78f67f8c 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java @@ -237,6 +237,7 @@ public void stop(Throwable cause) throws Exception { @Test public void eachClosureNonCps() { story.addStep(new Statement() { @Override public void evaluate() throws Throwable { + ScriptApproval.get().approveSignature("staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods plus java.util.Collection java.lang.Object"); p = jenkins().createProject(WorkflowJob.class, "demo"); p.setDefinition(new CpsFlowDefinition( "@NonCPS def fine() {\n" + @@ -246,7 +247,9 @@ public void stop(Throwable cause) throws Exception { "}\n" + "def takesMyOwnClosure(body) {\n" + " node {\n" + - " echo body()\n" + + " def list = []\n" + + " list += body\n" + + " echo list[0]()\n" + " }\n" + "}\n" + "takesMyOwnClosure {\n" + From 0d2af49117df1f3d2a2967812681a02f96dce000 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 6 Jun 2016 10:29:18 -0400 Subject: [PATCH 3/5] java.lang.reflect.Executable does not exist in Java 7. --- .../workflow/cps/GroovyClassLoaderWhitelist.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java index dc55b4304..0aeaa0118 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java @@ -6,8 +6,8 @@ import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist; import java.lang.reflect.Constructor; -import java.lang.reflect.Executable; import java.lang.reflect.Field; +import java.lang.reflect.Member; import java.lang.reflect.Method; import org.codehaus.groovy.runtime.DefaultGroovyMethods; import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException; @@ -45,15 +45,15 @@ private boolean permits(Class declaringClass) { } @Override public boolean permitsMethod(Method method, Object receiver, Object[] args) { - return checkJenkins26481(args, method) || delegate.permitsMethod(method, receiver, args); + return checkJenkins26481(args, method, method.getParameterTypes()) || delegate.permitsMethod(method, receiver, args); } @Override public boolean permitsConstructor(Constructor constructor, Object[] args) { - return checkJenkins26481(args, constructor) || delegate.permitsConstructor(constructor, args); + return checkJenkins26481(args, constructor, constructor.getParameterTypes()) || delegate.permitsConstructor(constructor, args); } @Override public boolean permitsStaticMethod(Method method, Object[] args) { - return checkJenkins26481(args, method) || delegate.permitsStaticMethod(method, args); + return checkJenkins26481(args, method, method.getParameterTypes()) || delegate.permitsStaticMethod(method, args); } @Override public boolean permitsFieldGet(Field field, Object receiver) { @@ -77,11 +77,10 @@ private boolean permits(Class declaringClass) { * Only improves diagnostics for scripts using the sandbox. * Note that we do not simply return false for these cases, as that would throw {@link RejectedAccessException} without a meaningful explanation. */ - private boolean checkJenkins26481(Object[] args, Executable method) throws UnsupportedOperationException { + private boolean checkJenkins26481(Object[] args, /* TODO Java 8: just take Executable */ Member method, Class[] parameterTypes) throws UnsupportedOperationException { if (permits(method.getDeclaringClass())) { // fine for source-defined methods to take closures return true; } - Class[] parameterTypes = method.getParameterTypes(); for (int i = 0; i < args.length; i++) { if (args[i] instanceof CpsClosure && parameterTypes.length > i && parameterTypes[i] == Closure.class) { throw new UnsupportedOperationException("Calling " + method + " on a CPS-transformed closure is not yet supported (JENKINS-26481); encapsulate in a @NonCPS method, or use Java-style loops"); From ae55312c6bdbda1d10f7c1b781d98136250212a3 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 9 Jun 2016 12:13:45 -0400 Subject: [PATCH 4/5] @oleg-nenashev requested that a PR comment be moved into Javadoc. --- .../jenkinsci/plugins/workflow/SerializationTest.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java b/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java index e83123fa5..b540a47d9 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java @@ -1,5 +1,6 @@ package org.jenkinsci.plugins.workflow; +import groovy.lang.Closure; import hudson.model.Result; import hudson.slaves.DumbSlave; import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval; @@ -234,6 +235,15 @@ public void stop(Throwable cause) throws Exception { }); } + /** + * Verifies that we are not throwing {@link UnsupportedOperationException} too aggressively. + * In particular: + *
    + *
  • on non-CPS-transformed {@link Closure}s + *
  • on closures passed to methods defined in Pipeline script + *
  • on closures passed to methods which did not declare {@link Closure} as a parameter type and so presumably are not going to try to call them + *
+ */ @Issue("JENKINS-26481") @Test public void eachClosureNonCps() { story.addStep(new Statement() { From 4266f0ce6b1d1eb07fd4c273592d938cf95bfd23 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 9 Jun 2016 16:24:06 -0400 Subject: [PATCH 5/5] =?UTF-8?q?[JENKINS-26481]=20Verifying=20that=20we=20c?= =?UTF-8?q?an=20still=20pass=20closures=20to=20functions=20defined=20in=20?= =?UTF-8?q?load=E2=80=99d=20scripts.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../workflow/cps/steps/RestartingLoadStepTest.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/steps/RestartingLoadStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/steps/RestartingLoadStepTest.java index 016e4184e..1513b89d7 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/steps/RestartingLoadStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/steps/RestartingLoadStepTest.java @@ -7,9 +7,11 @@ import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep; import static org.junit.Assert.*; +import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; import org.junit.runners.model.Statement; +import org.jvnet.hudson.test.BuildWatcher; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.RestartableJenkinsRule; @@ -19,6 +21,8 @@ * @author Kohsuke Kawaguchi */ public class RestartingLoadStepTest { + + @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher(); @Rule public RestartableJenkinsRule story = new RestartableJenkinsRule(); @Inject @@ -34,8 +38,8 @@ public void persistenceOfLoadedScripts() throws Exception { WorkflowJob p = jenkins.createProject(WorkflowJob.class, "p"); jenkins.getWorkspaceFor(p).child("test.groovy").write( "def answer(i) { return i*2; }\n" + - "def foo() {\n" + - " def i=21;\n" + + "def foo(body) {\n" + + " def i = body()\n" + " semaphore 'watchA'\n" + " return answer(i);\n" + "}\n" + @@ -44,7 +48,7 @@ public void persistenceOfLoadedScripts() throws Exception { "node {\n" + " println 'started'\n" + " def o = load 'test.groovy'\n" + - " println 'o=' + o.foo();\n" + + " println 'o=' + o.foo({21})\n" + "}")); // get the build going