From afb290b4bd3414d292890a5f09333ef04c4fb820 Mon Sep 17 00:00:00 2001 From: James Nord Date: Tue, 11 Jul 2023 10:08:33 +0100 Subject: [PATCH] [SECURITY-3103] --- .gitignore | 4 +- README.md | 20 ++--- .../scripts/ScriptApproval.java | 31 +++++-- .../FormValidationPageDecorator/header.jelly | 4 + .../FormValidationPageDecorator/validate.js | 15 ++++ .../groovy/SecureGroovyScriptTest.java | 84 ++++++++++++++++--- 6 files changed, 129 insertions(+), 29 deletions(-) create mode 100644 src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/FormValidationPageDecorator/header.jelly create mode 100644 src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/FormValidationPageDecorator/validate.js diff --git a/.gitignore b/.gitignore index 75bad940c..d055f499a 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,8 @@ target work - +/.classpath +/.project +/.settings/ .idea *.iml .*.sw* diff --git a/README.md b/README.md index 70a941d0b..00c335a39 100644 --- 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 index 9c485c4a4..6e4256e96 100644 --- 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 = "Approve script"; 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 " + - "Script Approval Configuration page"); + "Either modify the script to match an already approved script, approve it explicitly on the " + + "Script Approval Configuration 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 new file mode 100644 index 000000000..f9cca940b --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval/FormValidationPageDecorator/header.jelly @@ -0,0 +1,4 @@ + + + + \ No newline at end of file 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 new file mode 100644 index 000000000..b5bf0698b --- /dev/null +++ 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); + } +}); \ No newline at end of file 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 index 0e99789c5..5bf0fa30b 100644 --- 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 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 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 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() { }