Skip to content

Commit

Permalink
[JENKINS-35210] Fix for SECURITY-170
Browse files Browse the repository at this point in the history
  • Loading branch information
@amuniz
amuniz committed Jun 6, 2016
1 parent 8a35859 commit 72cde84
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 8 deletions.
29 changes: 28 additions & 1 deletion src/main/java/org/jenkinsci/plugins/p4/review/ReviewAction.java
View file
Expand Up @@ -3,6 +3,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Enumeration;
import java.util.Iterator;
import java.util.List;
import java.util.logging.Logger;

Expand Down Expand Up @@ -95,7 +96,9 @@ public void doBuild(StaplerRequest req, StaplerResponse rsp) throws IOException,
// Schedule build
TimeDuration delay = new TimeDuration(project.getQuietPeriod());
CauseAction cause = new CauseAction(new Cause.UserIdCause());
ParametersAction params = new ParametersAction(values);

List<ParameterValue> internalParams = extractAndRemoveInternalParameters(values);
ParametersAction params = new SafeParametersAction(values, internalParams);

Jenkins j = Jenkins.getInstance();
if (j != null) {
Expand All @@ -107,6 +110,30 @@ public void doBuild(StaplerRequest req, StaplerResponse rsp) throws IOException,
}
}

/**
* It extracts and removes internal parameters from the full list of parameters as they need to be managed
* in a special way for security reasons (related to SECURITY-170).
*
* @param values internal parameters values (internal parameters are defined in {@link #getParameterDefinitions()}
* @return internal parameters values
*/
private List<ParameterValue> extractAndRemoveInternalParameters(List<ParameterValue> values) {
List<ParameterValue> internal = new ArrayList<ParameterValue>();
List<ParameterDefinition> parameterDefinitions = getParameterDefinitions();
Iterator<ParameterValue> it = values.iterator();
while(it.hasNext()) {
ParameterValue next = it.next();
for (ParameterDefinition pd : parameterDefinitions) {
if (next.getName().equals(pd.getName())) {
internal.add(next);
it.remove();
break;
}
}
}
return internal;
}

