diff --git a/io/src/main/scala/sbt/io/IO.scala b/io/src/main/scala/sbt/io/IO.scala index 3172fac5..bdd28035 100644 --- a/io/src/main/scala/sbt/io/IO.scala +++ b/io/src/main/scala/sbt/io/IO.scala @@ -387,16 +387,23 @@ 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) @@ -404,18 +411,19 @@ object IO { 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 new file mode 100644 index 00000000..38b3f499 Binary files /dev/null and b/io/src/test/resources/zip-slip.zip differ diff --git a/io/src/test/scala/sbt/io/FileUtilitiesSpecification.scala b/io/src/test/scala/sbt/io/FileUtilitiesSpecification.scala index 394aacb6..d7c378ca 100644 --- 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")))