Merge branch 'security-stable-1.609' into security-stable-1.625

core/src/main/java/hudson/model/ParametersAction.java

@@ -32,9 +32,13 @@ import hudson.model.queue.SubTask;
 import hudson.tasks.BuildStep;
 import hudson.tasks.BuildWrapper;
 import hudson.util.VariableResolver;
+import jenkins.model.RunAction2;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.export.Exported;
 import org.kohsuke.stapler.export.ExportedBean;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -42,6 +46,8 @@ import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 import static com.google.common.collect.Lists.newArrayList;
 import static com.google.common.collect.Sets.newHashSet;
@@ -57,16 +63,30 @@ import javax.annotation.Nonnull;
  * that were specified when scheduling.
  */
 @ExportedBean
-public class ParametersAction implements Action, Iterable<ParameterValue>, QueueAction, EnvironmentContributingAction, LabelAssignmentAction {
+public class ParametersAction implements RunAction2, Iterable<ParameterValue>, QueueAction, EnvironmentContributingAction, LabelAssignmentAction {
+
+    @Restricted(NoExternalUse.class)
+    public static final String KEEP_UNDEFINED_PARAMETERS_SYSTEM_PROPERTY_NAME = ParametersAction.class.getName() +
+            ".keepUndefinedParameters";
+
+    @Restricted(NoExternalUse.class)
+    public static final String SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME = ParametersAction.class.getName() +
+            ".safeParameters";
+
+    private transient List<String> safeParameters;
 
     private final List<ParameterValue> parameters;
 
+    private List<String> parameterDefinitionNames;
+
     /**
      * @deprecated since 1.283; kept to avoid warnings loading old build data, but now transient.
      */
     @Deprecated
     private transient AbstractBuild<?, ?> build;
 
+    private transient Run<?, ?> run;
+
     public ParametersAction(List<ParameterValue> parameters) {
         this.parameters = parameters;
     }
@@ -76,7 +96,7 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
     }
 
     public void createBuildWrappers(AbstractBuild<?,?> build, Collection<? super BuildWrapper> result) {
-        for (ParameterValue p : parameters) {
+        for (ParameterValue p : getParameters()) {
             if (p == null) continue;
             BuildWrapper w = p.createBuildWrapper(build);
             if(w!=null) result.add(w);
@@ -84,7 +104,7 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
     }
 
     public void buildEnvVars(AbstractBuild<?,?> build, EnvVars env) {
-        for (ParameterValue p : parameters) {
+        for (ParameterValue p : getParameters()) {
             if (p == null) continue;
             p.buildEnvironment(build, env); 
         }
@@ -106,9 +126,9 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
      * If you are a {@link BuildStep}, most likely you should call {@link AbstractBuild#getBuildVariableResolver()}. 
      */
     public VariableResolver<String> createVariableResolver(AbstractBuild<?,?> build) {
-        VariableResolver[] resolvers = new VariableResolver[parameters.size()+1];
+        VariableResolver[] resolvers = new VariableResolver[getParameters().size()+1];
         int i=0;
-        for (ParameterValue p : parameters) {
+        for (ParameterValue p : getParameters()) {
             if (p == null) continue;
             resolvers[i++] = p.createVariableResolver(build);
         }
@@ -119,12 +139,12 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
     }
     
     public Iterator<ParameterValue> iterator() {
-        return parameters.iterator();
+        return getParameters().iterator();
     }
 
     @Exported(visibility=2)
     public List<ParameterValue> getParameters() {
-        return Collections.unmodifiableList(parameters);
+        return Collections.unmodifiableList(filter(parameters));
     }
 
     public ParameterValue getParameter(String name) {
@@ -137,7 +157,7 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
     }
 
     public Label getAssignedLabel(SubTask task) {
-        for (ParameterValue p : parameters) {
+        for (ParameterValue p : getParameters()) {
             if (p == null) continue;
             Label l = p.getAssignedLabel(task);
             if (l!=null)    return l;
@@ -212,7 +232,7 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
         if (overrides == null) {
             return new ParametersAction(parameters);
         }
-        return createUpdated(overrides.getParameters());
+        return createUpdated(overrides.parameters);
     }
 
     private Object readResolve() {
@@ -220,4 +240,78 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
             OldDataMonitor.report(build, "1.283");
         return this;
     }
+
+    @Override
+    public void onAttached(Run<?, ?> r) {
+        ParametersDefinitionProperty p = r.getParent().getProperty(ParametersDefinitionProperty.class);
+        if (p != null) {
+            this.parameterDefinitionNames = p.getParameterDefinitionNames();
+        } else {
+            this.parameterDefinitionNames = Collections.emptyList();
+        }
+        this.run = r;
+    }
+
+    @Override
+    public void onLoad(Run<?, ?> r) {
+        this.run = r;
+    }
+
+    private List<? extends ParameterValue> filter(List<ParameterValue> parameters) {
+        if (this.run == null) {
+            return parameters;
+        }
+
+        if (this.parameterDefinitionNames == null) {
+            return parameters;
+        }
+
+        if (Boolean.getBoolean(KEEP_UNDEFINED_PARAMETERS_SYSTEM_PROPERTY_NAME)) {
+            return parameters;
+        }
+
+        List<ParameterValue> filteredParameters = new ArrayList<ParameterValue>();
+
+        for (ParameterValue v : this.parameters) {
+            if (this.parameterDefinitionNames.contains(v.getName()) || isSafeParameter(v.getName())) {
+                filteredParameters.add(v);
+            } else {
+                LOGGER.log(Level.WARNING, "Skipped parameter `{0}` as it is undefined on `{1}`. Set `-D{2}`=true to allow "
+                        + "undefined parameters to be injected as environment variables or `-D{3}=[comma-separated list]` to whitelist specific parameter names, "
+                        + "even though it represents a security breach",
+                        new Object [] { v.getName(), run.getParent().getFullName(), KEEP_UNDEFINED_PARAMETERS_SYSTEM_PROPERTY_NAME, SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME });
+            }
+        }
+
+        return filteredParameters;
+    }
+
+    /**
+     * Returns all parameters.
+     *
+     * Be careful in how you process them. It will return parameters even not being defined as
+     * {@link ParametersDefinitionProperty} in the job, so any external
+     * caller could inject any parameter (using any key) here. <strong>Treat it as untrusted data</strong>.
+     *
+     * @return all parameters defined here.
+     * @since TODO
+     */
+    public List<ParameterValue> getAllParameters() {
+        return Collections.unmodifiableList(parameters);
+    }
+
+    private boolean isSafeParameter(String name) {
+        if (safeParameters == null) {
+            String paramNames = System.getProperty(SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME);
+            if (paramNames != null) {
+                safeParameters = Arrays.asList(paramNames.split(","));
+            } else {
+                safeParameters = Collections.emptyList();
+            }
+        }
+        return safeParameters.contains(name);
+    }
+
+    private static final Logger LOGGER = Logger.getLogger(ParametersAction.class.getName());
+
 }

test/src/test/groovy/hudson/cli/SetBuildParameterCommandTest.groovy

@@ -4,7 +4,10 @@ import hudson.Launcher
 import hudson.model.AbstractBuild
 import hudson.model.BuildListener
 import hudson.model.ParametersAction
+import hudson.model.ParametersDefinitionProperty
+import hudson.model.ParameterDefinition
 import hudson.model.Result
+import hudson.model.StringParameterDefinition
 import hudson.tasks.Shell
 import jenkins.model.JenkinsLocationConfiguration
 import org.junit.Assert
@@ -34,6 +37,8 @@ public class SetBuildParameterCommandTest {
                 return true;
             }
         });
+        List<ParameterDefinition> pd = [new StringParameterDefinition("a", ""), new StringParameterDefinition("b", "")];
+        p.addProperty(new ParametersDefinitionProperty(pd))
         p.buildersList.add(new Shell("java -jar cli.jar set-build-parameter a b"))
         p.buildersList.add(new Shell("java -jar cli.jar set-build-parameter a x"))
         p.buildersList.add(new Shell("java -jar cli.jar set-build-parameter b y"))

test/src/test/java/hudson/model/ParametersActionTest2.java

+package hudson.model;
+
+import hudson.Launcher;
+import hudson.tasks.Builder;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.recipes.LocalData;
+
+import java.io.IOException;
+import java.util.Arrays;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class ParametersActionTest2 {
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+
+    @Test
+    @Issue("SECURITY-170")
+    public void undefinedParameters() throws Exception {
+        FreeStyleProject p = j.createFreeStyleProject();
+        p.addProperty(new ParametersDefinitionProperty(Arrays.asList(new ParameterDefinition[]{
+                new StringParameterDefinition("foo", "foo"),
+                new StringParameterDefinition("bar", "bar")
+        })));
+        ParametersCheckBuilder b = new ParametersCheckBuilder(false);
+        p.getBuildersList().add(b);
+        p.save();
+
+        j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
+                new StringParameterValue("foo", "baz"),
+                new StringParameterValue("undef", "undef")
+        )));
+    }
+
+    @Test
+    @Issue("SECURITY-170")
+    public void undefinedParametersOverride() throws Exception {
+        FreeStyleProject p = j.createFreeStyleProject();
+        p.addProperty(new ParametersDefinitionProperty(Arrays.asList(new ParameterDefinition[]{
+                new StringParameterDefinition("foo", "foo"),
+                new StringParameterDefinition("bar", "bar")
+        })));
+        ParametersCheckBuilder b = new ParametersCheckBuilder(true);
+        p.getBuildersList().add(b);
+        p.save();
+        try {
+            System.setProperty(ParametersAction.KEEP_UNDEFINED_PARAMETERS_SYSTEM_PROPERTY_NAME, "true");
+
+            j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
+                    new StringParameterValue("foo", "baz"),
+                    new StringParameterValue("undef", "undef")
+            )));
+        } finally {
+            System.clearProperty(ParametersAction.KEEP_UNDEFINED_PARAMETERS_SYSTEM_PROPERTY_NAME);
+        }
+    }
+
+    @Test
+    @Issue("SECURITY-170")
+    @LocalData
+    public void backwardCompatibility() throws Exception {
+        // Local data contains a parameterized job with two parameters (FOO and BAR) and one build
+        // with pre-fix format (generated with 1.609.3) with FOO, BAR and UNDEF.
+        FreeStyleProject p = j.jenkins.getItemByFullName("parameterized", FreeStyleProject.class);
+        FreeStyleBuild b1 = p.getBuildByNumber(1);
+        ParametersAction pa = b1.getAction(ParametersAction.class);
+        hasParameterWithName(pa, "FOO");
+        hasParameterWithName(pa, "BAR");
+        // legacy behaviour expected (UNDEF returned by getParameters())
+        hasParameterWithName(pa, "UNDEF");
+
+        // A new build should work as expected (undef is not published to env)
+        ParametersCheckBuilder b = new ParametersCheckBuilder(false);
+        p.getBuildersList().add(b);
+        p.save();
+
+        j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
+                new StringParameterValue("foo", "baz"),
+                new StringParameterValue("undef", "undef")
+        )));
+    }
+
+    @Test
+    @Issue("SECURITY-170")
+    public void parametersDefinitionChange() throws Exception {
+        FreeStyleProject p = j.createFreeStyleProject();
+        p.addProperty(new ParametersDefinitionProperty(Arrays.<ParameterDefinition>asList(
+                new StringParameterDefinition("foo", "foo"),
+                new StringParameterDefinition("bar", "bar"))));
+
+        FreeStyleBuild build = j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
+                new StringParameterValue("foo", "baz"),
+                new StringParameterValue("bar", "bar"),
+                new StringParameterValue("undef", "undef")
+        )));
+
+        assertTrue("undef parameter is not listed in getParameters",
+                !hasParameterWithName(build.getAction(ParametersAction.class), "undef"));
+
+        p.removeProperty(ParametersDefinitionProperty.class);
+        p.addProperty(new ParametersDefinitionProperty(Arrays.<ParameterDefinition>asList(
+                new StringParameterDefinition("foo", "foo"),
+                new StringParameterDefinition("bar", "bar"),
+                new StringParameterDefinition("undef", "undef"))));
+
+        // undef is still not listed even after being added to the job parameters definition
+        assertTrue("undef parameter is not listed in getParameters",
+                !hasParameterWithName(build.getAction(ParametersAction.class), "undef"));
+
+        // remove bar and undef from parameters definition
+        p.removeProperty(ParametersDefinitionProperty.class);
+        p.addProperty(new ParametersDefinitionProperty(Arrays.<ParameterDefinition>asList(
+                new StringParameterDefinition("foo", "foo"))));
+
+        assertTrue("the build still have 2 parameters", build.getAction(ParametersAction.class).getParameters().size() == 2);
+
+        p.removeProperty(ParametersDefinitionProperty.class);
+        assertTrue("the build still have 2 parameters", build.getAction(ParametersAction.class).getParameters().size() == 2);
+    }
+
+    @Test
+    @Issue("SECURITY-170")
+    public void whitelistedParameter() throws Exception {
+        FreeStyleProject p = j.createFreeStyleProject();
+        p.addProperty(new ParametersDefinitionProperty(Arrays.<ParameterDefinition>asList(
+                new StringParameterDefinition("foo", "foo"),
+                new StringParameterDefinition("bar", "bar"))));
+
+        try {
+            System.setProperty(ParametersAction.SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME, "whitelisted1,whitelisted2");
+            FreeStyleBuild build = j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
+                    new StringParameterValue("foo", "baz"),
+                    new StringParameterValue("bar", "bar"),
+                    new StringParameterValue("whitelisted1", "x"),
+                    new StringParameterValue("whitelisted2", "y")
+            )));
+
+            assertTrue("whitelisted1 parameter is listed in getParameters",
+                    hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted1"));
+            assertTrue("whitelisted2 parameter is listed in getParameters",
+                    hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted2"));
+        } finally {
+            System.clearProperty(ParametersAction.SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME);
+        }
+    }
+
+    @Test
+    @Issue("SECURITY-170")
+    public void nonParameterizedJob() throws Exception {
+        FreeStyleProject p = j.createFreeStyleProject();
+        FreeStyleBuild build = j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
+                new StringParameterValue("foo", "baz"),
+                new StringParameterValue("bar", "bar")
+        )));
+
+        assertTrue("foo parameter is not listed in getParameters",
+                !hasParameterWithName(build.getAction(ParametersAction.class), "foo"));
+        assertTrue("bar parameter is not listed in getParameters",
+                !hasParameterWithName(build.getAction(ParametersAction.class), "bar"));
+    }
+
+    @Test
+    @Issue("SECURITY-170")
+    public void nonParameterizedJobButWhitelisted() throws Exception {
+        FreeStyleProject p = j.createFreeStyleProject();
+
+        try {
+            System.setProperty(ParametersAction.SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME, "foo,bar");
+            FreeStyleBuild build2 = j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction(
+                    new StringParameterValue("foo", "baz"),
+                    new StringParameterValue("bar", "bar")
+            )));
+
+            assertTrue("foo parameter is listed in getParameters",
+                    hasParameterWithName(build2.getAction(ParametersAction.class), "foo"));
+            assertTrue("bar parameter is listed in getParameters",
+                    hasParameterWithName(build2.getAction(ParametersAction.class), "bar"));
+        } finally {
+            System.clearProperty(ParametersAction.SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME);
+        }
+    }
+
+    public static boolean hasParameterWithName(Iterable<ParameterValue> values, String name) {
+        for (ParameterValue v : values) {
+            if (v.getName().equals(name)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    public static class ParametersCheckBuilder extends Builder {
+
+        private final boolean expectLegacyBehavior;
+
+        public ParametersCheckBuilder(boolean expectLegacyBehavior) {
+            this.expectLegacyBehavior = expectLegacyBehavior;
+        }
+
+        @Override
+        public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListener listener)
+                throws InterruptedException, IOException {
+            ParametersAction pa = build.getAction(ParametersAction.class);
+            assertEquals("foo value expected changed", "baz", pa.getParameter("foo").getValue());
+
+            if (expectLegacyBehavior) {
+                assertTrue("undef parameter is listed in getParameters", hasParameterWithName(pa.getParameters(), "undef"));
+                assertTrue("undef parameter is listed in iterator", hasParameterWithName(pa, "undef"));
+                assertTrue("undef in environment", build.getEnvironment(listener).keySet().contains("undef"));
+                assertTrue("UNDEF in environment", build.getEnvironment(listener).keySet().contains("UNDEF"));
+            } else {
+                assertFalse("undef parameter is not listed in getParameters", hasParameterWithName(pa.getParameters(), "undef"));
+                assertFalse("undef parameter is not listed in iterator", hasParameterWithName(pa, "undef"));
+                assertFalse("undef not in environment", build.getEnvironment(listener).keySet().contains("undef"));
+                assertFalse("UNDEF not in environment", build.getEnvironment(listener).keySet().contains("UNDEF"));
+            }
+
+            assertTrue("undef parameter is listed in getAllParameters", hasParameterWithName(pa.getAllParameters(), "undef"));
+            assertEquals("undef parameter direct access expected to work", "undef", pa.getParameter("undef").getValue());
+
+            return true;
+        }
+    }
+}