Merge pull request #633 from sandumjacob/master

Fixed Directory Traversal Issue in unzipTo and added tests

src/main/scala/better/files/File.scala

@@ -1074,21 +1074,47 @@ class File private (val path: Path)(implicit val fileSystem: FileSystem = path.g
   def zip(compressionLevel: Int = Deflater.DEFAULT_COMPRESSION, charset: Charset = DefaultCharset): File =
     zipTo(destination = File.newTemporaryFile(prefix = name, suffix = ".zip"), compressionLevel, charset)
 
+  /**
+    * Skips unzipping a ZipEntry from a ZipFile if the entry is going outside the target directory
+    *
+    * @param destination destination folder to extract zipEntry to
+    * @param zipEntry Instantiated ZipEntry to unzip
+    * @param zipFile Instantiated ZipFile to unzip from
+    */
+  private def safeExtractTo(
+      destination: File,
+      zipEntry: ZipEntry,
+      zipFile: ZipFile
+  ): destination.type = {
+    // https://developer.android.com/topic/security/risks/zip-path-traversal
+    val entryName          = zipEntry.getName()
+    val entryDestination   = File(destination, entryName)
+    val entryCanonicalPath = entryDestination.canonicalPath
+    if (entryCanonicalPath.startsWith(destination.canonicalPath + JFile.separator)) {
+      zipEntry.extractTo(destination, zipFile.getInputStream(zipEntry))
+    }
+    destination
+  }
+
   /** Unzips this zip file
     *
     * @param destination destination folder; Creates this if it does not exist
     * @param zipFilter An optional param to reject or accept unzipping a file
+    * @param charset Optional charset parameter
+    * @param safeUnzip Optional parameter to set whether unzipTo should unzip entries containing directory traversal characters to directories outside the destination
     * @return The destination where contents are unzipped
     */
   def unzipTo(
       destination: File = File.newTemporaryDirectory(name.stripSuffix(".zip")),
       zipFilter: ZipEntry => Boolean = _ => true,
-      charset: Charset = DefaultCharset
+      charset: Charset = DefaultCharset,
+      safeUnzip: Boolean = true
   ): destination.type = {
     for {
       zipFile <- new ZipFile(toJava, charset).autoClosed
       entry   <- zipFile.entries().asScala if zipFilter(entry)
-    } entry.extractTo(destination, zipFile.getInputStream(entry))
+    } if (safeUnzip) safeExtractTo(destination, entry, zipFile)
+    else entry.extractTo(destination, zipFile.getInputStream(entry))
     destination
   }
 

src/test/scala/better/files/FileSpec.scala

@@ -1,6 +1,7 @@
 package better.files
 
 import java.nio.file.{FileAlreadyExistsException, Files => JFiles, FileSystems}
+import java.nio.file.AccessDeniedException
 
 import scala.collection.compat.immutable.LazyList
 import scala.language.postfixOps
@@ -533,6 +534,34 @@ class FileSpec extends CommonSpec {
     assert(list.length === 3)
   }
 
+  it should "unzip safely by default" in {
+    val list = File("src/test/resources/better/files/issue-624.zip")
+      .unzipTo()
+      .listRecursively()
+      .toList
+    assert(
+      list.length === 1
+    ) // Three total entries in the zip file: 1 without directory traversal characters and 2 with. Default secure unzip should only unzip the entry without directory traversal characters.
+  }
+
+  it should "unzip unsafely when safe unzip is disabled" in {
+    // Unsafe unzip with a zipslip attack may result in OS access denied exceptions. If these occur, the test should still pass.
+    try {
+      val list = File("src/test/resources/better/files/issue-624.zip")
+        .unzipTo(safeUnzip = false)
+        .listRecursively()
+        .toList
+      assert(
+        list.length === 3
+      ) // Three total entries in the zip file: 1 without directory traversal characters and 2 with. Unsafe unzip should try to extract all entries, might result in access denied exception from OS.
+    } catch {
+      case exception: Throwable =>
+        assert(
+          exception.isInstanceOf[AccessDeniedException]
+        )
+    }
+  }
+
   it should "ungzip" in {
     val data = Seq("hello", "world")
     for {