Skip to content

Commit

Permalink
[SECURITY-2196]
Browse files Browse the repository at this point in the history
  • Loading branch information
@rsandell
rsandell committed May 9, 2023
1 parent 8e8289e commit 0ba4f32
Show file tree
Hide file tree
Showing 8 changed files with 248 additions and 7 deletions.
31 changes: 31 additions & 0 deletions src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/AbstractFileCallable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,12 @@
import hudson.FilePath;
import jenkins.MasterToSlaveFileCallable;

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

public abstract class AbstractFileCallable<T> extends MasterToSlaveFileCallable<T> {
private FilePath destination;
private boolean allowExtractionOutsideDestination = false;

public FilePath getDestination() {
return destination;
Expand All @@ -37,4 +41,31 @@ public FilePath getDestination() {
public void setDestination(FilePath destination) {
this.destination = destination;
}

/**
* SECURITY-2169 escape hatch.
* Controlled by {@link DecompressStepExecution#ALLOW_EXTRACTION_OUTSIDE_DESTINATION}.
*
* @return true if so.
*/
public boolean isAllowExtractionOutsideDestination() {
return allowExtractionOutsideDestination;
}

public void setAllowExtractionOutsideDestination(boolean allowExtractionOutsideDestination) {
this.allowExtractionOutsideDestination = allowExtractionOutsideDestination;
}

protected boolean isDescendantOfDestination(FilePath f) throws IOException {
if (allowExtractionOutsideDestination) {
return true;
}
//Assumes destination and f is on the local host
if (destination == null) {
return false;
}
File dst = new File(destination.getRemote()).getCanonicalFile();
File child = new File(f.getRemote()).getCanonicalFile();
return child.toPath().startsWith(dst.toPath());
}
}
18 changes: 18 additions & 0 deletions src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/DecompressStepExecution.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@
package org.jenkinsci.plugins.pipeline.utility.steps;

import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.FilePath;
import hudson.model.TaskListener;
import jenkins.util.SystemProperties;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.plugins.workflow.steps.StepContext;
import org.jenkinsci.plugins.workflow.steps.SynchronousNonBlockingStepExecution;
Expand All @@ -39,6 +41,13 @@
* @author Robert Sandell &lt;[email protected]&gt;.
*/
public abstract class DecompressStepExecution extends SynchronousNonBlockingStepExecution<Object> {

/**
* SECURITY-2169 escape hatch.
*/
@SuppressFBWarnings(value={"MS_SHOULD_BE_FINAL"}, justification="Non final so that an admin can adjust the value through the groovy script console without restarting the instance.")
public static /*almost final*/ boolean ALLOW_EXTRACTION_OUTSIDE_DESTINATION = SystemProperties.getBoolean(DecompressStepExecution.class.getName() + ".ALLOW_EXTRACTION_OUTSIDE_DESTINATION", false);

private transient AbstractFileCallable<? extends Object> callable;
private transient final AbstractFileDecompressStep step;

Expand All @@ -49,6 +58,9 @@ protected DecompressStepExecution(@NonNull AbstractFileDecompressStep step, @Non

protected void setCallable(final AbstractFileCallable<? extends Object> callable) {
this.callable = callable;
if (callable != null) {
callable.setAllowExtractionOutsideDestination(ALLOW_EXTRACTION_OUTSIDE_DESTINATION);
}
}

@Override
Expand Down Expand Up @@ -87,6 +99,12 @@ private Object test(TaskListener listener, FilePath workspace) throws IOExceptio
listener.error(source.getRemote() + " is a directory.");
return Boolean.FALSE;
}
FilePath destination = workspace;
if (!StringUtils.isBlank(step.getDir())) {
destination = workspace.child(step.getDir());
}

callable.setDestination(destination);
return source.act(callable);
}
}
43 changes: 37 additions & 6 deletions src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/tar/UnTarStepExecution.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@

import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand Down Expand Up @@ -95,17 +96,36 @@ public Void invoke(File tarFile, VirtualChannel channel) throws IOException, Int
PrintStream logger = listener.getLogger();
boolean doGlob = !StringUtils.isBlank(glob);

InputStream fileStream = new FileInputStream(tarFile);
FileInputStream fileStream = new FileInputStream(tarFile);

FileChannel fileChannel = fileStream.getChannel();

byte[] signature = new byte[2];
try {
//check if matches standard gzip magic number
fileStream = new GzipCompressorInputStream(fileStream);
int read = fileStream.read(signature);
fileChannel.position(0);
if (read <= 0) {
logger.println("File is empty.");
}
} catch (IOException exception) {
// Eat exception, may be not compressed file
fileStream.close();
throw new IOException("Error reading tar/tgz file: " + exception.getMessage(), exception);
} finally {
logger.flush();
}

InputStream inputStream = fileStream;
if(GzipCompressorInputStream.matches(signature, signature.length)) {
try {
//check if matches standard gzip magic number
inputStream = new GzipCompressorInputStream(fileStream);
} catch (IOException exception) {
// Eat exception, may be not compressed file
}
}

getDestination().mkdirs();
try (TarArchiveInputStream tarStream = new TarArchiveInputStream(fileStream)) {
try (TarArchiveInputStream tarStream = new TarArchiveInputStream(inputStream)) {
logger.println("Extracting from " + tarFile.getAbsolutePath());
TarArchiveEntry entry;
Integer fileCount = 0;
Expand All @@ -115,6 +135,9 @@ public Void invoke(File tarFile, VirtualChannel channel) throws IOException, Int
}

FilePath f = getDestination().child(entry.getName());
if (!isDescendantOfDestination(f)) {
throw new FileNotFoundException(f.getRemote() + " is out of bounds!");
}
if (entry.isDirectory()) {
f.mkdirs();
} else {
Expand Down Expand Up @@ -151,7 +174,7 @@ boolean matches(String path, String glob) {
}

/**
* Performs a test of a tar file on the slave where the file is.
* Performs a test of a tar file on the agent where the file is.
*/
static class TestTarFileCallable extends AbstractFileCallable<Boolean> {
private TaskListener listener;
Expand Down Expand Up @@ -205,6 +228,14 @@ public Boolean invoke(File f, VirtualChannel channel) throws IOException, Interr
if (!entry.isCheckSumOK()) {
throw new IOException("Not a tar archive");
}
FilePath destination = getDestination();
if (destination != null) {
FilePath ef = destination.child(entry.getName());
if (!isDescendantOfDestination(ef)) {
listener.error(ef.getRemote() + " is out of bounds!");
return false;
}
}
}
} catch (IOException exception) {
listener.error("Error validating tar file: " + exception.getMessage());
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/UnZipStepExecution.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.jenkinsci.plugins.workflow.steps.StepContext;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand Down Expand Up @@ -114,6 +115,9 @@ public Map<String, String> invoke(File zipFile, VirtualChannel channel) throws I
continue;
}
FilePath f = getDestination().child(entry.getName());
if (!isDescendantOfDestination(f)) {
throw new FileNotFoundException(f.getRemote() + " is out of bounds!");
}
if (entry.isDirectory()) {
if (!read) {
f.mkdirs();
Expand Down Expand Up @@ -191,6 +195,14 @@ public Boolean invoke(File f, VirtualChannel channel) throws IOException, Interr

ZipEntry entry = entries.nextElement();
if (!entry.isDirectory()) {
FilePath destination = getDestination();
if (destination != null) {
FilePath ef = destination.child(entry.getName());
if (!isDescendantOfDestination(ef)) {
listener.error(ef.getRemote() + " is out of bounds!");
return false;
}
}
try (InputStream inputStream = zip.getInputStream(entry)) {
int length;
while ((length = IOUtils.read(inputStream, buffer)) > 0) {
Expand Down
81 changes: 81 additions & 0 deletions src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/tar/UnTarStepTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
package org.jenkinsci.plugins.pipeline.utility.steps.tar;

import hudson.model.Label;
import hudson.model.Result;
import org.jenkinsci.plugins.pipeline.utility.steps.DecompressStepExecution;
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
Expand All @@ -33,14 +35,18 @@
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;

import java.io.File;
import java.net.URL;
import java.net.URLDecoder;
import java.nio.charset.StandardCharsets;

import static org.jenkinsci.plugins.pipeline.utility.steps.FilenameTestsUtils.separatorsToSystemEscaped;
import static org.junit.Assert.assertFalse;
import static org.junit.Assume.assumeTrue;

/**
* Tests for {@link UnTarStep}.
Expand All @@ -51,6 +57,8 @@ public class UnTarStepTest {

@Rule
public JenkinsRule j = new JenkinsRule();
@Rule
public BuildWatcher watcher = new BuildWatcher();

@Before
public void setup() throws Exception {
Expand Down Expand Up @@ -273,4 +281,77 @@ public void untarKeepPermissions() throws Exception {
WorkflowRun run = j.assertBuildStatusSuccess(p.scheduleBuild2(0));
j.assertLogContains("Hello World!", run);
}

@Test @Issue("SECURITY-2196")
public void testingAbsolutePathsShouldFail() throws Exception {
assumeTrue("Can only run in a gnu unix environment", File.pathSeparatorChar == ':');
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
URL resource = getClass().getResource("absolute.tar");
String tgz = new File(URLDecoder.decode(resource.getPath(), StandardCharsets.UTF_8)).getAbsolutePath().replace('\\', '/');
p.setDefinition(new CpsFlowDefinition(
"node {\n" +
" def result = untar file: '" + separatorsToSystemEscaped(tgz) + "', test: true\n" +
" if (result)\n" +
" error('Should be fail!')\n" +
"}", true));
WorkflowRun run = j.buildAndAssertSuccess(p);
j.assertLogContains("is out of bounds!", run);
}

@Test @Issue("SECURITY-2196")
public void testingAbsolutePathsShouldNotFailWithEscapeHatch() throws Exception {
assumeTrue("Can only run in a gnu unix environment", File.pathSeparatorChar == ':');
try {
DecompressStepExecution.ALLOW_EXTRACTION_OUTSIDE_DESTINATION = true;
j.createOnlineSlave(Label.get("bbb"));
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
URL resource = getClass().getResource("absolute.tar");
String tgz = new File(URLDecoder.decode(resource.getPath(), StandardCharsets.UTF_8)).getAbsolutePath().replace('\\', '/');
p.setDefinition(new CpsFlowDefinition(
"node('bbb') {\n" +
" def result = untar file: '" + separatorsToSystemEscaped(tgz) + "', test: true\n" +
" if (!result)\n" +
" error('Should not be fail!')\n" +
"}", true));
WorkflowRun run = j.buildAndAssertSuccess(p);
j.assertLogNotContains("is out of bounds!", run);
} finally {
DecompressStepExecution.ALLOW_EXTRACTION_OUTSIDE_DESTINATION = false;
}
}

@Test @Issue("SECURITY-2196")
public void absolutePathsShouldFailBuild() throws Exception {
assumeTrue("Can only run in a gnu unix environment", File.pathSeparatorChar == ':');
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
URL resource = getClass().getResource("absolute.tar");
String tgz = new File(URLDecoder.decode(resource.getPath(), StandardCharsets.UTF_8)).getAbsolutePath().replace('\\', '/');
p.setDefinition(new CpsFlowDefinition(
"node {\n" +
" untar file: '" + separatorsToSystemEscaped(tgz) + "'\n" +
"}", true));
WorkflowRun run = j.buildAndAssertStatus(Result.FAILURE, p);
j.assertLogContains("is out of bounds!", run);
}

@Test
@Issue("SECURITY-2196")
public void absolutePathsShouldNotFailBuildWithEscapeHatch() throws Exception {
assumeTrue("Can only run in a gnu unix environment", File.pathSeparatorChar == ':');
try {
DecompressStepExecution.ALLOW_EXTRACTION_OUTSIDE_DESTINATION = true;

WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
URL resource = getClass().getResource("absolute.tar");
String tgz = new File(URLDecoder.decode(resource.getPath(), StandardCharsets.UTF_8)).getAbsolutePath().replace('\\', '/');
p.setDefinition(new CpsFlowDefinition(
"node {\n" +
" untar file: '" + separatorsToSystemEscaped(tgz) + "'\n" +
"}", true));
WorkflowRun run = j.buildAndAssertStatus(Result.SUCCESS, p);
j.assertLogNotContains("is out of bounds!", run);
} finally {
DecompressStepExecution.ALLOW_EXTRACTION_OUTSIDE_DESTINATION = false;
}
}
}
Loading

0 comments on commit 0ba4f32

Please sign in to comment.