[SECURITY-3103]

jtnord · julieheard · commit afb290b4bd34 · 2023-08-07T11:27:00.000+01:00
diff --git a/.gitignore b/.gitignore
@@ -1,6 +1,8 @@
 target
 work
-
+/.classpath
+/.project
+/.settings/
 .idea
 *.iml
 .*.sw*
diff --git a/README.md b/README.md
@@ -22,17 +22,17 @@ The first, and simpler, security system is to allow any kind of script to be run
 with an administrator’s approval. There is a globally maintained list of approved scripts 
 which are judged to not perform any malicious actions.
 
-When an administrator saves some kind of configuration (for example, a job), those scripts
-that were edited by admin are automatically approved and are ready to run with no further
-intervention. For scripts that were submitted by lower privileged users there will be
+When a user saves some kind of configuration (for example, a job), there will be
 appropriate warnings indicating that approval is required. Administrators may approve those
-scripts using the Script Approval configuration page or by editing the script and saving it.
-In previous versions of Script Security Plugin, administrators could automatically approve
-scripts submitted by unprivileged users by saving them without making any changes, but this
-functionality was disabled to prevent social engineering-based attacks. (“Saving” usually
-means from the web UI, but could also mean uploading a new XML configuration via REST or CLI.)
-
-When a non-administrator saves a template configuration, a check is done whether any 
+scripts using the Script Approval configuration page or following the approval link in the 
+configuration.
+In previous versions of Script Security Plugin, scripts saved by administrators where 
+automatically approved when saving them, but this functionality was disabled to prevent
+a variety of social engineering-based attacks. (“Saving” usually means from the web UI, but
+could also mean uploading a new XML configuration via REST or CLI.) or merely by creating a
+new item copying an existing one.
+
+When a user saves a template configuration, a check is done whether any 
 contained scripts have been edited from an approved text. (More precisely, whether the 
 requested content has ever been approved before.) If it has not been approved, a request 
 for approval of this script is added to a queue. (A warning is also displayed in the 
diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/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.model.PageDecorator;
 import hudson.security.ACLContext;
 import jenkins.model.GlobalConfiguration;
 import jenkins.model.GlobalConfigurationCategory;
@@ -82,7 +83,9 @@
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.DataBoundSetter;
+import org.kohsuke.stapler.QueryParameter;
 import org.kohsuke.stapler.bind.JavaScriptMethod;
+import org.kohsuke.stapler.verb.POST;
 
 /**
  * Manages approved scripts.
@@ -95,6 +98,10 @@ public class ScriptApproval extends GlobalConfiguration implements RootAction {
     public static /* non-final */ boolean ADMIN_AUTO_APPROVAL_ENABLED =
             SystemProperties.getBoolean(ScriptApproval.class.getName() + ".ADMIN_AUTO_APPROVAL_ENABLED");
 
+    @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "for script console")
+    public static /* non-final */ boolean ALLOW_ADMIN_APPROVAL_ENABLED =
+            SystemProperties.getBoolean(ScriptApproval.class.getName() + ".ALLOW_ADMIN_APPROVAL_ENABLED");
+
     private static final Logger LOG = Logger.getLogger(ScriptApproval.class.getName());
 
     private static final XStream2 XSTREAM2 = new XStream2();