private List<ParameterDefinition> getParameterDefinitions() {
List<ParameterDefinition> swarm = new ArrayList<ParameterDefinition>();

Expand Down
74 changes: 74 additions & 0 deletions src/main/java/org/jenkinsci/plugins/p4/review/SafeParametersAction.java
View file
@@ -0,0 +1,74 @@
package org.jenkinsci.plugins.p4.review;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.EnvVars;
import hudson.Extension;
import hudson.model.EnvironmentContributor;
import hudson.model.ParameterValue;
import hudson.model.ParametersAction;
import hudson.model.Run;
import hudson.model.TaskListener;

@Restricted(NoExternalUse.class)
public class SafeParametersAction extends ParametersAction {

private List<ParameterValue> internalParameters;

public SafeParametersAction(@NonNull List<ParameterValue> params, @NonNull List<ParameterValue> internalParameters) {
// Apply security to regular parameters
super(params);
this.internalParameters = internalParameters;
}

@Override
public List<ParameterValue> getParameters() {
List<ParameterValue> params = new ArrayList<ParameterValue>();
List<ParameterValue> p = super.getParameters();
params.addAll(p);
params.addAll(internalParameters);
return Collections.unmodifiableList(params);
}

@Override
public ParameterValue getParameter(String name) {
ParameterValue param = super.getParameter(name);
if (param != null) {
return param;
}
for (ParameterValue p : internalParameters) {
if (p == null) continue;
if (p.getName().equals(name))
return p;
}
return null;
}

List<ParameterValue> getInternalParameters() {
return Collections.unmodifiableList(internalParameters);
}

@Extension
public static final class SafeParametersActionEnvironmentContributor extends EnvironmentContributor {

@Override
public void buildEnvironmentFor(Run r, EnvVars envs, TaskListener listener)
throws IOException, InterruptedException {
SafeParametersAction action = r.getAction(SafeParametersAction.class);
if (action != null) {
for (ParameterValue pv : action.getInternalParameters()) {
envs.put(pv.getName(), String.valueOf(pv.getValue()));
}
}
}

}

}
15 changes: 8 additions & 7 deletions src/test/java/org/jenkinsci/plugins/p4/client/ConnectionTest.java
View file
Expand Up @@ -10,6 +10,7 @@
import java.net.InetAddress;
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -26,6 +27,7 @@
import org.jenkinsci.plugins.p4.populate.AutoCleanImpl;
import org.jenkinsci.plugins.p4.populate.Populate;
import org.jenkinsci.plugins.p4.review.ReviewProp;
import org.jenkinsci.plugins.p4.review.SafeParametersAction;
import org.jenkinsci.plugins.p4.workspace.ManualWorkspaceImpl;
import org.jenkinsci.plugins.p4.workspace.StaticWorkspaceImpl;
import org.jenkinsci.plugins.p4.workspace.StreamWorkspaceImpl;
Expand Down Expand Up @@ -60,7 +62,6 @@
import hudson.model.FreeStyleBuild;
import hudson.model.FreeStyleProject;
import hudson.model.ParameterValue;
import hudson.model.ParametersAction;
import hudson.model.Result;
import hudson.model.StringParameterValue;
import hudson.model.Cause.UserIdCause;
Expand Down Expand Up @@ -232,7 +233,7 @@ public void testFreeStyleProject_buildChange() throws Exception {
List<ParameterValue> list = new ArrayList<ParameterValue>();
list.add(new StringParameterValue(ReviewProp.STATUS.toString(), "committed"));
list.add(new StringParameterValue(ReviewProp.CHANGE.toString(), "9"));
Action actions = new ParametersAction(list);
Action actions = new SafeParametersAction(new ArrayList<ParameterValue>(), list);

FreeStyleBuild build;
UserIdCause cause = new Cause.UserIdCause();
Expand Down Expand Up @@ -287,7 +288,7 @@ public void testFreeStyleProject_buildLabel() throws Exception {
list.add(new StringParameterValue(ReviewProp.STATUS.toString(), "committed"));
list.add(new StringParameterValue(ReviewProp.LABEL.toString(), "auto15"));
list.add(new StringParameterValue(ReviewProp.PASS.toString(), HTTP_URL + "/pass"));
Action actions = new ParametersAction(list);
Action actions = new SafeParametersAction(new ArrayList<ParameterValue>(), list);

FreeStyleBuild build;
UserIdCause cause = new Cause.UserIdCause();
Expand Down Expand Up @@ -344,7 +345,7 @@ public void testFreeStyleProject_buildShelf() throws Exception {
list.add(new StringParameterValue(ReviewProp.STATUS.toString(), "shelved"));
list.add(new StringParameterValue(ReviewProp.REVIEW.toString(), "19"));
list.add(new StringParameterValue(ReviewProp.PASS.toString(), HTTP_URL + "/pass"));
Action actions = new ParametersAction(list);
Action actions = new SafeParametersAction(new ArrayList<ParameterValue>(), list);

FreeStyleBuild build;
UserIdCause cause = new Cause.UserIdCause();
Expand Down Expand Up @@ -541,7 +542,7 @@ public void testTPI95() throws Exception {
List<ParameterValue> list = new ArrayList<ParameterValue>();
list.add(new StringParameterValue(ReviewProp.STATUS.toString(), "shelved"));
list.add(new StringParameterValue(ReviewProp.REVIEW.toString(), "19"));
Action actions = new ParametersAction(list);
Action actions = new SafeParametersAction(new ArrayList<ParameterValue>(), list);

FreeStyleBuild build;
UserIdCause cause = new Cause.UserIdCause();
Expand Down Expand Up @@ -578,7 +579,7 @@ public void testPolling_Pin() throws Exception {
List<ParameterValue> list = new ArrayList<ParameterValue>();
list.add(new StringParameterValue(ReviewProp.STATUS.toString(), "submitted"));
list.add(new StringParameterValue(ReviewProp.CHANGE.toString(), "3"));
Action actions = new ParametersAction(list);
Action actions = new SafeParametersAction(new ArrayList<ParameterValue>(), list);

FreeStyleBuild build;
UserIdCause cause = new Cause.UserIdCause();
Expand Down Expand Up @@ -617,7 +618,7 @@ public void testPolling_Inc() throws Exception {
List<ParameterValue> list = new ArrayList<ParameterValue>();
list.add(new StringParameterValue(ReviewProp.STATUS.toString(), "submitted"));
list.add(new StringParameterValue(ReviewProp.CHANGE.toString(), "3"));
Action actions = new ParametersAction(list);
Action actions = new SafeParametersAction(new ArrayList<ParameterValue>(), list);

FreeStyleBuild build;
UserIdCause cause = new Cause.UserIdCause();
Expand Down

0 comments on commit 72cde84

Please sign in to comment.