From f4c0bb9b58e105b4fc6b62be0f7f2daa46178190 Mon Sep 17 00:00:00 2001 From: Yaroslav Afenkin Date: Wed, 6 Apr 2022 18:32:16 +0300 Subject: [PATCH] [SECURITY-2450] --- README.md | 13 +- .../sandbox/groovy/SecureGroovyScript.java | 25 +- .../scripts/ClasspathEntry.java | 46 ++- .../scripts/ScriptApproval.java | 88 +++++- .../groovy/SecureGroovyScript/config.jelly | 1 + .../scripts/ClasspathEntry/config.jelly | 15 +- .../scripts/ClasspathEntry/resources.js | 28 ++ .../groovy/SecureGroovyScriptTest.java | 262 ++++++++++++++---- .../scripts/ScriptApprovalTest.java | 2 +- 9 files changed, 402 insertions(+), 78 deletions(-) create mode 100644 src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry/resources.js diff --git a/README.md b/README.md index 56e8db989..41bc7f93d 100644 --- a/README.md +++ b/README.md @@ -25,10 +25,15 @@ 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), any scripts -it contains are automatically added to the approved list. They are ready to run with no -further intervention. (“Saving” usually means from the web UI, but could also mean -uploading a new XML configuration via REST or CLI.) +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 +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 contained scripts have been edited from an approved text. (More precisely, whether the diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java index 9303365cd..f4cca7010 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java @@ -58,6 +58,7 @@ import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; import jenkins.model.Jenkins; +import org.apache.commons.lang.StringUtils; import org.codehaus.groovy.control.CompilationUnit; import org.codehaus.groovy.control.CompilerConfiguration; import org.codehaus.groovy.control.SourceUnit; @@ -68,7 +69,10 @@ import org.jenkinsci.plugins.scriptsecurity.scripts.UnapprovedClasspathException; import org.jenkinsci.plugins.scriptsecurity.scripts.UnapprovedUsageException; import org.jenkinsci.plugins.scriptsecurity.scripts.languages.GroovyLanguage; +import org.kohsuke.accmod.Restricted; +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.Stapler; import org.kohsuke.stapler.StaplerRequest; @@ -86,6 +90,7 @@ public final class SecureGroovyScript extends AbstractDescribableImpl classpath; + private transient String oldScript; private transient boolean calledConfiguring; static final Logger LOGGER = Logger.getLogger(SecureGroovyScript.class.getName()); @@ -117,6 +122,20 @@ public boolean isSandbox() { return classpath != null ? classpath : Collections.emptyList(); } + public String getOldScript() { + return oldScript; + } + + @DataBoundSetter + public void setOldScript(String oldScript) { + this.oldScript = oldScript; + } + + @Restricted(NoExternalUse.class) + public boolean isScriptAutoApprovalEnabled() { + return ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED; + } + /** * To be called in your own {@link DataBoundConstructor} when storing the field of this type. * @param context an approval context @@ -125,7 +144,7 @@ public boolean isSandbox() { public SecureGroovyScript configuring(ApprovalContext context) { calledConfiguring = true; if (!sandbox) { - ScriptApproval.get().configuring(script, GroovyLanguage.get(), context); + ScriptApproval.get().configuring(script, GroovyLanguage.get(), context, !StringUtils.equals(this.oldScript, this.script)); } for (ClasspathEntry entry : getClasspath()) { ScriptApproval.get().configuring(entry, context); @@ -457,13 +476,13 @@ private final class CleanClassCollector extends ClassCollector { @SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED", justification = "Irrelevant without SecurityManager.") @RequirePOST - public FormValidation doCheckScript(@QueryParameter String value, @QueryParameter boolean sandbox) { + public FormValidation doCheckScript(@QueryParameter String value, @QueryParameter boolean sandbox, @QueryParameter String oldScript) { FormValidation validationResult = GroovySandbox.checkScriptForCompilationErrors(value, new GroovyClassLoader(Jenkins.get().getPluginManager().uberClassLoader)); if (validationResult.kind != FormValidation.Kind.OK) { return validationResult; } - return sandbox ? FormValidation.ok() : ScriptApproval.get().checking(value, GroovyLanguage.get()); + return sandbox ? FormValidation.ok() : ScriptApproval.get().checking(value, GroovyLanguage.get(), !StringUtils.equals(oldScript, value)); } } diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry.java index 8df12b7a6..9ad7f4867 100644 --- a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry.java +++ b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry.java @@ -29,7 +29,10 @@ import java.io.Serializable; import org.apache.commons.lang.StringUtils; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.DataBoundConstructor; +import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.QueryParameter; import hudson.Extension; @@ -53,7 +56,10 @@ public final class ClasspathEntry extends AbstractDescribableImplIf the script has already been approved, this does nothing. - * Otherwise, if this user has the {@link Jenkins#ADMINISTER} permission (and is not {@link ACL#SYSTEM}), or Jenkins is running without security, it is added to the approved list. + * Otherwise, if this user has the {@link Jenkins#ADMINISTER} permission (and is not {@link ACL#SYSTEM}) + * and a corresponding flag is set to {@code true}, or Jenkins is running without security, it is added to the approved list. * Otherwise, it is added to the pending list. * @param script the text of a possibly novel script * @param language the language in which it is written * @param context any additional information about how where or by whom this is being configured + * @param approveIfAdmin indicates whether script should be approved if current user has admin permissions * @return {@code script}, for convenience - * @throws IllegalStateException {@link Jenkins} instance is not ready */ - public synchronized String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context) { + public synchronized String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context, boolean approveIfAdmin) { final String hash = hash(script, language.getName()); if (!approvedScriptHashes.contains(hash)) { - if (!Jenkins.get().isUseSecurity() || Jenkins.getAuthentication() != ACL.SYSTEM && Jenkins.get().hasPermission(Jenkins.ADMINISTER)) { + if (!Jenkins.get().isUseSecurity() || + ((Jenkins.getAuthentication() != ACL.SYSTEM && Jenkins.get().hasPermission(Jenkins.ADMINISTER)) + && (ADMIN_AUTO_APPROVAL_ENABLED || approveIfAdmin))) { approvedScriptHashes.add(hash); + removePendingScript(hash); } else { String key = context.getKey(); if (key != null) { @@ -449,6 +463,14 @@ public synchronized String configuring(@NonNull String script, @NonNull Language return script; } + /** + * @deprecated Use {@link #configuring(String, Language, ApprovalContext, boolean)} instead + */ + @Deprecated + public String configuring(@NonNull String script, @NonNull Language language, @NonNull ApprovalContext context) { + return this.configuring(script, language, context, false); + } + /** * Called when a script is about to be used (evaluated). * @param script a possibly unapproved script @@ -477,7 +499,7 @@ synchronized boolean isScriptHashApproved(String hash) { /** * Called when configuring a classpath entry. - * Usage is similar to {@link #configuring(String, Language, ApprovalContext)}. + * Usage is similar to {@link #configuring(String, Language, ApprovalContext, boolean)}. * @param entry entry to be configured * @param context any additional information * @throws IllegalStateException {@link Jenkins} instance is not ready @@ -507,7 +529,9 @@ public synchronized void configuring(@NonNull ClasspathEntry entry, @NonNull App if (!approvedClasspathEntries.contains(acp)) { boolean shouldSave = false; PendingClasspathEntry pcp = new PendingClasspathEntry(hash, url, context); - if (!Jenkins.get().isUseSecurity() || (Jenkins.getAuthentication() != ACL.SYSTEM && Jenkins.get().hasPermission(Jenkins.ADMINISTER))) { + if (!Jenkins.get().isUseSecurity() || + ((Jenkins.getAuthentication() != ACL.SYSTEM && Jenkins.get().hasPermission(Jenkins.ADMINISTER)) + && (ADMIN_AUTO_APPROVAL_ENABLED || entry.isShouldBeApproved() || !StringUtils.equals(entry.getOldPath(), entry.getPath())))) { LOG.log(Level.FINE, "Classpath entry {0} ({1}) is approved as configured with ADMINISTER permission.", new Object[] {url, hash}); pendingClasspathEntries.remove(pcp); approvedClasspathEntries.add(acp); @@ -525,7 +549,7 @@ public synchronized void configuring(@NonNull ClasspathEntry entry, @NonNull App } /** - * Like {@link #checking(String, Language)} but for classpath entries. + * Like {@link #checking(String, Language, boolean)} but for classpath entries. * (This is automatic if use {@link ClasspathEntry} as a configuration element.) * @param entry the classpath entry to verify * @return whether it will be approved @@ -584,14 +608,48 @@ public synchronized void using(@NonNull ClasspathEntry entry) throws IOException * To be used from form validation, in a {@code doCheckFieldName} method. * @param script a possibly unapproved script * @param language the language in which it is written - * @return a warning in case the script is not yet approved and this user lacks {@link Jenkins#ADMINISTER}, else {@link FormValidation#ok()} + * @param willBeApproved whether script is going to be approved after configuration is saved + * @return a warning indicating that admin approval will be needed in case current user does not have + * {@link Jenkins#ADMINISTER} permission; a warning indicating that script is not yet approved if user has such + * permission and {@code willBeApproved} is false; a message indicating that script will be approved if user + * has such permission and {@code willBeApproved} is true; nothing if script is empty; a corresponding message + * if script is approved */ - public synchronized FormValidation checking(@NonNull String script, @NonNull Language language) { - if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER) && !approvedScriptHashes.contains(hash(script, language.getName()))) { - return FormValidation.warningWithMarkup("A Jenkins administrator will need to approve this script before it can be used."); - } else { + public synchronized FormValidation checking(@NonNull String script, @NonNull Language language, boolean willBeApproved) { + if (StringUtils.isEmpty(script)) { return FormValidation.ok(); } + if (approvedScriptHashes.contains(hash(script, language.getName()))) { + return FormValidation.okWithMarkup("The script is already approved"); + } + + 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"); + } + + 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"); + } + } + + /** + * @deprecated Use {@link #checking(String, Language, boolean)} instead + */ + @Deprecated + public synchronized FormValidation checking(@NonNull String script, @NonNull Language language) { + return this.checking(script, language, false); + } + + synchronized boolean isClasspathEntryApproved(URL url) { + try { + return approvedClasspathEntries.contains(new ApprovedClasspathEntry(hashClasspathEntry(url), url)); + } catch (IOException e) { + return false; + } } /** @@ -711,7 +769,7 @@ public synchronized void setApprovedScriptHashes(String[] scriptHashes) throws I reconfigure(); } - @Restricted(NoExternalUse.class) // Jelly, implementation + @Restricted(NoExternalUse.class) // Jelly, tests, implementation public synchronized String[] getApprovedScriptHashes() { return approvedScriptHashes.toArray(new String[approvedScriptHashes.size()]); } @@ -774,7 +832,7 @@ public Set getPendingScripts() { save(); } - private synchronized void removePendingScript(String hash) { + synchronized void removePendingScript(String hash) { Iterator it = pendingScripts.iterator(); while (it.hasNext()) { if (it.next().getHash().equals(hash)) { diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly index 1bef20137..057504ee7 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly @@ -25,6 +25,7 @@ THE SOFTWARE. + diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry/config.jelly b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry/config.jelly index 9ac2f21ba..9efe88b7d 100644 --- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry/config.jelly @@ -24,10 +24,21 @@ THE SOFTWARE. --> - + + + + + + + - + + + + + +
diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry/resources.js b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry/resources.js new file mode 100644 index 000000000..3f5444a01 --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry/resources.js @@ -0,0 +1,28 @@ +document.addEventListener("DOMContentLoaded", function() { + function adjustCheckboxAvailability(event) { + // event.target is the path/url input + // parent is the first common parent for root level elements in config.jelly + // from parent we can target child elements via querySelector to not harm other elements on the page + const parent = event.target.parentElement.parentElement.parentElement; + + const classpathEntryPath = parent.querySelector(".secure-groovy-script__classpath-entry-path"); + const classpathApproveCheckbox = parent.querySelector(".secure-groovy-script__classpath-approve"); + if (!classpathApproveCheckbox) { + return; + } + + const classpathEntryOldPath = parent.querySelector(".secure-groovy-script__classpath-entry-old-path"); + const classpathEntryPathEdited = classpathEntryPath.value !== classpathEntryOldPath.value; + + if (classpathEntryPathEdited) { + classpathApproveCheckbox.setAttribute("disabled", "true"); + } else { + classpathApproveCheckbox.removeAttribute("disabled"); + } + } + + const classpaths = document.querySelectorAll(".secure-groovy-script__classpath-entry-path"); + classpaths.forEach(function(classpath) { + classpath.addEventListener("blur", adjustCheckboxAvailability); + }) +}); \ 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 9217d61e3..31c30c5f0 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 @@ -315,10 +315,10 @@ private List getAllUpdatedJarFiles() throws URISyntaxException { classpath )); TestGroovyRecorder recorder2 = r.configRoundtrip(recorder); - r.assertEqualDataBoundBeans(recorder, recorder2); + r.assertEqualBeans(recorder.getScript(), recorder2.getScript(), "script,sandbox,classpath"); classpath.clear(); recorder2 = r.configRoundtrip(recorder); - r.assertEqualDataBoundBeans(recorder, recorder2); + r.assertEqualBeans(recorder.getScript(), recorder2.getScript(), "script,sandbox,classpath"); } @Test public void testClasspathInSandbox() throws Exception { @@ -684,7 +684,7 @@ private List getAllUpdatedJarFiles() throws URISyntaxException { } } - @Test public void testClasspathAutomaticApprove() throws Exception { + @Test public void testClasspathApproval() throws Exception { r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); @@ -697,12 +697,7 @@ private List getAllUpdatedJarFiles() throws URISyntaxException { } r.jenkins.setAuthorizationStrategy(mockStrategy); - JenkinsRule.WebClient wcDevel = r.createWebClient(); - wcDevel.login("devel"); - - JenkinsRule.WebClient wcApprover = r.createWebClient(); - wcApprover.login("approver"); - + JenkinsRule.WebClient wc = r.createWebClient(); List classpath = new ArrayList<>(); @@ -726,7 +721,7 @@ private List getAllUpdatedJarFiles() throws URISyntaxException { // Deny classpath. { List pcps = ScriptApproval.get().getPendingClasspathEntries(); - assertNotEquals(0, pcps.size()); + assertEquals(classpath.size(), pcps.size()); for(ScriptApproval.PendingClasspathEntry pcp: pcps) { ScriptApproval.get().denyClasspathEntry(pcp.getHash()); } @@ -734,74 +729,243 @@ private List getAllUpdatedJarFiles() throws URISyntaxException { assertEquals(0, ScriptApproval.get().getPendingClasspathEntries().size()); assertEquals(0, ScriptApproval.get().getApprovedClasspathEntries().size()); } - - // If configured by a user with ADMINISTER, the classpath is automatically approved + + // If configured by a user with ADMINISTER, the classpath is approved if corresponding checkbox is set { - r.submit(wcApprover.getPage(p, "configure").getFormByName("config")); + wc.login("approver"); + HtmlForm config = wc.getPage(p, "configure").getFormByName("config"); + List checkboxes = config.getInputsByName("_.shouldBeApproved"); + // Get the last one, because previous ones might be from Lockable Resources during PCT. + HtmlInput checkbox = checkboxes.get(checkboxes.size() - 1); + // there's only one classpath being configured, so we only set one checkbox + checkbox.setChecked(true); + r.submit(config); List pcps = ScriptApproval.get().getPendingClasspathEntries(); assertEquals(0, pcps.size()); List acps = ScriptApproval.get().getApprovedClasspathEntries(); - assertNotEquals(0, acps.size()); + assertEquals(classpath.size(), acps.size()); + // cleaning up for next tests for(ScriptApproval.ApprovedClasspathEntry acp: acps) { ScriptApproval.get().denyApprovedClasspathEntry(acp.getHash()); } - assertEquals(0, ScriptApproval.get().getPendingClasspathEntries().size()); assertEquals(0, ScriptApproval.get().getApprovedClasspathEntries().size()); } - - // If configured by a user without ADMINISTER, approval is requested + + // If configured by a user with ADMINISTER, but approval checkbox is not set then approval is requested { - r.submit(wcDevel.getPage(p, "configure").getFormByName("config")); + wc.login("approver"); + r.submit(wc.getPage(p, "configure").getFormByName("config")); List pcps = ScriptApproval.get().getPendingClasspathEntries(); - assertNotEquals(0, pcps.size()); + assertEquals(classpath.size(), pcps.size()); List acps = ScriptApproval.get().getApprovedClasspathEntries(); assertEquals(0, acps.size()); - - // don't remove pending classpaths. + + // cleaning up for next tests + for(ScriptApproval.PendingClasspathEntry pcp: pcps) { + ScriptApproval.get().denyClasspathEntry(pcp.getHash()); + } + assertEquals(0, ScriptApproval.get().getPendingClasspathEntries().size()); + assertEquals(0, ScriptApproval.get().getApprovedClasspathEntries().size()); } - - // If configured by a user with ADMINISTER, the classpath is automatically approved, and removed from approval request. + + // If configured by a user without ADMINISTER approval is requested { - assertNotEquals(0, ScriptApproval.get().getPendingClasspathEntries().size()); - assertEquals(0, ScriptApproval.get().getApprovedClasspathEntries().size()); - - r.submit(wcApprover.getPage(p, "configure").getFormByName("config")); - + wc.login("devel"); + r.submit(wc.getPage(p, "configure").getFormByName("config")); + List pcps = ScriptApproval.get().getPendingClasspathEntries(); - assertEquals(0, pcps.size()); + assertEquals(classpath.size(), pcps.size()); List acps = ScriptApproval.get().getApprovedClasspathEntries(); - assertNotEquals(0, acps.size()); - - for(ScriptApproval.ApprovedClasspathEntry acp: acps) { - ScriptApproval.get().denyApprovedClasspathEntry(acp.getHash()); + assertEquals(0, acps.size()); + + // cleaning up for next tests + for(ScriptApproval.PendingClasspathEntry pcp: pcps) { + ScriptApproval.get().denyClasspathEntry(pcp.getHash()); } - assertEquals(0, ScriptApproval.get().getPendingClasspathEntries().size()); assertEquals(0, ScriptApproval.get().getApprovedClasspathEntries().size()); } + // If configured by a user without ADMINISTER while escape hatch in enabled approval is requested + { + wc.login("devel"); + boolean original = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED; + ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = true; + try { + r.submit(wc.getPage(p, "configure").getFormByName("config")); + + List pcps = ScriptApproval.get().getPendingClasspathEntries(); + assertEquals(classpath.size(), pcps.size()); + List acps = ScriptApproval.get().getApprovedClasspathEntries(); + assertEquals(0, acps.size()); + + // cleaning up for next tests + for(ScriptApproval.PendingClasspathEntry pcp: pcps) { + ScriptApproval.get().denyClasspathEntry(pcp.getHash()); + } + assertEquals(0, ScriptApproval.get().getPendingClasspathEntries().size()); + assertEquals(0, ScriptApproval.get().getApprovedClasspathEntries().size()); + } finally { + ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = original; + } + } + + // If configured by a user with ADMINISTER while escape hatch in enabled approval happens upon save + { + wc.login("approver"); + boolean original = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED; + ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = true; + try { + r.submit(wc.getPage(p, "configure").getFormByName("config")); + + List pcps = ScriptApproval.get().getPendingClasspathEntries(); + assertEquals(0, pcps.size()); + List acps = ScriptApproval.get().getApprovedClasspathEntries(); + assertEquals(classpath.size(), acps.size()); + + // cleaning up for next tests + for(ScriptApproval.ApprovedClasspathEntry acp: acps) { + ScriptApproval.get().denyApprovedClasspathEntry(acp.getHash()); + } + assertEquals(0, ScriptApproval.get().getPendingClasspathEntries().size()); + assertEquals(0, ScriptApproval.get().getApprovedClasspathEntries().size()); + } finally { + ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = original; + } + } + // If run with SYSTEM user, an approval is requested. { r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get()); - + List pcps = ScriptApproval.get().getPendingClasspathEntries(); - assertNotEquals(0, pcps.size()); + assertEquals(classpath.size(), pcps.size()); List acps = ScriptApproval.get().getApprovedClasspathEntries(); assertEquals(0, acps.size()); - + + // cleaning up for next tests for(ScriptApproval.PendingClasspathEntry pcp: pcps) { ScriptApproval.get().denyClasspathEntry(pcp.getHash()); } - assertEquals(0, ScriptApproval.get().getPendingClasspathEntries().size()); assertEquals(0, ScriptApproval.get().getApprovedClasspathEntries().size()); } } + @Test @Issue("SECURITY-2450") + public void testScriptApproval() throws Exception { + r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); + MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy(); + mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin"); + mockStrategy.grant(Jenkins.READ).everywhere().to("devel"); + for (Permission p : Item.PERMISSIONS.getPermissions()) { + mockStrategy.grant(p).everywhere().to("admin"); + mockStrategy.grant(p).everywhere().to("devel"); + } + r.jenkins.setAuthorizationStrategy(mockStrategy); + + FreeStyleProject p = r.createFreeStyleProject(); + String initialGroovyScript = "echo 'testScriptApproval'"; + p.getPublishersList().add(new TestGroovyRecorder(new SecureGroovyScript( + initialGroovyScript, + false, + new ArrayList<>() + ))); + + // clear all pending and approved scripts if there are any + { + ScriptApproval.get().preapproveAll(); + ScriptApproval.get().clearApprovedScripts(); + + assertEquals(0, ScriptApproval.get().getPendingScripts().size()); + assertEquals(0, ScriptApproval.get().getApprovedScriptHashes().length); + } + + JenkinsRule.WebClient wc = r.createWebClient(); + + // If configured by a user with ADMINISTER script is approved if edited by that user + { + wc.login("admin"); + HtmlForm config = wc.getPage(p, "configure").getFormByName("config"); + 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 = "echo 'testScriptApproval modified by admin'"; + script.setText(groovy); + r.submit(config); + + assertTrue(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get())); + + // clean up for next tests + ScriptApproval.get().preapproveAll(); + ScriptApproval.get().clearApprovedScripts(); + } + + // If configured by a user without ADMINISTER approval is requested + { + wc.login("devel"); + HtmlForm config = wc.getPage(p, "configure").getFormByName("config"); + 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 = "echo 'testScriptApproval modified by devel'"; + script.setText(groovy); + r.submit(config); + + assertFalse(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get())); + assertEquals(1, ScriptApproval.get().getPendingScripts().size()); + + // clean up for next tests + ScriptApproval.get().preapproveAll(); + ScriptApproval.get().clearApprovedScripts(); + } + + // If configured by a user with ADMINISTER while escape hatch is on script is approved upon save + { + wc.login("admin"); + boolean original = ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED; + ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = true; + try { + HtmlForm config = wc.getPage(p, "configure").getFormByName("config"); + 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 currentScriptValue = script.getText(); + r.submit(config); + + assertTrue(ScriptApproval.get().isScriptApproved(currentScriptValue, GroovyLanguage.get())); + + // clean up for next tests + ScriptApproval.get().preapproveAll(); + ScriptApproval.get().clearApprovedScripts(); + } finally { + ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = original; + } + } + + // 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; + ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = true; + try { + r.submit(wc.getPage(p, "configure").getFormByName("config")); + + assertFalse(ScriptApproval.get().isScriptApproved(initialGroovyScript, GroovyLanguage.get())); + + // clean up for next tests + ScriptApproval.get().preapproveAll(); + ScriptApproval.get().clearApprovedScripts(); + } finally { + ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED = original; + } + } + } + @Test @Issue("JENKINS-25348") public void testSandboxClassResolution() throws Exception { File jar = Which.jarFile(Checker.class); @@ -871,7 +1035,7 @@ public void blockASTTest() throws Exception { "import hudson.model.FreeStyleProject\n" + "@ASTTest(value={ assert Jenkins.getInstance().createProject(FreeStyleProject.class, \"should-not-exist\") })\n" + "@Field int x\n" + - "echo 'hello'\n", false).toString(), containsString("Annotation ASTTest cannot be used in the sandbox")); + "echo 'hello'\n", false, null).toString(), containsString("Annotation ASTTest cannot be used in the sandbox")); assertNull(r.jenkins.getItem("should-not-exist")); } @@ -880,7 +1044,7 @@ public void blockASTTest() throws Exception { @Test public void blockGrab() throws Exception { SecureGroovyScript.DescriptorImpl d = r.jenkins.getDescriptorByType(SecureGroovyScript.DescriptorImpl.class); - assertThat(d.doCheckScript("@Grab(group='foo', module='bar', version='1.0')\ndef foo\n", false).toString(), + assertThat(d.doCheckScript("@Grab(group='foo', module='bar', version='1.0')\ndef foo\n", false, null).toString(), containsString("Annotation Grab cannot be used in the sandbox")); } @@ -888,7 +1052,7 @@ public void blockGrab() throws Exception { @Test public void blockGrapes() throws Exception { SecureGroovyScript.DescriptorImpl d = r.jenkins.getDescriptorByType(SecureGroovyScript.DescriptorImpl.class); - assertThat(d.doCheckScript("@Grapes([@Grab(group='foo', module='bar', version='1.0')])\ndef foo\n", false).toString(), + assertThat(d.doCheckScript("@Grapes([@Grab(group='foo', module='bar', version='1.0')])\ndef foo\n", false, null).toString(), containsString("Annotation Grapes cannot be used in the sandbox")); } @@ -896,7 +1060,7 @@ public void blockGrapes() throws Exception { @Test public void blockGrabConfig() throws Exception { SecureGroovyScript.DescriptorImpl d = r.jenkins.getDescriptorByType(SecureGroovyScript.DescriptorImpl.class); - assertThat(d.doCheckScript("@GrabConfig(autoDownload=false)\ndef foo\n", false).toString(), + assertThat(d.doCheckScript("@GrabConfig(autoDownload=false)\ndef foo\n", false, null).toString(), containsString("Annotation GrabConfig cannot be used in the sandbox")); } @@ -904,7 +1068,7 @@ public void blockGrabConfig() throws Exception { @Test public void blockGrabExclude() throws Exception { SecureGroovyScript.DescriptorImpl d = r.jenkins.getDescriptorByType(SecureGroovyScript.DescriptorImpl.class); - assertThat(d.doCheckScript("@GrabExclude(group='org.mortbay.jetty', module='jetty-util')\ndef foo\n", false).toString(), + assertThat(d.doCheckScript("@GrabExclude(group='org.mortbay.jetty', module='jetty-util')\ndef foo\n", false, null).toString(), containsString("Annotation GrabExclude cannot be used in the sandbox")); } @@ -912,7 +1076,7 @@ public void blockGrabExclude() throws Exception { @Test public void blockGrabResolver() throws Exception { SecureGroovyScript.DescriptorImpl d = r.jenkins.getDescriptorByType(SecureGroovyScript.DescriptorImpl.class); - assertThat(d.doCheckScript("@GrabResolver(name='restlet.org', root='http://maven.restlet.org')\ndef foo\n", false).toString(), + assertThat(d.doCheckScript("@GrabResolver(name='restlet.org', root='http://maven.restlet.org')\ndef foo\n", false, null).toString(), containsString("Annotation GrabResolver cannot be used in the sandbox")); } @@ -922,7 +1086,7 @@ public void blockArbitraryAnnotation() throws Exception { try { System.setProperty(RejectASTTransformsCustomizer.class.getName() + ".ADDITIONAL_BLOCKED_TRANSFORMS", "groovy.transform.Field,groovy.transform.Immutable"); SecureGroovyScript.DescriptorImpl d = r.jenkins.getDescriptorByType(SecureGroovyScript.DescriptorImpl.class); - assertThat(d.doCheckScript("@Field\ndef foo\n", false).toString(), + assertThat(d.doCheckScript("@Field\ndef foo\n", false, null).toString(), containsString("Annotation Field cannot be used in the sandbox")); } finally { System.clearProperty(RejectASTTransformsCustomizer.class.getName() + ".ADDITIONAL_BLOCKED_TRANSFORMS"); @@ -939,7 +1103,7 @@ public void blockAnnotationCollector() throws Exception { "@AnnotationCollector([ASTTest]) @interface Lol {}\n" + "@Lol(value={ assert Jenkins.getInstance().createProject(FreeStyleProject.class, \"should-not-exist\") })\n" + "@Field int x\n" + - "echo 'hello'\n", false).toString(), containsString("Annotation AnnotationCollector cannot be used in the sandbox")); + "echo 'hello'\n", false, null).toString(), containsString("Annotation AnnotationCollector cannot be used in the sandbox")); assertNull(r.jenkins.getItem("should-not-exist")); } @@ -953,7 +1117,7 @@ public void blockFQCN() throws Exception { "import hudson.model.FreeStyleProject\n" + "@groovy.transform.ASTTest(value={ assert Jenkins.getInstance().createProject(FreeStyleProject.class, \"should-not-exist\") })\n" + "@Field int x\n" + - "echo 'hello'\n", false).toString(), containsString("Annotation groovy.transform.ASTTest cannot be used in the sandbox")); + "echo 'hello'\n", false, null).toString(), containsString("Annotation groovy.transform.ASTTest cannot be used in the sandbox")); assertNull(r.jenkins.getItem("should-not-exist")); } @@ -967,7 +1131,7 @@ public void blockImportAsBlockedAnnotation() throws Exception { "import hudson.model.FreeStyleProject\n" + "@lolwut(value={ assert Jenkins.getInstance().createProject(FreeStyleProject.class, \"should-not-exist\") })\n" + "int x\n" + - "echo 'hello'\n", false).toString(), containsString("Annotation groovy.transform.ASTTest cannot be used in the sandbox")); + "echo 'hello'\n", false, null).toString(), containsString("Annotation groovy.transform.ASTTest cannot be used in the sandbox")); assertNull(r.jenkins.getItem("should-not-exist")); } @@ -982,7 +1146,7 @@ public void blockConstructorInvocationInCheck() throws Exception { " public DoNotRunConstructor() {\n" + " assert Jenkins.getInstance().createProject(FreeStyleProject.class, \"should-not-exist\")\n" + " }\n" + - "}\n", false).toString(), containsString("OK")); + "}\n", false, null).toString(), containsString("OK")); assertNull(r.jenkins.getItem("should-not-exist")); } diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java index 1473e63f6..35295973c 100644 --- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java +++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java @@ -201,7 +201,7 @@ static final class Script extends Approvable