diff --git a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt index 39f668e428..07d7ddd71d 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -245,6 +245,14 @@ object AssetsInstallationHelper { Files.createDirectories(destDir) // Normalize and make destDir absolute for secure path validation val normalizedDestDir = destDir.toAbsolutePath().normalize() + val realDestDir = normalizedDestDir.toRealPath() + + // Zip entries are commonly clustered by directory (e.g. dozens of files + // under the same build-tools// prefix); cache the last-verified + // parent so consecutive entries under it skip a redundant toRealPath() call. + // Nothing below can turn an already-verified real directory into a symlink + // mid-run, so caching by lexical parent equality is safe. + var lastVerifiedParent: Path? = null ZipInputStream(srcStream.buffered()).useEntriesEach { zipInput, entry -> // Validate entry name doesn't contain dangerous patterns @@ -260,9 +268,28 @@ object AssetsInstallationHelper { throw IllegalStateException("Entry is outside of the target dir: ${entry.name}") } + // The checks above are lexical (entry name only) and don't catch a symlink + // already present on disk (e.g. destDir merged/reused across installer + // runs). Reject writing through an existing symlink up front, then + // re-check containment against the real, on-disk path once created. + if (Files.isSymbolicLink(destFile)) { + throw IllegalStateException("Refusing to extract over an existing symlink: ${entry.name}") + } + if (entry.isDirectory) { Files.createDirectories(destFile) + if (!destFile.toRealPath().startsWith(realDestDir)) { + throw IllegalStateException("Entry escapes the target dir via symlink: ${entry.name}") + } } else { + Files.createDirectories(destFile.parent) + if (destFile.parent != lastVerifiedParent) { + if (!destFile.parent.toRealPath().startsWith(realDestDir)) { + throw IllegalStateException("Entry parent escapes the target dir via symlink: ${entry.name}") + } + lastVerifiedParent = destFile.parent + } + Files.newOutputStream(destFile).use { dest -> zipInput.copyTo(dest) } diff --git a/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt b/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt index 9c7b46297a..33fcfc988f 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt @@ -29,7 +29,6 @@ import java.io.FileNotFoundException import java.io.IOException import java.nio.file.Files import java.nio.file.Path -import java.util.zip.ZipInputStream import kotlin.io.path.ExperimentalPathApi import kotlin.io.path.deleteRecursively @@ -174,27 +173,7 @@ data object BundledAssetsInstaller : BaseAssetsInstaller() { val assetPath = ToolsManager.getCommonAsset("$entryName.br") assets.open(assetPath).use { assetStream -> BrotliInputStream(assetStream).use { brotliStream -> - ZipInputStream(brotliStream).use { pluginZip -> - var pluginEntry = pluginZip.nextEntry - while (pluginEntry != null) { - if (!pluginEntry.isDirectory) { - val targetPath = pluginDirPath.resolve(pluginEntry.name).normalize() - // Security check: prevent path traversal attacks - if (!targetPath.startsWith(pluginDirPath)) { - throw IllegalStateException( - "Zip entry '${pluginEntry.name}' would escape target directory", - ) - } - val targetFile = targetPath.toFile() - targetFile.parentFile?.mkdirs() - logger.debug("Extracting '{}' to {}", pluginEntry.name, targetFile) - targetFile.outputStream().use { output -> - pluginZip.copyTo(output) - } - } - pluginEntry = pluginZip.nextEntry - } - } + AssetsInstallationHelper.extractZipToDir(brotliStream, pluginDirPath) } } logger.debug("Completed extracting plugin artifacts") diff --git a/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt b/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt index a7268c2215..ba532d2f8a 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt @@ -23,7 +23,6 @@ import java.io.FileNotFoundException import java.nio.file.Files import java.nio.file.Path import java.util.zip.ZipFile -import java.util.zip.ZipInputStream import kotlin.io.path.ExperimentalPathApi import kotlin.io.path.deleteRecursively import kotlin.system.measureTimeMillis @@ -167,27 +166,7 @@ data object SplitAssetsInstaller : BaseAssetsInstaller() { } Files.createDirectories(pluginDirPath) - ZipInputStream(zipInput).use { pluginZip -> - var pluginEntry = pluginZip.nextEntry - while (pluginEntry != null) { - if (!pluginEntry.isDirectory) { - val targetPath = pluginDirPath.resolve(pluginEntry.name).normalize() - // Security check: prevent path traversal attacks - if (!targetPath.startsWith(pluginDirPath)) { - throw IllegalStateException( - "Zip entry '${pluginEntry.name}' would escape target directory", - ) - } - val targetFile = targetPath.toFile() - targetFile.parentFile?.mkdirs() - logger.debug("Extracting '{}' to {}", pluginEntry.name, targetFile) - targetFile.outputStream().use { output -> - pluginZip.copyTo(output) - } - } - pluginEntry = pluginZip.nextEntry - } - } + AssetsInstallationHelper.extractZipToDir(zipInput, pluginDirPath) logger.debug("Completed extracting plugin artifacts") } diff --git a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt index 1cd9c8455b..2a4a8e8c28 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt @@ -7,11 +7,23 @@ import io.mockk.every import io.mockk.mockk import io.mockk.mockkObject import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test +import java.io.ByteArrayInputStream +import java.io.ByteArrayOutputStream import java.io.FileNotFoundException +import java.io.IOException +import java.nio.file.FileVisitResult +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.SimpleFileVisitor +import java.nio.file.attribute.BasicFileAttributes +import java.util.zip.ZipEntry +import java.util.zip.ZipOutputStream class AssetsInstallationHelperTest { private val ctx: Context = mockk(relaxed = true) @@ -48,4 +60,94 @@ class AssetsInstallationHelperTest { (failure.cause?.cause) is FileNotFoundException, ) } + + @Test + fun `extractZipToDir creates parent directories for nested entries with no directory entries`() { + val destDir = Files.createTempDirectory("extract-zip-to-dir-test") + try { + val content = "test notice content" + val zipBytes = + ByteArrayOutputStream().use { baos -> + ZipOutputStream(baos).use { zos -> + // No directory entries, matching how android-sdk.zip is packaged. + zos.putNextEntry(ZipEntry("build-tools/35.0.0/NOTICE.txt")) + zos.write(content.toByteArray()) + zos.closeEntry() + } + baos.toByteArray() + } + + AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir) + + val extracted = destDir.resolve("build-tools/35.0.0/NOTICE.txt") + assertTrue("Expected extracted file to exist", Files.exists(extracted)) + assertEquals(content, String(Files.readAllBytes(extracted))) + } finally { + destDir.toFile().deleteRecursively() + } + } + + @Test + fun `extractZipToDir rejects a file entry whose pre-existing symlinked grandparent escapes destDir`() { + val destDir = Files.createTempDirectory("extract-zip-to-dir-test") + val outsideDir = Files.createTempDirectory("extract-zip-to-dir-outside") + try { + Files.createSymbolicLink(destDir.resolve("linked"), outsideDir) + + val content = "escaping content" + val zipBytes = + ByteArrayOutputStream().use { baos -> + ZipOutputStream(baos).use { zos -> + // Two levels below the symlink ("linked/sub/nested.txt", no directory + // entries), not one: for a one-level entry ("linked/nested.txt"), + // destFile.parent IS the symlink, so Files.createDirectories() throws + // FileAlreadyExistsException (NOFOLLOW_LINKS rejects the existing + // symlink-to-dir) before the toRealPath() guard below it ever runs. One + // level deeper, createDirectories() silently traverses the symlink to + // create "sub" for real inside outsideDir, and only then does the + // toRealPath() check on destFile.parent fire -- which is what this test + // exercises. + zos.putNextEntry(ZipEntry("linked/sub/nested.txt")) + zos.write(content.toByteArray()) + zos.closeEntry() + } + baos.toByteArray() + } + + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir) + } + } finally { + outsideDir.deleteRecursivelyWithoutFollowingLinks() + destDir.deleteRecursivelyWithoutFollowingLinks() + } + } + + // Deletes a directory tree without following symlinks it contains, unlike + // File.deleteRecursively(). Files.walkFileTree() doesn't follow symlinks unless + // FileVisitOption.FOLLOW_LINKS is passed (it isn't here), so a symlink is visited + // as a leaf via visitFile() -- deleting it unlinks the link itself, never the + // target it points to. Needed because the symlink test above symlinks out of destDir. + private fun Path.deleteRecursivelyWithoutFollowingLinks() { + Files.walkFileTree( + this, + object : SimpleFileVisitor() { + override fun visitFile( + file: Path, + attrs: BasicFileAttributes, + ): FileVisitResult { + Files.delete(file) + return FileVisitResult.CONTINUE + } + + override fun postVisitDirectory( + dir: Path, + exc: IOException?, + ): FileVisitResult { + Files.delete(dir) + return FileVisitResult.CONTINUE + } + }, + ) + } } diff --git a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt index 9f4ab7f879..92868c1daf 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt @@ -6,16 +6,26 @@ import org.junit.Assert.assertTrue import org.junit.Test import java.io.ByteArrayInputStream import java.io.ByteArrayOutputStream +import java.io.IOException +import java.nio.file.FileVisitResult import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.SimpleFileVisitor +import java.nio.file.attribute.BasicFileAttributes import java.util.zip.ZipEntry import java.util.zip.ZipOutputStream class ExtractZipToDirMergeTest { - // Real archives merged in production (e.g. plugin-maven-repo.zip, built by Gradle's - // Zip task -- verified via `unzip -l assets/plugin-maven-repo.zip`) always carry an - // explicit directory entry for every ancestor path. extractZipToDir relies on that - // (it only Files.createDirectories() on directory entries, not on every file - // entry's parent), so mirror that shape here rather than writing bare file entries. + // extractZipToDir now calls Files.createDirectories(destFile.parent) for every file + // entry, not just directory entries, so a bare file entry with no ancestor directory + // entries would extract fine too. zipOf still injects a directory entry for every + // ancestor because that's how real archives merged in production (e.g. + // plugin-maven-repo.zip, built by Gradle's Zip task -- verified via `unzip -l + // assets/plugin-maven-repo.zip`) are actually packaged. The no-directory-entry path + // (e.g. how android-sdk.zip is packaged) is exercised separately by + // AssetsInstallationHelperTest's `extractZipToDir creates parent directories for + // nested entries with no directory entries` and `extractZipToDir rejects a file + // entry whose pre-existing symlinked grandparent escapes destDir`. private fun zipOf(vararg entries: Pair): ByteArrayInputStream { val bos = ByteArrayOutputStream() ZipOutputStream(bos).use { zip -> @@ -40,6 +50,34 @@ class ExtractZipToDirMergeTest { return ByteArrayInputStream(bos.toByteArray()) } + // Deletes a directory tree without following symlinks it contains, unlike + // File.deleteRecursively(). Files.walkFileTree() doesn't follow symlinks unless + // FileVisitOption.FOLLOW_LINKS is passed (it isn't here), so a symlink is visited + // as a leaf via visitFile() -- deleting it unlinks the link itself, never the + // target it points to. Needed because several tests below symlink out of dest. + private fun Path.deleteRecursivelyWithoutFollowingLinks() { + Files.walkFileTree( + this, + object : SimpleFileVisitor() { + override fun visitFile( + file: Path, + attrs: BasicFileAttributes, + ): FileVisitResult { + Files.delete(file) + return FileVisitResult.CONTINUE + } + + override fun postVisitDirectory( + dir: Path, + exc: IOException?, + ): FileVisitResult { + Files.delete(dir) + return FileVisitResult.CONTINUE + } + }, + ) + } + @Test fun `overlay merges without wiping existing files`() { val dest = @@ -47,27 +85,111 @@ class ExtractZipToDirMergeTest { Files.createDirectories(it.resolve("com/foo/1.0")) Files.write(it.resolve("com/foo/1.0/foo-1.0.jar"), "harvested".toByteArray()) } + try { + AssetsInstallationHelper.extractZipToDir( + zipOf("com/itsaky/androidide/plugin-api/1.0.0/plugin-api-1.0.0.jar" to "fat"), + dest, + ) - AssetsInstallationHelper.extractZipToDir( - zipOf("com/itsaky/androidide/plugin-api/1.0.0/plugin-api-1.0.0.jar" to "fat"), - dest, - ) + assertTrue( + "harvested file must survive the merge", + Files.exists(dest.resolve("com/foo/1.0/foo-1.0.jar")), + ) + assertEquals( + "fat", + String(Files.readAllBytes(dest.resolve("com/itsaky/androidide/plugin-api/1.0.0/plugin-api-1.0.0.jar"))), + ) + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + } + } - assertTrue( - "harvested file must survive the merge", - Files.exists(dest.resolve("com/foo/1.0/foo-1.0.jar")), - ) - assertEquals( - "fat", - String(Files.readAllBytes(dest.resolve("com/itsaky/androidide/plugin-api/1.0.0/plugin-api-1.0.0.jar"))), - ) + @Test + fun `extracts multiple sibling files under the same directory`() { + val dest = Files.createTempDirectory("mvn") + try { + AssetsInstallationHelper.extractZipToDir( + zipOf( + "com/foo/1.0/a.txt" to "a", + "com/foo/1.0/b.txt" to "b", + ), + dest, + ) + + assertEquals("a", String(Files.readAllBytes(dest.resolve("com/foo/1.0/a.txt")))) + assertEquals("b", String(Files.readAllBytes(dest.resolve("com/foo/1.0/b.txt")))) + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + } } @Test fun `rejects path traversal`() { val dest = Files.createTempDirectory("mvn") - assertThrows(IllegalStateException::class.java) { - AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest) + try { + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest) + } + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + } + } + + @Test + fun `rejects extraction over an existing symlink`() { + val dest = Files.createTempDirectory("mvn") + val outside = Files.createTempDirectory("outside") + try { + val outsideTarget = outside.resolve("payload") + Files.createSymbolicLink(dest.resolve("evil.jar"), outsideTarget) + + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest) + } + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + outside.deleteRecursivelyWithoutFollowingLinks() + } + } + + @Test + fun `rejects extraction into a symlinked parent that escapes destDir`() { + val dest = Files.createTempDirectory("mvn") + val outside = Files.createTempDirectory("outside") + try { + Files.createSymbolicLink(dest.resolve("linked"), outside) + + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("linked/nested.txt" to "x"), dest) + } + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + outside.deleteRecursivelyWithoutFollowingLinks() + } + } + + @Test + fun `rejects a bare directory entry that resolves to an existing symlink escaping destDir`() { + val dest = Files.createTempDirectory("mvn") + val outside = Files.createTempDirectory("outside") + try { + Files.createSymbolicLink(dest.resolve("linked"), outside) + + val zipBytes = + ByteArrayOutputStream().use { baos -> + ZipOutputStream(baos).use { zip -> + zip.putNextEntry(ZipEntry("linked/")) + zip.closeEntry() + } + baos.toByteArray() + } + + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), dest) + } + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + outside.deleteRecursivelyWithoutFollowingLinks() } } }