diff --git a/src/main/scala/better/files/File.scala b/src/main/scala/better/files/File.scala index 4634db00..83ac3f2c 100644 --- a/src/main/scala/better/files/File.scala +++ b/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 } diff --git a/src/test/resources/better/files/issue-624.zip b/src/test/resources/better/files/issue-624.zip new file mode 100644 index 00000000..eeaeda40 Binary files /dev/null and b/src/test/resources/better/files/issue-624.zip differ diff --git a/src/test/scala/better/files/FileSpec.scala b/src/test/scala/better/files/FileSpec.scala index f60cfd8e..e6f5ae47 100644 --- a/src/test/scala/better/files/FileSpec.scala +++ b/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 {