Merge pull request #360 from eed3si9n/wip/zip_issue

Fixes zip-slip vulnerability
sbt · Oct 22, 2023 · 1245383 · 1245383
2 parents dee89c8 + f928cdd
commit 1245383
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 8 deletions.
diff --git a/io/src/main/scala/sbt/io/IO.scala b/io/src/main/scala/sbt/io/IO.scala
@@ -387,35 +387,43 @@ object IO {
       preserveLastModified: Boolean = true
   ): Set[File] = {
     createDirectory(toDirectory)
-    zipInputStream(from)(zipInput => extract(zipInput, toDirectory, filter, preserveLastModified))
+    zipInputStream(from)(zipInput =>
+      extract(zipInput, toDirectory.toPath, filter, preserveLastModified)
+    )
   }
 
   private def extract(
       from: ZipInputStream,
-      toDirectory: File,
+      toDirectory: NioPath,
       filter: NameFilter,
       preserveLastModified: Boolean
   ) = {
-    val set = new HashSet[File]
+    val set = new HashSet[NioPath]
+    val canonicalDirPath = toDirectory.normalize().toString
+    def validateExtractPath(name: String, target: NioPath): Unit =
+      if (!target.normalize().toString.startsWith(canonicalDirPath)) {
+        throw new RuntimeException(s"Entry ($name) is outside of the target directory")
+      }
     @tailrec def next(): Unit = {
       val entry = from.getNextEntry
       if (entry == null)
         ()
       else {
         val name = entry.getName
         if (filter.accept(name)) {
-          val target = new File(toDirectory, name)
+          val target = toDirectory.resolve(name)
+          validateExtractPath(name, target)
           // log.debug("Extracting zip entry '" + name + "' to '" + target + "'")
           if (entry.isDirectory)
-            createDirectory(target)
+            createDirectory(target.toFile)
           else {
             set += target
             translate("Error extracting zip entry '" + name + "' to '" + target + "': ") {
-              fileOutputStream(false)(target)(out => transfer(from, out))
+              fileOutputStream(false)(target.toFile)(out => transfer(from, out))
             }
           }
           if (preserveLastModified)
-            setModifiedTimeOrFalse(target, entry.getTime)
+            setModifiedTimeOrFalse(target.toFile, entry.getTime)
         } else {
           // log.debug("Ignoring zip entry '" + name + "'")
         }
@@ -424,7 +432,7 @@ object IO {
       }
     }
     next()
-    Set() ++ set
+    (Set() ++ set).map(_.toFile)
   }
 
   // TODO: provide a better API to download things.

diff --git a/io/src/test/resources/zip-slip.zip b/io/src/test/resources/zip-slip.zip
diff --git a/io/src/test/scala/sbt/io/FileUtilitiesSpecification.scala b/io/src/test/scala/sbt/io/FileUtilitiesSpecification.scala
@@ -24,6 +24,7 @@ object WriteContentSpecification extends Properties("Write content") {
   property("Append string appends") = forAll(appendAndCheckStrings _)
   property("Append bytes appends") = forAll(appendAndCheckBytes _)
   property("Unzip doesn't stack overflow") = largeUnzip()
+  property("Unzip errors given parent traversal") = testZipSlip()
 
   implicit lazy val validChar: Arbitrary[Char] = Arbitrary(
     for (i <- Gen.choose(0, 0xd7ff)) yield i.toChar
@@ -39,6 +40,20 @@ object WriteContentSpecification extends Properties("Write content") {
     true
   }
 
+  private def testZipSlip() = {
+    val badFile0 = new File("io/src/test/resources/zip-slip.zip")
+    val badFile1 = new File("src/test/resources/zip-slip.zip")
+    val badFile =
+      if (badFile0.exists()) badFile0
+      else badFile1
+    try {
+      unzipFile(badFile)
+      false
+    } catch {
+      case e: RuntimeException => e.getMessage.contains("outside of the target directory")
+    }
+  }
+
   private def testUnzip[T](implicit mf: Manifest[T]) =
     unzipFile(IO.classLocationFileOption(mf.runtimeClass).getOrElse(sys.error(s"$mf")))