Fix zip traversal vulnerability

README.md

@@ -92,7 +92,7 @@ Dependencies
 Compatibility
 -------------
 
-* Java 6 and 7
+* Java 7
 * Currently only tested for *nix file systems.
 
 ### OSGi

pom.xml

@@ -84,8 +84,8 @@
       <plugin>
         <artifactId>maven-compiler-plugin</artifactId>
         <configuration>
-          <source>1.6</source>
-          <target>1.6</target>
+          <source>1.7</source>
+          <target>1.7</target>
         </configuration>
       </plugin>
       <plugin>

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();

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();

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<Path> iterator = normalizedPath.iterator();
+        List<String> list = new ArrayList<String>();
+        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();
+    }
 }

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<String> extractedItems = new HashSet<String>(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<String> extractedItems = new HashSet<String>(Arrays.asList(flatRelativeArray(ARCHIVE_EXTRACT_DIR)));
+        assertEquals(2, extractedItems.size());
+        assertTrue(extractedItems.contains("safe.txt"));
+        assertTrue(extractedItems.contains("unsafe.txt"));
+    }
 }