From 2fdb050c7666e487e55a783de1ae8a0e2fa8a4bb Mon Sep 17 00:00:00 2001 From: Artem Egorkine Date: Sat, 9 Mar 2024 00:11:14 +0200 Subject: [PATCH] Fix issue #256 logic and test * Fix else-logic error in File.unzipTo; * Fix test for issue 624 by creating a file structure safe for unpacking provided ZIP file; --- src/main/scala/better/files/File.scala | 4 +++- src/test/scala/better/files/FileSpec.scala | 17 +++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/main/scala/better/files/File.scala b/src/main/scala/better/files/File.scala index f90f28cd..05472e14 100644 --- a/src/main/scala/better/files/File.scala +++ b/src/main/scala/better/files/File.scala @@ -1094,7 +1094,9 @@ class File private (val path: Path)(implicit val fileSystem: FileSystem = path.g } if (safeUnzip && File(destination, entry.getName).canonicalPath.startsWith(destination.canonicalPath + JFile.separator)) { // https://developer.android.com/topic/security/risks/zip-path-traversal entry.extractTo(destination, zipFile.getInputStream(entry)) - } else entry.extractTo(destination, zipFile.getInputStream(entry)) + } else if (!safeUnzip) { + entry.extractTo(destination, zipFile.getInputStream(entry)) + } destination } diff --git a/src/test/scala/better/files/FileSpec.scala b/src/test/scala/better/files/FileSpec.scala index 9c62bcd8..03c138e1 100644 --- a/src/test/scala/better/files/FileSpec.scala +++ b/src/test/scala/better/files/FileSpec.scala @@ -544,19 +544,28 @@ class FileSpec extends CommonSpec { } it should "unzip unsafely when safe unzip is disabled" in { - assume(isLinux) // See: https://github.com/pathikrit/better-files/pull/633#issuecomment-1741292532 // Unsafe unzip with a zipslip attack may result in OS access denied exceptions. If these occur, the test should still pass. - var list = List.empty[File] + var destList = List.empty[File] + var list = List.empty[File] try { - list = File("src/test/resources/better/files/issue-624.zip") - .unzipTo(safeUnzip = false) + // create the directory structure safe for issue 624 zip file + val toplevel = File.newTemporaryDirectory("issue-624") + val dest = (toplevel / "a" / "b").createDirectories() + + destList = File("src/test/resources/better/files/issue-624.zip") + .unzipTo(destination = dest, safeUnzip = false) + .listRecursively() + .toList + list = toplevel .listRecursively() + .filterNot(_.isDirectory()) .toList } catch { // Unsafe unzip should try to extract all entries, might result in access denied exception from OS. case exception: Throwable => assert(exception.isInstanceOf[AccessDeniedException]) } // Three total entries in the zip file: 1 without directory traversal characters and 2 with. + assert(destList.length === 1) assert(list.length === 3) }