[SECURITY-2450]

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 

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<SecureGroo
     private final @NonNull String script;
     private final boolean sandbox;
     private final @CheckForNull List<ClasspathEntry> 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.<ClasspathEntry>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));
         }
 
     }

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 AbstractDescribableImpl<ClasspathEntry
 
     private static final long serialVersionUID = -2873408550951192200L;
     private final @NonNull URL url;
-
+    private transient String oldPath;
+    private transient boolean shouldBeApproved;
+
+    @SuppressFBWarnings(value = {"SE_TRANSIENT_FIELD_NOT_RESTORED"}, justification = "Null is the expected value for deserealized instances of this class")
     @DataBoundConstructor
     public ClasspathEntry(@NonNull String path) throws MalformedURLException {
         url = pathToURL(path);
@@ -133,7 +139,36 @@ public boolean isClassDirectory() {
             return null;
         }
     }
-
+
+    @Restricted(NoExternalUse.class) // for jelly view
+    public String getOldPath() {
+        return oldPath;
+    }
+
+    @DataBoundSetter
+    public void setOldPath(String oldPath) {
+        this.oldPath = oldPath;
+    }
+
+    public boolean isShouldBeApproved() {
+        return shouldBeApproved;
+    }
+
+    @DataBoundSetter
+    public void setShouldBeApproved(boolean shouldBeApproved) {
+        this.shouldBeApproved = shouldBeApproved;
+    }
+
+    @Restricted(NoExternalUse.class) // for jelly view
+    public boolean isScriptAutoApprovalEnabled() {
+        return ScriptApproval.ADMIN_AUTO_APPROVAL_ENABLED;
+    }
+
+    @Restricted(NoExternalUse.class) // for jelly view
+    public boolean isEntryApproved() {
+        return ScriptApproval.get().isClasspathEntryApproved(url);
+    }
+
     @Override
     public String toString() {
         return url.toString();
@@ -167,12 +202,15 @@ public String getDisplayName() {
             return "ClasspathEntry";
         }
 
-        public FormValidation doCheckPath(@QueryParameter String value) {
+        public FormValidation doCheckPath(@QueryParameter String value, @QueryParameter String oldPath, @QueryParameter boolean shouldBeApproved) {
             if (StringUtils.isBlank(value)) {
                 return FormValidation.warning("Enter a file path or URL."); // TODO I18N
             }
             try {
-                return ScriptApproval.get().checking(new ClasspathEntry(value));
+                ClasspathEntry entry = new ClasspathEntry(value);
+                entry.setShouldBeApproved(shouldBeApproved);
+                entry.setOldPath(oldPath);
+                return ScriptApproval.get().checking(entry);
             } catch (MalformedURLException x) {
                 return FormValidation.error(x, "Could not parse: " + value); // TODO I18N
             }

src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java

@@ -24,10 +24,13 @@
 
 package org.jenkinsci.plugins.scriptsecurity.scripts;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import jenkins.model.GlobalConfiguration;
 import jenkins.model.GlobalConfigurationCategory;
+import jenkins.util.SystemProperties;
 import net.sf.json.JSONArray;
 import net.sf.json.JSONObject;
+import org.apache.commons.lang.StringUtils;
 import org.jenkinsci.Symbol;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.AclAwareWhitelist;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.ProxyWhitelist;
@@ -54,7 +57,6 @@
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.Iterator;
@@ -86,6 +88,10 @@
 @Extension
 public class ScriptApproval extends GlobalConfiguration implements RootAction {
 
+    @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "for script console")
+    public static /* non-final */ boolean ADMIN_AUTO_APPROVAL_ENABLED =
+            SystemProperties.getBoolean(ScriptApproval.class.getName() + ".ADMIN_AUTO_APPROVAL_ENABLED");
+
     private static final Logger LOG = Logger.getLogger(ScriptApproval.class.getName());
 
     private static final XStream2 XSTREAM2 = new XStream2();
@@ -181,6 +187,10 @@ boolean isClassDirectory() {
         approvedClasspathEntries.add(acp);
     }
 
+    public boolean isScriptApproved(String script, Language language) {
+        return this.isScriptHashApproved(hash(script, language.getName()));
+    }
+
     @Restricted(NoExternalUse.class) // for use from Jelly
     public static abstract class PendingThing {
 
@@ -424,19 +434,23 @@ static String hashClasspathEntry(URL entry) throws IOException {
      * It should also be called from a {@code readResolve} method (which may then simply return {@code this}),
      * so that administrators can for example POST to {@code config.xml} and have their scripts be considered approved.
      * <p>If 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 " +
+                    "<a target='blank' href='"+ Jenkins.get().getRootUrl() + ScriptApproval.get().getUrlName() + "'>Script Approval Configuration</a> 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<PendingScript> getPendingScripts() {
         save();
     }
 
-    private synchronized void removePendingScript(String hash) {
+    synchronized void removePendingScript(String hash) {
         Iterator<PendingScript> it = pendingScripts.iterator();
         while (it.hasNext()) {
             if (it.next().getHash().equals(hash)) {

src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript/config.jelly

@@ -25,6 +25,7 @@ THE SOFTWARE.
 
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
+    <input type="hidden" name="oldScript" value="${instance.script}" />
     <f:entry field="script" title="${%Groovy Script}">
         <!-- TODO <st:adjunct includes="org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.JENKINS-15604"/> -->
         <!-- TODO https://github.com/stapler/stapler-adjunct-codemirror/issues/1 means no true Groovy support -->

src/main/resources/org/jenkinsci/plugins/scriptsecurity/scripts/ClasspathEntry/config.jelly

@@ -24,10 +24,21 @@ THE SOFTWARE.
 -->
 
 <?jelly escape-by-default='true'?>
-<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
+<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:st="jelly:stapler">
+    <j:if test="${!instance.scriptAutoApprovalEnabled}">
+        <f:invisibleEntry>
+            <f:textbox clazz="secure-groovy-script__classpath-entry-old-path" field="oldPath" value="${instance.path}"/>
+        </f:invisibleEntry>
+        <st:adjunct includes="org.jenkinsci.plugins.scriptsecurity.scripts.ClasspathEntry.resources"/>
+    </j:if>
     <f:entry field="path" title="${%JAR file path or URL}">
-        <f:textbox />
+        <f:textbox clazz="secure-groovy-script__classpath-entry-path" />
     </f:entry>
+    <j:if test="${!instance.scriptAutoApprovalEnabled and h.hasPermission(app.ADMINISTER) and !instance.entryApproved}">
+        <f:entry field="shouldBeApproved">
+            <f:checkbox class="secure-groovy-script__classpath-approve" title="${%Approve the classpath entry on save}" checked="false"/>
+        </f:entry>
+    </j:if>
     <f:entry title="">
       <div align="right">
         <f:repeatableDeleteButton />