Fixed potential security vulnerability reported by Snyk Security Rese…

…arch Team

This is an arbitrary file write vulnerability, that can be achieved using a specially crafted zip archive, that holds path traversal filenames. So when the filename gets concatenated to the target extraction directory, the final path ends up outside of the target folder.

src/main/java/org/zeroturnaround/zip/ZipUtil.java

@@ -1150,6 +1150,15 @@ public void process(InputStream in, ZipEntry zipEntry) throws IOException {
       String name = mapper.map(zipEntry.getName());
       if (name != null) {
         File file = new File(outputDir, name);
+
+        /* If we see the relative traversal string of ".." we need to make sure
+         * that the outputdir + name doesn't leave the outputdir. See
+         * DirectoryTraversalMaliciousTest for details.
+         */
+        if (name.indexOf("..") != -1 && !file.getCanonicalPath().startsWith(outputDir.getCanonicalPath())) {
+          throw new ZipException("The file "+name+" is trying to leave the target output directory of "+outputDir+". Ignoring this file.");
+        }
+
         if (zipEntry.isDirectory()) {
           FileUtils.forceMkdir(file);
         }
@@ -1218,11 +1227,29 @@ public void process(InputStream in, ZipEntry zipEntry) throws IOException {
             parentDirectory = file;
           }
           File destFile = new File(parentDirectory, dirs[dirs.length - 1]);
+
+          /* If we see the relative traversal string of ".." we need to make sure
+           * that the outputdir + name doesn't leave the outputdir. See
+           * DirectoryTraversalMaliciousTest for details.
+           */
+          if (name.indexOf("..") != -1 && !destFile.getCanonicalPath().startsWith(outputDir.getCanonicalPath())) {
+            throw new ZipException("The file "+name+" is trying to leave the target output directory of "+outputDir+". Ignoring this file.");
+          }
+
           FileUtils.copy(in, destFile);
         }
         // it could be that there are just top level files that the unpacker is used for
         else {
           File destFile = new File(outputDir, name);
+
+          /* If we see the relative traversal string of ".." we need to make sure
+           * that the outputdir + name doesn't leave the outputdir. See
+           * DirectoryTraversalMaliciousTest for details.
+           */
+          if (name.indexOf("..") != -1 && !destFile.getCanonicalPath().startsWith(outputDir.getCanonicalPath())) {
+            throw new ZipException("The file "+name+" is trying to leave the target output directory of "+outputDir+". Ignoring this file.");
+          }
+
           FileUtils.copy(in, destFile);
         }
       }
@@ -1258,6 +1285,15 @@ else if (!rootDir.equals(root)) {
       String name = mapper.map(getUnrootedName(root, zipEntry.getName()));
       if (name != null) {
         File file = new File(outputDir, name);
+
+        /* If we see the relative traversal string of ".." we need to make sure
+         * that the outputdir + name doesn't leave the outputdir. See
+         * DirectoryTraversalMaliciousTest for details.
+         */
+        if (name.indexOf("..") != -1 && !file.getCanonicalPath().startsWith(outputDir.getCanonicalPath())) {
+          throw new ZipException("The file "+name+" is trying to leave the target output directory of "+outputDir+". Ignoring this file.");
+        }
+
         if (zipEntry.isDirectory()) {
           FileUtils.forceMkdir(file);
         }

src/test/java/org/zeroturnaround/zip/DirectoryTraversalMaliciousTest.java

@@ -0,0 +1,61 @@
+package org.zeroturnaround.zip;
+
+/**
+ *    Copyright (C) 2012 ZeroTurnaround LLC <[email protected]>
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+import java.io.File;
+
+import junit.framework.TestCase;
+
+public class DirectoryTraversalMaliciousTest extends TestCase {
+  /*
+   * This is the contents of the file. There is one evil file that tries to get out of the
+   * target.
+   *
+   * $ unzip -t zip-slip.zip
+   * Archive: zip-slip.zip
+   * testing: good.txt OK
+   * testing: ../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/evil.txt OK
+   * No errors detected in compressed data of zip-slip.zip.
+   */
+  private static final File badFile = new File("src/test/resources/zip-malicious-traversal.zip");
+  private static final File badFileBackslashes = new File("src/test/resources/zip-malicious-traversal-backslashes.zip");
+
+  public void testUnpackDoesntLeaveTarget() throws Exception {
+    File file = File.createTempFile("temp", null);
+    File tmpDir = file.getParentFile();
+
+    try {
+      ZipUtil.unpack(badFile, tmpDir);
+      fail();
+    }
+    catch (ZipException e) {
+      assertTrue(true);
+    }
+  }
+
+  public void testUnwrapDoesntLeaveTarget() throws Exception {
+    File file = File.createTempFile("temp", null);
+    File tmpDir = file.getParentFile();
+
+    try {
+      ZipUtil.iterate(badFileBackslashes, new ZipUtil.BackslashUnpacker(tmpDir));
+      fail();
+    }
+    catch (ZipException e) {
+      assertTrue(true);
+    }
+  }
+}