From 28da8db47dd4c9d42647443d22e1e21603c6f7ff Mon Sep 17 00:00:00 2001
From: Yaroslav Afenkin <yaroslav.afenkin@gmail.com>
Date: Wed, 4 May 2022 15:04:19 -0400
Subject: [PATCH] [SECURITY-2450]
(cherry picked from commit 6bd4e8bf837ba1df4394efdd003c70bb1b9ac33f)
---
pom.xml | 2 +
.../workflow/cps/CpsFlowDefinition.java | 37 ++++-
.../cps/CpsFlowDefinition/config.jelly | 1 +
.../workflow/cps/CpsFlowDefinition2Test.java | 150 +++++++++++++++++-
.../cps/CpsFlowDefinitionRJRTest.java | 24 +++
5 files changed, 209 insertions(+), 5 deletions(-)
create mode 100644 src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionRJRTest.java
diff --git a/pom.xml b/pom.xml
index 588c938f3..3d6c73147 100644
--- a/pom.xml
+++ b/pom.xml
@@ -103,6 +103,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
+ <version>1.78.1</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
@@ -151,6 +152,7 @@
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-job</artifactId>
<scope>test</scope>
+ <version>2.41.1</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java
index 02287b613..8b34a17e0 100644
--- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java
+++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java
@@ -24,6 +24,7 @@
package org.jenkinsci.plugins.workflow.cps;
+import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import hudson.model.Action;
import hudson.model.Item;
@@ -33,6 +34,8 @@
import hudson.model.TaskListener;
import hudson.util.FormValidation;
import hudson.util.StreamTaskListener;
+import net.sf.json.JSONObject;
+import org.apache.commons.lang.StringUtils;
import org.jenkinsci.plugins.workflow.cps.persistence.PersistIn;
import org.jenkinsci.plugins.workflow.flow.DurabilityHintProvider;
import org.jenkinsci.plugins.workflow.flow.FlowDefinition;
@@ -80,7 +83,8 @@ public CpsFlowDefinition(String script) {
@DataBoundConstructor
public CpsFlowDefinition(String script, boolean sandbox) {
StaplerRequest req = Stapler.getCurrentRequest();
- this.script = sandbox ? script : ScriptApproval.get().configuring(script, GroovyLanguage.get(), ApprovalContext.create().withCurrentUser().withItemAsKey(req != null ? req.findAncestorObject(Item.class) : null));
+ this.script = sandbox ? script : ScriptApproval.get().configuring(script, GroovyLanguage.get(),
+ ApprovalContext.create().withCurrentUser().withItemAsKey(req != null ? req.findAncestorObject(Item.class) : null), req == null);
this.sandbox = sandbox;
}
@@ -123,14 +127,41 @@ public CpsFlowExecution create(FlowExecutionOwner owner, TaskListener listener,
@Extension
public static class DescriptorImpl extends FlowDefinitionDescriptor {
+ /* In order to fix SECURITY-2450 without causing significant UX regressions, we decided to continue to
+ * automatically approve scripts on save if the script was modified by an administrator. To make this possible,
+ * we added a new hidden input field to the config.jelly to track the pre-save version of the script. Since
+ * CpsFlowDefinition calls ScriptApproval.configuring in its @DataBoundConstructor, the normal way to handle
+ * things would be to add an oldScript parameter to the constructor and perform the relevant logic there.
+ *
+ * However, that would have compatibility implications for tools like JobDSL, since @DataBoundConstructor
+ * parameters are required. We cannot use a @DataBoundSetter with a corresponding field and getter to trivially
+ * make oldScript optional, because we would need to call ScriptApproval.configuring after all
+ * @DataBoundSetters have been invoked (rather than in the @DataBoundConstructor), which is why we use Descriptor.newInstance.
+ */
+ @Override
+ public FlowDefinition newInstance(@NonNull StaplerRequest req, @NonNull JSONObject formData) throws FormException {
+ CpsFlowDefinition cpsFlowDefinition = (CpsFlowDefinition) super.newInstance(req, formData);
+ if (!cpsFlowDefinition.sandbox && formData.get("oldScript") != null) {
+ String oldScript = formData.getString("oldScript");
+ boolean approveIfAdmin = !StringUtils.equals(oldScript, cpsFlowDefinition.script);
+ if (approveIfAdmin) {
+ ScriptApproval.get().configuring(cpsFlowDefinition.script, GroovyLanguage.get(),
+ ApprovalContext.create().withCurrentUser().withItemAsKey(req.findAncestorObject(Item.class)), true);
+ }
+ }
+ return cpsFlowDefinition;
+ }
+
@Override
public String getDisplayName() {
return "Pipeline script";
}
@RequirePOST
- public FormValidation doCheckScript(@QueryParameter String value, @QueryParameter boolean sandbox) {
- return sandbox ? FormValidation.ok() : ScriptApproval.get().checking(value, GroovyLanguage.get());
+ public FormValidation doCheckScript(@QueryParameter String value, @QueryParameter String oldScript,
+ @QueryParameter boolean sandbox) {
+ return sandbox ? FormValidation.ok() :
+ ScriptApproval.get().checking(value, GroovyLanguage.get(), !StringUtils.equals(oldScript, value));
}
@RequirePOST
diff --git a/src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly b/src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly
index 1e0ef0360..c075a1fd2 100644
--- a/src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly
+++ b/src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly
@@ -24,6 +24,7 @@
-->
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:st="jelly:stapler" xmlns:wfe="/org/jenkinsci/plugins/workflow/editor">
+ <input type="hidden" name="oldScript" value="${instance.script}"/>
<f:entry title="${%Script}" field="script">
<wfe:workflow-editor />
</f:entry>
diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java
index f09aaf4f9..ed22abecb 100644
--- a/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java
+++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java
@@ -25,19 +25,29 @@
package org.jenkinsci.plugins.workflow.cps;
import com.cloudbees.groovy.cps.CpsTransformer;
+import com.gargoylesoftware.htmlunit.html.HtmlCheckBoxInput;
+import com.gargoylesoftware.htmlunit.html.HtmlForm;
+import com.gargoylesoftware.htmlunit.html.HtmlInput;
+import com.gargoylesoftware.htmlunit.html.HtmlTextArea;
import hudson.Functions;
import hudson.model.Computer;
import hudson.model.Describable;
import hudson.model.Executor;
+import hudson.model.Item;
import hudson.model.Result;
import java.io.Serializable;
import java.util.Collections;
+import java.util.List;
import java.util.Set;
import java.util.logging.Level;
+
+import hudson.security.Permission;
import jenkins.model.Jenkins;
import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
+import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
+import org.jenkinsci.plugins.scriptsecurity.scripts.languages.GroovyLanguage;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.jenkinsci.plugins.workflow.steps.Step;
@@ -58,6 +68,7 @@
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.LoggerRule;
+import org.jvnet.hudson.test.MockAuthorizationStrategy;
import org.jvnet.hudson.test.TestExtension;
import org.kohsuke.stapler.DataBoundConstructor;
@@ -85,7 +96,7 @@ public void endlessRecursion() throws Exception {
WorkflowRun r = jenkins.assertBuildStatus(Result.FAILURE, job.scheduleBuild2(0).get());
jenkins.assertLogContains("look for unbounded recursion", r);
- Assert.assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty());
+ assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty());
for (Computer c : jenkins.jenkins.getComputers()) {
for (Executor ex : c.getExecutors()) {
@@ -113,7 +124,7 @@ public void endlessRecursionNonCPS() throws Exception {
// Should have failed with error about excessive recursion depth
WorkflowRun r = jenkins.assertBuildStatus(Result.FAILURE, job.scheduleBuild2(0).get());
- Assert.assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty());
+ assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty());
for (Computer c : jenkins.jenkins.getComputers()) {
for (Executor ex : c.getExecutors()) {
@@ -830,6 +841,141 @@ public void scriptInitializerCallsCpsTransformedMethod() throws Exception {
assertNull(Jenkins.get().getDescription());
}
+ @Issue("SECURITY-2450")
+ @Test
+ public void cpsScriptNonAdminConfiguration() throws Exception {
+ jenkins.jenkins.setSecurityRealm(jenkins.createDummySecurityRealm());
+
+ MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
+ mockStrategy.grant(Jenkins.READ).everywhere().to("devel");
+ for (Permission p : Item.PERMISSIONS.getPermissions()) {
+ mockStrategy.grant(p).everywhere().to("devel");
+ }
+ jenkins.jenkins.setAuthorizationStrategy(mockStrategy);
+
+ JenkinsRule.WebClient wcDevel = jenkins.createWebClient();
+ wcDevel.login("devel");
+
+ WorkflowJob p = jenkins.createProject(WorkflowJob.class);
+
+ HtmlForm config = wcDevel.getPage(p, "configure").getFormByName("config");
+ List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
+ // Get the last one, because previous ones might be from Lockable Resources during PCT.
+ HtmlTextArea script = scripts.get(scripts.size() - 1);
+ String groovy = "echo 'hi from cpsScriptNonAdminConfiguration'";
+ script.setText(groovy);
+
+ List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
+ // Get the last one, because previous ones might be from Lockable Resources during PCT.
+ HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
+ assertTrue(sandbox.isChecked());
+ sandbox.setChecked(false);
+
+ jenkins.submit(config);
+
+ assertEquals(1, ScriptApproval.get().getPendingScripts().size());
+ assertFalse(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get()));
+ }
+
+ @Issue("SECURITY-2450")
+ @Test
+ public void cpsScriptAdminConfiguration() throws Exception {
+ jenkins.jenkins.setSecurityRealm(jenkins.createDummySecurityRealm());
+
+ MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
+ mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin");
+ for (Permission p : Item.PERMISSIONS.getPermissions()) {
+ mockStrategy.grant(p).everywhere().to("admin");
+ }
+ jenkins.jenkins.setAuthorizationStrategy(mockStrategy);
+
+ JenkinsRule.WebClient admin = jenkins.createWebClient();
+ admin.login("admin");
+
+ WorkflowJob p = jenkins.createProject(WorkflowJob.class);
+
+ HtmlForm config = admin.getPage(p, "configure").getFormByName("config");
+ List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
+ // Get the last one, because previous ones might be from Lockable Resources during PCT.
+ HtmlTextArea script = scripts.get(scripts.size() - 1);
+ String groovy = "echo 'hi from cpsScriptAdminConfiguration'";
+ script.setText(groovy);
+
+ List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
+ // Get the last one, because previous ones might be from Lockable Resources during PCT.
+ HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
+ assertTrue(sandbox.isChecked());
+ sandbox.setChecked(false);
+
+ jenkins.submit(config);
+
+ assertTrue(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get()));
+ }
+
+ @Issue("SECURITY-2450")
+ @Test
+ public void cpsScriptAdminModification() throws Exception {
+ jenkins.jenkins.setSecurityRealm(jenkins.createDummySecurityRealm());
+
+ MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
+ mockStrategy.grant(Jenkins.READ).everywhere().to("devel");
+ mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin");
+ for (Permission p : Item.PERMISSIONS.getPermissions()) {
+ mockStrategy.grant(p).everywhere().to("devel");
+ mockStrategy.grant(p).everywhere().to("admin");
+ }
+ jenkins.jenkins.setAuthorizationStrategy(mockStrategy);
+
+ JenkinsRule.WebClient wc = jenkins.createWebClient();
+ wc.login("devel");
+
+ WorkflowJob p = jenkins.createProject(WorkflowJob.class);
+ String userGroovy = "echo 'hi from devel'";
+ String adminGroovy = "echo 'hi from admin'";
+
+ // initial configuration by user, script ends up in pending
+ {
+ HtmlForm config = wc.getPage(p, "configure").getFormByName("config");
+ List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
+ // Get the last one, because previous ones might be from Lockable Resources during PCT.
+ HtmlTextArea script = scripts.get(scripts.size() - 1);
+ script.setText(userGroovy);
+
+ List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
+ // Get the last one, because previous ones might be from Lockable Resources during PCT.
+ HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
+ assertTrue(sandbox.isChecked());
+ sandbox.setChecked(false);
+
+ jenkins.submit(config);
+
+ assertFalse(ScriptApproval.get().isScriptApproved(userGroovy, GroovyLanguage.get()));
+ }
+
+ wc.login("admin");
+
+ // modification by admin, script gets approved automatically
+ {
+ HtmlForm config = wc.getPage(p, "configure").getFormByName("config");
+ List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
+ // Get the last one, because previous ones might be from Lockable Resources during PCT.
+ HtmlTextArea script = scripts.get(scripts.size() - 1);
+ script.setText(adminGroovy);
+
+ List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
+ // Get the last one, because previous ones might be from Lockable Resources during PCT.
+ HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
+ assertFalse(sandbox.isChecked());
+
+ jenkins.submit(config);
+
+ // script content was modified by admin, so it should be approved upon save
+ // the one that had been submitted by the user previously stays in pending
+ assertTrue(ScriptApproval.get().isScriptApproved(adminGroovy, GroovyLanguage.get()));
+ assertFalse(ScriptApproval.get().isScriptApproved(userGroovy, GroovyLanguage.get()));
+ }
+ }
+
public static class UnsafeParameterStep extends Step implements Serializable {
private final UnsafeDescribable val;
@DataBoundConstructor
diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionRJRTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionRJRTest.java
new file mode 100644
index 000000000..f1ef48d35
--- /dev/null
+++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionRJRTest.java
@@ -0,0 +1,24 @@
+package org.jenkinsci.plugins.workflow.cps;
+
+import org.jenkinsci.plugins.workflow.job.WorkflowJob;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.RealJenkinsRule;
+
+public class CpsFlowDefinitionRJRTest {
+
+ @Rule
+ public RealJenkinsRule rjr = new RealJenkinsRule();
+
+ @Test
+ public void smokes() throws Throwable {
+ rjr.then(CpsFlowDefinitionRJRTest::doesItSmoke);
+ }
+
+ private static void doesItSmoke(JenkinsRule r) throws Exception {
+ WorkflowJob p = r.createProject(WorkflowJob.class, "p");
+ p.setDefinition(new CpsFlowDefinition("print Jenkins.get().getRootDir().toString()", false));
+ r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+ }
+}