From 0ba4f329ee27c023609653e25bdd5604c5e46a11 Mon Sep 17 00:00:00 2001 From: rsandell Date: Tue, 9 May 2023 15:58:30 +0200 Subject: [PATCH] [SECURITY-2196] --- .../utility/steps/AbstractFileCallable.java | 31 +++++++ .../steps/DecompressStepExecution.java | 18 ++++ .../utility/steps/tar/UnTarStepExecution.java | 43 ++++++++-- .../utility/steps/zip/UnZipStepExecution.java | 12 +++ .../utility/steps/tar/UnTarStepTest.java | 81 ++++++++++++++++++ .../utility/steps/zip/UnZipStepTest.java | 70 ++++++++++++++- .../pipeline/utility/steps/tar/absolute.tar | Bin 0 -> 10240 bytes .../pipeline/utility/steps/zip/malicious.zip | Bin 0 -> 3387 bytes 8 files changed, 248 insertions(+), 7 deletions(-) create mode 100644 src/test/resources/org/jenkinsci/plugins/pipeline/utility/steps/tar/absolute.tar create mode 100644 src/test/resources/org/jenkinsci/plugins/pipeline/utility/steps/zip/malicious.zip diff --git a/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/AbstractFileCallable.java b/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/AbstractFileCallable.java index 2230457a..ad60c917 100644 --- a/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/AbstractFileCallable.java +++ b/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/AbstractFileCallable.java @@ -27,8 +27,12 @@ import hudson.FilePath; import jenkins.MasterToSlaveFileCallable; +import java.io.File; +import java.io.IOException; + public abstract class AbstractFileCallable extends MasterToSlaveFileCallable { private FilePath destination; + private boolean allowExtractionOutsideDestination = false; public FilePath getDestination() { return destination; @@ -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()); + } } \ No newline at end of file diff --git a/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/DecompressStepExecution.java b/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/DecompressStepExecution.java index a6308148..56ec6ae1 100644 --- a/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/DecompressStepExecution.java +++ b/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/DecompressStepExecution.java @@ -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; @@ -39,6 +41,13 @@ * @author Robert Sandell <rsandell@cloudbees.com>. */ public abstract class DecompressStepExecution extends SynchronousNonBlockingStepExecution { + + /** + * 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 callable; private transient final AbstractFileDecompressStep step; @@ -49,6 +58,9 @@ protected DecompressStepExecution(@NonNull AbstractFileDecompressStep step, @Non protected void setCallable(final AbstractFileCallable callable) { this.callable = callable; + if (callable != null) { + callable.setAllowExtractionOutsideDestination(ALLOW_EXTRACTION_OUTSIDE_DESTINATION); + } } @Override @@ -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); } } diff --git a/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/tar/UnTarStepExecution.java b/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/tar/UnTarStepExecution.java index 830edc51..310c136c 100644 --- a/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/tar/UnTarStepExecution.java +++ b/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/tar/UnTarStepExecution.java @@ -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; @@ -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; @@ -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 { @@ -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 { private TaskListener listener; @@ -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()); diff --git a/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/UnZipStepExecution.java b/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/UnZipStepExecution.java index c51f2bbe..a6e8ef18 100644 --- a/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/UnZipStepExecution.java +++ b/src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/UnZipStepExecution.java @@ -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; @@ -114,6 +115,9 @@ public Map 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(); @@ -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) { diff --git a/src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/tar/UnTarStepTest.java b/src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/tar/UnTarStepTest.java index 471ffb2d..9ac4739c 100644 --- a/src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/tar/UnTarStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/tar/UnTarStepTest.java @@ -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; @@ -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}. @@ -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 { @@ -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; + } + } } diff --git a/src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/UnZipStepTest.java b/src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/UnZipStepTest.java index ede203c4..51c1c5fb 100644 --- a/src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/UnZipStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/UnZipStepTest.java @@ -24,7 +24,11 @@ package org.jenkinsci.plugins.pipeline.utility.steps.zip; +import hudson.Functions; import hudson.model.Label; +import hudson.model.Result; +import hudson.model.queue.QueueTaskFuture; +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; @@ -33,14 +37,17 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import java.io.File; +import java.io.IOException; import java.net.URL; import java.net.URLDecoder; 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 UnZipStep}. @@ -172,7 +179,7 @@ public void globReadingMore() throws Exception { @Test public void zipTest() throws Exception { - Assume.assumeTrue("Can only run in a gnu unix environment", File.pathSeparatorChar == ':'); + assumeTrue("Can only run in a gnu unix environment", File.pathSeparatorChar == ':'); WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p"); p.setDefinition(new CpsFlowDefinition( "node('slaves') {\n" + @@ -279,4 +286,65 @@ public void unzipQuietReading() throws Exception { j.assertLogContains("Read: 2 files", run); j.assertLogContains("Text: Hello World!", run); } + + @Test @Issue("SECURITY-2196") + public void unZipMaliciousFailsTheTest() throws Exception { + assumeTrue("Can only run in a gnu unix environment", File.pathSeparatorChar == ':'); + /* + This test uses a prepared zip file with a malicious payload. + */ + WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p"); + URL resource = getClass().getResource("malicious.zip"); + String zip = new File(URLDecoder.decode(resource.getPath(), "UTF-8")).getAbsolutePath().replace('\\', '/'); + p.setDefinition(new CpsFlowDefinition( + "node {\n" + + " def result = unzip zipFile: '" + separatorsToSystemEscaped(zip) + "', test: true\n" + + " if (result)\n" + + " error('Should be failed!')\n" + + "}", true)); + WorkflowRun run = j.assertBuildStatusSuccess(p.scheduleBuild2(0)); + j.assertLogContains("is out of bounds!", run); + } + + @Test @Issue("SECURITY-2196") + public void unZipMaliciousDoesNotFailTheTestWithEscapeHath() throws Exception { + assumeTrue("Can only run in a gnu unix environment", File.pathSeparatorChar == ':'); + try { + DecompressStepExecution.ALLOW_EXTRACTION_OUTSIDE_DESTINATION = true; + /* + This test uses a prepared zip file with a malicious payload. + */ + j.createOnlineSlave(Label.get("bbb")); + WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p"); + URL resource = getClass().getResource("malicious.zip"); + String zip = new File(URLDecoder.decode(resource.getPath(), "UTF-8")).getAbsolutePath().replace('\\', '/'); + p.setDefinition(new CpsFlowDefinition( + "node('bbb') {\n" + + " def result = unzip zipFile: '" + separatorsToSystemEscaped(zip) + "', test: true\n" + + " if (!result)\n" + + " error('Should not fail!')\n" + + "}", true)); + WorkflowRun run = j.assertBuildStatusSuccess(p.scheduleBuild2(0)); + j.assertLogNotContains("is out of bounds!", run); + } finally { + DecompressStepExecution.ALLOW_EXTRACTION_OUTSIDE_DESTINATION = false; + } + } + + @Test @Issue("SECURITY-2196") + public void unZipMaliciousFailsTheBuild() throws Exception { + assumeTrue("Can only run in a gnu unix environment", File.pathSeparatorChar == ':'); + /* + This test uses a prepared zip file with a malicious payload. + */ + WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p"); + URL resource = getClass().getResource("malicious.zip"); + String zip = new File(URLDecoder.decode(resource.getPath(), "UTF-8")).getAbsolutePath().replace('\\', '/'); + p.setDefinition(new CpsFlowDefinition( + "node {\n" + + " unzip zipFile: '" + separatorsToSystemEscaped(zip) + "'\n" + + "}", true)); + WorkflowRun run = j.buildAndAssertStatus(Result.FAILURE, p); + j.assertLogContains("is out of bounds!", run); + } } diff --git a/src/test/resources/org/jenkinsci/plugins/pipeline/utility/steps/tar/absolute.tar b/src/test/resources/org/jenkinsci/plugins/pipeline/utility/steps/tar/absolute.tar new file mode 100644 index 0000000000000000000000000000000000000000..f6dea59cabacb3f846ec67b3decc1100cd4dcea4 GIT binary patch literal 10240 zcmeIwOA3Q96oui8y9!s(JetRP0u>zC2bzM*Z&DDo4mwe2>3=8DTnX3jM5fDKHgS+* zzf!qEa3U`{I6YrWtCY~zDi@43CohyX!5C4w{YIKje)K7JRVBJUUe9&gPMzh`r~dNm zpLvsG{!RY(+c6Dk`_W6l-rITZ<2@*?T>dS8>wL~%Z9mHT#6Qpf75ib$!VUrmAbSLKP!0L$=qMS262ovUG^F9-TojTd9r4kC`+?JF zt4nHblq&=&H{wyusT(du6{aFx$j}Yf$J)^6CCHD_&~e1JG$s%8GV9T|5Gn`x7|nGZ zgYR_&Yq=u5PtaU z3q5Cddu?VWBa?be`gc1C)M$B+gGgQXoEu2TR#~8By)xAuWJCjUW=t_aE9}Crw00;U z=H<(m{(vpXBVsFQO}>;)EJAhQLd%K0@+;N<5LPS1l@C8sFd9(!c_pv01heUMQ5mg% z5w$Yz>O`Gd5J1|!a&HW`wNV;J%XxaZ-*6cVv-X};^8527S&MFS)Rmo%Q* zp|)Nhq=YEq*OPN*&27CBm1MRCy4}erOXO-6)@9)mPZ#X<$5sf|Lup;MiJZ^U2IJoEF+XXG6`ucXKcGiS;}7O4igx)YC?wuP`C-PUpIp>}TP1ckDF z?02%##+1%u$Yr+>>oXj3I(x$XQc^52XULxI)Apox!Y1PV# z-I|K+QZP&}RVtl42f*F+o-I}jQdGHcgsY=C+IKgVC66OpN|`Q#OxaS@s6}3*5sy#! z)Ge{o&5~1rVAf@##NJAD2yo{DGUK>KOmTWK-S;wy@_5-V?VB2?i@=mpFH3g`h=AQV z;ccpFGd8;f!Oi4i$B8XdNQnYhj4m3i(9+>L053Hbvr6QuT-X{l5NsFIl)~C|LZdJBbL}H5-QyndcaiyLz&F!LWVL+1re_+Gm}-# zrhe&DIB$p@WZbp)cGZ+wP{~`=)rGSWLJ8NRo%vnVwW{?fW3?b*LeFJz6c6L^IB2ik ze#G0Ko@(_es7c6&HcD~h9S@JkjB zW=K=1WS5uc>`dkPR@gma0BfBJ09llb_;flw?F8;7_)(I(YE90beevS?e}CV&4