Skip to content

Commit

Permalink
Dissallow traversal entry even for byte[]
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
garyrussell authored and artembilan committed May 10, 2018
1 parent 8c4e18f commit d10f537
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -129,17 +129,7 @@ public void process(InputStream zipEntryInputStream, ZipEntry zipEntry) throws I
}

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
Expand All @@ -151,6 +141,7 @@ public void process(InputStream zipEntryInputStream, ZipEntry zipEntry) throws I
}
else if (ZipResultType.BYTE_ARRAY.equals(zipResultType)) {
if (!zipEntry.isDirectory()) {
checkPath(message, zipEntryName);
byte[] data = IOUtils.toByteArray(zipEntryInputStream);
uncompressedData.put(zipEntryName, data);
}
Expand All @@ -159,6 +150,21 @@ else if (ZipResultType.BYTE_ARRAY.equals(zipResultType)) {
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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -28,6 +32,7 @@
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;
Expand All @@ -36,8 +41,10 @@
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;

/**
*
Expand Down Expand Up @@ -146,6 +153,25 @@ public void unZipWithMultipleEntries() throws Exception {

}

@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<byte[]> 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 {
Expand Down

0 comments on commit d10f537

Please sign in to comment.