From d50c072af0e08d766991cf43962aa36cb3b7e63a Mon Sep 17 00:00:00 2001 From: Sergej Jaskiewicz Date: Mon, 21 Nov 2022 15:44:45 +0100 Subject: [PATCH] [klib] Validate paths when unpacking zip archives --- .../org/jetbrains/kotlin/konan/file/File.kt | 34 +-------- .../jetbrains/kotlin/konan/file/ZipUtil.kt | 67 +++++++++++++++-- .../kotlin/library/KotlinLibraryUtils.kt | 4 +- .../library/impl/KotlinLibraryLayoutImpl.kt | 12 +++- .../jetbrains/kotlin/cli/klib/test/ZipTest.kt | 71 +++++++++++++++++++ 5 files changed, 146 insertions(+), 42 deletions(-) create mode 100644 kotlin-native/klib/src/test/kotlin/org/jetbrains/kotlin/cli/klib/test/ZipTest.kt diff --git a/compiler/util-io/src/org/jetbrains/kotlin/konan/file/File.kt b/compiler/util-io/src/org/jetbrains/kotlin/konan/file/File.kt index 7c2348190b6..974f5c44adb 100644 --- a/compiler/util-io/src/org/jetbrains/kotlin/konan/file/File.kt +++ b/compiler/util-io/src/org/jetbrains/kotlin/konan/file/File.kt @@ -13,9 +13,7 @@ import java.io.RandomAccessFile import java.nio.MappedByteBuffer import java.nio.channels.FileChannel import java.nio.file.* -import java.nio.file.attribute.BasicFileAttributeView import java.nio.file.attribute.BasicFileAttributes -import java.nio.file.attribute.FileTime data class File constructor(internal val javaPath: Path) { constructor(parent: Path, child: String): this(parent.resolve(child)) @@ -64,12 +62,6 @@ data class File constructor(internal val javaPath: Path) { Files.copy(javaPath, destination.javaPath, StandardCopyOption.REPLACE_EXISTING) } - fun recursiveCopyTo(destination: File, resetTimeAttributes: Boolean = false) { - val sourcePath = javaPath - val destPath = destination.javaPath - sourcePath.recursiveCopyTo(destPath, resetTimeAttributes = resetTimeAttributes) - } - fun renameTo(destination: File) = javaPath.toFile().renameTo(destination.javaPath.toFile()) fun mkdirs() = Files.createDirectories(javaPath) @@ -198,30 +190,6 @@ fun createTempFile(name: String, suffix: String? = null) fun createTempDir(name: String): File = Files.createTempDirectory(name).File() -fun Path.recursiveCopyTo(destPath: Path, resetTimeAttributes: Boolean = false) { - val sourcePath = this - Files.walk(sourcePath).forEach next@ { oldPath -> - - val relative = sourcePath.relativize(oldPath) - val destFs = destPath.getFileSystem() - // We are copying files between file systems, - // so pass the relative path through the String. - val newPath = destFs.getPath(destPath.toString(), relative.toString()) - - // File systems don't allow replacing an existing root. - if (newPath == newPath.getRoot()) return@next - if (Files.isDirectory(newPath)) { - Files.createDirectories(newPath) - } else { - Files.copy(oldPath, newPath, StandardCopyOption.REPLACE_EXISTING) - } - if (resetTimeAttributes) { - val zero = FileTime.fromMillis(0) - Files.getFileAttributeView(newPath, BasicFileAttributeView::class.java).setTimes(zero, zero, zero); - } - } -} - fun bufferedReader(errorStream: InputStream?) = BufferedReader(InputStreamReader(errorStream)) // stdlib `use` function adapted for AutoCloseable. @@ -241,4 +209,4 @@ inline fun T.use(block: (T) -> R): R { this?.close() } } -} \ No newline at end of file +} diff --git a/compiler/util-io/src/org/jetbrains/kotlin/konan/file/ZipUtil.kt b/compiler/util-io/src/org/jetbrains/kotlin/konan/file/ZipUtil.kt index 054e2dc70b3..3f781efd041 100644 --- a/compiler/util-io/src/org/jetbrains/kotlin/konan/file/ZipUtil.kt +++ b/compiler/util-io/src/org/jetbrains/kotlin/konan/file/ZipUtil.kt @@ -5,9 +5,11 @@ package org.jetbrains.kotlin.konan.file -import java.net.URI import java.nio.file.* +import java.nio.file.attribute.BasicFileAttributeView +import java.nio.file.attribute.FileTime import java.nio.file.spi.FileSystemProvider +import java.util.zip.ZipException // Zip filesystem provider doesn't allow creating several instances of ZipFileSystem from the same URI, // so newFileSystem(URI, ...) throws a FileSystemAlreadyExistsException in this case. @@ -15,7 +17,7 @@ import java.nio.file.spi.FileSystemProvider // See also: // https://bugs.java.com/bugdatabase/view_bug.do?bug_id=7001822 // https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6994161 -fun File.zipFileSystem(create: Boolean = false): FileSystem { +private fun File.zipFileSystem(create: Boolean = false): FileSystem { val attributes = hashMapOf("create" to create.toString()) // There is no FileSystems.newFileSystem overload accepting the attribute map. @@ -47,16 +49,67 @@ fun File.zipDirAs(unixFile: File) { } } -fun Path.unzipTo(directory: Path) { - val zipUri = URI.create("jar:" + this.toUri()) - FileSystems.newFileSystem(zipUri, emptyMap(), null).use { zipfs -> - val zipPath = zipfs.getPath("/") - zipPath.recursiveCopyTo(directory) +/** + * Unpacks the contents of a zip archive located in [this] into the [destinationDirectory]. + * + * @param destinationDirectory The directory to unpack the contents to. + * @param resetTimeAttributes Whether to set the newly created files' time attributes + * (creation time, last access time, and last modification time) to zero. + * @param fromSubdirectory A subdirectory inside the archive to unpack. Specify "/" if you need to unpack the whole archive. + */ +fun File.unzipTo(destinationDirectory: File, fromSubdirectory: File = File("/"), resetTimeAttributes: Boolean = false) { + withZipFileSystem { + it.file(fromSubdirectory).recursiveCopyTo(destinationDirectory, resetTimeAttributes) } } +/** + * Unpacks the contents of a zip archive located in [this] into the [destinationDirectory]. + * + * @param destinationDirectory The directory to unpack the contents to. + * @param resetTimeAttributes Whether to set the newly created files' time attributes + * (creation time, last access time, and last modification time) to zero. + * @param fromSubdirectory A subdirectory inside the archive to unpack. Specify "/" if you need to unpack the whole archive. + */ +fun Path.unzipTo(destinationDirectory: Path, fromSubdirectory: Path = Paths.get("/"), resetTimeAttributes: Boolean = false) { + File(this).unzipTo(File(destinationDirectory), File(fromSubdirectory), resetTimeAttributes) +} + fun File.withZipFileSystem(create: Boolean, action: (FileSystem) -> T): T { return this.zipFileSystem(create).use(action) } fun File.withZipFileSystem(action: (FileSystem) -> T): T = this.withZipFileSystem(false, action) + +// TODO: Make this function private after boostrap advance +fun File.recursiveCopyTo(destination: File, resetTimeAttributes: Boolean = false) { + val sourcePath = javaPath + val destPath = destination.javaPath + val destFs = destPath.fileSystem + val normalizedDestPath = destPath.normalize() + Files.walk(sourcePath).forEach next@{ oldPath -> + + val relative = sourcePath.relativize(oldPath) + + // We are copying files between file systems, + // so pass the relative path through the String. + val newPath = destFs.getPath(destPath.toString(), relative.toString()) + + // NOTE: this check is important, it prevents a potential ZipSlip vulnerability + if (!newPath.normalize().startsWith(normalizedDestPath)) { + throw ZipException("$relative attempted to escape the destination directory $destination") + } + + // File systems don't allow replacing an existing root. + if (newPath == newPath.root) return@next + if (Files.isDirectory(newPath)) { + Files.createDirectories(newPath) + } else { + Files.copy(oldPath, newPath, StandardCopyOption.REPLACE_EXISTING) + } + if (resetTimeAttributes) { + val zero = FileTime.fromMillis(0) + Files.getFileAttributeView(newPath, BasicFileAttributeView::class.java).setTimes(zero, zero, zero); + } + } +} diff --git a/compiler/util-klib/src/org/jetbrains/kotlin/library/KotlinLibraryUtils.kt b/compiler/util-klib/src/org/jetbrains/kotlin/library/KotlinLibraryUtils.kt index 539f8dd811f..7c458473136 100644 --- a/compiler/util-klib/src/org/jetbrains/kotlin/library/KotlinLibraryUtils.kt +++ b/compiler/util-klib/src/org/jetbrains/kotlin/library/KotlinLibraryUtils.kt @@ -2,6 +2,7 @@ package org.jetbrains.kotlin.library import org.jetbrains.kotlin.konan.file.File import org.jetbrains.kotlin.konan.file.file +import org.jetbrains.kotlin.konan.file.recursiveCopyTo import org.jetbrains.kotlin.konan.file.withZipFileSystem import org.jetbrains.kotlin.library.impl.zippedKotlinLibraryChecks @@ -23,6 +24,7 @@ fun File.unpackZippedKonanLibraryTo(newDir: File) { newDir.delete() } + // TODO: Replace this with this.unzipTo(newDir) after bootstrap advance this.withZipFileSystem { it.file("/").recursiveCopyTo(newDir) } @@ -32,4 +34,4 @@ fun File.unpackZippedKonanLibraryTo(newDir: File) { val List.toUnresolvedLibraries get() = this.map { UnresolvedLibrary(it, null) - } \ No newline at end of file + } diff --git a/compiler/util-klib/src/org/jetbrains/kotlin/library/impl/KotlinLibraryLayoutImpl.kt b/compiler/util-klib/src/org/jetbrains/kotlin/library/impl/KotlinLibraryLayoutImpl.kt index 597f90f7916..f09ae73a549 100644 --- a/compiler/util-klib/src/org/jetbrains/kotlin/library/impl/KotlinLibraryLayoutImpl.kt +++ b/compiler/util-klib/src/org/jetbrains/kotlin/library/impl/KotlinLibraryLayoutImpl.kt @@ -2,6 +2,8 @@ package org.jetbrains.kotlin.library.impl import org.jetbrains.kotlin.konan.file.File import org.jetbrains.kotlin.konan.file.file +import org.jetbrains.kotlin.konan.file.unzipTo +import org.jetbrains.kotlin.konan.file.recursiveCopyTo import org.jetbrains.kotlin.konan.file.withZipFileSystem import org.jetbrains.kotlin.library.* import org.jetbrains.kotlin.util.removeSuffixIfPresent @@ -109,6 +111,14 @@ private fun extract(zipFile: File, file: File) = zipFile.withZipFileSystem { zip fun KotlinLibraryLayoutImpl.extractDir(directory: File): File = extractDir(this.klib, directory) +// TODO: Use this implementation after bootstrap advance +//private fun extractDir(zipFile: File, directory: File): File { +// val temporary = org.jetbrains.kotlin.konan.file.createTempDir(directory.name) +// temporary.deleteOnExitRecursively() +// zipFile.unzipTo(temporary, fromSubdirectory = directory) +// return temporary +//} + private fun extractDir(zipFile: File, directory: File): File = zipFile.withZipFileSystem { zipFileSystem -> val temporary = org.jetbrains.kotlin.konan.file.createTempDir(directory.name) zipFileSystem.file(directory).recursiveCopyTo(temporary) @@ -162,4 +172,4 @@ internal fun zippedKotlinLibraryChecks(klibFile: File) { check(extension.isEmpty() || extension == KLIB_FILE_EXTENSION || extension == "jar") { "KLIB path has unexpected extension: $klibFile" } -} \ No newline at end of file +} diff --git a/kotlin-native/klib/src/test/kotlin/org/jetbrains/kotlin/cli/klib/test/ZipTest.kt b/kotlin-native/klib/src/test/kotlin/org/jetbrains/kotlin/cli/klib/test/ZipTest.kt new file mode 100644 index 00000000000..d95aab0e0c0 --- /dev/null +++ b/kotlin-native/klib/src/test/kotlin/org/jetbrains/kotlin/cli/klib/test/ZipTest.kt @@ -0,0 +1,71 @@ +/* + * Copyright 2010-2022 JetBrains s.r.o. and Kotlin Programming Language contributors. + * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file. + */ + +package org.jetbrains.kotlin.cli.klib.test + +import org.jetbrains.kotlin.konan.file.File +import org.jetbrains.kotlin.konan.file.unzipTo +import org.jetbrains.kotlin.konan.file.use +import org.jetbrains.kotlin.library.impl.javaFile +import org.junit.After +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TestName +import java.io.FileOutputStream +import java.io.IOException +import java.util.zip.ZipEntry +import java.util.zip.ZipException +import java.util.zip.ZipOutputStream +import kotlin.test.assertFailsWith + +class ZipTest { + + @Rule + @JvmField + val currentTestName = TestName() + + private lateinit var tmpDir: File + + @Before + fun setUp() { + tmpDir = org.jetbrains.kotlin.konan.file.createTempDir(currentTestName.methodName) + tmpDir.deleteOnExitRecursively() + } + + @Test + fun testZipSlip() { + // https://security.snyk.io/research/zip-slip-vulnerability + val zipArchive = tmpDir.child("sneaky.klib") + + createMaliciousArchive(zipArchive) + + try { + zipArchive.unzipTo(tmpDir.child("unpacked")) + } catch (e: Exception) { + if (e !is IOException && e.cause !is IOException) throw e + } + + assert(!tmpDir.child("definitelySafe.txt").exists) { "ZipSlip vulnerability found!" } + } + + @Test + fun testZipSlipValidation() { + val zipArchive = tmpDir.child("sneaky.klib") + + createMaliciousArchive(zipArchive) + + assertFailsWith { + zipArchive.unzipTo(tmpDir.child("unpacked")) + } + } +} + +private fun createMaliciousArchive(file: File) { + ZipOutputStream(FileOutputStream(file.javaFile())).use { + it.putNextEntry(ZipEntry("../definitelySafe.txt")) + it.closeEntry() + } +}