@@ -593,8 +600,9 @@ public synchronized String configuring(@NonNull String script, @NonNull Language
         final ConversionCheckResult result = checkAndConvertApprovedScript(script, language);
         if (!result.approved) {
             if (!Jenkins.get().isUseSecurity() || 
+                    (ALLOW_ADMIN_APPROVAL_ENABLED &&
                     ((Jenkins.getAuthentication2() != ACL.SYSTEM2 && Jenkins.get().hasPermission(Jenkins.ADMINISTER))
-                            && (ADMIN_AUTO_APPROVAL_ENABLED || approveIfAdmin))) {
+                            && (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
                 removePendingScript(result.newHash);
@@ -768,16 +776,24 @@ public synchronized FormValidation checking(@NonNull String script, @NonNull Lan
         if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER)) {
             return FormValidation.warningWithMarkup("A Jenkins administrator will need to approve this script before it can be used");
         } else {
-            if (willBeApproved || ADMIN_AUTO_APPROVAL_ENABLED) {
-                return FormValidation.ok("The script has not yet been approved, but it will be approved on save");
+            if ((ALLOW_ADMIN_APPROVAL_ENABLED && (willBeApproved || ADMIN_AUTO_APPROVAL_ENABLED)) || !Jenkins.get().isUseSecurity()) {
+                return FormValidation.okWithMarkup("The script has not yet been approved, but it will be approved on save.");
             }
-
+            String approveScript = "<a class='jenkins-button script-approval-approve-link' data-base-url='" + Jenkins.get().getRootUrl() + ScriptApproval.get().getUrlName() + "' data-hash='" + result.newHash + "'>Approve script</a>";
             return FormValidation.okWithMarkup("The script is not approved and will not be approved on save. " +
-                    "Modify the script to approve it on save, or approve it explicitly on the " +
-                    "<a target='blank' href='"+ Jenkins.get().getRootUrl() + ScriptApproval.get().getUrlName() + "'>Script Approval Configuration</a> page");
+                    "Either modify the script to match an already approved script, approve it explicitly on the " +
+                    "<a target='blank' href='"+ Jenkins.get().getRootUrl() + ScriptApproval.get().getUrlName() + "'>Script Approval Configuration</a> page after save, or approve this version of the script. " +
+                    approveScript);
         }
     }
 
+    @Restricted(NoExternalUse.class)
+    @POST
+    // can not call this method doApproveScript as that collides with the javascript binding in #approveScript
+    public synchronized void doApproveScriptHash(@QueryParameter(required=true) String hash) throws IOException {
+        approveScript(hash);
+    }
+
     /**
      * @deprecated Use {@link #checking(String, Language, boolean)} instead
      */
@@ -1235,4 +1251,7 @@ public synchronized JSON clearApprovedClasspathEntries() throws IOException {
         return getClasspathRenderInfo();
     }
 
+    @Restricted(NoExternalUse.class)
+    @Extension
+    public static class FormValidationPageDecorator extends PageDecorator {}
 }
diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/FormValidationPageDecorator/header.jelly b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/FormValidationPageDecorator/header.jelly
@@ -0,0 +1,4 @@
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler">
+  <st:adjunct includes="org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval.FormValidationPageDecorator.validate" />
+</j:jelly>
diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/FormValidationPageDecorator/validate.js b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/FormValidationPageDecorator/validate.js
@@ -0,0 +1,15 @@
+Behaviour.specify('.script-approval-approve-link', 'ScriptApproval.FormValidationPageDecorator', 0, function (element) {
+    element.onclick = function (ev) {
+        const approvalUrl = ev.target.dataset.baseUrl;
+        const hash = ev.target.dataset.hash;
+        const xmlhttp = new XMLHttpRequest();
+        xmlhttp.onload = function () {
+            alert('Script approved');
+        };
+        xmlhttp.open('POST', approvalUrl + "/approveScriptHash");
+        const data = new FormData();
+        data.append('hash', hash);
+        xmlhttp.setRequestHeader(crumb.fieldName, crumb.value);
+        xmlhttp.send(data);
+    }
+});
diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScriptTest.java
@@ -24,6 +24,7 @@
 
 package org.jenkinsci.plugins.scriptsecurity.sandbox.groovy;
 
+import org.htmlunit.CollectingAlertHandler;
 import org.htmlunit.html.HtmlCheckBoxInput;
 import org.htmlunit.html.HtmlInput;
 import groovy.lang.Binding;
@@ -61,7 +62,12 @@
 import org.jenkinsci.plugins.scriptsecurity.scripts.UnapprovedUsageException;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.arrayWithSize;
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.emptyArray;
+import static org.hamcrest.Matchers.is;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.Whitelisted;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -163,7 +169,7 @@ public class SecureGroovyScriptTest {
 
 
     /**
-     * Test where the user has ADMINISTER privs, default to non sandbox mode.
+     * Test where the user has ADMINISTER privs, default to non sandbox mode, but require approval
      */
     @Test public void testSandboxDefault_with_ADMINISTER_privs() throws Exception {
         r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
@@ -198,12 +204,12 @@ public class SecureGroovyScriptTest {
         // The user has ADMINISTER privs => should default to non sandboxed
         assertFalse(publisher.getScript().isSandbox());
 
-        // Because it has ADMINISTER privs, the script should not have ended up pending approval
+        // even though it has ADMINISTER privs, the script should still require approval
         Set<ScriptApproval.PendingScript> pendingScripts = ScriptApproval.get().getPendingScripts();
-        assertEquals(0, pendingScripts.size());
+        assertEquals(1, pendingScripts.size());
 
         // Test that the script is executable. If it's not, we will get an UnapprovedUsageException
-        assertEquals(groovy, ScriptApproval.get().using(groovy, GroovyLanguage.get()));
+        assertThrows(UnapprovedUsageException.class, () -> ScriptApproval.get().using(groovy, GroovyLanguage.get()));
     }
 
     /**
@@ -892,7 +898,7 @@ public void testScriptApproval() throws Exception {
 
         JenkinsRule.WebClient wc = r.createWebClient();
         
-        // If configured by a user with ADMINISTER script is approved if edited by that user
+        // If configured by a user with ADMINISTER script is not approved and approval is requested
         {
             wc.login("admin");
             HtmlForm config = wc.getPage(p, "configure").getFormByName("config");
@@ -903,8 +909,9 @@ public void testScriptApproval() throws Exception {
             script.setText(groovy);
             r.submit(config);
 
-            assertTrue(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get()));
-            
+            assertFalse(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get()));
+            assertEquals(1, ScriptApproval.get().getPendingScripts().size());
+
             // clean up for next tests
             ScriptApproval.get().preapproveAll();
             ScriptApproval.get().clearApprovedScripts();
@@ -929,11 +936,14 @@ public void testScriptApproval() throws Exception {
             ScriptApproval.get().clearApprovedScripts();
         }
         
-        // If configured by a user with ADMINISTER while escape hatch is on script is approved upon save
+        // If configured by a user with ADMINISTER while escape hatches are on script is approved upon save
         {
             wc.login("admin");
-            boolean original = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED;
+            boolean originalAdminAutoApprove = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED;
             ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = true;
+            boolean originalAllowAdminApproval = ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED;
+            ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED = true;
+
             try {
                 HtmlForm config = wc.getPage(p, "configure").getFormByName("config");
                 List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
@@ -948,15 +958,18 @@ public void testScriptApproval() throws Exception {
                 ScriptApproval.get().preapproveAll();
                 ScriptApproval.get().clearApprovedScripts();
             } finally {
-                ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = original;
+                ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = originalAdminAutoApprove;
+                ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED = originalAllowAdminApproval;
             }
         }
 
         // If configured by a user without ADMINISTER while escape hatch is on script is not approved
         {
             wc.login("devel");
-            boolean original = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED;
+            boolean originalAdminAutoApprove = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED;
             ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = true;
+            boolean originalAllowAdminApproval = ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED;
+            ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED = true;
             try {
                 r.submit(wc.getPage(p, "configure").getFormByName("config"));
 
@@ -966,7 +979,8 @@ public void testScriptApproval() throws Exception {
                 ScriptApproval.get().preapproveAll();
                 ScriptApproval.get().clearApprovedScripts();
             } finally {
-                ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = original;
+                ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = originalAdminAutoApprove;
+                ScriptApproval.ALLOW_ADMIN_APPROVAL_ENABLED = originalAllowAdminApproval;
             }
         }
     }
@@ -1271,6 +1285,52 @@ public void testScriptAtFieldInitializers() throws Exception {
         r.assertLogContains("new java.io.File java.lang.String", b);
     }
 
+    @Test public void testApprovalFromFormValidation() throws Exception {
+        r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
+        MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
+        mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin");
+        for (Permission p : Item.PERMISSIONS.getPermissions()) {
+            mockStrategy.grant(p).everywhere().to("admin");
+        }
+        r.jenkins.setAuthorizationStrategy(mockStrategy);
+
+        FreeStyleProject p = r.createFreeStyleProject("p");
+        try (JenkinsRule.WebClient wc = r.createWebClient()) {
+            CollectingAlertHandler altertHandler = new CollectingAlertHandler();
+            wc.setAlertHandler(altertHandler);
+
+            wc.login("admin");
+            HtmlPage page = wc.getPage(p, "configure");
+            HtmlForm config = page.getFormByName("config");
+            HtmlFormUtil.getButtonByCaption(config, "Add post-build action").click(); // lib/hudson/project/config-publishers2.jelly
+            page.getAnchorByText(r.jenkins.getExtensionList(BuildStepDescriptor.class).get(TestGroovyRecorder.DescriptorImpl.class).getDisplayName()).click();
+            wc.waitForBackgroundJavaScript(10000);
+            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 = "build.externalizableId";
+            script.setText(groovy);
+            // nothing is approved or pending (no save)
+            assertThat(ScriptApproval.get().getPendingScripts(), is(empty()));
+            assertThat(ScriptApproval.get().getApprovedScriptHashes(), is(emptyArray()));
+
+            wc.waitForBackgroundJavaScript(10_000); // FormValidation to display
+
+            page.getAnchorByText("Approve script").click();
+
+            wc.waitForBackgroundJavaScript(10_000); // wait for the ajax approval to complete
+
+            assertThat(altertHandler.getCollectedAlerts(), contains("Script approved"));
+            // script is approved
+            assertThat(ScriptApproval.get().getPendingScripts(), is(empty()));
+            assertThat(ScriptApproval.get().getApprovedScriptHashes(), is(arrayWithSize(1)));
+
+            r.submit(config);
+
+            FreeStyleBuild b = r.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0));
+        }
+    }
+
     public static class HasMainMethod {
         @Whitelisted
         public HasMainMethod() { }
