Skip to content

Commit

Permalink
[SECURITY-2705]
Browse files Browse the repository at this point in the history
  • Loading branch information
jtnord committed Jun 14, 2022
1 parent 37cea9a commit 77f0e8b
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 7 deletions.
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package org.jenkinsci.plugins.workflow.support.steps.input;

import hudson.Extension;
import hudson.ExtensionList;
import hudson.Util;
import hudson.model.FileParameterDefinition;
import hudson.model.ParameterDefinition;
import hudson.model.PasswordParameterDefinition;
import hudson.model.Run;
import hudson.model.TaskListener;
import hudson.model.ParameterDefinition.ParameterDescriptor;
import hudson.util.Secret;
import java.io.Serializable;
import java.util.Collections;
Expand Down Expand Up @@ -247,5 +250,14 @@ private static <T> Map<String, Object> copyMapReplacingEntry(Map<String, ?> map,
}
return newMap;
}

/** For the pipeline syntax generator page. */
public List<ParameterDescriptor> getParametersDescriptors() {
// See SECURITY-2705 on why we ban FileParemeterDefinition
return ExtensionList.lookup(ParameterDescriptor.class).stream().
filter(descriptor -> descriptor.clazz != FileParameterDefinition.class).
collect(Collectors.toList());
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

import com.cloudbees.plugins.credentials.CredentialsParameterValue;
import com.cloudbees.plugins.credentials.builds.CredentialsParameterBinder;
import hudson.AbortException;
import hudson.FilePath;
import hudson.Util;
import hudson.console.HyperlinkNote;
import hudson.model.Failure;
import hudson.model.FileParameterDefinition;
import hudson.model.FileParameterValue;
import hudson.model.Job;
import hudson.model.ModelObject;
Expand All @@ -29,6 +31,8 @@
import org.jenkinsci.plugins.workflow.steps.AbstractStepExecutionImpl;
import org.jenkinsci.plugins.workflow.support.actions.PauseAction;
import org.jenkinsci.plugins.workflow.graph.FlowNode;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.interceptor.RequirePOST;
Expand All @@ -45,6 +49,8 @@
import java.util.concurrent.TimeoutException;
import java.util.logging.Level;
import java.util.logging.Logger;

import jenkins.util.SystemProperties;
import jenkins.util.Timer;
import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException;
import org.jenkinsci.plugins.workflow.steps.StepContext;
Expand All @@ -56,6 +62,13 @@ public class InputStepExecution extends AbstractStepExecutionImpl implements Mod

private static final Logger LOGGER = Logger.getLogger(InputStepExecution.class.getName());

// for testing only
static final String UNSAFE_PARAMETER_ALLOWED_PROPERTY_NAME = InputStepExecution.class.getName() + ".supportUnsafeParameters";

private static boolean isAllowUnsafeParameters() {
return SystemProperties.getBoolean(UNSAFE_PARAMETER_ALLOWED_PROPERTY_NAME);
}

/**
* Result of the input.
*/
Expand All @@ -70,6 +83,20 @@ public class InputStepExecution extends AbstractStepExecutionImpl implements Mod

@Override
public boolean start() throws Exception {
// SECURITY-2705 if the escape hatch is allowed just warn about pending removal, otherwise fail the build before waiting
if (getHasUnsafeParameters()) {
if (isAllowUnsafeParameters()) {
getListener().getLogger().println("Support for FileParameters in the input step has been enabled via "
+ UNSAFE_PARAMETER_ALLOWED_PROPERTY_NAME + " which will be removed in a future release." +
System.lineSeparator() +
"Details on how to migrate your pipeline can be found online: https://jenkins.io/redirect/plugin/pipeline-input-step/file-parameters.");
} else {
throw new AbortException("Support for FileParameters in the input step is disabled and will be removed in a future release. " +
System.lineSeparator() + "Details on how to migrate your pipeline can be found online: " +
"https://jenkins.io/redirect/plugin/pipeline-input-step/file-parameters.");
}
}

Run<?, ?> run = getRun();
TaskListener listener = getListener();
FlowNode node = getNode();
Expand Down Expand Up @@ -406,15 +433,28 @@ private Map<String,Object> parseValue(StaplerRequest request) throws ServletExce
}

private Object convert(String name, ParameterValue v) throws IOException, InterruptedException {
if (v instanceof FileParameterValue) {
FileParameterValue fv = (FileParameterValue) v;
FilePath fp = new FilePath(getRun().getRootDir()).child(name);
fp.copyFrom(fv.getFile());
return fp;
if (v instanceof FileParameterValue) { // SECURITY-2705
if (isAllowUnsafeParameters()) {
FileParameterValue fv = (FileParameterValue) v;
FilePath fp = new FilePath(getRun().getRootDir()).child(name);
fp.copyFrom(fv.getFile());
return fp;
} else {
// whilst the step would be aborted in start() if the pipeline was in the input step at the point of
// upgrade it will be allowed to pass so we pick it up here.
throw new AbortException("Support for FileParameters in the input step is disabled and will be removed in a future release. " +
System.lineSeparator() + "Details on how to migrate your pipeline can be found online: " +
"https://jenkins.io/redirect/plugin/pipeline-input-step/file-parameters.");
}
} else {
return v.getValue();
}
}

@Restricted(NoExternalUse.class) // jelly access only
public boolean getHasUnsafeParameters() {
return input.getParameters().stream().anyMatch(parameter -> parameter.getClass() == FileParameterDefinition.class);
}

private static final long serialVersionUID = 1L;
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@
<h1>${it.input.message}</h1>
<j:if test="${!it.completed}">
<f:form method="post" action="${it.id}/submit" name="${it.id}">
<j:if test="${it.hasUnsafeParameters}">
<div class="alert alert-warning">
Support for <code>FileParameter</code>s will be removed in a future release.
Details on how to migrate your pipeline can be found
<a rel="noopener noreferrer" href="https://jenkins.io/redirect/plugin/pipeline-input-step/file-parameters">online</a>.
</div>
</j:if>
<j:forEach var="param" items="${it.input.parameters}">
<j:set var="escapeEntryTitleAndDescription" value="true"/>
<st:include page="index.jelly" it="${param}"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.gargoylesoftware.htmlunit.WebRequest;
import com.gargoylesoftware.htmlunit.html.HtmlAnchor;
import com.gargoylesoftware.htmlunit.html.HtmlElementUtil;
import com.gargoylesoftware.htmlunit.html.HtmlFileInput;
import com.gargoylesoftware.htmlunit.html.HtmlForm;
import com.gargoylesoftware.htmlunit.html.HtmlPage;
import com.google.common.base.Predicate;
Expand All @@ -48,12 +49,14 @@
import hudson.model.queue.QueueTaskFuture;


import java.io.File;
import java.io.IOException;

import hudson.security.ACL;
import hudson.security.ACLContext;
import hudson.util.Secret;
import jenkins.model.IdStrategy;
import jenkins.model.InterruptedBuildAction;
import jenkins.model.Jenkins;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.plugins.plaincredentials.impl.StringCredentialsImpl;
Expand All @@ -63,11 +66,11 @@
import org.jenkinsci.plugins.workflow.graphanalysis.DepthFirstScanner;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.junit.Assert;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.FlagRule;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;

Expand All @@ -80,15 +83,28 @@
import edu.umd.cs.findbugs.annotations.Nullable;
import org.jvnet.hudson.test.recipes.LocalData;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertNotNull;

/**
* @author Kohsuke Kawaguchi
*/
public class InputStepTest extends Assert {
public class InputStepTest {
@Rule public JenkinsRule j = new JenkinsRule();

@ClassRule
public static BuildWatcher buildWatcher = new BuildWatcher();

@Rule public FlagRule<String> allowUnsafeParams = FlagRule.systemProperty(InputStepExecution.UNSAFE_PARAMETER_ALLOWED_PROPERTY_NAME, null);

/**
* Try out a parameter.
*/
Expand Down Expand Up @@ -475,6 +491,59 @@ public void passwordParameters() throws Exception {
j.assertLogContains("Password is mySecret", b);
}

@Issue("SECURITY-2705")
@Test
public void fileParameterWithEscapeHatch() throws Exception {
System.setProperty(InputStepExecution.UNSAFE_PARAMETER_ALLOWED_PROPERTY_NAME, "true");
WorkflowJob foo = j.jenkins.createProject(WorkflowJob.class, "foo");
foo.setDefinition(new CpsFlowDefinition("node {\n" +
"input message: 'Please provide a file', parameters: [file('paco.txt')], id: 'Id' \n" +
" }",true));

// get the build going, and wait until workflow pauses
QueueTaskFuture<WorkflowRun> q = foo.scheduleBuild2(0);
WorkflowRun b = q.waitForStart();
j.waitForMessage("Input requested", b);

InputAction action = b.getAction(InputAction.class);
assertEquals(1, action.getExecutions().size());

// submit the input, and expect a failure, no need to set any file value as the check we are testing takes
// place before we try to interact with the file
JenkinsRule.WebClient wc = j.createWebClient();
HtmlPage p = wc.getPage(b, action.getUrlName());
HtmlForm f = p.getFormByName("Id");
HtmlFileInput fileInput = f.getInputByName("file");
fileInput.setValueAttribute("dummy.txt");
fileInput.setContentType("text/csv");
String currentTime = "Current time " + System.currentTimeMillis();
fileInput.setData(currentTime.getBytes());
j.submit(f, "proceed");

j.assertBuildStatus(Result.SUCCESS, j.waitForCompletion(b));
assertTrue(new File(b.getRootDir(), "paco.txt").exists());
assertThat(JenkinsRule.getLog(b),
allOf(containsString(InputStepExecution.UNSAFE_PARAMETER_ALLOWED_PROPERTY_NAME),
containsString("will be removed in a future release"),
containsString("https://jenkins.io/redirect/plugin/pipeline-input-step/file-parameters")));
}

@Issue("SECURITY-2705")
@Test
public void fileParameterShouldFailAtRuntime() throws Exception {
WorkflowJob foo = j.jenkins.createProject(WorkflowJob.class, "foo");
foo.setDefinition(new CpsFlowDefinition("input message: 'Please provide a file', parameters: [file('paco.txt')], id: 'Id'",true));

// get the build going, and wait until workflow pauses
QueueTaskFuture<WorkflowRun> q = foo.scheduleBuild2(0);
WorkflowRun b = q.waitForStart();

j.assertBuildStatus(Result.FAILURE, j.waitForCompletion(b));
assertThat(JenkinsRule.getLog(b),
allOf(not(containsString(InputStepExecution.UNSAFE_PARAMETER_ALLOWED_PROPERTY_NAME)),
containsString("https://jenkins.io/redirect/plugin/pipeline-input-step/file-parameters")));
}

@LocalData
@Test public void serialForm() throws Exception {
WorkflowJob p = j.jenkins.getItemByFullName("p", WorkflowJob.class);
Expand Down

0 comments on commit 77f0e8b

Please sign in to comment.