diff --git a/README.md b/README.md
index 31a96f7..eaf71f4 100644
--- a/README.md
+++ b/README.md
@@ -92,7 +92,7 @@ Dependencies
Compatibility
-------------
-* Java 6 and 7
+* Java 7
* Currently only tested for *nix file systems.
### OSGi
diff --git a/pom.xml b/pom.xml
index 92d42ab..7fb8ed9 100644
--- a/pom.xml
+++ b/pom.xml
@@ -84,8 +84,8 @@
maven-compiler-plugin
- 1.6
- 1.6
+ 1.7
+ 1.7
diff --git a/src/main/java/org/rauschig/jarchivelib/CommonsArchiveEntry.java b/src/main/java/org/rauschig/jarchivelib/CommonsArchiveEntry.java
index a7aa37c..0cbe776 100644
--- a/src/main/java/org/rauschig/jarchivelib/CommonsArchiveEntry.java
+++ b/src/main/java/org/rauschig/jarchivelib/CommonsArchiveEntry.java
@@ -68,7 +68,7 @@ public File extract(File destination) throws IOException, IllegalStateException,
assertState();
IOUtils.requireDirectory(destination);
- File file = new File(destination, entry.getName());
+ File file = IOUtils.createResourceInDestination(destination, entry.getName());
if (entry.isDirectory()) {
file.mkdirs();
diff --git a/src/main/java/org/rauschig/jarchivelib/CommonsArchiver.java b/src/main/java/org/rauschig/jarchivelib/CommonsArchiver.java
index 27f7558..fc5c9de 100644
--- a/src/main/java/org/rauschig/jarchivelib/CommonsArchiver.java
+++ b/src/main/java/org/rauschig/jarchivelib/CommonsArchiver.java
@@ -93,8 +93,11 @@ public void extract(InputStream archive, File destination) throws IOException {
private void extract(ArchiveInputStream input, File destination) throws IOException {
ArchiveEntry entry;
+ String destinationCanonicalPath = destination.getCanonicalPath();
+
while ((entry = input.getNextEntry()) != null) {
- File file = new File(destination, entry.getName());
+ File file =
+ IOUtils.createResourceInDestination(destination, entry.getName(), destinationCanonicalPath);
if (entry.isDirectory()) {
file.mkdirs();
diff --git a/src/main/java/org/rauschig/jarchivelib/IOUtils.java b/src/main/java/org/rauschig/jarchivelib/IOUtils.java
index 33ce53d..3eea110 100644
--- a/src/main/java/org/rauschig/jarchivelib/IOUtils.java
+++ b/src/main/java/org/rauschig/jarchivelib/IOUtils.java
@@ -21,6 +21,11 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
/**
* Utility class for I/O operations.
@@ -153,4 +158,60 @@ public static File[] filesContainedIn(File source) {
}
}
+ /**
+ * Returns a resource after guaranteeing that it is created inside the destination directory
+ *
+ * @param destination the destination directory to place the resource in
+ * @param entryName the name of the resource to create in the destination
+ * @return the created resource after it is placed in the destination directory
+ */
+ public static File createResourceInDestination(File destination, String entryName) throws IOException {
+ return createResourceInDestination(destination, entryName, destination.getCanonicalPath());
+ }
+
+ /**
+ * Returns a resource after guaranteeing that it is created inside the destination directory
+ *
+ * @param destination the destination directory to place the resource in
+ * @param entryName the name of the resource to create in the destination
+ * @param destinationCanonicalPath the canonical path of the destination
+ * @return the created resource after it is placed in the destination directory
+ */
+ public static File createResourceInDestination(File destination,
+ String entryName,
+ String destinationCanonicalPath) throws IOException
+ {
+ File file = new File(destination, entryName);
+ if (!file.getCanonicalPath().startsWith(destinationCanonicalPath)) {
+ file = new File(destination, cleanEntryName(entryName));
+ }
+ return file;
+ }
+
+ /**
+ * Cleans up a path by normalizing it and removing any leading ..
+ *
+ * @param entry a file path entry to clean
+ * @return the cleaned path
+ */
+ public static String cleanEntryName(String entry) {
+ Path normalizedPath = Paths.get(entry).normalize();
+ Iterator iterator = normalizedPath.iterator();
+ List list = new ArrayList();
+ while (iterator.hasNext()) {
+ String next = iterator.next().toString();
+ if (!"..".equals(next)) {
+ list.add(next);
+ }
+ }
+ String firstElement = "";
+ if (list.size() > 0) {
+ firstElement = list.remove(0);
+ }
+ String[] remainingElements = new String[list.size()];
+ if (list.size() > 0) {
+ remainingElements = list.toArray(remainingElements);
+ }
+ return Paths.get(firstElement, remainingElements).toString();
+ }
}
diff --git a/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java b/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java
index 11b4047..92d442b 100644
--- a/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java
+++ b/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java
@@ -16,9 +16,53 @@
package org.rauschig.jarchivelib;
import java.io.File;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.HashSet;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
public class ArchiverZipTest extends AbstractArchiverTest {
+ /**
+ * Contains 2 files:
+ * 1- safe.txt
+ * 2- ../../../../../../../../../../../../../../../../../../../../../../../../../../
+ * ../../../../../../../../../../../../../../../tmp/unsafe.txt
+ *
+ * safe.txt is a safe file located at the root of the target directory and unsafe.txt that attempts to traverse the
+ * tree all the way to / and down to tmp. This should be placed at target/tmp/unsafe.txt when extracted
+ */
+ private static final String ZIP_TRAVERSAL_FILE_1 = "zip_traversal.zip";
+
+ /**
+ * Contains 2 files:
+ * 1- safe.txt
+ * 2- ../../../unsafe.txt
+ *
+ * safe.txt is a safe file located at the root of the target directory and unsafe.txt that
+ * attempts to traverse the tree outside the target directory but not high enough to make it to /.
+ * This should be placed at target/unsafe.txt when extracted
+ */
+ private static final String ZIP_TRAVERSAL_FILE_2 = "zip_traversal_2.zip";
+
+ /**
+ * Contains 2 files:
+ * 1- safe.txt
+ * 2- subDirectory/../../../../../../../../../../../../../../../../../../../../../
+ * ../../../../../../../../../../../../../../../../../../../../../tmp/unsafe.txt
+ *
+ * safe.txt is a safe file located at the root of the target directory and unsafe.txt that
+ * attempts to traverse the tree all the way to / and down to tmp. This should be placed at target/tmp/unsafe.txt
+ * when extracted. The difference between this file and ZIP_TRAVERSAL_FILE_1 is that the unsafe file relative path
+ * is not normalized.
+ */
+ private static final String ZIP_TRAVERSAL_FILE_3 = "zip_traversal_3.zip";
+
@Override
protected Archiver getArchiver() {
return ArchiverFactory.createArchiver(ArchiveFormat.ZIP);
@@ -29,4 +73,73 @@ protected File getArchive() {
return new File(RESOURCES_DIR, "archive.zip");
}
+ @Test
+ public void zip_traversal_test_entry_extraction() throws Exception {
+ archiveExtractorHelper(ZIP_TRAVERSAL_FILE_1);
+ assertZipTraversal();
+ }
+
+ @Test
+ public void zip_traversal_test_archiver_extraction() throws Exception {
+ File archive = new File(RESOURCES_DIR, ZIP_TRAVERSAL_FILE_1);
+ getArchiver().extract(archive, ARCHIVE_EXTRACT_DIR);
+ assertZipTraversal();
+ }
+
+ @Test
+ public void zip_traversal_test_entry_extraction_target_directory_as_root() throws Exception {
+ archiveExtractorHelper(ZIP_TRAVERSAL_FILE_2);
+ assertTargetDirectoryAsRoot();
+ }
+
+ @Test
+ public void zip_traversal_test_archiver_extraction_target_directory_as_root() throws Exception {
+ File archive = new File(RESOURCES_DIR, ZIP_TRAVERSAL_FILE_2);
+ getArchiver().extract(archive, ARCHIVE_EXTRACT_DIR);
+ assertTargetDirectoryAsRoot();
+ }
+
+ @Test
+ public void zip_traversal_test_entry_extraction_for_non_normalized_path() throws Exception {
+ archiveExtractorHelper(ZIP_TRAVERSAL_FILE_3);
+ assertZipTraversal();
+ }
+
+ @Test
+ public void zip_traversal_test_archiver_extraction_for_non_normalized_path() throws Exception {
+ File archive = new File(RESOURCES_DIR, ZIP_TRAVERSAL_FILE_3);
+ getArchiver().extract(archive, ARCHIVE_EXTRACT_DIR);
+ assertZipTraversal();
+ }
+
+ private void archiveExtractorHelper(final String fileName) throws IOException {
+ File archive = new File(RESOURCES_DIR, fileName);
+ ArchiveStream stream = null;
+ try {
+ stream = getArchiver().stream(archive);
+ ArchiveEntry entry;
+ while ((entry = stream.getNextEntry()) != null) {
+ entry.extract(ARCHIVE_EXTRACT_DIR);
+ }
+ } finally {
+ IOUtils.closeQuietly(stream);
+ }
+ }
+
+ private void assertZipTraversal () throws Exception {
+ HashSet extractedItems = new HashSet(Arrays.asList(flatRelativeArray(ARCHIVE_EXTRACT_DIR)));
+ assertEquals(3, extractedItems.size());
+ assertTrue(extractedItems.contains("safe.txt"));
+ assertTrue(extractedItems.contains("tmp"));
+ assertTrue(extractedItems.contains("tmp/unsafe.txt"));
+ assertFalse("This unsafe file should not exist as it is outside the target directory.",
+ new File("/tmp/unsafe.txt").exists());
+ }
+
+ private void assertTargetDirectoryAsRoot() throws Exception {
+ HashSet extractedItems = new HashSet(Arrays.asList(flatRelativeArray(ARCHIVE_EXTRACT_DIR)));
+ assertEquals(2, extractedItems.size());
+ assertTrue(extractedItems.contains("safe.txt"));
+ assertTrue(extractedItems.contains("unsafe.txt"));
+ }
}
diff --git a/src/test/resources/zip_traversal.zip b/src/test/resources/zip_traversal.zip
new file mode 100644
index 0000000..753f6c3
Binary files /dev/null and b/src/test/resources/zip_traversal.zip differ
diff --git a/src/test/resources/zip_traversal_2.zip b/src/test/resources/zip_traversal_2.zip
new file mode 100644
index 0000000..dc4f4b1
Binary files /dev/null and b/src/test/resources/zip_traversal_2.zip differ
diff --git a/src/test/resources/zip_traversal_3.zip b/src/test/resources/zip_traversal_3.zip
new file mode 100644
index 0000000..5a8cd24
Binary files /dev/null and b/src/test/resources/zip_traversal_3.zip differ