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..8f64bc5a7e 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,8 @@ 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.ContainedPathResolver.Resolution import com.itsaky.androidide.utils.Environment.DEFAULT_ROOT import com.itsaky.androidide.utils.useEntriesEach import kotlinx.coroutines.Dispatchers @@ -29,7 +31,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 @@ -254,62 +258,88 @@ object AssetsInstallationHelper { destDir: Path, ) = extractZipToDir(Files.newInputStream(srcFile), destDir) + /** + * 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 -- 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( 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}") + // 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 = normalizedDestDir.resolve(entry.name).normalize() + val destFile = + when (val resolution = contained.resolve(entry.name)) { + is Resolution.Contained -> { + resolution.file.toPath() + } - // 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}") - } + 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, + ) + } + } - // 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}") + // 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) } - lastVerifiedParent = destFile.parent - } - - Files.newOutputStream(destFile).use { dest -> - zipInput.copyTo(dest) - } } } } 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/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt b/app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt index 9bdd5b52a6..119c9756a8 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() @@ -198,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 92868c1daf..2284910bf8 100644 --- a/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt +++ b/app/src/test/java/com/itsaky/androidide/assets/ExtractZipToDirMergeTest.kt @@ -123,18 +123,48 @@ class ExtractZipToDirMergeTest { } } + // The lexical guard used to be a bare substring reject, so an entry legitimately named with + // 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") + 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.deleteRecursivelyWithoutFollowingLinks() + } + } + @Test 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") @@ -143,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 new file mode 100644 index 0000000000..4a8c9b9ecc --- /dev/null +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -0,0 +1,264 @@ +/* + * 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.slf4j.LoggerFactory +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. + * + * 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. 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, + * 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 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. 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 + * 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. + */ +class ContainedPathResolver( + baseDir: File, +) { + private val log = LoggerFactory.getLogger(ContainedPathResolver::class.java) + + internal val base: Path = baseDir.toPath().toAbsolutePath().normalize() + + /** + * 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). + */ + 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 + } + 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 + } + + /** How [resolve] judged [relativePath] against the base directory -- see [Resolution]. */ + fun resolve(relativePath: String): Resolution { + val resolved = lexicalResolve(relativePath) ?: return Resolution.Rejected(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 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 -- 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 + } catch (e: IOException) { + log.warn("Cannot resolve {} to a real path; refusing every path under it", base, e) + return Resolution.Unverifiable(e) + } + + 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 (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( + "Cannot resolve {} (nearest existing ancestor of {}) to a real path; refusing the path", + ancestor, + relativePath, + e, + ) + 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) + } + return Resolution.Contained(resolved.toFile()) + } + + 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 == ".." } + + /** + * 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). 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 = + !isLexicallyRejected(relativePath) && relativePath.split('/', '\\').all { it.isEmpty() || it == "." } + } +} + +/** + * [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 + * entry points ship as one reviewed unit. + */ +fun resolveWithinDirectory( + baseDir: File, + relativePath: String, +): 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 a21c2ab5f0..34d48bc1e7 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/ZipUtils.kt @@ -17,49 +17,204 @@ 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.LinkOption +import java.nio.file.Path +import java.nio.file.StandardOpenOption import java.util.zip.ZipFile object ZipUtils { + private val log = LoggerFactory.getLogger(ZipUtils::class.java) + + /** + * 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, + ) + + /** + * 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 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 an + * [UnzipResult] reporting the files it wrote and the entries it skipped. + * + * 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) fun unzipFile( zipFile: File, destDir: File, - ): List { + ): UnzipResult { destDir.mkdirs() - val destDirPath = destDir.canonicalPath + File.separator - val result = mutableListOf() + val contained = ContainedPathResolver(destDir) + val extracted = mutableListOf() + val skipped = 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}") + // 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 = + 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 + // 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 } if (entry.isDirectory) { 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) } } - result.add(outFile) + extracted.add(outFile) } } - return result + return UnzipResult(extracted, skipped) + } + + /** + * 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. 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( + base: Path, + candidate: Path, + ): Boolean { + var ancestor = candidate.parent + while (ancestor != null && ancestor != base) { + if (Files.isSymbolicLink(ancestor)) { + return false + } + ancestor = ancestor.parent + } + 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 new file mode 100644 index 0000000000..3db000be61 --- /dev/null +++ b/common/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt @@ -0,0 +1,353 @@ +/* + * 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 com.itsaky.androidide.utils.ContainedPathResolver.Resolution +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.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 `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 + // 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`() { + // 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")) + } + + @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") + + createSymlinkOrSkipTest(File(root, "evil").toPath(), outside.toPath()) + + 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. 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 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")).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 + // 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")).isInstanceOf(Resolution.Unverifiable::class.java) + assertThat(ContainedPathResolver(base).resolve("child.txt")).isInstanceOf(Resolution.Unverifiable::class.java) + } finally { + root.setExecutable(true, false) + } + } + + // 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") + val base = File(root, "base") + val resolver = ContainedPathResolver(base) + + val outside = File(root, "outside").apply { mkdirs() } + File(outside, "secret.txt").writeText("secret") + base.mkdirs() + createSymlinkOrSkipTest(File(base, "link").toPath(), outside.toPath()) + + // 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 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 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.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) + } + + // 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") + 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. + @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 + // 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() + } + + // "/" 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() + } +} 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 a8c2acc349..37654c2ee9 100644 --- a/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt +++ b/common/src/test/java/com/itsaky/androidide/utils/ZipUtilsTest.kt @@ -5,8 +5,10 @@ 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 import java.util.zip.ZipEntry import java.util.zip.ZipOutputStream @@ -36,10 +38,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 @@ -58,4 +61,263 @@ 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() + 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, + // 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 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(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") + } + + // 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") + 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") + } + + // 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") + } + + // 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. + @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("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") + } }