From 03daec211ac5798f853af76c05a381d6305bcefe Mon Sep 17 00:00:00 2001 From: Alex Metry Date: Thu, 28 Jun 2018 15:37:06 -0500 Subject: [PATCH 1/7] Add tests that fails if an entry of the archive is extracted outside the destination folder --- .../rauschig/jarchivelib/ArchiverZipTest.java | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java b/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java index 11b4047..4ef8878 100644 --- a/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java +++ b/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java @@ -16,9 +16,32 @@ 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. safe.txt that 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. safe.txt that 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"; + @Override protected Archiver getArchiver() { return ArchiverFactory.createArchiver(ArchiveFormat.ZIP); @@ -29,4 +52,60 @@ 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(); + } + + 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")); + } } From 019a543dfd4d0776658e17614ecd89c9bb0cf3c4 Mon Sep 17 00:00:00 2001 From: Alex Metry Date: Thu, 28 Jun 2018 15:38:13 -0500 Subject: [PATCH 2/7] Change source and target to 1.7 in preparation for the use of java.nio --- pom.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 From 65a86c5539c131ce3c34c1f7514bd1fa1d0f772d Mon Sep 17 00:00:00 2001 From: Alex Metry Date: Thu, 28 Jun 2018 15:39:28 -0500 Subject: [PATCH 3/7] Fix the vulnerability by forcing entries that attempt to traverse outside the destination folder to remain inside the folder --- .../jarchivelib/CommonsArchiveEntry.java | 2 +- .../rauschig/jarchivelib/CommonsArchiver.java | 6 +- .../org/rauschig/jarchivelib/IOUtils.java | 62 ++++++++++++++++++ src/test/resources/zip_traversal.zip | Bin 0 -> 620 bytes src/test/resources/zip_traversal_2.zip | Bin 0 -> 384 bytes 5 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 src/test/resources/zip_traversal.zip create mode 100644 src/test/resources/zip_traversal_2.zip 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..707ce17 100644 --- a/src/main/java/org/rauschig/jarchivelib/CommonsArchiver.java +++ b/src/main/java/org/rauschig/jarchivelib/CommonsArchiver.java @@ -20,6 +20,7 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; +import java.nio.file.Path; import org.apache.commons.compress.archivers.ArchiveEntry; import org.apache.commons.compress.archivers.ArchiveException; @@ -93,8 +94,11 @@ public void extract(InputStream archive, File destination) throws IOException { private void extract(ArchiveInputStream input, File destination) throws IOException { ArchiveEntry entry; + Path destinationNormalizedAbsolutePath = destination.toPath().toAbsolutePath().normalize(); + while ((entry = input.getNextEntry()) != null) { - File file = new File(destination, entry.getName()); + File file = + IOUtils.createResourceInDestination(destination, entry.getName(), destinationNormalizedAbsolutePath); 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..6e4d347 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,61 @@ 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) { + return createResourceInDestination(destination, entryName, destination.toPath().toAbsolutePath().normalize()); + } + + /** + * 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 destinationNormalizedAbsolutePath the normalized absolute path of the destination + * @return the created resource after it is placed in the destination directory + */ + public static File createResourceInDestination(File destination, + String entryName, + Path destinationNormalizedAbsolutePath) + { + File file = new File(destination, entryName); + Path normalizedFile = file.toPath().toAbsolutePath().normalize(); + if (!normalizedFile.startsWith(destinationNormalizedAbsolutePath)) { + 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/resources/zip_traversal.zip b/src/test/resources/zip_traversal.zip new file mode 100644 index 0000000000000000000000000000000000000000..753f6c3f43185ded74e19fa8c3a89be9a0f63c71 GIT binary patch literal 620 zcmWIWW@h1H00Fn)J3ea#6s3iMY!K#PkYOlJOiR@(sVE5z;bdSI{$~>H#bg>?TEWf0 z$nuqufq_K?s4gTUvseL$5*0wI71Az9D+2>Ch!~bM zf>;PESs?+57Kq5EVTKv9X`w*V1}gY~_M(R%&^w?I#PAL)8_3&CKqw0g1xrv!FaQ8t CrF(<` literal 0 HcmV?d00001 diff --git a/src/test/resources/zip_traversal_2.zip b/src/test/resources/zip_traversal_2.zip new file mode 100644 index 0000000000000000000000000000000000000000..dc4f4b1cf029546f2a042d6e097633eb7e2df732 GIT binary patch literal 384 zcmWIWW@h1H00Fn)J3ea#6s3iMY!K#PkYOlJOiR@(sVE5z;bdSI{$~>1%VZi|TEWf0 z$nuqufq_K?s4gTUvseL$5*0wI71Ayb?h1)7G{FF=Dqe!(!9l?~(oCLml6qz{8Q3;^60P>}!t literal 0 HcmV?d00001 From 352bd1c41a1941965ef217c4e21d2b15b41a3f0c Mon Sep 17 00:00:00 2001 From: Alex Metry Date: Thu, 28 Jun 2018 16:15:07 -0500 Subject: [PATCH 4/7] Update README to drop Java 6 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 132c2f594392644beba39233446281761428021f Mon Sep 17 00:00:00 2001 From: Alex Metry Date: Fri, 29 Jun 2018 10:07:11 -0500 Subject: [PATCH 5/7] Adding test cases for non-normalized paths Enhancing java doc --- .../rauschig/jarchivelib/ArchiverZipTest.java | 42 +++++++++++++++++-- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java b/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java index 4ef8878..92d442b 100644 --- a/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java +++ b/src/test/java/org/rauschig/jarchivelib/ArchiverZipTest.java @@ -29,19 +29,40 @@ public class ArchiverZipTest extends AbstractArchiverTest { /** - * Contains 2 files. safe.txt that 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 + * 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. safe.txt that is a safe file located at the root of the target directory and unsafe.txt that + * 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); @@ -78,6 +99,19 @@ public void zip_traversal_test_archiver_extraction_target_directory_as_root() th 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; From 3d49af1e650db259ea7f13c3b595b6ff862a786e Mon Sep 17 00:00:00 2001 From: Alex Metry Date: Fri, 29 Jun 2018 10:10:09 -0500 Subject: [PATCH 6/7] Add zip_traversal_3.zip --- src/test/resources/zip_traversal_3.zip | Bin 0 -> 652 bytes 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 src/test/resources/zip_traversal_3.zip diff --git a/src/test/resources/zip_traversal_3.zip b/src/test/resources/zip_traversal_3.zip new file mode 100644 index 0000000000000000000000000000000000000000..5a8cd24c5e3d2deb207404985c97e9a19adf648c GIT binary patch literal 652 zcmWIWW@h1H00Fn)J3ea#6s3iMY!K#PkYOlJOiR@(sVE5z;bdSI{$~zk$9K62HwLcLoMvkTEQ21hEiSvO)qDEl`n7!wf@Y(?UU}4c-t1dJhz$7~W%L0|f&U5b6R$ J#S;`V3;@x_heZGY literal 0 HcmV?d00001 From f570ff5ed4fe894b6d221494c68d71fae8732dc0 Mon Sep 17 00:00:00 2001 From: Alex Metry Date: Fri, 29 Jun 2018 10:26:49 -0500 Subject: [PATCH 7/7] Use CanonicalPath where possible --- .../org/rauschig/jarchivelib/CommonsArchiver.java | 5 ++--- src/main/java/org/rauschig/jarchivelib/IOUtils.java | 11 +++++------ 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/rauschig/jarchivelib/CommonsArchiver.java b/src/main/java/org/rauschig/jarchivelib/CommonsArchiver.java index 707ce17..fc5c9de 100644 --- a/src/main/java/org/rauschig/jarchivelib/CommonsArchiver.java +++ b/src/main/java/org/rauschig/jarchivelib/CommonsArchiver.java @@ -20,7 +20,6 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; -import java.nio.file.Path; import org.apache.commons.compress.archivers.ArchiveEntry; import org.apache.commons.compress.archivers.ArchiveException; @@ -94,11 +93,11 @@ public void extract(InputStream archive, File destination) throws IOException { private void extract(ArchiveInputStream input, File destination) throws IOException { ArchiveEntry entry; - Path destinationNormalizedAbsolutePath = destination.toPath().toAbsolutePath().normalize(); + String destinationCanonicalPath = destination.getCanonicalPath(); while ((entry = input.getNextEntry()) != null) { File file = - IOUtils.createResourceInDestination(destination, entry.getName(), destinationNormalizedAbsolutePath); + 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 6e4d347..3eea110 100644 --- a/src/main/java/org/rauschig/jarchivelib/IOUtils.java +++ b/src/main/java/org/rauschig/jarchivelib/IOUtils.java @@ -165,8 +165,8 @@ public static File[] filesContainedIn(File source) { * @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) { - return createResourceInDestination(destination, entryName, destination.toPath().toAbsolutePath().normalize()); + public static File createResourceInDestination(File destination, String entryName) throws IOException { + return createResourceInDestination(destination, entryName, destination.getCanonicalPath()); } /** @@ -174,16 +174,15 @@ public static File createResourceInDestination(File destination, String entryNam * * @param destination the destination directory to place the resource in * @param entryName the name of the resource to create in the destination - * @param destinationNormalizedAbsolutePath the normalized absolute path of 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, - Path destinationNormalizedAbsolutePath) + String destinationCanonicalPath) throws IOException { File file = new File(destination, entryName); - Path normalizedFile = file.toPath().toAbsolutePath().normalize(); - if (!normalizedFile.startsWith(destinationNormalizedAbsolutePath)) { + if (!file.getCanonicalPath().startsWith(destinationCanonicalPath)) { file = new File(destination, cleanEntryName(entryName)); } return file;