From 46b6091e42d051fd33e1da5d3ddf42444237d299 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 11:41:34 -0700 Subject: [PATCH 01/14] ADFA-5257: Share one path-containment check instead of two divergent copies ZipUtils.unzipFile checked only that a canonical path started with the destination prefix: no lexical rejection of a ".." segment, and nothing to stop an entry writing through a symlink already present at its target. AssetsInstallationHelper.extractZipToDir had the elaborate version -- lexical reject, Path.startsWith, a refusal to follow an existing symlink, and a hand-rolled per-parent cache over toRealPath. Each carried a comment asking whoever fixed one to remember the other. Both now call ContainedPathResolver in common. The file is plain java.io/java.nio with no Android dependency and app already depends on common, so the reason the copies gave for existing was never true in the direction that mattered. The installer's substring reject of ".." goes with it: an archive entry legitimately named notes..txt used to abort an entire asset installation. Only a literal ".." segment can name a parent directory, so the per-segment rule loses nothing. What is deliberately not shared is the policy for an existing symlink at a target whose destination is still inside the base. Unzipping a user's project skips the entry and leaves their own gradlew symlink alone; the installer refuses to write through any symlink. That check stays at each call site, one line, labelled as policy. The resolver carries the ancestor caching the installer did by hand, so a bootstrap archive clustering thousands of entries under a few directories still resolves each ancestor once. Verified: 342 tests pass across both modules, and ZipUtils' symlink test fails against the previous implementation -- this is a stronger guard, not a move. Co-Authored-By: Claude Opus 5 --- .../assets/AssetsInstallationHelper.kt | 54 ++---- .../assets/ExtractZipToDirMergeTest.kt | 19 +++ .../itsaky/androidide/utils/PathTraversal.kt | 137 +++++++++++++++ .../com/itsaky/androidide/utils/ZipUtils.kt | 20 ++- .../androidide/utils/PathTraversalTest.kt | 160 ++++++++++++++++++ .../itsaky/androidide/utils/ZipUtilsTest.kt | 69 ++++++++ 6 files changed, 419 insertions(+), 40 deletions(-) create mode 100644 common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt create mode 100644 common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt 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 9359ece5aa..38abc78446 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -6,6 +6,7 @@ import androidx.annotation.WorkerThread import com.aayushatharva.brotli4j.Brotli4jLoader import com.itsaky.androidide.app.configuration.IDEBuildConfigProvider import com.itsaky.androidide.resources.R +import com.itsaky.androidide.utils.ContainedPathResolver import com.itsaky.androidide.utils.Environment.DEFAULT_ROOT import com.itsaky.androidide.utils.useEntriesEach import kotlinx.coroutines.Dispatchers @@ -254,59 +255,40 @@ object AssetsInstallationHelper { destDir: Path, ) = extractZipToDir(Files.newInputStream(srcFile), destDir) + /** + * Containment is [ContainedPathResolver]'s, shared with `ZipUtils.unzipFile` and the deep-link + * reader. It also carries the caching this loop used to do by hand: bootstrap archives cluster + * thousands of entries under a handful of directories, and the resolver memoizes an ancestor once + * it is proven contained. + * + * What stays local is the policy: this refuses to write through *any* existing symlink at an + * entry's target, even one pointing inside destDir. An installer directory reused across runs is + * the case that matters, and unlike unzipping a user's project there is no legitimate reason for + * a symlink to be there. + */ @WorkerThread internal fun extractZipToDir( srcStream: InputStream, destDir: Path, ) { 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 + val contained = ContainedPathResolver(destDir.toFile()) ZipInputStream(srcStream.buffered()).useEntriesEach { zipInput, entry -> - // Validate entry name doesn't contain dangerous patterns - if (entry.name.contains("..") || entry.name.startsWith("/") || entry.name.startsWith("\\")) { - throw IllegalStateException("Zip entry contains dangerous path components: ${entry.name}") - } - - val destFile = normalizedDestDir.resolve(entry.name).normalize() - - // Use Path.startsWith() for proper path validation instead of string comparison - if (!destFile.startsWith(normalizedDestDir)) { - // DO NOT allow extraction to outside of the target dir - throw IllegalStateException("Entry is outside of the target dir: ${entry.name}") - } + val destFile = + contained.resolve(entry.name)?.toPath() + ?: throw IllegalStateException("Zip entry escapes 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. + // Policy, not containment: the resolver allows a symlink whose target is still inside + // destDir, and this caller does not. 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/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt index 92868c1daf..d1a1ac2cd7 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt @@ -123,6 +123,25 @@ class ExtractZipToDirMergeTest { } } + // The lexical guard used to be a bare substring reject, so an entry legitimately named with + // consecutive dots aborted the whole installation. Sharing ContainedPathResolver with ZipUtils + // and the deep-link reader brought the per-segment rule here too. + @Test + fun `extracts an entry whose name merely contains a double dot`() { + val destDir = Files.createTempDirectory("assets-dots") + try { + AssetsInstallationHelper.extractZipToDir( + zipOf("lib/notes..txt" to "kept", "lib/a..b/c.txt" to "also kept"), + destDir, + ) + + assertEquals("kept", destDir.resolve("lib/notes..txt").toFile().readText()) + assertEquals("also kept", destDir.resolve("lib/a..b/c.txt").toFile().readText()) + } finally { + destDir.toFile().deleteRecursively() + } + } + @Test fun `rejects path traversal`() { val dest = Files.createTempDirectory("mvn") diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt new file mode 100644 index 0000000000..ee1b815830 --- /dev/null +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -0,0 +1,137 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import java.io.File +import java.io.IOException +import java.nio.file.Files +import java.nio.file.InvalidPathException +import java.nio.file.LinkOption +import java.nio.file.Path + +/** + * Decides whether a relative path is safely inside a base directory -- the one containment + * algorithm in the codebase, and the only place it should be implemented. + * + * Built for attacker-controllable input: the `{filename}` segment of a deep-link URL, and the entry + * names in a zip. It lives in `common` because it is plain `java.io`/`java.nio` with no Android + * dependency, so both the app and this module's own [ZipUtils] can call it. Three near-copies used + * to exist, each with a comment asking whoever fixed one to remember the other two; they had + * already drifted apart on the `..` rule by the time this replaced them. + * + * Three layers: + * 1. A lexical reject of an empty string, a `..` *segment*, or a leading `/` or `\`. Per segment, + * not as a substring: `notes..txt` and `a..b/c.kt` are legitimate filenames, and only a literal + * `..` segment can name a parent directory, so nothing is lost. + * 2. Resolve + normalize against the base and verify with [java.nio.file.Path.startsWith] (not + * string prefix matching, which would wrongly accept `/project` as inside `/project-evil`). This + * operates on Java's own resolved path, so it is not fooled by however `..` reached the string. + * 3. Resolve the nearest *existing* ancestor of that path to its real, on-disk path via + * [java.nio.file.Path.toRealPath] and re-verify containment -- layer 2 is purely lexical and + * will not catch a symlink already present inside the base (a project cloned with git, an + * installer directory reused across runs). Walking up to the nearest existing ancestor handles a + * path that does not exist yet. Skipped when the base itself does not exist: there is nothing on + * disk to symlink-escape through. + * + * What this deliberately does *not* decide is what to do about an existing symlink *at the target* + * whose destination is still inside the base. Callers disagree: unzipping leaves a user's own + * `gradlew` symlink alone, the asset installer refuses to write through any symlink at all, and the + * deep-link reader is content to follow one. That is policy, and it stays visible at each call site + * rather than being buried here. + * + * Not thread-safe: [resolve] memoizes verified ancestors, which is what makes it usable per zip + * entry. Extraction is single-threaded; construct one per operation. + */ +class ContainedPathResolver( + baseDir: File, +) { + private val base: Path = baseDir.toPath().toAbsolutePath().normalize() + + // Null when the base does not exist on disk, which makes layer 3 unnecessary. + private val realBase: Path? = + try { + if (Files.exists(base)) base.toRealPath() else null + } catch (_: IOException) { + null + } + + // Zip entries cluster by directory (dozens of files under one build-tools//), so the + // same ancestor is otherwise re-resolved for each of them. Only ever holds directories already + // proven contained, and nothing here can turn a verified real directory into a symlink mid-run. + private val verifiedAncestors = HashSet() + + /** + * The resolved file [relativePath] names inside the base directory, or null when it is invalid + * or escapes -- including when it is not a representable path at all (a decoded NUL byte, say: + * `Uri.pathSegments` percent-decodes before this ever sees the string, so `%00` arrives as a + * literal NUL, which [java.nio.file.Path] rejects rather than silently ignoring). + */ + fun resolve(relativePath: String): File? { + // Split on both separators: '\' is not a path separator on Android, but a caller handing + // over a Windows-style path should not have it treated as one long filename. + if (relativePath.isEmpty() || + relativePath.startsWith("/") || + relativePath.startsWith("\\") || + relativePath.split('/', '\\').any { it == ".." } + ) { + return null + } + + return try { + val resolved = base.resolve(relativePath).normalize() + if (!resolved.startsWith(base)) { + return null + } + + val realBase = realBase ?: return resolved.toFile() + + var ancestor = resolved + // NOFOLLOW_LINKS: plain Files.exists() follows symlinks, so a *dangling* symlink (one + // whose target does not currently exist) would read as absent here, walking straight + // past it to its parent instead of stopping to verify it. toRealPath() below throws + // IOException for a genuinely dangling target, correctly rejecting the path rather than + // trusting whatever ends up on the far side of it later. + while (!Files.exists(ancestor, LinkOption.NOFOLLOW_LINKS)) { + ancestor = ancestor.parent ?: return null + } + if (ancestor in verifiedAncestors) { + return resolved.toFile() + } + if (!ancestor.toRealPath().startsWith(realBase)) { + return null + } + if (Files.isDirectory(ancestor)) { + verifiedAncestors.add(ancestor) + } + resolved.toFile() + } catch (_: InvalidPathException) { + null + } catch (_: IOException) { + null + } + } +} + +/** + * [ContainedPathResolver.resolve] for a single path, where there is nothing to reuse a resolver for. + * Prefer the class when validating many paths against one base -- a zip's entries, say. + */ +fun resolveWithinDirectory( + baseDir: File, + relativePath: String, +): File? = ContainedPathResolver(baseDir).resolve(relativePath) diff --git a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt index a21c2ab5f0..e01ed151b7 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt @@ -19,6 +19,7 @@ package com.itsaky.androidide.utils import java.io.File import java.io.IOException +import java.nio.file.Files import java.util.zip.ZipFile object ZipUtils { @@ -34,19 +35,30 @@ object ZipUtils { destDir: File, ): List { destDir.mkdirs() - val destDirPath = destDir.canonicalPath + File.separator + val contained = ContainedPathResolver(destDir) val result = mutableListOf() ZipFile(zipFile).use { zip -> val entries = zip.entries() while (entries.hasMoreElements()) { val entry = entries.nextElement() - val outFile = File(destDir, entry.name) - if (!outFile.canonicalPath.startsWith(destDirPath)) { - throw IOException("Zip entry is outside of the target directory: ${entry.name}") + // Policy before containment, deliberately: a user's own symlink inside their project + // -- gradlew, or gradle/wrapper pointed at a shared location -- is legitimate, so the + // entry is skipped and the symlink left alone, where asking the resolver first would + // reject one pointing outside destDir and abort the whole archive. Reading a path's + // link status cannot itself escape. + if (Files.isSymbolicLink(File(destDir, entry.name).toPath())) { + continue } + // Containment is ContainedPathResolver's, shared with the asset installer: a canonical + // path prefix alone accepted a "..", and could not tell a symlinked ancestor from a + // real directory (ADFA-5257). + val outFile = + contained.resolve(entry.name) + ?: throw IOException("Zip entry escapes the target directory: ${entry.name}") + if (entry.isDirectory) { outFile.mkdirs() } else { diff --git a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt new file mode 100644 index 0000000000..d24ab8efc8 --- /dev/null +++ b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt @@ -0,0 +1,160 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import org.junit.Assume +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import java.io.File +import java.nio.file.FileSystemException +import java.nio.file.Files + +class PathTraversalTest { + private val baseDir = File("/project/root") + private val nulCharacter = 0.toChar() + + @JvmField + @Rule + val tempFolder = TemporaryFolder() + + @Test + fun `plain relative path resolves inside base`() { + val resolved = resolveWithinDirectory(baseDir, "src/Main.kt") + assertThat(resolved).isEqualTo(File(baseDir, "src/Main.kt").absoluteFile) + } + + @Test + fun `literal dot-dot is rejected`() { + assertThat(resolveWithinDirectory(baseDir, "../../etc/passwd")).isNull() + } + + @Test + fun `empty relative path is rejected instead of resolving to baseDir itself`() { + // Regression test: java.nio.file.Path.resolve("") is a documented no-op, returning the base + // path unchanged -- without an explicit empty-string check, the containment check below + // would trivially pass and this function would violate its own "returns null" contract, + // silently returning baseDir. DeepLinkRequest.parse's own documented "known limitation" (a + // file path whose entire content is just the "line" keyword) produces exactly this shape. + assertThat(resolveWithinDirectory(baseDir, "")).isNull() + } + + @Test + fun `dot-dot buried in the middle of a path is rejected`() { + // The shape produced once android.net.Uri decodes a single raw segment containing an + // encoded slash, e.g. the URL segment "foo%2f..%2f..%2fetc%2fpasswd" -- decoded to one + // string, but still containing ".." once decoded. + assertThat(resolveWithinDirectory(baseDir, "foo/../../etc/passwd")).isNull() + } + + @Test + fun `leading slash is rejected`() { + assertThat(resolveWithinDirectory(baseDir, "/etc/passwd")).isNull() + } + + @Test + fun `leading backslash is rejected`() { + assertThat(resolveWithinDirectory(baseDir, "\\Windows\\System32")).isNull() + } + + @Test + fun `embedded NUL character is rejected instead of throwing`() { + // android.net.Uri.pathSegments percent-decodes before this function ever sees the string, so + // a URL's "%00" arrives here as a literal NUL character. java.nio.file.Path throws + // InvalidPathException for that -- must be caught, not left to crash the caller. + assertThat(resolveWithinDirectory(baseDir, "foo" + nulCharacter + ".txt")).isNull() + } + + // This used to be rejected, on the reasoning that project files never legitimately need + // consecutive dots. They do -- and a deep link to one failing with no explanation is a bug, not + // a safe trade-off. Nothing is given up: only a literal ".." *segment* can name a parent, and + // the tests below cover every way of writing one. + @Test + fun `a filename containing dot-dot is resolved, not rejected`() { + assertThat(resolveWithinDirectory(baseDir, "notes..txt")) + .isEqualTo(File(baseDir, "notes..txt").absoluteFile) + assertThat(resolveWithinDirectory(baseDir, "a..b/c.kt")) + .isEqualTo(File(baseDir, "a..b/c.kt").absoluteFile) + assertThat(resolveWithinDirectory(baseDir, "....gitignore")) + .isEqualTo(File(baseDir, "....gitignore").absoluteFile) + } + + // The segment itself, in every position, is still refused. + @Test + fun `a dot-dot segment is rejected wherever it appears`() { + assertThat(resolveWithinDirectory(baseDir, "..")).isNull() + assertThat(resolveWithinDirectory(baseDir, "../x")).isNull() + assertThat(resolveWithinDirectory(baseDir, "a/../b")).isNull() + assertThat(resolveWithinDirectory(baseDir, "a/..")).isNull() + assertThat(resolveWithinDirectory(baseDir, "a\\..\\b")).isNull() + } + + // Percent-decoding happens in Uri.pathSegments before this function sees the string, so an + // encoded traversal arrives as a literal ".." segment and is caught above. A double-encoded one + // arrives as the harmless filename "%2e%2e", which cannot name a parent directory. + @Test + fun `a double-encoded dot-dot is an ordinary filename`() { + assertThat(resolveWithinDirectory(baseDir, "%2e%2e/x")) + .isEqualTo(File(baseDir, "%2e%2e/x").absoluteFile) + } + + @Test + fun `multi-segment path resolves and normalizes redundant separators`() { + val resolved = resolveWithinDirectory(baseDir, "app/src/main/Main.kt") + assertThat(resolved).isEqualTo(File("/project/root/app/src/main/Main.kt")) + } + + @Test + fun `plain file inside a real base directory still resolves`() { + val root = tempFolder.newFolder("real-project") + File(root, "src").mkdirs() + val target = File(root, "src/Main.kt").apply { writeText("fun main() {}") } + + val resolved = resolveWithinDirectory(root, "src/Main.kt") + assertThat(resolved?.canonicalFile).isEqualTo(target.canonicalFile) + } + + @Test + fun `symlink inside base pointing outside it is rejected`() { + // Regression test: the lexical/normalize check alone doesn't catch a symlink physically + // present inside the project directory (e.g. from a git clone, which supports symlinks) that + // points outside it -- resolveWithinDirectory must also verify the real, on-disk path. + val root = tempFolder.newFolder("real-project") + val outside = tempFolder.newFolder("outside") + File(outside, "secret.txt").writeText("secret") + + val symlinkCreated = + try { + Files.createSymbolicLink(File(root, "evil").toPath(), outside.toPath()) + true + } catch (e: UnsupportedOperationException) { + // The filesystem itself doesn't support symlinks (e.g. FAT32). + false + } catch (e: FileSystemException) { + // Windows NTFS supports symlinks but requires an elevated/Developer Mode privilege to + // create them -- without it, creation fails with this (a permission error), not + // UnsupportedOperationException. + false + } + // Report as skipped, not silently passed, when this environment can't create symlinks. + Assume.assumeTrue("Symlinks are not supported/permitted on this filesystem", symlinkCreated) + + assertThat(resolveWithinDirectory(root, "evil/secret.txt")).isNull() + } +} diff --git a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt index a8c2acc349..b21ff2bf06 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt @@ -2,11 +2,14 @@ package com.itsaky.androidide.utils import com.google.common.truth.Truth.assertThat import org.junit.Assert.assertThrows +import org.junit.Assume import org.junit.Rule import org.junit.Test import org.junit.rules.TemporaryFolder import java.io.File import java.io.IOException +import java.nio.file.FileSystemException +import java.nio.file.Files import java.util.zip.ZipEntry import java.util.zip.ZipOutputStream @@ -58,4 +61,70 @@ class ZipUtilsTest { val escapedFile = File(destDir.parentFile, "evil.txt") assertThat(escapedFile.exists()).isFalse() } + + @Test + fun `unzipFile skips an entry that would extract over an existing symlink, without aborting the rest`() { + val destDir = tempFolder.newFolder("dest") + val realFile = File(destDir, "real.txt").apply { writeText("original") } + val linkPath = File(destDir, "link.txt").toPath() + val symlinkCreated = + try { + Files.createSymbolicLink(linkPath, realFile.toPath()) + true + } catch (e: UnsupportedOperationException) { + // The filesystem itself doesn't support symlinks (e.g. FAT32). + false + } catch (e: FileSystemException) { + // Windows NTFS supports symlinks but requires an elevated/Developer Mode privilege to + // create them -- without it, creation fails with this specific reason (a permission + // error), not UnsupportedOperationException. Any other reason is a real, unexpected + // failure and must not be silently swallowed. + if (e.reason?.contains("privilege", ignoreCase = true) != true) throw e + false + } + // Report as skipped, not silently passed, when this environment can't create symlinks. + Assume.assumeTrue("Symlinks are not supported/permitted on this filesystem", symlinkCreated) + + // The symlink's target is inside destDir, so the canonical-path containment check alone + // would pass -- this isolates the separate, explicit isSymbolicLink guard. A second, + // unrelated entry proves a skip doesn't abort the whole archive (e.g. a user's legitimately + // symlinked gradlew alongside a normal Gradle wrapper zip entry). + val zipFile = tempFolder.newFile("archive.zip") + ZipOutputStream(zipFile.outputStream()).use { zip -> + zip.putNextEntry(ZipEntry("link.txt")) + zip.write("payload".toByteArray()) + zip.closeEntry() + + zip.putNextEntry(ZipEntry("unrelated.txt")) + zip.write("unrelated content".toByteArray()) + zip.closeEntry() + } + + val extracted = ZipUtils.unzipFile(zipFile, destDir) + + assertThat(Files.isSymbolicLink(linkPath)).isTrue() + assertThat(realFile.readText()).isEqualTo("original") + assertThat(File(destDir, "unrelated.txt").readText()).isEqualTo("unrelated content") + assertThat(extracted.map { it.name }).containsExactly("unrelated.txt") + } + + @Test + fun `unzipFile allows a harmless double-dot inside a path segment`() { + val zipFile = tempFolder.newFile("archive.zip") + ZipOutputStream(zipFile.outputStream()).use { zip -> + zip.putNextEntry(ZipEntry("notes..txt")) + zip.write("note content".toByteArray()) + zip.closeEntry() + + zip.putNextEntry(ZipEntry("a..b/c.txt")) + zip.write("nested content".toByteArray()) + zip.closeEntry() + } + + val destDir = tempFolder.newFolder("dest") + ZipUtils.unzipFile(zipFile, destDir) + + assertThat(File(destDir, "notes..txt").readText()).isEqualTo("note content") + assertThat(File(destDir, "a..b/c.txt").readText()).isEqualTo("nested content") + } } From 6bdadef23fdfd749d0351feba6cd8117e22965a5 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 15:03:26 -0700 Subject: [PATCH 02/14] ADFA-5257: Drop the ancestor cache, and fail properly on an unusable entry name The cached fast path answered a later path under an already-verified directory without looking at it again, so anything that replaced that directory with a symlink in between would be followed. Measuring settled whether the guarantee was affordable: a real 1.8 GB asset installation on device takes 48.0 s with every resolve revalidating, against 51.4 s with the cache and 51.3 s with the hand-rolled cache it replaced. Extraction is I/O and inflate; the check is noise. The cache is gone and the numbers are in the comment. File(destDir, entry.name).toPath() threw InvalidPathException for a name the platform cannot represent -- an unchecked exception escaping unzipFile's declared IOException contract before the resolver ever saw the entry. It now arrives as the IOException the function promises, with a test. PathTraversalTest swallowed every FileSystemException into a skipped test, which could have quietly removed the symlink-escape assertion from CI. It now skips only the known Windows privilege restriction and rethrows anything else, matching ZipUtilsTest. Co-Authored-By: Claude Opus 5 --- .../itsaky/androidide/utils/PathTraversal.kt | 22 ++++++++----------- .../com/itsaky/androidide/utils/ZipUtils.kt | 12 +++++++++- .../androidide/utils/PathTraversalTest.kt | 7 ++++-- .../itsaky/androidide/utils/ZipUtilsTest.kt | 17 ++++++++++++++ 4 files changed, 42 insertions(+), 16 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index ee1b815830..d410a256cb 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -54,8 +54,8 @@ import java.nio.file.Path * deep-link reader is content to follow one. That is policy, and it stays visible at each call site * rather than being buried here. * - * Not thread-safe: [resolve] memoizes verified ancestors, which is what makes it usable per zip - * entry. Extraction is single-threaded; construct one per operation. + * Holds no state beyond the base directory, and verifies against the filesystem on every call -- + * see [resolve] for why it does not memoize what it has already proven. */ class ContainedPathResolver( baseDir: File, @@ -70,11 +70,6 @@ class ContainedPathResolver( null } - // Zip entries cluster by directory (dozens of files under one build-tools//), so the - // same ancestor is otherwise re-resolved for each of them. Only ever holds directories already - // proven contained, and nothing here can turn a verified real directory into a symlink mid-run. - private val verifiedAncestors = HashSet() - /** * The resolved file [relativePath] names inside the base directory, or null when it is invalid * or escapes -- including when it is not a representable path at all (a decoded NUL byte, say: @@ -109,15 +104,16 @@ class ContainedPathResolver( while (!Files.exists(ancestor, LinkOption.NOFOLLOW_LINKS)) { ancestor = ancestor.parent ?: return null } - if (ancestor in verifiedAncestors) { - return resolved.toFile() - } + // Re-resolved on every call, deliberately. Caching a directory once proven contained saves + // a toRealPath() per entry, but it answers later paths under that directory without + // looking -- so if anything replaces it with a symlink in between, the answer is stale and + // the caller writes through the link. The cache was measured against a real 1.8 GB asset + // installation on device and bought nothing: 48.0 s without it, 51.4 s with it, 51.3 s for + // the hand-rolled cache it replaced. Extraction is I/O and inflate; this is noise + // (ADFA-5257 review). if (!ancestor.toRealPath().startsWith(realBase)) { return null } - if (Files.isDirectory(ancestor)) { - verifiedAncestors.add(ancestor) - } resolved.toFile() } catch (_: InvalidPathException) { null diff --git a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt index e01ed151b7..95a927556a 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt @@ -20,6 +20,7 @@ package com.itsaky.androidide.utils import java.io.File import java.io.IOException import java.nio.file.Files +import java.nio.file.InvalidPathException import java.util.zip.ZipFile object ZipUtils { @@ -48,7 +49,16 @@ object ZipUtils { // entry is skipped and the symlink left alone, where asking the resolver first would // reject one pointing outside destDir and abort the whole archive. Reading a path's // link status cannot itself escape. - if (Files.isSymbolicLink(File(destDir, entry.name).toPath())) { + // toPath() throws InvalidPathException for a name the platform cannot represent (an + // embedded NUL, say) -- an unchecked exception that would escape unzipFile's declared + // IOException contract before the resolver ever saw the entry. + val target = + try { + File(destDir, entry.name).toPath() + } catch (e: InvalidPathException) { + throw IOException("Zip entry has an unusable path: ${entry.name}", e) + } + if (Files.isSymbolicLink(target)) { continue } diff --git a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt index d24ab8efc8..c6d2906e05 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt @@ -148,8 +148,11 @@ class PathTraversalTest { false } catch (e: FileSystemException) { // Windows NTFS supports symlinks but requires an elevated/Developer Mode privilege to - // create them -- without it, creation fails with this (a permission error), not - // UnsupportedOperationException. + // create them -- without it, creation fails with this specific reason (a permission + // error), not UnsupportedOperationException. Any other reason is a real, unexpected + // failure and must not be silently swallowed into a skipped test, which would take + // the symlink-escape assertion out of CI without anyone noticing. + if (e.reason?.contains("privilege", ignoreCase = true) != true) throw e false } // Report as skipped, not silently passed, when this environment can't create symlinks. diff --git a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt index b21ff2bf06..395f4989b9 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt @@ -127,4 +127,21 @@ class ZipUtilsTest { assertThat(File(destDir, "notes..txt").readText()).isEqualTo("note content") assertThat(File(destDir, "a..b/c.txt").readText()).isEqualTo("nested content") } + + // An entry name the platform cannot turn into a path is a broken archive, not a crash: it has to + // arrive as the IOException this function declares, rather than an unchecked InvalidPathException + // escaping from the link check. + @Test + fun `unzipFile reports an entry whose name is not a usable path`() { + val zip = tempFolder.newFile("bad-name.zip") + ZipOutputStream(zip.outputStream()).use { out -> + out.putNextEntry(ZipEntry("bad\u0000name.txt")) + out.write("x".toByteArray()) + out.closeEntry() + } + + val destDir = tempFolder.newFolder("out-bad-name") + val thrown = assertThrows(IOException::class.java) { ZipUtils.unzipFile(zip, destDir) } + assertThat(thrown).hasMessageThat().contains("bad") + } } From 8e6e33258d7cd7444c0a13ca805ba99dbe69620c Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 24 Aug 2026 19:07:14 -0700 Subject: [PATCH 03/14] ADFA-5257: Fail closed when the base directory cannot be resolved Review of #1736 found the containment check could quietly fall back to lexical-only matching -- weaker than the canonical-prefix check it replaced, and silent about it. Two ways in, both fixed by resolving the base per call instead of pinning it in the constructor: - The constructor caught the IOException from toRealPath() and nulled the field, disabling layer 3 for the resolver's whole life. An unresolvable base is now refused outright, with a warning. - Files.exists() is false both for "absent" and for "cannot be determined", so a base under a non-traversable parent read as absent and skipped layer 3. Confirmed-absent is now distinguished by catching NoSuchFileException from toRealPath() itself, which also drops a redundant stat. Pinning the base at construction was stale besides: the asset installer builds its resolver before the directory exists, so layer 3 never ran again even after extraction created the tree. A symlink planted into the base after construction now gets caught. Also in ZipUtils, containment is checked before the existing-symlink policy. In the old order an entry aiming outside the target could hit a symlink first and be skipped as a benign "leave the user's link alone" case, masking the zip-slip rejection; the skip is now logged. Both new tests were confirmed to fail against the unfixed code, for the reasons they are named for. Docs corrected where they overclaimed: the resolver is not yet the only containment check in the tree (ZipRecipeExecutor and PluginLoader remain -- ADFA-5266), it does not memoize, and unzipFile does not extract literally every entry. The deliberate narrowing over the old canonical-prefix check (a/../b.txt now fails) is documented and pinned by a test. --- .../assets/AssetsInstallationHelper.kt | 8 +-- .../assets/ExtractZipToDirMergeTest.kt | 6 +- .../itsaky/androidide/utils/PathTraversal.kt | 70 +++++++++++++------ .../com/itsaky/androidide/utils/ZipUtils.kt | 54 ++++++++------ .../androidide/utils/PathTraversalTest.kt | 64 +++++++++++++++++ 5 files changed, 152 insertions(+), 50 deletions(-) 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 38abc78446..cec5e0f8e5 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -256,10 +256,10 @@ object AssetsInstallationHelper { ) = extractZipToDir(Files.newInputStream(srcFile), destDir) /** - * Containment is [ContainedPathResolver]'s, shared with `ZipUtils.unzipFile` and the deep-link - * reader. It also carries the caching this loop used to do by hand: bootstrap archives cluster - * thousands of entries under a handful of directories, and the resolver memoizes an ancestor once - * it is proven contained. + * Containment is [ContainedPathResolver]'s, shared with `ZipUtils.unzipFile`. It does *not* + * memoize: the per-parent cache this loop used to keep was measured against a real 1.8 GB asset + * installation and bought nothing (48.0s without it, 51.4s with), so every path is re-verified + * against the filesystem rather than trusting an ancestor proven earlier. * * What stays local is the policy: this refuses to write through *any* existing symlink at an * entry's target, even one pointing inside destDir. An installer directory reused across runs is 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 d1a1ac2cd7..496d3a10ce 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt @@ -124,8 +124,8 @@ class ExtractZipToDirMergeTest { } // The lexical guard used to be a bare substring reject, so an entry legitimately named with - // consecutive dots aborted the whole installation. Sharing ContainedPathResolver with ZipUtils - // and the deep-link reader brought the per-segment rule here too. + // consecutive dots aborted the whole installation. The shared resolver rejects a ".." *segment* + // instead, which lets a name like this through. @Test fun `extracts an entry whose name merely contains a double dot`() { val destDir = Files.createTempDirectory("assets-dots") @@ -138,7 +138,7 @@ class ExtractZipToDirMergeTest { assertEquals("kept", destDir.resolve("lib/notes..txt").toFile().readText()) assertEquals("also kept", destDir.resolve("lib/a..b/c.txt").toFile().readText()) } finally { - destDir.toFile().deleteRecursively() + destDir.deleteRecursivelyWithoutFollowingLinks() } } diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index d410a256cb..9615dab487 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -17,6 +17,7 @@ package com.itsaky.androidide.utils +import org.slf4j.LoggerFactory import java.io.File import java.io.IOException import java.nio.file.Files @@ -25,14 +26,19 @@ import java.nio.file.LinkOption import java.nio.file.Path /** - * Decides whether a relative path is safely inside a base directory -- the one containment - * algorithm in the codebase, and the only place it should be implemented. + * Decides whether a relative path is safely inside a base directory. * * Built for attacker-controllable input: the `{filename}` segment of a deep-link URL, and the entry * names in a zip. It lives in `common` because it is plain `java.io`/`java.nio` with no Android - * dependency, so both the app and this module's own [ZipUtils] can call it. Three near-copies used - * to exist, each with a comment asking whoever fixed one to remember the other two; they had - * already drifted apart on the `..` rule by the time this replaced them. + * dependency, so both the app and this module's own [ZipUtils] can call it. It replaces the copies + * in [ZipUtils] and `AssetsInstallationHelper`, which had already drifted apart on the `..` rule. + * Two hand-rolled containment checks remain unmigrated -- `ZipRecipeExecutor` and `PluginLoader` + * (ADFA-5266) -- so this is not yet the only implementation in the tree. + * + * A `..` segment is rejected outright, which is stricter than the canonical-prefix check [ZipUtils] + * used to apply: an entry like `a/../b.txt` normalizes back inside the base and used to extract, and + * now fails the archive. That is a deliberate narrowing, matching what the asset installer already + * enforced, and it fails loudly rather than silently. * * Three layers: * 1. A lexical reject of an empty string, a `..` *segment*, or a leading `/` or `\`. Per segment, @@ -41,18 +47,19 @@ import java.nio.file.Path * 2. Resolve + normalize against the base and verify with [java.nio.file.Path.startsWith] (not * string prefix matching, which would wrongly accept `/project` as inside `/project-evil`). This * operates on Java's own resolved path, so it is not fooled by however `..` reached the string. - * 3. Resolve the nearest *existing* ancestor of that path to its real, on-disk path via + * 3. Resolve the base and the nearest *existing* ancestor of that path to its real, on-disk path via * [java.nio.file.Path.toRealPath] and re-verify containment -- layer 2 is purely lexical and * will not catch a symlink already present inside the base (a project cloned with git, an * installer directory reused across runs). Walking up to the nearest existing ancestor handles a - * path that does not exist yet. Skipped when the base itself does not exist: there is nothing on - * disk to symlink-escape through. + * path that does not exist yet. Skipped only when the base is *confirmed* absent -- there is then + * nothing on disk to symlink-escape through. A base that cannot be resolved is refused outright, + * never quietly downgraded to layer 2. * * What this deliberately does *not* decide is what to do about an existing symlink *at the target* - * whose destination is still inside the base. Callers disagree: unzipping leaves a user's own - * `gradlew` symlink alone, the asset installer refuses to write through any symlink at all, and the - * deep-link reader is content to follow one. That is policy, and it stays visible at each call site - * rather than being buried here. + * whose destination is still inside the base. The two callers disagree -- unzipping leaves a user's + * own `gradlew` symlink alone, the asset installer refuses to write through any symlink -- so that + * check stays visible at each call site rather than being buried here. Each caller applies it + * *after* asking this class, so the check only ever sees a path already proven contained. * * Holds no state beyond the base directory, and verifies against the filesystem on every call -- * see [resolve] for why it does not memoize what it has already proven. @@ -60,15 +67,9 @@ import java.nio.file.Path class ContainedPathResolver( baseDir: File, ) { - private val base: Path = baseDir.toPath().toAbsolutePath().normalize() + private val log = LoggerFactory.getLogger(ContainedPathResolver::class.java) - // Null when the base does not exist on disk, which makes layer 3 unnecessary. - private val realBase: Path? = - try { - if (Files.exists(base)) base.toRealPath() else null - } catch (_: IOException) { - null - } + private val base: Path = baseDir.toPath().toAbsolutePath().normalize() /** * The resolved file [relativePath] names inside the base directory, or null when it is invalid @@ -93,7 +94,30 @@ class ContainedPathResolver( return null } - val realBase = realBase ?: return resolved.toFile() + // Resolved per call, not once in a constructor. Two reasons, both of which bit this class: + // the asset installer builds its resolver *before* the directory exists, so a base pinned at + // construction stays null for the resolver's whole life and layer 3 never runs again even + // once extraction has created the tree; and notExists() is not !exists() -- both are false + // when the answer cannot be determined (a parent denying execute), and treating that as + // "absent, nothing to symlink through" is the same silent downgrade to lexical containment. + // Confirmed-absent skips layer 3; anything else must resolve or be refused. + val realBase = + try { + base.toRealPath() + // Fully qualified on purpose: Kotlin auto-imports kotlin.io.NoSuchFileException, which + // toRealPath() never throws, so catching that one would quietly disable this branch. + } catch (_: java.nio.file.NoSuchFileException) { + // Confirmed absent, so there is nothing on disk to symlink through and layer 3 has + // nothing to check. (A base that is itself a dangling symlink lands here too; a write + // under it fails at the write, and layer 2 still holds.) Distinguished from a failure + // this way rather than via a separate notExists() probe, which costs a second stat and + // answers "false" for both absent and undeterminable. + null + } catch (e: IOException) { + log.warn("Cannot resolve {} to a real path; refusing every path under it", base, e) + return null + } + realBase ?: return resolved.toFile() var ancestor = resolved // NOFOLLOW_LINKS: plain Files.exists() follows symlinks, so a *dangling* symlink (one @@ -126,6 +150,10 @@ class ContainedPathResolver( /** * [ContainedPathResolver.resolve] for a single path, where there is nothing to reuse a resolver for. * Prefer the class when validating many paths against one base -- a zip's entries, say. + * + * Nothing in `common` calls this yet; the caller is the deep-link handler in the app module (#1651), + * which validates one `{filename}` per request. Kept here rather than landing with that PR so both + * entry points ship as one reviewed unit. */ fun resolveWithinDirectory( baseDir: File, diff --git a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt index 95a927556a..742d48ef41 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt @@ -17,6 +17,7 @@ package com.itsaky.androidide.utils +import org.slf4j.LoggerFactory import java.io.File import java.io.IOException import java.nio.file.Files @@ -24,10 +25,18 @@ import java.nio.file.InvalidPathException import java.util.zip.ZipFile object ZipUtils { + private val log = LoggerFactory.getLogger(ZipUtils::class.java) + /** - * Extracts every entry of [zipFile] into [destDir], preserving directory structure, and - * returns the list of extracted files. Rejects entries that would extract outside [destDir] - * (zip-slip). + * Extracts [zipFile] into [destDir], preserving directory structure, and returns the list of + * files it wrote. + * + * Two kinds of entry do not appear in that list. An entry that would land outside [destDir] + * (zip-slip) fails the whole call with an [IOException] -- containment is checked before + * anything else, so a malicious entry cannot be quietly turned into a skip by the rule below. + * An entry whose target is an existing symlink is skipped and extraction continues: the target + * is already proven contained by then, and this keeps a user's own symlink (a `gradlew`, an SDK + * link) from being overwritten by an archive. */ @JvmStatic @Throws(IOException::class) @@ -44,31 +53,32 @@ object ZipUtils { while (entries.hasMoreElements()) { val entry = entries.nextElement() - // Policy before containment, deliberately: a user's own symlink inside their project - // -- gradlew, or gradle/wrapper pointed at a shared location -- is legitimate, so the - // entry is skipped and the symlink left alone, where asking the resolver first would - // reject one pointing outside destDir and abort the whole archive. Reading a path's - // link status cannot itself escape. - // toPath() throws InvalidPathException for a name the platform cannot represent (an - // embedded NUL, say) -- an unchecked exception that would escape unzipFile's declared - // IOException contract before the resolver ever saw the entry. - val target = + // Containment first, then policy. The order used to be reversed, which meant + // Files.isSymbolicLink ran on an unnormalized File(destDir, entry.name): for an entry + // like ../../etc/x the kernel resolved the .. segments, the stat landed on a path + // outside destDir, and if that happened to be a symlink the entry was skipped -- a + // zip-slip attempt discarded quietly, where the same entry naming a regular file + // correctly threw. Resolving first means the link check only ever sees a path already + // proven to be inside destDir (ADFA-5257). + val outFile = try { - File(destDir, entry.name).toPath() + contained.resolve(entry.name) } catch (e: InvalidPathException) { + // A name the platform cannot represent (an embedded NUL). Unchecked, and it + // would otherwise escape this function's declared IOException contract. throw IOException("Zip entry has an unusable path: ${entry.name}", e) - } - if (Files.isSymbolicLink(target)) { + } ?: throw IOException("Zip entry escapes the target directory: ${entry.name}") + + // Policy, not containment: a user's own symlink inside their own project -- gradlew, or + // gradle/wrapper pointed at a shared location -- is legitimate, so the entry is skipped + // and their symlink left alone rather than written through. Logged, because the caller + // is told the archive extracted and would otherwise have no way to know an entry did + // not. + if (Files.isSymbolicLink(outFile.toPath())) { + log.info("Leaving the existing symlink at {} alone; that zip entry was not extracted", outFile) continue } - // Containment is ContainedPathResolver's, shared with the asset installer: a canonical - // path prefix alone accepted a "..", and could not tell a symlinked ancestor from a - // real directory (ADFA-5257). - val outFile = - contained.resolve(entry.name) - ?: throw IOException("Zip entry escapes the target directory: ${entry.name}") - if (entry.isDirectory) { outFile.mkdirs() } else { diff --git a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt index c6d2906e05..6de37f7199 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt @@ -23,6 +23,7 @@ import org.junit.Rule import org.junit.Test import org.junit.rules.TemporaryFolder import java.io.File +import java.io.IOException import java.nio.file.FileSystemException import java.nio.file.Files @@ -116,6 +117,11 @@ class PathTraversalTest { @Test fun `multi-segment path resolves and normalizes redundant separators`() { + // Actually redundant: a doubled separator and a "." segment, which the old input had neither + // of -- so the normalization this test is named for went unpinned. + assertThat(resolveWithinDirectory(baseDir, "app//src/./main/Main.kt")) + .isEqualTo(File("/project/root/app/src/main/Main.kt")) + val resolved = resolveWithinDirectory(baseDir, "app/src/main/Main.kt") assertThat(resolved).isEqualTo(File("/project/root/app/src/main/Main.kt")) } @@ -160,4 +166,62 @@ class PathTraversalTest { assertThat(resolveWithinDirectory(root, "evil/secret.txt")).isNull() } + + // A containment check must fail closed. Resolving the base used to happen once in the constructor, + // catching the IOException and nulling the field, which permanently downgraded every later + // resolve() to lexical containment alone -- weaker than the canonical-prefix check this class + // replaced, and silent about it. + @Test + fun `a base that cannot be resolved rejects everything`() { + val root = tempFolder.newFolder("unresolvable-root") + val base = File(root, "base").apply { mkdirs() } + val resolver = ContainedPathResolver(base) + assertThat(resolver.resolve("child.txt")).isNotNull() + + // Make the base unresolvable by removing traverse permission on its parent, then check that + // this environment actually produced the failure -- as root, or on a filesystem that ignores + // the mode, it will not, and the test has nothing to assert. + root.setExecutable(false, false) + try { + val reallyUnresolvable = + try { + base.toPath().toRealPath() + false + } catch (_: IOException) { + true + } + Assume.assumeTrue("This environment still resolves a base under a non-traversable parent", reallyUnresolvable) + + // Both the resolver built while the base was fine and a fresh one: the check is per call. + assertThat(resolver.resolve("child.txt")).isNull() + assertThat(ContainedPathResolver(base).resolve("child.txt")).isNull() + } finally { + root.setExecutable(true, false) + } + } + + // Layer 3 is skipped only when the base is *confirmed* absent. Pinning the base at construction + // meant a resolver built before its directory existed -- which is exactly how the asset installer + // builds one -- skipped the symlink check forever, including for the tree extraction then created. + @Test + fun `a symlink planted after the resolver was built is still caught`() { + val root = tempFolder.newFolder("late-symlink") + val base = File(root, "base") + val resolver = ContainedPathResolver(base) + + val outside = File(root, "outside").apply { mkdirs() } + File(outside, "secret.txt").writeText("secret") + base.mkdirs() + Files.createSymbolicLink(File(base, "link").toPath(), outside.toPath()) + + assertThat(resolver.resolve("link/secret.txt")).isNull() + } + + // The narrowing this class deliberately makes over ZipUtils' old canonical-prefix check: an entry + // that normalizes back inside the base is still refused, because a ".." segment is refused before + // anything is resolved. Stated here so the behaviour change is pinned rather than incidental. + @Test + fun `a dot-dot segment is refused even when it normalizes back inside`() { + assertThat(resolveWithinDirectory(baseDir, "a/../b.txt")).isNull() + } } From f364ccca6677cb7fd4d793506e02ab9bd99cb71b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 15:54:16 +0000 Subject: [PATCH 04/14] ADFA-5257: Address review: report skips, tolerate dangling links, reject "." Review fixes on the shared containment PR (#1736): - unzipFile now returns an UnzipResult (extracted + skipped) so callers can tell when an entry was left unextracted over an existing symlink. doInstallWrapper verifies the wrapper files actually exist under the project dir instead of trusting a non-empty extraction list. - A dangling symlink inside destDir no longer aborts the archive as an escape: a lexically-contained symlink at the entry's path takes the same skip branch as a live one -- nothing is written at or through it. - Drop the unreachable catch(InvalidPathException): the resolver catches it internally and returns null, so an unusable entry name now surfaces through the one IOException, and the NUL-name test asserts a message substring unique to the branch that fires. - ContainedPathResolver rejects "." and "./" (they normalize to the base itself, which is not a path inside it), and warns instead of silently swallowing an unexpected IOException from ancestor.toRealPath(); a NoSuchFileException there is the dangling-link rejection working and stays quiet. - Reword the ZipUtils ordering comment as a present-tense invariant (the claimed history was false against stage) and the per-call base resolution comments to their true grounds (a caller may construct before the base exists; an existing base can gain a symlink later). - Extract the guarded symlink-creation test helper into SymlinkTestSupport.kt and use it in all three call sites, including the previously unguarded one; add regression tests for the dangling in-base symlink skip and for "." / "./". --- .../services/builder/GradleBuildService.kt | 14 ++- .../tasks/callables/UnzipCallable.java | 2 +- .../itsaky/androidide/utils/PathTraversal.kt | 34 +++++-- .../com/itsaky/androidide/utils/ZipUtils.kt | 90 +++++++++++++------ .../androidide/utils/PathTraversalTest.kt | 37 +++----- .../androidide/utils/SymlinkTestSupport.kt | 49 ++++++++++ .../itsaky/androidide/utils/ZipUtilsTest.kt | 66 ++++++++------ 7 files changed, 205 insertions(+), 87 deletions(-) create mode 100644 common/src/test/java/com/itsaky/androidide/utils/SymlinkTestSupport.kt diff --git a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt index ca74c79bb6..1182029806 100644 --- a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt +++ b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt @@ -550,10 +550,20 @@ class GradleBuildService : } try { val projectDir = ProjectManagerImpl.getInstance().projectDir - val files = ZipUtils.unzipFile(extracted, projectDir) - if (files.isNotEmpty()) { + val result = ZipUtils.unzipFile(extracted, projectDir) + if (result.skipped.isNotEmpty()) { + log.warn("Gradle wrapper entries not extracted (existing symlinks left alone): {}", result.skipped) + } + + // Success means the wrapper is actually usable, not merely that unzipFile returned: an + // entry skipped over a user's own symlink is fine as long as the files it needs exist. + val missing = + listOf("gradlew", "gradle/wrapper/gradle-wrapper.jar", "gradle/wrapper/gradle-wrapper.properties") + .filter { !File(projectDir, it).exists() } + if (missing.isEmpty()) { return GradleWrapperCheckResult(true) } + log.error("Gradle wrapper installation is incomplete; missing: {}", missing) } catch (e: IOException) { log.error("An error occurred while extracting Gradle wrapper", e) } diff --git a/app/src/main/java/com/itsaky/androidide/tasks/callables/UnzipCallable.java b/app/src/main/java/com/itsaky/androidide/tasks/callables/UnzipCallable.java index 06bcdb840a..2813145216 100755 --- a/app/src/main/java/com/itsaky/androidide/tasks/callables/UnzipCallable.java +++ b/app/src/main/java/com/itsaky/androidide/tasks/callables/UnzipCallable.java @@ -37,6 +37,6 @@ public UnzipCallable(File src, File dest) { @Override public List call() throws Exception { - return ZipUtils.unzipFile(src, dest); + return ZipUtils.unzipFile(src, dest).getExtracted(); } } diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index 9615dab487..799e17b9a5 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -93,11 +93,17 @@ class ContainedPathResolver( if (!resolved.startsWith(base)) { return null } + if (resolved == base) { + // "." and "./" normalize to the base itself -- not a path *inside* it, and a caller + // treats a non-null result as a usable target. + return null + } - // Resolved per call, not once in a constructor. Two reasons, both of which bit this class: - // the asset installer builds its resolver *before* the directory exists, so a base pinned at - // construction stays null for the resolver's whole life and layer 3 never runs again even - // once extraction has created the tree; and notExists() is not !exists() -- both are false + // Resolved per call, not once in a constructor. Two reasons: nothing stops a caller from + // constructing the resolver before the base exists, so a base pinned at construction could + // stay null for the resolver's whole life and layer 3 would never run even once the tree is + // created -- and an existing base can gain a symlink later, so a resolution proven once can + // go stale. Also, notExists() is not !exists() -- both are false // when the answer cannot be determined (a parent denying execute), and treating that as // "absent, nothing to symlink through" is the same silent downgrade to lexical containment. // Confirmed-absent skips layer 3; anything else must resolve or be refused. @@ -135,7 +141,25 @@ class ContainedPathResolver( // installation on device and bought nothing: 48.0 s without it, 51.4 s with it, 51.3 s for // the hand-rolled cache it replaced. Extraction is I/O and inflate; this is noise // (ADFA-5257 review). - if (!ancestor.toRealPath().startsWith(realBase)) { + val realAncestor = + try { + ancestor.toRealPath() + // Fully qualified for the same reason as the base branch above. + } catch (_: java.nio.file.NoSuchFileException) { + // Expected for a dangling symlink: the NOFOLLOW walk stopped at a link that exists, + // but its target does not, so there is no real path to prove contained. Refused + // without the warning below -- this is the link check working, not a failure. + return null + } catch (e: IOException) { + log.warn( + "Cannot resolve {} (nearest existing ancestor of {}) to a real path; refusing the path", + ancestor, + relativePath, + e, + ) + return null + } + if (!realAncestor.startsWith(realBase)) { return null } resolved.toFile() diff --git a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt index 742d48ef41..a87900983e 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt @@ -28,54 +28,67 @@ object ZipUtils { private val log = LoggerFactory.getLogger(ZipUtils::class.java) /** - * Extracts [zipFile] into [destDir], preserving directory structure, and returns the list of - * files it wrote. + * What [unzipFile] did with each entry: [extracted] holds the files it wrote, [skipped] the + * names of entries it did not extract because a symlink already sits at their target. A caller + * that needs specific entries on disk must check [skipped] (or the files themselves) rather + * than trusting a normal return. + */ + data class UnzipResult( + val extracted: List, + val skipped: List, + ) + + /** + * Extracts [zipFile] into [destDir], preserving directory structure, and returns an + * [UnzipResult] reporting the files it wrote and the entries it skipped. * - * Two kinds of entry do not appear in that list. An entry that would land outside [destDir] - * (zip-slip) fails the whole call with an [IOException] -- containment is checked before - * anything else, so a malicious entry cannot be quietly turned into a skip by the rule below. - * An entry whose target is an existing symlink is skipped and extraction continues: the target - * is already proven contained by then, and this keeps a user's own symlink (a `gradlew`, an SDK - * link) from being overwritten by an archive. + * An entry whose target is an existing symlink -- live, dangling, or even pointing outside + * [destDir] -- is skipped and extraction continues: nothing is written at or through the link, + * which keeps a user's own symlink (a `gradlew`, an SDK link) from being overwritten by an + * archive. The skip applies only to a link lexically inside [destDir]; any other entry that + * would land outside [destDir] (zip-slip), or whose name is not a representable path, fails + * the whole call with an [IOException]. */ @JvmStatic @Throws(IOException::class) fun unzipFile( zipFile: File, destDir: File, - ): List { + ): UnzipResult { destDir.mkdirs() val contained = ContainedPathResolver(destDir) - val result = mutableListOf() + val extracted = mutableListOf() + val skipped = mutableListOf() ZipFile(zipFile).use { zip -> val entries = zip.entries() while (entries.hasMoreElements()) { val entry = entries.nextElement() - // Containment first, then policy. The order used to be reversed, which meant - // Files.isSymbolicLink ran on an unnormalized File(destDir, entry.name): for an entry - // like ../../etc/x the kernel resolved the .. segments, the stat landed on a path - // outside destDir, and if that happened to be a symlink the entry was skipped -- a - // zip-slip attempt discarded quietly, where the same entry naming a regular file - // correctly threw. Resolving first means the link check only ever sees a path already - // proven to be inside destDir (ADFA-5257). + // Containment first, then link policy: the link check must only ever see a path + // already proven contained. Checking File(destDir, entry.name) before containment + // would stat outside destDir for a ../ entry and could quietly skip a zip-slip + // attempt (ADFA-5257). val outFile = - try { - contained.resolve(entry.name) - } catch (e: InvalidPathException) { - // A name the platform cannot represent (an embedded NUL). Unchecked, and it - // would otherwise escape this function's declared IOException contract. - throw IOException("Zip entry has an unusable path: ${entry.name}", e) - } ?: throw IOException("Zip entry escapes the target directory: ${entry.name}") + contained.resolve(entry.name) + ?: if (isContainedSymlink(destDir, entry.name)) { + // The resolver refuses a symlink it cannot verify -- dangling, or pointing + // outside -- but a link that sits lexically inside destDir is the user's + // own, and skipping writes nothing at or through it. Same policy as below. + log.info("Leaving the existing symlink at {}/{} alone; that zip entry was not extracted", destDir, entry.name) + skipped.add(entry.name) + continue + } else { + throw IOException("Zip entry does not resolve to a safe path inside the target directory: ${entry.name}") + } // Policy, not containment: a user's own symlink inside their own project -- gradlew, or // gradle/wrapper pointed at a shared location -- is legitimate, so the entry is skipped - // and their symlink left alone rather than written through. Logged, because the caller - // is told the archive extracted and would otherwise have no way to know an entry did - // not. + // and their symlink left alone rather than written through. Reported via the result, + // because the caller is otherwise told the archive extracted. if (Files.isSymbolicLink(outFile.toPath())) { log.info("Leaving the existing symlink at {} alone; that zip entry was not extracted", outFile) + skipped.add(entry.name) continue } @@ -88,10 +101,29 @@ object ZipUtils { } } - result.add(outFile) + extracted.add(outFile) } } - return result + return UnzipResult(extracted, skipped) + } + + /** + * Whether a symlink exists at [entryName]'s lexically-normalized path inside [destDir]. Purely + * lexical containment before the one NOFOLLOW stat, so nothing outside [destDir] is ever + * touched -- a `../` entry fails `startsWith` and is never stat'd. + */ + private fun isContainedSymlink( + destDir: File, + entryName: String, + ): Boolean { + val base = destDir.toPath().toAbsolutePath().normalize() + val candidate = + try { + base.resolve(entryName).normalize() + } catch (_: InvalidPathException) { + return false + } + return candidate != base && candidate.startsWith(base) && Files.isSymbolicLink(candidate) } } diff --git a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt index 6de37f7199..b17679050a 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt @@ -24,8 +24,6 @@ import org.junit.Test import org.junit.rules.TemporaryFolder import java.io.File import java.io.IOException -import java.nio.file.FileSystemException -import java.nio.file.Files class PathTraversalTest { private val baseDir = File("/project/root") @@ -56,6 +54,14 @@ class PathTraversalTest { assertThat(resolveWithinDirectory(baseDir, "")).isNull() } + @Test + fun `a path that normalizes to baseDir itself is rejected`() { + // "." and "./" survive the lexical layer but normalize to the base -- not a path *inside* + // it, and a caller treats a non-null result as a usable target. + assertThat(resolveWithinDirectory(baseDir, ".")).isNull() + assertThat(resolveWithinDirectory(baseDir, "./")).isNull() + } + @Test fun `dot-dot buried in the middle of a path is rejected`() { // The shape produced once android.net.Uri decodes a single raw segment containing an @@ -145,24 +151,7 @@ class PathTraversalTest { val outside = tempFolder.newFolder("outside") File(outside, "secret.txt").writeText("secret") - val symlinkCreated = - try { - Files.createSymbolicLink(File(root, "evil").toPath(), outside.toPath()) - true - } catch (e: UnsupportedOperationException) { - // The filesystem itself doesn't support symlinks (e.g. FAT32). - false - } catch (e: FileSystemException) { - // Windows NTFS supports symlinks but requires an elevated/Developer Mode privilege to - // create them -- without it, creation fails with this specific reason (a permission - // error), not UnsupportedOperationException. Any other reason is a real, unexpected - // failure and must not be silently swallowed into a skipped test, which would take - // the symlink-escape assertion out of CI without anyone noticing. - if (e.reason?.contains("privilege", ignoreCase = true) != true) throw e - false - } - // Report as skipped, not silently passed, when this environment can't create symlinks. - Assume.assumeTrue("Symlinks are not supported/permitted on this filesystem", symlinkCreated) + createSymlinkOrSkipTest(File(root, "evil").toPath(), outside.toPath()) assertThat(resolveWithinDirectory(root, "evil/secret.txt")).isNull() } @@ -200,9 +189,9 @@ class PathTraversalTest { } } - // Layer 3 is skipped only when the base is *confirmed* absent. Pinning the base at construction - // meant a resolver built before its directory existed -- which is exactly how the asset installer - // builds one -- skipped the symlink check forever, including for the tree extraction then created. + // Layer 3 is skipped only when the base is *confirmed* absent. Nothing stops a caller from + // building a resolver before its directory exists, so pinning the base at construction would + // skip the symlink check forever -- including for a tree created right after construction. @Test fun `a symlink planted after the resolver was built is still caught`() { val root = tempFolder.newFolder("late-symlink") @@ -212,7 +201,7 @@ class PathTraversalTest { val outside = File(root, "outside").apply { mkdirs() } File(outside, "secret.txt").writeText("secret") base.mkdirs() - Files.createSymbolicLink(File(base, "link").toPath(), outside.toPath()) + createSymlinkOrSkipTest(File(base, "link").toPath(), outside.toPath()) assertThat(resolver.resolve("link/secret.txt")).isNull() } diff --git a/common/src/test/java/com/itsaky/androidide/utils/SymlinkTestSupport.kt b/common/src/test/java/com/itsaky/androidide/utils/SymlinkTestSupport.kt new file mode 100644 index 0000000000..98190f1933 --- /dev/null +++ b/common/src/test/java/com/itsaky/androidide/utils/SymlinkTestSupport.kt @@ -0,0 +1,49 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import org.junit.Assume +import java.nio.file.FileSystemException +import java.nio.file.Files +import java.nio.file.Path + +/** + * Creates a symlink at [link] pointing to [target], or skips the calling test (via [Assume]) when + * this environment cannot create one: a filesystem without symlink support (FAT32 throws + * [UnsupportedOperationException]), or Windows NTFS without the elevated/Developer Mode privilege + * (a [FileSystemException] whose reason names the missing privilege). Any other + * [FileSystemException] is a real failure and is rethrown -- silently swallowing it into a skip + * would take a symlink assertion out of CI without anyone noticing. + */ +fun createSymlinkOrSkipTest( + link: Path, + target: Path, +) { + val created = + try { + Files.createSymbolicLink(link, target) + true + } catch (_: UnsupportedOperationException) { + false + } catch (e: FileSystemException) { + if (e.reason?.contains("privilege", ignoreCase = true) != true) throw e + false + } + // Report as skipped, not silently passed. + Assume.assumeTrue("Symlinks are not supported/permitted on this filesystem", created) +} diff --git a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt index 395f4989b9..7f342d0ec4 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt @@ -2,13 +2,11 @@ package com.itsaky.androidide.utils import com.google.common.truth.Truth.assertThat import org.junit.Assert.assertThrows -import org.junit.Assume import org.junit.Rule import org.junit.Test import org.junit.rules.TemporaryFolder import java.io.File import java.io.IOException -import java.nio.file.FileSystemException import java.nio.file.Files import java.util.zip.ZipEntry import java.util.zip.ZipOutputStream @@ -39,10 +37,11 @@ class ZipUtilsTest { } val destDir = tempFolder.newFolder("dest") - ZipUtils.unzipFile(zipFile, destDir) + val result = ZipUtils.unzipFile(zipFile, destDir) assertThat(File(destDir, "dir/nested.txt").readText()).isEqualTo("nested content") assertThat(File(destDir, "root.txt").readText()).isEqualTo("root content") + assertThat(result.skipped).isEmpty() } @Test @@ -67,23 +66,7 @@ class ZipUtilsTest { val destDir = tempFolder.newFolder("dest") val realFile = File(destDir, "real.txt").apply { writeText("original") } val linkPath = File(destDir, "link.txt").toPath() - val symlinkCreated = - try { - Files.createSymbolicLink(linkPath, realFile.toPath()) - true - } catch (e: UnsupportedOperationException) { - // The filesystem itself doesn't support symlinks (e.g. FAT32). - false - } catch (e: FileSystemException) { - // Windows NTFS supports symlinks but requires an elevated/Developer Mode privilege to - // create them -- without it, creation fails with this specific reason (a permission - // error), not UnsupportedOperationException. Any other reason is a real, unexpected - // failure and must not be silently swallowed. - if (e.reason?.contains("privilege", ignoreCase = true) != true) throw e - false - } - // Report as skipped, not silently passed, when this environment can't create symlinks. - Assume.assumeTrue("Symlinks are not supported/permitted on this filesystem", symlinkCreated) + createSymlinkOrSkipTest(linkPath, realFile.toPath()) // The symlink's target is inside destDir, so the canonical-path containment check alone // would pass -- this isolates the separate, explicit isSymbolicLink guard. A second, @@ -100,12 +83,43 @@ class ZipUtilsTest { zip.closeEntry() } - val extracted = ZipUtils.unzipFile(zipFile, destDir) + val result = ZipUtils.unzipFile(zipFile, destDir) assertThat(Files.isSymbolicLink(linkPath)).isTrue() assertThat(realFile.readText()).isEqualTo("original") assertThat(File(destDir, "unrelated.txt").readText()).isEqualTo("unrelated content") - assertThat(extracted.map { it.name }).containsExactly("unrelated.txt") + assertThat(result.extracted.map { it.name }).containsExactly("unrelated.txt") + assertThat(result.skipped).containsExactly("link.txt") + } + + // Regression test: a *dangling* symlink at an entry's target used to abort the whole archive -- + // the resolver refuses a link it cannot prove contained, and the escape exception fired. The + // link is lexically inside destDir and nothing is written at or through it, so this is the same + // leave-the-user's-symlink-alone skip as above, reported the same way. + @Test + fun `unzipFile skips an entry whose target is a dangling symlink inside the destination`() { + val destDir = tempFolder.newFolder("dest") + val linkPath = File(destDir, "link.txt").toPath() + createSymlinkOrSkipTest(linkPath, File(destDir, "missing-target.txt").toPath()) + + val zipFile = tempFolder.newFile("archive.zip") + ZipOutputStream(zipFile.outputStream()).use { zip -> + zip.putNextEntry(ZipEntry("link.txt")) + zip.write("payload".toByteArray()) + zip.closeEntry() + + zip.putNextEntry(ZipEntry("unrelated.txt")) + zip.write("unrelated content".toByteArray()) + zip.closeEntry() + } + + val result = ZipUtils.unzipFile(zipFile, destDir) + + assertThat(Files.isSymbolicLink(linkPath)).isTrue() + assertThat(File(destDir, "missing-target.txt").exists()).isFalse() + assertThat(File(destDir, "unrelated.txt").readText()).isEqualTo("unrelated content") + assertThat(result.extracted.map { it.name }).containsExactly("unrelated.txt") + assertThat(result.skipped).containsExactly("link.txt") } @Test @@ -128,9 +142,9 @@ class ZipUtilsTest { assertThat(File(destDir, "a..b/c.txt").readText()).isEqualTo("nested content") } - // An entry name the platform cannot turn into a path is a broken archive, not a crash: it has to - // arrive as the IOException this function declares, rather than an unchecked InvalidPathException - // escaping from the link check. + // An entry name the platform cannot turn into a path is a broken archive, not a crash: the + // resolver reports it as unresolvable (null), and unzipFile turns that into the one IOException + // it declares, rather than an unchecked InvalidPathException escaping. @Test fun `unzipFile reports an entry whose name is not a usable path`() { val zip = tempFolder.newFile("bad-name.zip") @@ -142,6 +156,6 @@ class ZipUtilsTest { val destDir = tempFolder.newFolder("out-bad-name") val thrown = assertThrows(IOException::class.java) { ZipUtils.unzipFile(zip, destDir) } - assertThat(thrown).hasMessageThat().contains("bad") + assertThat(thrown).hasMessageThat().contains("does not resolve to a safe path") } } From d40c55075118cf30db5a80d513c5bec0a251e802 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 16:08:21 +0000 Subject: [PATCH 05/14] ADFA-5257: Reject traversal syntax before the symlink-skip fallback Review of #1736 found a gap between the resolver and unzipFile's symlink-skip fallback: the resolver rejects a ".." segment lexically, but the fallback normalized the entry name before its symlink check, so an entry named a/../link.txt -- with an existing symlink at destDir/link.txt -- was silently skipped as "the user's own link" instead of failing the archive. The narrowing this PR documents ("a ../ entry fails the archive") thus had one path around it whenever a symlink happened to sit at the normalized target. The lexical reject is now extracted from resolve() into ContainedPathResolver.isLexicallyRejected and applied by isContainedSymlink before it looks at the filesystem: an entry that fails on syntax is a bad archive however the disk looks, never fallback material. One shared predicate rather than a duplicate, so the two cannot drift. The new test was confirmed to fail against the unfixed code: the entry was skipped, no IOException. --- .../itsaky/androidide/utils/PathTraversal.kt | 25 +++++++++++++------ .../com/itsaky/androidide/utils/ZipUtils.kt | 19 +++++++++----- .../itsaky/androidide/utils/ZipUtilsTest.kt | 23 +++++++++++++++++ 3 files changed, 54 insertions(+), 13 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index 799e17b9a5..12809ef9ca 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -78,13 +78,7 @@ class ContainedPathResolver( * literal NUL, which [java.nio.file.Path] rejects rather than silently ignoring). */ fun resolve(relativePath: String): File? { - // Split on both separators: '\' is not a path separator on Android, but a caller handing - // over a Windows-style path should not have it treated as one long filename. - if (relativePath.isEmpty() || - relativePath.startsWith("/") || - relativePath.startsWith("\\") || - relativePath.split('/', '\\').any { it == ".." } - ) { + if (isLexicallyRejected(relativePath)) { return null } @@ -169,6 +163,23 @@ class ContainedPathResolver( null } } + + companion object { + /** + * Layer 1 alone: whether [relativePath] is rejected before any filesystem look -- empty, + * absolute (leading `/` or `\`), or containing a literal `..` segment. Exposed so a caller + * with a more lenient fallback for paths [resolve] refused ([ZipUtils]' skip of an existing + * symlink) can apply the same reject first: an entry that fails here is a bad archive + * however the filesystem looks, never fallback material. + */ + internal fun isLexicallyRejected(relativePath: String): Boolean = + // Split on both separators: '\' is not a path separator on Android, but a caller handing + // over a Windows-style path should not have it treated as one long filename. + relativePath.isEmpty() || + relativePath.startsWith("/") || + relativePath.startsWith("\\") || + relativePath.split('/', '\\').any { it == ".." } + } } /** diff --git a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt index a87900983e..8a076b3593 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt @@ -45,9 +45,10 @@ object ZipUtils { * An entry whose target is an existing symlink -- live, dangling, or even pointing outside * [destDir] -- is skipped and extraction continues: nothing is written at or through the link, * which keeps a user's own symlink (a `gradlew`, an SDK link) from being overwritten by an - * archive. The skip applies only to a link lexically inside [destDir]; any other entry that - * would land outside [destDir] (zip-slip), or whose name is not a representable path, fails - * the whole call with an [IOException]. + * archive. The skip applies only to a link lexically inside [destDir] whose entry name carries + * no traversal syntax; an entry that would land outside [destDir] (zip-slip), names its target + * through a `..` segment, or is not a representable path fails the whole call with an + * [IOException]. */ @JvmStatic @Throws(IOException::class) @@ -109,14 +110,20 @@ object ZipUtils { } /** - * Whether a symlink exists at [entryName]'s lexically-normalized path inside [destDir]. Purely - * lexical containment before the one NOFOLLOW stat, so nothing outside [destDir] is ever - * touched -- a `../` entry fails `startsWith` and is never stat'd. + * Whether a symlink exists at [entryName]'s lexically-normalized path inside [destDir]. Applies + * the resolver's own lexical reject first, so an entry the resolver refused for its *syntax* (a + * `..` segment, an absolute path) stays a bad archive even when a symlink happens to sit at its + * normalized target -- `a/../link.txt` must fail, not ride the skip meant for `link.txt`. Only + * then the one NOFOLLOW stat, on a path proven inside [destDir], so nothing outside is ever + * touched. */ private fun isContainedSymlink( destDir: File, entryName: String, ): Boolean { + if (ContainedPathResolver.isLexicallyRejected(entryName)) { + return false + } val base = destDir.toPath().toAbsolutePath().normalize() val candidate = try { diff --git a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt index 7f342d0ec4..cef96409e4 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt @@ -142,6 +142,29 @@ class ZipUtilsTest { assertThat(File(destDir, "a..b/c.txt").readText()).isEqualTo("nested content") } + // Regression: the symlink-skip fallback must not resurrect an entry the resolver rejected for + // its syntax. "a/../link.txt" normalizes to an existing symlink inside destDir, so without the + // lexical reject in the fallback the entry was silently skipped instead of failing the archive. + @Test + fun `unzipFile rejects a dot-dot entry even when a symlink sits at its normalized target`() { + val destDir = tempFolder.newFolder("dest") + val realFile = File(destDir, "real.txt").apply { writeText("original") } + val linkPath = File(destDir, "link.txt").toPath() + createSymlinkOrSkipTest(linkPath, realFile.toPath()) + + val zipFile = tempFolder.newFile("evil.zip") + ZipOutputStream(zipFile.outputStream()).use { zip -> + zip.putNextEntry(ZipEntry("a/../link.txt")) + zip.write("payload".toByteArray()) + zip.closeEntry() + } + + val thrown = assertThrows(IOException::class.java) { ZipUtils.unzipFile(zipFile, destDir) } + assertThat(thrown).hasMessageThat().contains("does not resolve to a safe path") + assertThat(Files.isSymbolicLink(linkPath)).isTrue() + assertThat(realFile.readText()).isEqualTo("original") + } + // An entry name the platform cannot turn into a path is a broken archive, not a crash: the // resolver reports it as unresolvable (null), and unzipFile turns that into the one IOException // it declares, rather than an unchecked InvalidPathException escaping. From e544ebc6eb1c23a2c6fd66d8a7158dc7a7abb291 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 16:22:08 +0000 Subject: [PATCH 06/14] ADFA-5257: Reject symlink-ancestor escapes in the symlink-skip fallback The fallback stat'ed the entry's normalized path, which follows an ancestor symlink: with dest/a -> /outside and entry "a/link.txt", it stat'ed /outside/link.txt, saw a symlink there, and skipped the entry -- silently tolerating an escaping archive instead of failing it. Now every ancestor between destDir and the candidate must itself be a non-link, so only a symlink whose whole path is real directories inside destDir qualifies for the skip; anything else fails the archive with the containment IOException. The dangling-symlink and existing-symlink skip behaviors are unchanged. Adds a regression test where destDir/a links to an outside directory whose link.txt is itself a symlink; the entry must throw, not skip. --- .../com/itsaky/androidide/utils/ZipUtils.kt | 21 +++++++++++++--- .../itsaky/androidide/utils/ZipUtilsTest.kt | 25 +++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt index 8a076b3593..5960e3876f 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt @@ -113,9 +113,12 @@ object ZipUtils { * Whether a symlink exists at [entryName]'s lexically-normalized path inside [destDir]. Applies * the resolver's own lexical reject first, so an entry the resolver refused for its *syntax* (a * `..` segment, an absolute path) stays a bad archive even when a symlink happens to sit at its - * normalized target -- `a/../link.txt` must fail, not ride the skip meant for `link.txt`. Only - * then the one NOFOLLOW stat, on a path proven inside [destDir], so nothing outside is ever - * touched. + * normalized target -- `a/../link.txt` must fail, not ride the skip meant for `link.txt`. Then + * every ancestor between [destDir] and the candidate must be a non-link: stat'ing the candidate + * *follows* an ancestor symlink, so with `a -> /outside`, `a/link.txt` would stat + * `/outside/link.txt` and a link found there would ride the skip -- an escaping archive + * tolerated instead of rejected. Only a link whose every ancestor is a real directory inside + * [destDir] qualifies, and the stats stay on paths proven lexically inside [destDir]. */ private fun isContainedSymlink( destDir: File, @@ -131,6 +134,16 @@ object ZipUtils { } catch (_: InvalidPathException) { return false } - return candidate != base && candidate.startsWith(base) && Files.isSymbolicLink(candidate) + if (candidate == base || !candidate.startsWith(base)) { + return false + } + var ancestor = candidate.parent + while (ancestor != null && ancestor != base) { + if (Files.isSymbolicLink(ancestor)) { + return false + } + ancestor = ancestor.parent + } + return Files.isSymbolicLink(candidate) } } diff --git a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt index cef96409e4..1aa359e0dc 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt @@ -165,6 +165,31 @@ class ZipUtilsTest { assertThat(realFile.readText()).isEqualTo("original") } + // Regression: the symlink-skip fallback used to stat the entry's *normalized* path, which + // follows an ancestor symlink -- with dest/a -> outside, entry "a/link.txt" stat'ed + // outside/link.txt, found a symlink there, and the escaping archive was silently skipped + // instead of rejected. The skip is only for a link whose every ancestor is a real directory + // inside destDir. + @Test + fun `unzipFile rejects an entry that reaches a symlink through a symlinked ancestor`() { + val destDir = tempFolder.newFolder("dest") + val outsideDir = tempFolder.newFolder("outside") + val outsideTarget = File(outsideDir, "target.txt").apply { writeText("outside content") } + createSymlinkOrSkipTest(File(outsideDir, "link.txt").toPath(), outsideTarget.toPath()) + createSymlinkOrSkipTest(File(destDir, "a").toPath(), outsideDir.toPath()) + + val zipFile = tempFolder.newFile("evil.zip") + ZipOutputStream(zipFile.outputStream()).use { zip -> + zip.putNextEntry(ZipEntry("a/link.txt")) + zip.write("payload".toByteArray()) + zip.closeEntry() + } + + val thrown = assertThrows(IOException::class.java) { ZipUtils.unzipFile(zipFile, destDir) } + assertThat(thrown).hasMessageThat().contains("does not resolve to a safe path") + assertThat(outsideTarget.readText()).isEqualTo("outside content") + } + // An entry name the platform cannot turn into a path is a broken archive, not a crash: the // resolver reports it as unresolvable (null), and unzipFile turns that into the one IOException // it declares, rather than an unchecked InvalidPathException escaping. From fc94c9bb8badfc3790aa906112dfda7ff269c790 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 26 Aug 2026 11:56:40 -0700 Subject: [PATCH 07/14] ADFA-5257: Refuse to follow a symlink in the write itself, not just before it The symlink policy is a stat, and the write is a separate open, so a link appearing between them is followed: FileOutputStream resolves links, and Kotlin's File.outputStream() is a thin inline wrapper over it. Both write boundaries now pass LinkOption.NOFOLLOW_LINKS to Files.newOutputStream, which puts O_NOFOLLOW in the open(2) call, so there is no window between deciding and doing. This closes the final component only. A symlink substituted for one of the parent directories is still followed -- by mkdirs() and by the open -- because resolving a path relative to an already-open directory needs openat(2), which java.nio does not expose. Narrowing that further means JNI or a different extraction strategy, so it is recorded in both files rather than implied away. Worth stating the exposure while it is fresh: for the asset installer destDir is app-private storage, which another app cannot write to, so the race needs code execution in this process or root. For project archives extracted into user-visible storage the window is real. ZipUtilsTest covers the enforcement directly -- writeNoFollow is internal for that reason, since the policy check above it means a race is otherwise the only way to reach the open, and a race is not something a test can stage reliably. Without NOFOLLOW_LINKS the same test writes "payload" through the link and fails. 84 common tests and 294 app tests pass. Found in review of PR #1736. --- .../assets/AssetsInstallationHelper.kt | 19 ++++++++-- .../com/itsaky/androidide/utils/ZipUtils.kt | 36 +++++++++++++++++-- .../itsaky/androidide/utils/ZipUtilsTest.kt | 23 ++++++++++++ 3 files changed, 72 insertions(+), 6 deletions(-) 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 cec5e0f8e5..40b5dd8b7c 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -30,7 +30,9 @@ import java.io.FileNotFoundException import java.io.IOException import java.io.InputStream import java.nio.file.Files +import java.nio.file.LinkOption import java.nio.file.Path +import java.nio.file.StandardOpenOption import java.util.Locale import java.util.UUID import java.util.concurrent.ConcurrentHashMap @@ -289,9 +291,20 @@ object AssetsInstallationHelper { Files.createDirectories(destFile) } else { Files.createDirectories(destFile.parent) - Files.newOutputStream(destFile).use { dest -> - zipInput.copyTo(dest) - } + // NOFOLLOW_LINKS: the isSymbolicLink check above is a stat, and this is a separate + // open, so a link appearing in between would be followed. O_NOFOLLOW makes the + // refusal part of the open. Parent directories are still followed -- that needs + // openat(2), which java.nio does not expose (ADFA-5257 review). + Files + .newOutputStream( + destFile, + StandardOpenOption.WRITE, + StandardOpenOption.CREATE, + StandardOpenOption.TRUNCATE_EXISTING, + LinkOption.NOFOLLOW_LINKS, + ).use { dest -> + zipInput.copyTo(dest) + } } } } diff --git a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt index 5960e3876f..6d206042d0 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt @@ -20,8 +20,11 @@ package com.itsaky.androidide.utils import org.slf4j.LoggerFactory import java.io.File import java.io.IOException +import java.io.InputStream import java.nio.file.Files import java.nio.file.InvalidPathException +import java.nio.file.LinkOption +import java.nio.file.StandardOpenOption import java.util.zip.ZipFile object ZipUtils { @@ -38,6 +41,35 @@ object ZipUtils { val skipped: List, ) + /** + * Writes [input] to [outFile], refusing to write *through* a symlink at that path. + * + * The check above this is a check: it stats the path, then the write happens as a separate step, + * so a symlink appearing in between is followed -- `FileOutputStream` resolves links, and Kotlin's + * `File.outputStream()` is a thin inline wrapper over it. [StandardOpenOption] plus + * [LinkOption.NOFOLLOW_LINKS] moves the refusal into the `open(2)` call itself (`O_NOFOLLOW`), so + * there is no window between deciding and doing. + * + * This closes the *final component* only. A symlink substituted for one of the parent directories + * is still followed, by `mkdirs()` above and by the open here, because resolving a path relative + * to an already-open directory needs `openat(2)`, which `java.nio` does not expose. Narrowing that + * further would mean JNI or a different extraction strategy; it is recorded rather than implied + * away (ADFA-5257 review). + */ + internal fun writeNoFollow( + outFile: File, + input: InputStream, + ) { + Files + .newOutputStream( + outFile.toPath(), + StandardOpenOption.WRITE, + StandardOpenOption.CREATE, + StandardOpenOption.TRUNCATE_EXISTING, + LinkOption.NOFOLLOW_LINKS, + ).use { output -> input.copyTo(output) } + } + /** * Extracts [zipFile] into [destDir], preserving directory structure, and returns an * [UnzipResult] reporting the files it wrote and the entries it skipped. @@ -97,9 +129,7 @@ object ZipUtils { outFile.mkdirs() } else { outFile.parentFile?.mkdirs() - zip.getInputStream(entry).use { input -> - outFile.outputStream().use { output -> input.copyTo(output) } - } + zip.getInputStream(entry).use { input -> writeNoFollow(outFile, input) } } extracted.add(outFile) diff --git a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt index 1aa359e0dc..a3790cc826 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt @@ -5,6 +5,7 @@ import org.junit.Assert.assertThrows import org.junit.Rule import org.junit.Test import org.junit.rules.TemporaryFolder +import java.io.ByteArrayInputStream import java.io.File import java.io.IOException import java.nio.file.Files @@ -206,4 +207,26 @@ class ZipUtilsTest { val thrown = assertThrows(IOException::class.java) { ZipUtils.unzipFile(zip, destDir) } assertThat(thrown).hasMessageThat().contains("does not resolve to a safe path") } + + // The symlink policy above the write is a stat, and the write is a separate open: a link that + // appears in between is followed, because FileOutputStream resolves links. This pins the + // enforcement that closes that window -- O_NOFOLLOW in the open itself. It is tested directly + // because the policy check means the race is the only way to reach it in normal extraction, and + // a race is not something a test can stage reliably. + @Test + fun `writing refuses to follow a symlink at the target path`() { + val dir = tempFolder.newFolder("nofollow") + val outside = File(dir, "outside.txt").apply { writeText("original") } + val target = File(dir, "target.txt") + createSymlinkOrSkipTest(target.toPath(), outside.toPath()) + + val thrown = + assertThrows(IOException::class.java) { + ByteArrayInputStream("payload".toByteArray()).use { ZipUtils.writeNoFollow(target, it) } + } + + assertThat(thrown).isNotNull() + // The link's destination is untouched: nothing was written through it. + assertThat(outside.readText()).isEqualTo("original") + } } From fcf566662c4c5da8968136450d511300798631e3 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 26 Aug 2026 17:56:43 -0700 Subject: [PATCH 08/14] ADFA-5257: Correct a test comment that outlived the guard it describes The comment on the symlinked-grandparent test still explained the depth choice in terms of a toRealPath() check running after createDirectories(). This branch moved containment ahead of every mkdir, so that check is gone and neither depth reaches a mkdir at all. Two levels is still the right shape for the test, for a different reason: "linked/sub/nested.txt" has no ".." and does start with destDir, so it is exactly the case a lexical check alone lets through. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU --- .../assets/AssetsInstallationHelperTest.kt | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) 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 9bdd5b52a6..fe1f373587 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt @@ -183,14 +183,15 @@ class AssetsInstallationHelperTest { 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. + // entries), not one. The depth used to decide which guard caught it, back + // when containment was re-checked with toRealPath() after + // createDirectories() had already run: one level down, createDirectories() + // threw FileAlreadyExistsException on the symlink before that check was + // reached. ADFA-5257 moved containment ahead of every mkdir, so both + // depths are now refused by ContainedPathResolver with nothing created. + // Kept at two levels because that is the case a lexical check alone lets + // through -- "linked/sub/nested.txt" has no ".." and does start with + // destDir, so only resolving "linked" to its real path catches it. zos.putNextEntry(ZipEntry("linked/sub/nested.txt")) zos.write(content.toByteArray()) zos.closeEntry() From 67e6d3060ee5cdbe59e25875e92273f10fa0f11c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 14:34:41 +0000 Subject: [PATCH 09/14] ADFA-5257: Surface the resolver tri-state; fail outside-pointing symlinks Re-review follow-ups: - ContainedPathResolver.resolve() returns a sealed Resolution -- Contained / Rejected / Unverifiable -- so "escapes" and "could not be determined" no longer share one null. Both extraction call sites now throw distinct messages: escape, symlink refusal, and cannot-verify (naming the cause). - unzipFile's symlink-skip fallback skips only a pre-existing link that stays inside destDir; a link leading outside -- live, or dangling by its lexical target -- fails the archive again, restoring the old canonicalPath behavior. The KDoc states one policy instead of two. - The installer's "refusing to extract over an existing symlink" branch is reachable again: a symlink at the entry's own target (in-base, dangling, or outside) reports as that refusal, not as zip-slip. - A "." or "./" root directory entry is tolerated as a no-op at both extraction call sites; the resolver itself stays strict. - Layer 2 has one implementation, lexicalResolve() inside the resolver, and the rejected path travels to callers via Rejected.lexicalTarget, leaving only the ancestor/leaf link walk local to ZipUtils. Tests: outside-pointing symlink entries (live and dangling) fail the archive, in-base skips still pass, root entries no-op, all three messages pinned in both callers, and a symlink loop pins Unverifiable deterministically even where the permission-based test is skipped. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RKwjPVUcfXJdKP8StU5RDR --- .../assets/AssetsInstallationHelper.kt | 40 +++- .../assets/AssetsInstallationHelperTest.kt | 13 +- .../assets/ExtractZipToDirMergeTest.kt | 126 +++++++++- .../itsaky/androidide/utils/PathTraversal.kt | 217 +++++++++++------- .../com/itsaky/androidide/utils/ZipUtils.kt | 116 ++++++---- .../androidide/utils/PathTraversalTest.kt | 59 ++++- .../itsaky/androidide/utils/ZipUtilsTest.kt | 91 ++++++++ 7 files changed, 519 insertions(+), 143 deletions(-) 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 40b5dd8b7c..a32fd9150f 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -7,6 +7,7 @@ import com.aayushatharva.brotli4j.Brotli4jLoader import com.itsaky.androidide.app.configuration.IDEBuildConfigProvider import com.itsaky.androidide.resources.R import com.itsaky.androidide.utils.ContainedPathResolver +import com.itsaky.androidide.utils.ContainedPathResolver.Resolution import com.itsaky.androidide.utils.Environment.DEFAULT_ROOT import com.itsaky.androidide.utils.useEntriesEach import kotlinx.coroutines.Dispatchers @@ -264,9 +265,12 @@ object AssetsInstallationHelper { * against the filesystem rather than trusting an ancestor proven earlier. * * What stays local is the policy: this refuses to write through *any* existing symlink at an - * entry's target, even one pointing inside destDir. An installer directory reused across runs is - * the case that matters, and unlike unzipping a user's project there is no legitimate reason for - * a symlink to be there. + * entry's target -- in-base, dangling, or pointing outside destDir -- and says so. An installer + * directory reused across runs is the case that matters, and unlike unzipping a user's project + * there is no legitimate reason for a symlink to be there. The three failure messages are kept + * distinct on purpose: an escaping entry is a hostile archive, a symlink at the target is this + * policy, and unverifiable containment is a filesystem problem -- a 1.8 GB install that dies + * 9,000 entries in should name the real cause (ADFA-5257 review). */ @WorkerThread internal fun extractZipToDir( @@ -277,9 +281,35 @@ object AssetsInstallationHelper { val contained = ContainedPathResolver(destDir.toFile()) ZipInputStream(srcStream.buffered()).useEntriesEach { zipInput, entry -> + // A "." or "./" root directory entry names destDir itself, which already exists. The + // asset zips are refreshed from an external URL, and archivers that emit such an entry + // exist -- a no-op, not a reason to abort the installation (ADFA-5257 review). + if (entry.isDirectory && ContainedPathResolver.namesBase(entry.name)) { + return@useEntriesEach + } + val destFile = - contained.resolve(entry.name)?.toPath() - ?: throw IllegalStateException("Zip entry escapes the target dir: ${entry.name}") + when (val resolution = contained.resolve(entry.name)) { + is Resolution.Contained -> resolution.file.toPath() + is Resolution.Rejected -> { + // A pre-existing symlink at the entry's own target -- dangling, or leading + // outside destDir -- is this caller's refusal policy at work, not a + // zip-slip attempt; report it as such. + val overSymlink = resolution.lexicalTarget?.let { Files.isSymbolicLink(it) } == true + throw IllegalStateException( + if (overSymlink) { + "Refusing to extract over an existing symlink: ${entry.name}" + } else { + "Zip entry escapes the target dir: ${entry.name}" + }, + ) + } + is Resolution.Unverifiable -> + throw IllegalStateException( + "Cannot verify that a zip entry stays in the target dir: ${entry.name} (${resolution.cause})", + resolution.cause, + ) + } // Policy, not containment: the resolver allows a symlink whose target is still inside // destDir, and this caller does not. 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 fe1f373587..119c9756a8 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt @@ -199,9 +199,16 @@ class AssetsInstallationHelperTest { baos.toByteArray() } - assertThrows(IllegalStateException::class.java) { - AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir) - } + val thrown = + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir) + } + // The symlink sits at an *ancestor*, not at the entry's own target: this is an escape, + // and the message must say so -- distinct from the refusal over a symlink at the target. + assertTrue( + "expected an escape message, got: ${thrown.message}", + thrown.message!!.contains("escapes the target dir"), + ) } finally { outsideDir.deleteRecursivelyWithoutFollowingLinks() destDir.deleteRecursivelyWithoutFollowingLinks() 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 496d3a10ce..2284910bf8 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt @@ -146,14 +146,25 @@ class ExtractZipToDirMergeTest { fun `rejects path traversal`() { val dest = Files.createTempDirectory("mvn") try { - assertThrows(IllegalStateException::class.java) { - AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest) - } + val thrown = + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("../evil.jar" to "x"), dest) + } + // A real escape is reported as one -- distinct from the symlink-refusal and + // cannot-verify messages below. + assertTrue( + "expected an escape message, got: ${thrown.message}", + thrown.message!!.contains("escapes the target dir"), + ) } finally { dest.deleteRecursivelyWithoutFollowingLinks() } } + // The installer's own policy branch: any pre-existing symlink at an entry's target refuses the + // extraction, and says so. A *dangling* link is the case the resolver refuses before the + // explicit isSymbolicLink check is reached, so asserting the message (not just the type) pins + // that it still surfaces as the symlink refusal, not as a zip-slip accusation. @Test fun `rejects extraction over an existing symlink`() { val dest = Files.createTempDirectory("mvn") @@ -162,15 +173,118 @@ class ExtractZipToDirMergeTest { val outsideTarget = outside.resolve("payload") Files.createSymbolicLink(dest.resolve("evil.jar"), outsideTarget) - assertThrows(IllegalStateException::class.java) { - AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest) - } + val thrown = + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest) + } + assertTrue( + "expected the symlink refusal message, got: ${thrown.message}", + thrown.message!!.contains("Refusing to extract over an existing symlink"), + ) + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + outside.deleteRecursivelyWithoutFollowingLinks() + } + } + + // Same refusal for a live link whose target is inside destDir -- the resolver proves it + // contained, and the installer's explicit isSymbolicLink check refuses it anyway. + @Test + fun `rejects extraction over an existing symlink pointing inside destDir`() { + val dest = Files.createTempDirectory("mvn") + try { + Files.write(dest.resolve("real.jar"), "kept".toByteArray()) + Files.createSymbolicLink(dest.resolve("evil.jar"), dest.resolve("real.jar")) + + val thrown = + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest) + } + assertTrue( + "expected the symlink refusal message, got: ${thrown.message}", + thrown.message!!.contains("Refusing to extract over an existing symlink"), + ) + assertEquals("kept", String(Files.readAllBytes(dest.resolve("real.jar")))) + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + } + } + + // And for a live link pointing outside destDir -- refused by the resolver's real-path check, + // still reported as the symlink refusal it is, with nothing written through the link. + @Test + fun `rejects extraction over an existing symlink pointing outside destDir`() { + val dest = Files.createTempDirectory("mvn") + val outside = Files.createTempDirectory("outside") + try { + val outsideTarget = outside.resolve("payload") + Files.write(outsideTarget, "original".toByteArray()) + Files.createSymbolicLink(dest.resolve("evil.jar"), outsideTarget) + + val thrown = + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("evil.jar" to "x"), dest) + } + assertTrue( + "expected the symlink refusal message, got: ${thrown.message}", + thrown.message!!.contains("Refusing to extract over an existing symlink"), + ) + assertEquals("original", String(Files.readAllBytes(outsideTarget))) } finally { dest.deleteRecursivelyWithoutFollowingLinks() outside.deleteRecursivelyWithoutFollowingLinks() } } + // The third message: containment that cannot be *verified* (here a symlink loop, ELOOP) is + // neither an escape nor the symlink refusal -- it aborts naming the filesystem cause, so a + // failing install points at the disk, not at the archive. + @Test + fun `reports unverifiable containment distinctly`() { + val dest = Files.createTempDirectory("mvn") + try { + Files.createSymbolicLink(dest.resolve("loop-a"), dest.resolve("loop-b")) + Files.createSymbolicLink(dest.resolve("loop-b"), dest.resolve("loop-a")) + + val thrown = + assertThrows(IllegalStateException::class.java) { + AssetsInstallationHelper.extractZipToDir(zipOf("loop-a/file.txt" to "x"), dest) + } + assertTrue( + "expected the cannot-verify message, got: ${thrown.message}", + thrown.message!!.contains("Cannot verify"), + ) + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + } + } + + // A "." or "./" root directory entry names destDir itself. Some archivers emit one, and the + // asset zips are refreshed from an external URL -- it must be a no-op, not an aborted install. + @Test + fun `tolerates a root directory entry instead of aborting`() { + val dest = Files.createTempDirectory("mvn") + try { + val zipBytes = + ByteArrayOutputStream().use { baos -> + ZipOutputStream(baos).use { zip -> + zip.putNextEntry(ZipEntry("./")) + zip.closeEntry() + zip.putNextEntry(ZipEntry("com/foo/a.txt")) + zip.write("kept".toByteArray()) + zip.closeEntry() + } + baos.toByteArray() + } + + AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), dest) + + assertEquals("kept", String(Files.readAllBytes(dest.resolve("com/foo/a.txt")))) + } finally { + dest.deleteRecursivelyWithoutFollowingLinks() + } + } + @Test fun `rejects extraction into a symlinked parent that escapes destDir`() { val dest = Files.createTempDirectory("mvn") diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index 12809ef9ca..c62cc0b131 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -69,99 +69,138 @@ class ContainedPathResolver( ) { private val log = LoggerFactory.getLogger(ContainedPathResolver::class.java) - private val base: Path = baseDir.toPath().toAbsolutePath().normalize() + internal val base: Path = baseDir.toPath().toAbsolutePath().normalize() /** - * The resolved file [relativePath] names inside the base directory, or null when it is invalid - * or escapes -- including when it is not a representable path at all (a decoded NUL byte, say: - * `Uri.pathSegments` percent-decodes before this ever sees the string, so `%00` arrives as a - * literal NUL, which [java.nio.file.Path] rejects rather than silently ignoring). + * What [resolve] decided. Three states, not two: "escapes" and "could not be determined" both + * fail closed, but a caller that renders them with one message accuses a filesystem error of + * being a traversal attempt (ADFA-5257 review). */ - fun resolve(relativePath: String): File? { + sealed interface Resolution { + /** Proven inside the base; [file] is the target to use. */ + data class Contained( + val file: File, + ) : Resolution + + /** + * Refused: lexically invalid, escaping, naming the base itself, or sitting at/behind a + * symlink the filesystem check would not vouch for. [lexicalTarget] is the in-base path + * layer 2 produced when only the *filesystem* check refused -- handed out so a caller can + * report what actually sits there (an existing symlink, say) without re-deriving + * containment. Null when the name itself was rejected. + */ + data class Rejected( + val lexicalTarget: Path?, + ) : Resolution + + /** + * Containment could not be determined: the base or the path's nearest existing ancestor + * failed to resolve for a reason other than absence (EACCES after a mode change, EIO). + * Not an escape -- refused because unproven, and [cause] says why. + */ + data class Unverifiable( + val cause: IOException, + ) : Resolution + } + + /** + * Layers 1 and 2 alone: [relativePath] resolved against the base and normalized, or null when + * it is lexically rejected, not a representable path (a decoded NUL byte, say: `Uri.pathSegments` + * percent-decodes before this ever sees the string, so `%00` arrives as a literal NUL, which + * [java.nio.file.Path] rejects), or does not land strictly inside the base. The one + * implementation of the lexical containment rule -- [resolve] builds on it rather than beside + * it, so a fallback judging [Resolution.Rejected.lexicalTarget] cannot drift from it + * (ADFA-5257 review). + */ + internal fun lexicalResolve(relativePath: String): Path? { if (isLexicallyRejected(relativePath)) { return null } - - return try { - val resolved = base.resolve(relativePath).normalize() - if (!resolved.startsWith(base)) { - return null - } - if (resolved == base) { - // "." and "./" normalize to the base itself -- not a path *inside* it, and a caller - // treats a non-null result as a usable target. + val resolved = + try { + base.resolve(relativePath).normalize() + } catch (_: InvalidPathException) { return null } + if (resolved == base) { + // "." and "./" normalize to the base itself -- not a path *inside* it, and a caller + // treats a Contained result as a usable target. + return null + } + if (!resolved.startsWith(base)) { + return null + } + return resolved + } - // Resolved per call, not once in a constructor. Two reasons: nothing stops a caller from - // constructing the resolver before the base exists, so a base pinned at construction could - // stay null for the resolver's whole life and layer 3 would never run even once the tree is - // created -- and an existing base can gain a symlink later, so a resolution proven once can - // go stale. Also, notExists() is not !exists() -- both are false - // when the answer cannot be determined (a parent denying execute), and treating that as - // "absent, nothing to symlink through" is the same silent downgrade to lexical containment. - // Confirmed-absent skips layer 3; anything else must resolve or be refused. - val realBase = - try { - base.toRealPath() - // Fully qualified on purpose: Kotlin auto-imports kotlin.io.NoSuchFileException, which - // toRealPath() never throws, so catching that one would quietly disable this branch. - } catch (_: java.nio.file.NoSuchFileException) { - // Confirmed absent, so there is nothing on disk to symlink through and layer 3 has - // nothing to check. (A base that is itself a dangling symlink lands here too; a write - // under it fails at the write, and layer 2 still holds.) Distinguished from a failure - // this way rather than via a separate notExists() probe, which costs a second stat and - // answers "false" for both absent and undeterminable. - null - } catch (e: IOException) { - log.warn("Cannot resolve {} to a real path; refusing every path under it", base, e) - return null - } - realBase ?: return resolved.toFile() + /** How [resolve] judged [relativePath] against the base directory -- see [Resolution]. */ + fun resolve(relativePath: String): Resolution { + val resolved = lexicalResolve(relativePath) ?: return Resolution.Rejected(null) - var ancestor = resolved - // NOFOLLOW_LINKS: plain Files.exists() follows symlinks, so a *dangling* symlink (one - // whose target does not currently exist) would read as absent here, walking straight - // past it to its parent instead of stopping to verify it. toRealPath() below throws - // IOException for a genuinely dangling target, correctly rejecting the path rather than - // trusting whatever ends up on the far side of it later. - while (!Files.exists(ancestor, LinkOption.NOFOLLOW_LINKS)) { - ancestor = ancestor.parent ?: return null + // Resolved per call, not once in a constructor. Two reasons: nothing stops a caller from + // constructing the resolver before the base exists, so a base pinned at construction could + // stay null for the resolver's whole life and layer 3 would never run even once the tree is + // created -- and an existing base can gain a symlink later, so a resolution proven once can + // go stale. Also, notExists() is not !exists() -- both are false + // when the answer cannot be determined (a parent denying execute), and treating that as + // "absent, nothing to symlink through" is the same silent downgrade to lexical containment. + // Confirmed-absent skips layer 3; anything else must resolve or be refused. + val realBase = + try { + base.toRealPath() + // Fully qualified on purpose: Kotlin auto-imports kotlin.io.NoSuchFileException, which + // toRealPath() never throws, so catching that one would quietly disable this branch. + } catch (_: java.nio.file.NoSuchFileException) { + // Confirmed absent, so there is nothing on disk to symlink through and layer 3 has + // nothing to check. (A base that is itself a dangling symlink lands here too; a write + // under it fails at the write, and layer 2 still holds.) Distinguished from a failure + // this way rather than via a separate notExists() probe, which costs a second stat and + // answers "false" for both absent and undeterminable. + null + } catch (e: IOException) { + log.warn("Cannot resolve {} to a real path; refusing every path under it", base, e) + return Resolution.Unverifiable(e) } - // Re-resolved on every call, deliberately. Caching a directory once proven contained saves - // a toRealPath() per entry, but it answers later paths under that directory without - // looking -- so if anything replaces it with a symlink in between, the answer is stale and - // the caller writes through the link. The cache was measured against a real 1.8 GB asset - // installation on device and bought nothing: 48.0 s without it, 51.4 s with it, 51.3 s for - // the hand-rolled cache it replaced. Extraction is I/O and inflate; this is noise - // (ADFA-5257 review). - val realAncestor = - try { - ancestor.toRealPath() - // Fully qualified for the same reason as the base branch above. - } catch (_: java.nio.file.NoSuchFileException) { - // Expected for a dangling symlink: the NOFOLLOW walk stopped at a link that exists, - // but its target does not, so there is no real path to prove contained. Refused - // without the warning below -- this is the link check working, not a failure. - return null - } catch (e: IOException) { - log.warn( - "Cannot resolve {} (nearest existing ancestor of {}) to a real path; refusing the path", - ancestor, - relativePath, - e, - ) - return null - } - if (!realAncestor.startsWith(realBase)) { - return null + realBase ?: return Resolution.Contained(resolved.toFile()) + + var ancestor = resolved + // NOFOLLOW_LINKS: plain Files.exists() follows symlinks, so a *dangling* symlink (one + // whose target does not currently exist) would read as absent here, walking straight + // past it to its parent instead of stopping to verify it. toRealPath() below throws + // IOException for a genuinely dangling target, correctly rejecting the path rather than + // trusting whatever ends up on the far side of it later. + while (!Files.exists(ancestor, LinkOption.NOFOLLOW_LINKS)) { + ancestor = ancestor.parent ?: return Resolution.Rejected(resolved) + } + // Re-resolved on every call, deliberately. Caching a directory once proven contained saves + // a toRealPath() per entry, but it answers later paths under that directory without + // looking -- so if anything replaces it with a symlink in between, the answer is stale and + // the caller writes through the link. The cache was measured against a real 1.8 GB asset + // installation on device and bought nothing: 48.0 s without it, 51.4 s with it, 51.3 s for + // the hand-rolled cache it replaced. Extraction is I/O and inflate; this is noise + // (ADFA-5257 review). + val realAncestor = + try { + ancestor.toRealPath() + // Fully qualified for the same reason as the base branch above. + } catch (_: java.nio.file.NoSuchFileException) { + // Expected for a dangling symlink: the NOFOLLOW walk stopped at a link that exists, + // but its target does not, so there is no real path to prove contained. Refused + // without the warning below -- this is the link check working, not a failure. + return Resolution.Rejected(resolved) + } catch (e: IOException) { + log.warn( + "Cannot resolve {} (nearest existing ancestor of {}) to a real path; refusing the path", + ancestor, + relativePath, + e, + ) + return Resolution.Unverifiable(e) } - resolved.toFile() - } catch (_: InvalidPathException) { - null - } catch (_: IOException) { - null + if (!realAncestor.startsWith(realBase)) { + return Resolution.Rejected(resolved) } + return Resolution.Contained(resolved.toFile()) } companion object { @@ -179,12 +218,24 @@ class ContainedPathResolver( relativePath.startsWith("/") || relativePath.startsWith("\\") || relativePath.split('/', '\\').any { it == ".." } + + /** + * Whether [relativePath] merely names the base directory itself -- `.`, `./`, and + * equivalents. [resolve] deliberately refuses such a path (a Contained result must be a + * usable target *inside* the base), but an archive's root directory entry is a no-op to + * extract, not an escape -- some archivers emit one. Exposed so extraction call sites can + * apply that tolerance without loosening the resolver (ADFA-5257 review). + */ + fun namesBase(relativePath: String): Boolean = + relativePath.isNotEmpty() && relativePath.split('/', '\\').all { it.isEmpty() || it == "." } } } /** - * [ContainedPathResolver.resolve] for a single path, where there is nothing to reuse a resolver for. - * Prefer the class when validating many paths against one base -- a zip's entries, say. + * [ContainedPathResolver.resolve] for a single path, where there is nothing to reuse a resolver for, + * collapsed to contained-or-not: the file when contained, null otherwise. Prefer the class when + * validating many paths against one base -- a zip's entries, say -- or when the caller must tell an + * escape from a filesystem failure ([ContainedPathResolver.Resolution] keeps them apart). * * Nothing in `common` calls this yet; the caller is the deep-link handler in the app module (#1651), * which validates one `{filename}` per request. Kept here rather than landing with that PR so both @@ -193,4 +244,4 @@ class ContainedPathResolver( fun resolveWithinDirectory( baseDir: File, relativePath: String, -): File? = ContainedPathResolver(baseDir).resolve(relativePath) +): File? = (ContainedPathResolver(baseDir).resolve(relativePath) as? ContainedPathResolver.Resolution.Contained)?.file diff --git a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt index 6d206042d0..d0a79af5a9 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt @@ -17,13 +17,14 @@ package com.itsaky.androidide.utils +import com.itsaky.androidide.utils.ContainedPathResolver.Resolution import org.slf4j.LoggerFactory import java.io.File import java.io.IOException import java.io.InputStream import java.nio.file.Files -import java.nio.file.InvalidPathException import java.nio.file.LinkOption +import java.nio.file.Path import java.nio.file.StandardOpenOption import java.util.zip.ZipFile @@ -74,13 +75,15 @@ object ZipUtils { * Extracts [zipFile] into [destDir], preserving directory structure, and returns an * [UnzipResult] reporting the files it wrote and the entries it skipped. * - * An entry whose target is an existing symlink -- live, dangling, or even pointing outside - * [destDir] -- is skipped and extraction continues: nothing is written at or through the link, - * which keeps a user's own symlink (a `gradlew`, an SDK link) from being overwritten by an - * archive. The skip applies only to a link lexically inside [destDir] whose entry name carries - * no traversal syntax; an entry that would land outside [destDir] (zip-slip), names its target - * through a `..` segment, or is not a representable path fails the whole call with an - * [IOException]. + * An entry whose target is an existing symlink that *stays inside* [destDir] -- live with an + * in-base real target, or dangling with an in-base lexical one -- is skipped and extraction + * continues: nothing is written at or through the link, which keeps a user's own symlink (a + * `gradlew`, an SDK link) from being overwritten by an archive. A symlink leading outside + * [destDir] gets no such courtesy: like an entry that would land outside [destDir] (zip-slip), + * one that names its target through a `..` segment, or one that is not a representable path, + * it fails the whole call with an [IOException]. Containment that cannot be *verified* (a + * filesystem error, not an escape) also fails the call, saying so rather than accusing the + * archive. A `.`/`./` root directory entry names [destDir] itself and is ignored as a no-op. */ @JvmStatic @Throws(IOException::class) @@ -98,22 +101,44 @@ object ZipUtils { while (entries.hasMoreElements()) { val entry = entries.nextElement() + // A "." or "./" root directory entry names destDir itself, which already exists. + // The resolver stays strict about it (a Contained result must be *inside* the + // base), so the extract-it-as-a-no-op tolerance lives here (ADFA-5257 review). + if (entry.isDirectory && ContainedPathResolver.namesBase(entry.name)) { + continue + } + // Containment first, then link policy: the link check must only ever see a path // already proven contained. Checking File(destDir, entry.name) before containment // would stat outside destDir for a ../ entry and could quietly skip a zip-slip // attempt (ADFA-5257). val outFile = - contained.resolve(entry.name) - ?: if (isContainedSymlink(destDir, entry.name)) { - // The resolver refuses a symlink it cannot verify -- dangling, or pointing - // outside -- but a link that sits lexically inside destDir is the user's - // own, and skipping writes nothing at or through it. Same policy as below. - log.info("Leaving the existing symlink at {}/{} alone; that zip entry was not extracted", destDir, entry.name) - skipped.add(entry.name) - continue - } else { + when (val resolution = contained.resolve(entry.name)) { + is Resolution.Contained -> resolution.file + is Resolution.Rejected -> { + val link = resolution.lexicalTarget + if (link != null && isContainedSymlink(contained.base, link)) { + // The resolver refuses a symlink it cannot vouch for, but a link that + // stays inside destDir is the user's own, and skipping writes nothing + // at or through it. Same policy as below. + log.info( + "Leaving the existing symlink at {}/{} alone; that zip entry was not extracted", + destDir, + entry.name, + ) + skipped.add(entry.name) + continue + } throw IOException("Zip entry does not resolve to a safe path inside the target directory: ${entry.name}") } + is Resolution.Unverifiable -> + // Not an escape: refused because unproven. Distinct wording so a + // filesystem error is not reported as a hostile archive. + throw IOException( + "Cannot verify that a zip entry resolves inside the target directory: ${entry.name} (${resolution.cause})", + resolution.cause, + ) + } // Policy, not containment: a user's own symlink inside their own project -- gradlew, or // gradle/wrapper pointed at a shared location -- is legitimate, so the entry is skipped @@ -140,33 +165,26 @@ object ZipUtils { } /** - * Whether a symlink exists at [entryName]'s lexically-normalized path inside [destDir]. Applies - * the resolver's own lexical reject first, so an entry the resolver refused for its *syntax* (a - * `..` segment, an absolute path) stays a bad archive even when a symlink happens to sit at its - * normalized target -- `a/../link.txt` must fail, not ride the skip meant for `link.txt`. Then - * every ancestor between [destDir] and the candidate must be a non-link: stat'ing the candidate + * Whether [candidate] -- an entry's target the resolver already proved lexically inside [base] + * and then refused, via [Resolution.Rejected.lexicalTarget], so no containment logic is + * re-derived here -- is a pre-existing symlink that stays inside [base], the one refusal + * [unzipFile] downgrades to a skip. An entry the resolver refused for its *syntax* (a `..` + * segment, an absolute path) never gets here: it carries no lexical target, so `a/../link.txt` + * fails rather than riding the skip meant for `link.txt`. + * + * Every ancestor between [base] and the candidate must be a non-link: stat'ing the candidate * *follows* an ancestor symlink, so with `a -> /outside`, `a/link.txt` would stat * `/outside/link.txt` and a link found there would ride the skip -- an escaping archive - * tolerated instead of rejected. Only a link whose every ancestor is a real directory inside - * [destDir] qualifies, and the stats stay on paths proven lexically inside [destDir]. + * tolerated instead of rejected. The link's own target must stay inside [base] too: a live + * link is judged by its real path, a dangling one by where its text leads lexically (there is + * no real target to resolve). A link leading outside [base] is failed like any other escape -- + * the pre-resolver canonical-path behavior, kept on purpose -- never skipped (ADFA-5257 + * review). */ private fun isContainedSymlink( - destDir: File, - entryName: String, + base: Path, + candidate: Path, ): Boolean { - if (ContainedPathResolver.isLexicallyRejected(entryName)) { - return false - } - val base = destDir.toPath().toAbsolutePath().normalize() - val candidate = - try { - base.resolve(entryName).normalize() - } catch (_: InvalidPathException) { - return false - } - if (candidate == base || !candidate.startsWith(base)) { - return false - } var ancestor = candidate.parent while (ancestor != null && ancestor != base) { if (Files.isSymbolicLink(ancestor)) { @@ -174,6 +192,24 @@ object ZipUtils { } ancestor = ancestor.parent } - return Files.isSymbolicLink(candidate) + if (!Files.isSymbolicLink(candidate)) { + return false + } + return try { + candidate.toRealPath().startsWith(base.toRealPath()) + // Fully qualified: Kotlin auto-imports kotlin.io.NoSuchFileException, which + // toRealPath() never throws. + } catch (_: java.nio.file.NoSuchFileException) { + // Dangling link. In-base is decided on its lexical target instead; failing to read + // the link at all falls through to "not skippable". + val parent = candidate.parent ?: return false + try { + parent.resolve(Files.readSymbolicLink(candidate)).normalize().startsWith(base) + } catch (_: IOException) { + false + } + } catch (_: IOException) { + false + } } } diff --git a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt index b17679050a..44f62cff56 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt @@ -18,6 +18,7 @@ package com.itsaky.androidide.utils import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.ContainedPathResolver.Resolution import org.junit.Assume import org.junit.Rule import org.junit.Test @@ -159,13 +160,14 @@ class PathTraversalTest { // A containment check must fail closed. Resolving the base used to happen once in the constructor, // catching the IOException and nulling the field, which permanently downgraded every later // resolve() to lexical containment alone -- weaker than the canonical-prefix check this class - // replaced, and silent about it. + // replaced, and silent about it. The refusal is Unverifiable, not Rejected: a filesystem failure + // must reach the caller as itself, not as a traversal accusation. @Test - fun `a base that cannot be resolved rejects everything`() { + fun `a base that cannot be resolved refuses everything as unverifiable`() { val root = tempFolder.newFolder("unresolvable-root") val base = File(root, "base").apply { mkdirs() } val resolver = ContainedPathResolver(base) - assertThat(resolver.resolve("child.txt")).isNotNull() + assertThat(resolver.resolve("child.txt")).isInstanceOf(Resolution.Contained::class.java) // Make the base unresolvable by removing traverse permission on its parent, then check that // this environment actually produced the failure -- as root, or on a filesystem that ignores @@ -182,8 +184,8 @@ class PathTraversalTest { Assume.assumeTrue("This environment still resolves a base under a non-traversable parent", reallyUnresolvable) // Both the resolver built while the base was fine and a fresh one: the check is per call. - assertThat(resolver.resolve("child.txt")).isNull() - assertThat(ContainedPathResolver(base).resolve("child.txt")).isNull() + assertThat(resolver.resolve("child.txt")).isInstanceOf(Resolution.Unverifiable::class.java) + assertThat(ContainedPathResolver(base).resolve("child.txt")).isInstanceOf(Resolution.Unverifiable::class.java) } finally { root.setExecutable(true, false) } @@ -203,7 +205,52 @@ class PathTraversalTest { base.mkdirs() createSymlinkOrSkipTest(File(base, "link").toPath(), outside.toPath()) - assertThat(resolver.resolve("link/secret.txt")).isNull() + // Rejected, not Unverifiable: an escape is a definite answer, not a failure to answer. + assertThat(resolver.resolve("link/secret.txt")).isInstanceOf(Resolution.Rejected::class.java) + } + + // A filesystem-refused entry hands its lexically-resolved target back in the rejection, so a + // caller with its own policy for what sits there (an existing symlink, say) can inspect that + // path without re-deriving containment -- the drift the shared resolver exists to remove. + @Test + fun `a rejection at an existing symlink carries the lexical target`() { + val root = tempFolder.newFolder("dangling-link") + createSymlinkOrSkipTest(File(root, "link.txt").toPath(), File(root, "missing.txt").toPath()) + + val resolution = ContainedPathResolver(root).resolve("link.txt") + + assertThat(resolution).isInstanceOf(Resolution.Rejected::class.java) + assertThat((resolution as Resolution.Rejected).lexicalTarget) + .isEqualTo( + root + .toPath() + .toAbsolutePath() + .normalize() + .resolve("link.txt"), + ) + } + + // A deterministic Unverifiable, unlike the permission-based test above (which an environment + // running as root skips): a symlink loop makes toRealPath() throw FileSystemException (ELOOP), + // which is a filesystem failure, not an escape, and must be reported as itself. + @Test + fun `a symlink loop is unverifiable, not an escape`() { + val root = tempFolder.newFolder("loop") + createSymlinkOrSkipTest(File(root, "loop-a").toPath(), File(root, "loop-b").toPath()) + createSymlinkOrSkipTest(File(root, "loop-b").toPath(), File(root, "loop-a").toPath()) + + assertThat(ContainedPathResolver(root).resolve("loop-a/file.txt")) + .isInstanceOf(Resolution.Unverifiable::class.java) + } + + // A lexically-refused entry carries no target at all: nothing was resolved for it, and a + // fallback must not treat "../x" as if it named a real in-base path. + @Test + fun `a lexical rejection carries no target`() { + val resolution = ContainedPathResolver(baseDir).resolve("../x") + + assertThat(resolution).isInstanceOf(Resolution.Rejected::class.java) + assertThat((resolution as Resolution.Rejected).lexicalTarget).isNull() } // The narrowing this class deliberately makes over ZipUtils' old canonical-prefix check: an entry diff --git a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt index a3790cc826..37654c2ee9 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt @@ -123,6 +123,97 @@ class ZipUtilsTest { assertThat(result.skipped).containsExactly("link.txt") } + // The policy decision on the re-review finding: only a symlink that *stays inside* destDir is + // the user's own to keep. A pre-existing link pointing outside fails the archive -- the old + // canonicalPath behavior -- rather than riding the skip and letting the caller report success + // for an archive whose entry was never installed. + @Test + fun `unzipFile fails an entry whose target is a symlink pointing outside the destination`() { + val destDir = tempFolder.newFolder("dest") + val outsideDir = tempFolder.newFolder("outside") + val outsideFile = File(outsideDir, "target.txt").apply { writeText("outside content") } + val linkPath = File(destDir, "link.txt").toPath() + createSymlinkOrSkipTest(linkPath, outsideFile.toPath()) + + val zipFile = tempFolder.newFile("evil.zip") + ZipOutputStream(zipFile.outputStream()).use { zip -> + zip.putNextEntry(ZipEntry("link.txt")) + zip.write("payload".toByteArray()) + zip.closeEntry() + } + + val thrown = assertThrows(IOException::class.java) { ZipUtils.unzipFile(zipFile, destDir) } + assertThat(thrown).hasMessageThat().contains("does not resolve to a safe path") + assertThat(Files.isSymbolicLink(linkPath)).isTrue() + assertThat(outsideFile.readText()).isEqualTo("outside content") + } + + // Same policy for a dangling link: it has no real target, so it is judged by where its text + // leads -- and lexically outside destDir fails, unlike the in-base dangling link above. + @Test + fun `unzipFile fails an entry whose target is a dangling symlink leading outside the destination`() { + val destDir = tempFolder.newFolder("dest") + val outsideDir = tempFolder.newFolder("outside") + val missingTarget = File(outsideDir, "missing.txt") + val linkPath = File(destDir, "link.txt").toPath() + createSymlinkOrSkipTest(linkPath, missingTarget.toPath()) + + val zipFile = tempFolder.newFile("evil.zip") + ZipOutputStream(zipFile.outputStream()).use { zip -> + zip.putNextEntry(ZipEntry("link.txt")) + zip.write("payload".toByteArray()) + zip.closeEntry() + } + + val thrown = assertThrows(IOException::class.java) { ZipUtils.unzipFile(zipFile, destDir) } + assertThat(thrown).hasMessageThat().contains("does not resolve to a safe path") + assertThat(Files.isSymbolicLink(linkPath)).isTrue() + assertThat(missingTarget.exists()).isFalse() + } + + // The tri-state surfaced: containment that cannot be *verified* (here a symlink loop, ELOOP) + // is reported as exactly that, with the cause attached -- not as a hostile archive. + @Test + fun `unzipFile reports unverifiable containment distinctly from an escape`() { + val destDir = tempFolder.newFolder("dest") + createSymlinkOrSkipTest(File(destDir, "loop-a").toPath(), File(destDir, "loop-b").toPath()) + createSymlinkOrSkipTest(File(destDir, "loop-b").toPath(), File(destDir, "loop-a").toPath()) + + val zipFile = tempFolder.newFile("archive.zip") + ZipOutputStream(zipFile.outputStream()).use { zip -> + zip.putNextEntry(ZipEntry("loop-a/file.txt")) + zip.write("payload".toByteArray()) + zip.closeEntry() + } + + val thrown = assertThrows(IOException::class.java) { ZipUtils.unzipFile(zipFile, destDir) } + assertThat(thrown).hasMessageThat().contains("Cannot verify") + assertThat(thrown).hasCauseThat().isNotNull() + } + + // A "./" root directory entry names destDir itself. The resolver refuses it (a resolved path + // must be *inside* the base), but extracting it is a no-op, not an escape -- the archive must + // not abort on it. + @Test + fun `unzipFile treats a root directory entry as a no-op`() { + val zipFile = tempFolder.newFile("archive.zip") + ZipOutputStream(zipFile.outputStream()).use { zip -> + zip.putNextEntry(ZipEntry("./")) + zip.closeEntry() + + zip.putNextEntry(ZipEntry("root.txt")) + zip.write("root content".toByteArray()) + zip.closeEntry() + } + + val destDir = tempFolder.newFolder("dest") + val result = ZipUtils.unzipFile(zipFile, destDir) + + assertThat(File(destDir, "root.txt").readText()).isEqualTo("root content") + assertThat(result.extracted.map { it.name }).containsExactly("root.txt") + assertThat(result.skipped).isEmpty() + } + @Test fun `unzipFile allows a harmless double-dot inside a path segment`() { val zipFile = tempFolder.newFile("archive.zip") From fbf67695c5b679612b034d38f692b9e77923dcd8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 14:47:31 +0000 Subject: [PATCH 10/14] ADFA-5257: Brace every when entry the way Spotless formats them The Build Universal APK check failed on spotlessKotlinCheck: the ktlint ruleset Spotless runs braces all entries of a when whose other entries are braced, and separates multi-line entries with a blank line. Apply exactly the formatting its diff demanded to the two resolution whens. No behavior change. --- .../itsaky/androidide/assets/AssetsInstallationHelper.kt | 9 +++++++-- .../main/java/com/itsaky/androidide/utils/ZipUtils.kt | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) 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 a32fd9150f..8f64bc5a7e 100644 --- a/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt +++ b/app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt @@ -290,7 +290,10 @@ object AssetsInstallationHelper { val destFile = when (val resolution = contained.resolve(entry.name)) { - is Resolution.Contained -> resolution.file.toPath() + is Resolution.Contained -> { + resolution.file.toPath() + } + is Resolution.Rejected -> { // A pre-existing symlink at the entry's own target -- dangling, or leading // outside destDir -- is this caller's refusal policy at work, not a @@ -304,11 +307,13 @@ object AssetsInstallationHelper { }, ) } - is Resolution.Unverifiable -> + + is Resolution.Unverifiable -> { throw IllegalStateException( "Cannot verify that a zip entry stays in the target dir: ${entry.name} (${resolution.cause})", resolution.cause, ) + } } // Policy, not containment: the resolver allows a symlink whose target is still inside diff --git a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt index d0a79af5a9..34d48bc1e7 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt @@ -114,7 +114,10 @@ object ZipUtils { // attempt (ADFA-5257). val outFile = when (val resolution = contained.resolve(entry.name)) { - is Resolution.Contained -> resolution.file + is Resolution.Contained -> { + resolution.file + } + is Resolution.Rejected -> { val link = resolution.lexicalTarget if (link != null && isContainedSymlink(contained.base, link)) { @@ -131,13 +134,15 @@ object ZipUtils { } throw IOException("Zip entry does not resolve to a safe path inside the target directory: ${entry.name}") } - is Resolution.Unverifiable -> + + is Resolution.Unverifiable -> { // Not an escape: refused because unproven. Distinct wording so a // filesystem error is not reported as a hostile archive. throw IOException( "Cannot verify that a zip entry resolves inside the target directory: ${entry.name} (${resolution.cause})", resolution.cause, ) + } } // Policy, not containment: a user's own symlink inside their own project -- gradlew, or From 61d83000724fa15ec7655d3e12f089683e660b26 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 14:47:31 +0000 Subject: [PATCH 11/14] ADFA-5257: Deny absolute paths the root-entry tolerance in namesBase "/" and "\" split into all-empty segments just like "./", so namesBase answered true for them and an absolute directory entry would have been waved through as the archive's root entry (CodeRabbit review). Apply the existing lexical reject first -- it already refuses absolute paths and the empty string -- and pin the boundary with a test. --- .../com/itsaky/androidide/utils/PathTraversal.kt | 6 ++++-- .../itsaky/androidide/utils/PathTraversalTest.kt | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index c62cc0b131..6db71d23c9 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -224,10 +224,12 @@ class ContainedPathResolver( * equivalents. [resolve] deliberately refuses such a path (a Contained result must be a * usable target *inside* the base), but an archive's root directory entry is a no-op to * extract, not an escape -- some archivers emit one. Exposed so extraction call sites can - * apply that tolerance without loosening the resolver (ADFA-5257 review). + * apply that tolerance without loosening the resolver (ADFA-5257 review). The lexical reject + * applies first: an absolute `/` or `\` is all empty segments too, but it names the + * filesystem root, not the base (ADFA-5257 review). */ fun namesBase(relativePath: String): Boolean = - relativePath.isNotEmpty() && relativePath.split('/', '\\').all { it.isEmpty() || it == "." } + !isLexicallyRejected(relativePath) && relativePath.split('/', '\\').all { it.isEmpty() || it == "." } } } diff --git a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt index 44f62cff56..61aa964882 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt @@ -260,4 +260,18 @@ class PathTraversalTest { fun `a dot-dot segment is refused even when it normalizes back inside`() { assertThat(resolveWithinDirectory(baseDir, "a/../b.txt")).isNull() } + + // "/" and "\" split into all-empty segments just like "./", but they name the filesystem + // root, not the base -- namesBase must not grant an absolute path the root-entry tolerance + // (ADFA-5257 review). + @Test + fun `namesBase accepts only relative spellings of the base itself`() { + assertThat(ContainedPathResolver.namesBase(".")).isTrue() + assertThat(ContainedPathResolver.namesBase("./")).isTrue() + assertThat(ContainedPathResolver.namesBase(".\\")).isTrue() + assertThat(ContainedPathResolver.namesBase("/")).isFalse() + assertThat(ContainedPathResolver.namesBase("\\")).isFalse() + assertThat(ContainedPathResolver.namesBase("")).isFalse() + assertThat(ContainedPathResolver.namesBase("src")).isFalse() + } } From 91b180350c9f5a75505678e735157432bdbd4159 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 20:57:08 +0000 Subject: [PATCH 12/14] ADFA-5257: Verify an absent base's nearest existing ancestor resolve() accepted a confirmed-absent base outright, on the reasoning that nothing on disk could be symlinked through. Its *existing* ancestors are on disk, though: with base root/link/missing where root/link points outside root, resolve() returned Contained and a later mkdirs/newOutputStream followed the link, planting the whole "contained" tree outside the base (CodeRabbit, PR #1736). When the base is absent, walk to the nearest existing ancestor of the resolved path (the same NOFOLLOW walk layer 3 already uses). Everything between that ancestor and the target is absent, so the only place a link can hide is the ancestor itself: a symlink there -- a dangling- symlink base included -- is Rejected, and an ancestor that will not toRealPath() is Unverifiable. A plain missing tree beneath real ancestors still resolves to Contained, so first-run installer directories keep working. Regression tests: symlinked ancestor of an absent base, dangling- symlink base, absent base under real ancestors, and a symlink loop above an absent base. The first two fail against the previous code. --- .../itsaky/androidide/utils/PathTraversal.kt | 48 +++++++++++++---- .../androidide/utils/PathTraversalTest.kt | 51 +++++++++++++++++++ 2 files changed, 89 insertions(+), 10 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index 6db71d23c9..5a6738cecb 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -51,9 +51,11 @@ import java.nio.file.Path * [java.nio.file.Path.toRealPath] and re-verify containment -- layer 2 is purely lexical and * will not catch a symlink already present inside the base (a project cloned with git, an * installer directory reused across runs). Walking up to the nearest existing ancestor handles a - * path that does not exist yet. Skipped only when the base is *confirmed* absent -- there is then - * nothing on disk to symlink-escape through. A base that cannot be resolved is refused outright, - * never quietly downgraded to layer 2. + * path that does not exist yet. A *confirmed-absent* base has no real path to compare against, so + * its nearest existing ancestor is verified instead: it must be a resolvable non-symlink, because + * a link there would redirect everything later created "under" the base (ADFA-5257 review). A + * base or ancestor that cannot be resolved is refused outright, never quietly downgraded to + * layer 2. * * What this deliberately does *not* decide is what to do about an existing symlink *at the target* * whose destination is still inside the base. The two callers disagree -- unzipping leaves a user's @@ -144,24 +146,25 @@ class ContainedPathResolver( // go stale. Also, notExists() is not !exists() -- both are false // when the answer cannot be determined (a parent denying execute), and treating that as // "absent, nothing to symlink through" is the same silent downgrade to lexical containment. - // Confirmed-absent skips layer 3; anything else must resolve or be refused. + // Confirmed-absent shifts layer 3 onto the nearest existing ancestor; anything else must + // resolve or be refused. val realBase = try { base.toRealPath() // Fully qualified on purpose: Kotlin auto-imports kotlin.io.NoSuchFileException, which // toRealPath() never throws, so catching that one would quietly disable this branch. } catch (_: java.nio.file.NoSuchFileException) { - // Confirmed absent, so there is nothing on disk to symlink through and layer 3 has - // nothing to check. (A base that is itself a dangling symlink lands here too; a write - // under it fails at the write, and layer 2 still holds.) Distinguished from a failure - // this way rather than via a separate notExists() probe, which costs a second stat and - // answers "false" for both absent and undeterminable. + // Confirmed absent as a whole -- but its existing ancestors are still on disk, and a + // symlink among them redirects everything later created "under" the base. Verified + // against the nearest existing ancestor below instead of being accepted outright + // (ADFA-5257 review). Distinguished from a failure this way rather than via a separate + // notExists() probe, which costs a second stat and answers "false" for both absent and + // undeterminable. null } catch (e: IOException) { log.warn("Cannot resolve {} to a real path; refusing every path under it", base, e) return Resolution.Unverifiable(e) } - realBase ?: return Resolution.Contained(resolved.toFile()) var ancestor = resolved // NOFOLLOW_LINKS: plain Files.exists() follows symlinks, so a *dangling* symlink (one @@ -172,6 +175,31 @@ class ContainedPathResolver( while (!Files.exists(ancestor, LinkOption.NOFOLLOW_LINKS)) { ancestor = ancestor.parent ?: return Resolution.Rejected(resolved) } + + if (realBase == null) { + // The base is absent, so everything from it down to the target is absent too and the + // walk above stopped at or above the base. There is no real base to prove containment + // against, and the only place a symlink can hide is that existing ancestor itself -- + // the segments between it and the target do not exist. A link there (a base that is a + // dangling symlink included) is refused: a later mkdirs would follow it and plant the + // "contained" tree wherever it points (ADFA-5257 review). A real ancestor must still + // resolve -- a failure means containment is unproven, not disproven. + if (Files.isSymbolicLink(ancestor)) { + return Resolution.Rejected(resolved) + } + try { + ancestor.toRealPath() + } catch (e: IOException) { + log.warn( + "Cannot resolve {} (nearest existing ancestor of absent base {}) to a real path; refusing the path", + ancestor, + base, + e, + ) + return Resolution.Unverifiable(e) + } + return Resolution.Contained(resolved.toFile()) + } // Re-resolved on every call, deliberately. Caching a directory once proven contained saves // a toRealPath() per entry, but it answers later paths under that directory without // looking -- so if anything replaces it with a symlink in between, the answer is stale and diff --git a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt index 61aa964882..85bed120dc 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt @@ -209,6 +209,57 @@ class PathTraversalTest { assertThat(resolver.resolve("link/secret.txt")).isInstanceOf(Resolution.Rejected::class.java) } + // An absent base skips the real-base comparison (there is nothing to resolve), but that must not + // skip the symlink check on what *does* exist: with base root/link/missing and link pointing + // outside root, accepting the absent base means a later mkdirs follows the link and plants the + // whole "contained" tree outside it (ADFA-5257 review). + @Test + fun `an absent base beneath a symlinked ancestor is rejected`() { + val root = tempFolder.newFolder("absent-base") + val outside = tempFolder.newFolder("absent-base-outside") + createSymlinkOrSkipTest(File(root, "link").toPath(), outside.toPath()) + + val resolution = ContainedPathResolver(File(root, "link/missing")).resolve("a.txt") + + assertThat(resolution).isInstanceOf(Resolution.Rejected::class.java) + } + + // The same hole with the link at the base itself: a dangling symlink reads as an absent base to + // toRealPath(), but mkdirs happily creates its target's tree once anything springs up there. + @Test + fun `an absent base that is itself a dangling symlink is rejected`() { + val root = tempFolder.newFolder("dangling-base") + createSymlinkOrSkipTest(File(root, "base").toPath(), File(root, "not-yet").toPath()) + + assertThat(ContainedPathResolver(File(root, "base")).resolve("a.txt")) + .isInstanceOf(Resolution.Rejected::class.java) + } + + // The legitimate absent-base case stays accepted: plain missing directories beneath a real, + // resolvable ancestor are exactly what an installer creates on first run. + @Test + fun `an absent base beneath real ancestors still resolves`() { + val root = tempFolder.newFolder("first-run") + + val resolution = ContainedPathResolver(File(root, "not/yet/created")).resolve("a.txt") + + assertThat(resolution).isInstanceOf(Resolution.Contained::class.java) + assertThat((resolution as Resolution.Contained).file) + .isEqualTo(File(root, "not/yet/created/a.txt")) + } + + // A loop among the absent base's ancestors is a filesystem failure (ELOOP), not an escape -- + // same contract as the in-base loop test below. + @Test + fun `a symlink loop above an absent base is unverifiable, not an escape`() { + val root = tempFolder.newFolder("absent-loop") + createSymlinkOrSkipTest(File(root, "loop-a").toPath(), File(root, "loop-b").toPath()) + createSymlinkOrSkipTest(File(root, "loop-b").toPath(), File(root, "loop-a").toPath()) + + assertThat(ContainedPathResolver(File(root, "loop-a/missing")).resolve("a.txt")) + .isInstanceOf(Resolution.Unverifiable::class.java) + } + // A filesystem-refused entry hands its lexically-resolved target back in the rejection, so a // caller with its own policy for what sits there (an existing symlink, say) can inspect that // path without re-deriving containment -- the drift the shared resolver exists to remove. From 8bae363a60ea664d1cf6b688c70a1faf1056f56e Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 27 Aug 2026 15:51:27 -0700 Subject: [PATCH 13/14] ADFA-5257: Record why the absent-base branch is stricter than the other 91b180350 refuses a symlinked nearest-existing-ancestor when the base is absent. The existing-base path answers Contained for the same topology: with base root/link/missing it resolves root/link and the startsWith(realBase) comparison is satisfied, because the link relocates the base and the target together. Measured rather than argued -- creating the base between two otherwise identical calls flips Rejected to Contained. Comment only, no behaviour change. The asymmetry is worth keeping: the branch can only fail closed, and neither production caller reaches it, since ZipUtils.unzipFile and AssetsInstallationHelper.extractZipToDir both create destDir on the line above the resolver construction. The note says what to decide if a caller ever does resolve against a not-yet- created base -- whether an ancestor link is an escape from the base or just where the caller put it. Written down so the next reader does not re-file it as a defect. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU --- .../java/com/itsaky/androidide/utils/PathTraversal.kt | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index 5a6738cecb..9c6849efd3 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -184,6 +184,16 @@ class ContainedPathResolver( // dangling symlink included) is refused: a later mkdirs would follow it and plant the // "contained" tree wherever it points (ADFA-5257 review). A real ancestor must still // resolve -- a failure means containment is unproven, not disproven. + // + // Deliberately stricter than the existing-base path, which answers Contained for the + // same topology: with base root/link/missing, resolving root/link to its real path and + // comparing startsWith(realBase) is satisfied, because the link relocates the base and + // the target together. Measured, not assumed -- creating the base between two otherwise + // identical calls flips Rejected to Contained. The asymmetry is kept because this branch + // can only fail closed, and because neither production caller reaches it: ZipUtils and + // AssetsInstallationHelper both create destDir on the line above the construction. If a + // caller ever does resolve against a not-yet-created base, decide then whether an + // ancestor link is an escape from the base or merely where the caller put it. if (Files.isSymbolicLink(ancestor)) { return Resolution.Rejected(resolved) } From a4002dc0c090d35ff76f1f6b61996acdad46ca3d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 22:51:54 +0000 Subject: [PATCH 14/14] ADFA-5257: Judge an absent base against its real location, like a created one 91b180350 closed the absent-base hole by rejecting a symlinked nearest-existing-ancestor outright. Review measured the asymmetry that left: with base root/link/missing and root/link -> outside, the absent base answered Rejected while the identical tree after createDirectories(base) answered Contained, because the existing-base path resolves realBase *through* symlinks and compares real paths. A symlink between the base and the filesystem root is where the caller's base lives, not an escape from it, so the absent-base path now answers the same question the same way: walk to the nearest existing ancestor and resolve it with toRealPath(), sharing the existing-base branch's ancestor resolution. The segments below the ancestor are absent, layer-1-vetted plain names, so the base's real location is the ancestor's real path plus those segments and containment holds by construction once the ancestor resolves. Nothing is accepted unvalidated, and fail-closed is kept where there is no real location to judge against: a dangling link (a base that is itself one included) still throws NoSuchFileException from toRealPath() and stays Rejected, any other resolution failure stays Unverifiable (the symlink-loop test is unchanged), and CodeRabbit's original escape never resurfaces because a Contained answer always rests on a resolved real path. Tests: the symlinked-ancestor regression test now pins Contained with the resolved target, a new test pins the measured symmetry itself (same tree, absent then created, identical answers), and the dangling-base test keeps Rejected under the unified rule. --- .../itsaky/androidide/utils/PathTraversal.kt | 63 ++++++++----------- .../androidide/utils/PathTraversalTest.kt | 41 +++++++++--- 2 files changed, 58 insertions(+), 46 deletions(-) diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index 5a6738cecb..4a8c9b9ecc 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -51,11 +51,11 @@ import java.nio.file.Path * [java.nio.file.Path.toRealPath] and re-verify containment -- layer 2 is purely lexical and * will not catch a symlink already present inside the base (a project cloned with git, an * installer directory reused across runs). Walking up to the nearest existing ancestor handles a - * path that does not exist yet. A *confirmed-absent* base has no real path to compare against, so - * its nearest existing ancestor is verified instead: it must be a resolvable non-symlink, because - * a link there would redirect everything later created "under" the base (ADFA-5257 review). A - * base or ancestor that cannot be resolved is refused outright, never quietly downgraded to - * layer 2. + * path that does not exist yet. Containment is always judged against the base's *real* location, + * whether or not the base exists yet: a symlink between the base and the filesystem root is where + * the caller's base lives, not an escape from it, so a *confirmed-absent* base under such a link + * answers the same as the created one would (ADFA-5257 review). A base or ancestor that cannot be + * resolved is refused outright, never quietly downgraded to layer 2. * * What this deliberately does *not* decide is what to do about an existing symlink *at the target* * whose destination is still inside the base. The two callers disagree -- unzipping leaves a user's @@ -146,18 +146,17 @@ class ContainedPathResolver( // go stale. Also, notExists() is not !exists() -- both are false // when the answer cannot be determined (a parent denying execute), and treating that as // "absent, nothing to symlink through" is the same silent downgrade to lexical containment. - // Confirmed-absent shifts layer 3 onto the nearest existing ancestor; anything else must - // resolve or be refused. + // Confirmed-absent shifts layer 3's real-path judgment onto the nearest existing ancestor; + // anything else must resolve or be refused. val realBase = try { base.toRealPath() // Fully qualified on purpose: Kotlin auto-imports kotlin.io.NoSuchFileException, which // toRealPath() never throws, so catching that one would quietly disable this branch. } catch (_: java.nio.file.NoSuchFileException) { - // Confirmed absent as a whole -- but its existing ancestors are still on disk, and a - // symlink among them redirects everything later created "under" the base. Verified - // against the nearest existing ancestor below instead of being accepted outright - // (ADFA-5257 review). Distinguished from a failure this way rather than via a separate + // Confirmed absent as a whole -- there is no real base to compare against yet, so the + // judgment shifts to the nearest existing ancestor's real path below (ADFA-5257 + // review). Distinguished from a failure this way rather than via a separate // notExists() probe, which costs a second stat and answers "false" for both absent and // undeterminable. null @@ -176,30 +175,6 @@ class ContainedPathResolver( ancestor = ancestor.parent ?: return Resolution.Rejected(resolved) } - if (realBase == null) { - // The base is absent, so everything from it down to the target is absent too and the - // walk above stopped at or above the base. There is no real base to prove containment - // against, and the only place a symlink can hide is that existing ancestor itself -- - // the segments between it and the target do not exist. A link there (a base that is a - // dangling symlink included) is refused: a later mkdirs would follow it and plant the - // "contained" tree wherever it points (ADFA-5257 review). A real ancestor must still - // resolve -- a failure means containment is unproven, not disproven. - if (Files.isSymbolicLink(ancestor)) { - return Resolution.Rejected(resolved) - } - try { - ancestor.toRealPath() - } catch (e: IOException) { - log.warn( - "Cannot resolve {} (nearest existing ancestor of absent base {}) to a real path; refusing the path", - ancestor, - base, - e, - ) - return Resolution.Unverifiable(e) - } - return Resolution.Contained(resolved.toFile()) - } // Re-resolved on every call, deliberately. Caching a directory once proven contained saves // a toRealPath() per entry, but it answers later paths under that directory without // looking -- so if anything replaces it with a symlink in between, the answer is stale and @@ -212,9 +187,10 @@ class ContainedPathResolver( ancestor.toRealPath() // Fully qualified for the same reason as the base branch above. } catch (_: java.nio.file.NoSuchFileException) { - // Expected for a dangling symlink: the NOFOLLOW walk stopped at a link that exists, - // but its target does not, so there is no real path to prove contained. Refused - // without the warning below -- this is the link check working, not a failure. + // Expected for a dangling symlink (a base that is itself one included): the NOFOLLOW + // walk stopped at a link that exists, but its target does not, so there is no real + // path to prove contained. Refused without the warning below -- this is the link + // check working, not a failure. return Resolution.Rejected(resolved) } catch (e: IOException) { log.warn( @@ -225,6 +201,17 @@ class ContainedPathResolver( ) return Resolution.Unverifiable(e) } + if (realBase == null) { + // The base is confirmed absent, so the walk stopped at or above it and everything from + // the ancestor down to the target consists of absent, layer-1-vetted plain segments -- + // no links, no "..". The base's real location is the ancestor's real path plus those + // segments, and the target sits inside it by construction, so resolving the ancestor is + // the whole judgment. A symlink at or above the ancestor is where the caller's base + // lives, not an escape from it -- the absent base answers exactly as the created one + // would (ADFA-5257 review). Nothing is accepted unvalidated: a dangling link (a base + // that is itself one included) or an unresolvable ancestor was refused above. + return Resolution.Contained(resolved.toFile()) + } if (!realAncestor.startsWith(realBase)) { return Resolution.Rejected(resolved) } diff --git a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt index 85bed120dc..3db000be61 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt @@ -25,6 +25,7 @@ import org.junit.Test import org.junit.rules.TemporaryFolder import java.io.File import java.io.IOException +import java.nio.file.Files class PathTraversalTest { private val baseDir = File("/project/root") @@ -209,23 +210,47 @@ class PathTraversalTest { assertThat(resolver.resolve("link/secret.txt")).isInstanceOf(Resolution.Rejected::class.java) } - // An absent base skips the real-base comparison (there is nothing to resolve), but that must not - // skip the symlink check on what *does* exist: with base root/link/missing and link pointing - // outside root, accepting the absent base means a later mkdirs follows the link and plants the - // whole "contained" tree outside it (ADFA-5257 review). + // A symlink between the base and the filesystem root is where the caller's base lives, not an + // escape from it: root/link/missing/a.txt does not escape root/link/missing. With the base + // absent there is no real base to compare against, so containment is judged against the nearest + // existing ancestor's real path -- resolved, not refused, exactly as the existing-base branch + // resolves realBase through symlinks (ADFA-5257 review). @Test - fun `an absent base beneath a symlinked ancestor is rejected`() { + fun `an absent base beneath a symlinked ancestor resolves against its real location`() { val root = tempFolder.newFolder("absent-base") val outside = tempFolder.newFolder("absent-base-outside") createSymlinkOrSkipTest(File(root, "link").toPath(), outside.toPath()) val resolution = ContainedPathResolver(File(root, "link/missing")).resolve("a.txt") - assertThat(resolution).isInstanceOf(Resolution.Rejected::class.java) + assertThat(resolution).isInstanceOf(Resolution.Contained::class.java) + assertThat((resolution as Resolution.Contained).file) + .isEqualTo(File(root, "link/missing/a.txt")) + } + + // The symmetry the rule above preserves: the same tree must answer the same whether the base + // happens to exist yet or not -- the class answers one question, "is the entry inside the + // base's real location", and Files.createDirectories(base) between two calls must not change + // that answer (ADFA-5257 review). + @Test + fun `an absent base and the same base created answer identically`() { + val root = tempFolder.newFolder("symmetry") + val outside = tempFolder.newFolder("symmetry-outside") + createSymlinkOrSkipTest(File(root, "link").toPath(), outside.toPath()) + val base = File(root, "link/missing") + val resolver = ContainedPathResolver(base) + + val whileAbsent = resolver.resolve("a.txt") + Files.createDirectories(base.toPath()) + val onceCreated = resolver.resolve("a.txt") + + assertThat(whileAbsent).isInstanceOf(Resolution.Contained::class.java) + assertThat(onceCreated).isEqualTo(whileAbsent) } - // The same hole with the link at the base itself: a dangling symlink reads as an absent base to - // toRealPath(), but mkdirs happily creates its target's tree once anything springs up there. + // A *dangling* link is still refused: its target does not exist, so the base has no real + // location to judge against -- the same NoSuchFileException rule the existing-base branch + // applies to a dangling ancestor, not a special absent-base case. @Test fun `an absent base that is itself a dangling symlink is rejected`() { val root = tempFolder.newFolder("dangling-base")