Skip to content

Commit

Permalink
Disallow traversal entity in zip
Browse files Browse the repository at this point in the history
When the file name holds path traversal file names it gets
concatenated to the target extraction directory,
the final path ends up outside of the target folder.
  • Loading branch information
artembilan authored and garyrussell committed May 3, 2018
1 parent c5602c4 commit a5573eb
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 9 deletions.
6 changes: 3 additions & 3 deletions spring-integration-zip/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ ext {
linkScmConnection = 'https://github.com/spring-projects/spring-integration-extensions.git'
linkScmDevConnection = 'git@github.com:spring-projects/spring-integration-extensions.git'

slf4jVersion = "1.7.21"
springIntegrationVersion = '4.3.10.RELEASE'
ztZipVersion = '1.11'
slf4jVersion = "1.7.25"
springIntegrationVersion = '4.3.15.RELEASE'
ztZipVersion = '1.13'

idPrefix = 'zip'
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2016 the original author or authors.
* Copyright 2015-2018 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -29,6 +29,7 @@
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.zeroturnaround.zip.ZipEntryCallback;
import org.zeroturnaround.zip.ZipException;
import org.zeroturnaround.zip.ZipUtil;

import org.springframework.messaging.Message;
Expand All @@ -41,6 +42,7 @@
*
* @author Gunnar Hillert
* @author Artem Bilan
*
* @since 1.0
*
*/
Expand Down Expand Up @@ -104,7 +106,7 @@ else if (payload instanceof byte[]) {
}
else {
throw new IllegalArgumentException(String.format("Unsupported payload type '%s'. " +
"The only supported payload types are java.io.File, byte[] and java.io.InputStream",
"The only supported payload types are java.io.File, byte[] and java.io.InputStream",
payload.getClass().getSimpleName()));
}

Expand All @@ -122,7 +124,7 @@ public void process(InputStream zipEntryInputStream, ZipEntry zipEntry) throws I

if (logger.isInfoEnabled()) {
logger.info(String.format("Unpacking Zip Entry - Name: '%s',Time: '%s', " +
"Compressed Size: '%s', Type: '%s'",
"Compressed Size: '%s', Type: '%s'",
zipEntryName, zipEntryTime, zipEntryCompressedSize, type));
}

Expand All @@ -131,6 +133,15 @@ public void process(InputStream zipEntryInputStream, ZipEntry zipEntry) throws I
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 (zipEntryName.contains("..") &&
!destinationFile.getCanonicalPath().startsWith(workDirectory.getCanonicalPath())) {
throw new ZipException("The file " + zipEntryName +
" is trying to leave the target output directory of " + workDirectory);
}

if (zipEntry.isDirectory()) {
destinationFile.mkdirs(); //NOSONAR false positive
}
Expand Down Expand Up @@ -165,8 +176,8 @@ else if (ZipResultType.BYTE_ARRAY.equals(zipResultType)) {
}
else {
throw new MessagingException(message,
String.format("The UnZip operation extracted %s "
+ "result objects but expectSingleResult was 'true'.", uncompressedData.size()));
String.format("The UnZip operation extracted %s "
+ "result objects but expectSingleResult was 'true'.", uncompressedData.size()));
}
}
else {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2015-2016 the original author or authors.
* Copyright 2015-2018 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,6 +16,9 @@

package org.springframework.integration.zip.transformer;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.instanceOf;

import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
Expand All @@ -30,12 +33,15 @@
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
import org.zeroturnaround.zip.ZipException;

import org.springframework.beans.factory.annotation.Autowired;
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.MessageHandlingException;
import org.springframework.messaging.MessagingException;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
Expand All @@ -44,6 +50,7 @@
*
* @author Gunnar Hillert
* @author Artem Bilan
*
* @since 1.0
*
*/
Expand Down Expand Up @@ -253,4 +260,25 @@ public void unzipInvalidZipFile() throws IOException, InterruptedException {
}
}

@Test
public void testUnzipMaliciousTraversalZipFile() throws IOException {
final Resource resource = this.resourceLoader.getResource("classpath:testzipdata/zip-malicious-traversal.zip");
final InputStream is = resource.getInputStream();

final Message<InputStream> message = MessageBuilder.withPayload(is).build();

final UnZipTransformer unZipTransformer = new UnZipTransformer();
unZipTransformer.afterPropertiesSet();

try {
unZipTransformer.transform(message);
}
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"));
}
}
}
Binary file not shown.

0 comments on commit a5573eb

Please sign in to comment.