From d10f537283d90eabd28af57ac97f860a3913bf9b Mon Sep 17 00:00:00 2001 From: Gary Russell Date: Thu, 10 May 2018 09:58:37 -0400 Subject: [PATCH] Dissallow traversal entry even for byte[] The previous change prevents the transformer from writing a file outside of the working directory. However it still produced an entry for an errant file when producing just contents, and not writing to the file system. However, the errant path would be added to the message and might be used by subsequent components to write to the file system. This situation is present in the `UnZip2FileTests`. While this vulnerability is not directly exposed by the framework, user applications could be affected by it. --- .../zip/transformer/UnZipTransformer.java | 28 +++++++++++-------- .../integration/zip/UnZip2FileTests.java | 26 +++++++++++++++++ 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/spring-integration-zip/src/main/java/org/springframework/integration/zip/transformer/UnZipTransformer.java b/spring-integration-zip/src/main/java/org/springframework/integration/zip/transformer/UnZipTransformer.java index b9baf0e..8769d69 100644 --- a/spring-integration-zip/src/main/java/org/springframework/integration/zip/transformer/UnZipTransformer.java +++ b/spring-integration-zip/src/main/java/org/springframework/integration/zip/transformer/UnZipTransformer.java @@ -129,17 +129,7 @@ public class UnZipTransformer extends AbstractZipTransformer { } if (ZipResultType.FILE.equals(zipResultType)) { - final File tempDir = new File(workDirectory, message.getHeaders().getId().toString()); - tempDir.mkdirs(); //NOSONAR false positive - final File destinationFile = new File(tempDir, zipEntryName); - - /* If we see the relative traversal string of ".." we need to make sure - * that the outputdir + name doesn't leave the outputdir. - */ - if (!destinationFile.getCanonicalPath().startsWith(workDirectory.getCanonicalPath())) { - throw new ZipException("The file " + zipEntryName + - " is trying to leave the target output directory of " + workDirectory); - } + final File destinationFile = checkPath(message, zipEntryName); if (zipEntry.isDirectory()) { destinationFile.mkdirs(); //NOSONAR false positive @@ -151,6 +141,7 @@ public class UnZipTransformer extends AbstractZipTransformer { } else if (ZipResultType.BYTE_ARRAY.equals(zipResultType)) { if (!zipEntry.isDirectory()) { + checkPath(message, zipEntryName); byte[] data = IOUtils.toByteArray(zipEntryInputStream); uncompressedData.put(zipEntryName, data); } @@ -159,6 +150,21 @@ public class UnZipTransformer extends AbstractZipTransformer { throw new IllegalStateException("Unsupported zipResultType " + zipResultType); } } + + public File checkPath(final Message message, final String zipEntryName) throws IOException { + final File tempDir = new File(workDirectory, message.getHeaders().getId().toString()); + tempDir.mkdirs(); //NOSONAR false positive + final File destinationFile = new File(tempDir, zipEntryName); + + /* If we see the relative traversal string of ".." we need to make sure + * that the outputdir + name doesn't leave the outputdir. + */ + if (!destinationFile.getCanonicalPath().startsWith(workDirectory.getCanonicalPath())) { + throw new ZipException("The file " + zipEntryName + + " is trying to leave the target output directory of " + workDirectory); + } + return destinationFile; + } }); if (uncompressedData.isEmpty()) { diff --git a/spring-integration-zip/src/test/java/org/springframework/integration/zip/UnZip2FileTests.java b/spring-integration-zip/src/test/java/org/springframework/integration/zip/UnZip2FileTests.java index 2eae50a..c9d0753 100644 --- a/spring-integration-zip/src/test/java/org/springframework/integration/zip/UnZip2FileTests.java +++ b/spring-integration-zip/src/test/java/org/springframework/integration/zip/UnZip2FileTests.java @@ -16,6 +16,10 @@ package org.springframework.integration.zip; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.Assert.fail; + import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -28,6 +32,7 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.zeroturnaround.zip.ZipException; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; @@ -36,8 +41,10 @@ import org.springframework.context.annotation.ImportResource; import org.springframework.core.io.Resource; import org.springframework.core.io.ResourceLoader; import org.springframework.integration.support.MessageBuilder; +import org.springframework.integration.transformer.MessageTransformationException; import org.springframework.messaging.Message; import org.springframework.messaging.MessageChannel; +import org.springframework.messaging.MessageHandlingException; /** * @@ -146,6 +153,25 @@ public class UnZip2FileTests { } + @Test + public void unZipTraversal() throws Exception { + final Resource resource = this.resourceLoader.getResource("classpath:testzipdata/zip-malicious-traversal.zip"); + final InputStream is = resource.getInputStream(); + byte[] zipdata = IOUtils.toByteArray(is); + final Message message = MessageBuilder.withPayload(zipdata).build(); + try { + input.send(message); + fail("Expected Exception"); + } + catch (Exception e) { + Assert.assertThat(e, instanceOf(MessageTransformationException.class)); + Assert.assertThat(e.getCause(), instanceOf(MessageHandlingException.class)); + Assert.assertThat(e.getCause().getCause(), instanceOf(ZipException.class)); + Assert.assertThat(e.getCause().getCause().getMessage(), + containsString("is trying to leave the target output directory")); + } + } + @Configuration @ImportResource("classpath:org/springframework/integration/zip/UnZip2FileTests-context.xml") public static class ContextConfiguration {