Skip to content

Commit dcda33c

Browse files
davidschachterADFAclaude
davidschachterADFA
and
claude
authored
ADFA-5005: Fix SDK bootstrap crash extracting android-sdk.zip (#1621)
* ADFA-5005: Fix SDK bootstrap crash extracting android-sdk.zip The bundled android-sdk.zip.br has no directory entries, so its very first zip entry (build-tools/35.0.0/NOTICE.txt) failed to extract on every fresh install: extractZipToDir only created parent directories for entries explicitly flagged as directories, never for plain file entries, and ANDROID_HOME is wiped before each install. Create the parent directory unconditionally before writing each file entry. Verified end-to-end on-device: OOBE completes and build-tools/35.0.0/NOTICE.txt lands at the expected 1,068,025 bytes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ADFA-5005: Add regression test for extractZipToDir missing parent dirs Builds an in-memory zip with a single nested file entry and no directory entries, matching how android-sdk.zip is packaged, and asserts extraction creates the parent directories and preserves the file content. Confirmed the test fails with NoSuchFileException against the pre-fix code and passes with the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ADFA-5005: Harden extractZipToDir against on-disk symlink escapes The existing normalize()/startsWith() checks validate the zip entry name lexically, but not the actual filesystem state -- a symlink already present under destDir (e.g. from a merged/reused install dir in SplitAssetsInstaller) could redirect Files.createDirectories() or Files.newOutputStream() outside destDir undetected. Resolve destFile's real parent path after creating it and re-check containment against destDir's real path, and refuse to write through a destFile that already exists as a symlink. Added regression tests for both vectors; confirmed they fail without this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ADFA-5005: Close symlink-escape gap in extractZipToDir directory entries Review of PR #1621 flagged an asymmetry: the on-disk symlink checks added for file entries didn't cover directory entries, so Files.createDirectories(destFile) would silently no-op through a pre-existing symlink pointing outside destDir (or, if the symlink target didn't exist, create directories at the symlink's target outside destDir). Hoist the existing-symlink check above the isDirectory branch so it applies to both, and add the same real-path containment check after creating a directory entry. Added a regression test confirming a bare directory entry resolving to an escaping symlink is now rejected; confirmed it fails without this change and passes with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ADFA-5005: Cache last-verified parent to cut redundant toRealPath() calls Review of PR #1621 noted that destFile.parent.toRealPath() runs once per file entry even though zip entries are commonly clustered by directory (e.g. 20 files under build-tools/35.0.0/ alone) -- each consecutive sibling re-walks and re-resolves the same parent path. Cache the last-verified parent and skip the real-path containment check when the current entry's parent is unchanged. Nothing in the loop can turn an already-verified real directory into a symlink mid-run, so caching by lexical parent equality doesn't weaken the check. Added a test exercising multiple sibling files under one directory (the only test that hit the cache-hit branch); full assets test suite still green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ADFA-5005: Address review feedback on symlink hardening - Route SplitAssetsInstaller/BundledAssetsInstaller's plugin-zip extraction through the hardened extractZipToDir() instead of a lexical-only reimplementation, so all three extraction sites share one symlink-hardened path. - Add a regression test covering a file entry under a pre-existing symlinked parent with no directory entry -- the shape android-sdk.zip actually has, and the only path that previously exercised the toRealPath() escape check. - Fix a stale comment in ExtractZipToDirMergeTest's zipOf helper that no longer matched extractZipToDir's behavior. - Stop leaking temp dirs across the symlink tests, using a walker that won't follow symlinks into deletion. * ADFA-5005: Fix symlink-escape test to actually reach the toRealPath() guard hal-eisen-adfa found that the previous test (`linked/nested.txt`, one level below the symlink) made destFile.parent the symlink itself, so Files.createDirectories() threw FileAlreadyExistsException under NOFOLLOW_LINKS before the toRealPath() guard ever ran -- the test failed, and the guard stayed uncovered. Use a two-level entry (`linked/sub/nested.txt`) instead, per his repro: createDirectories() silently traverses the symlink to create `sub` for real inside the escape target, and only then does the toRealPath() check on destFile.parent fire and reject it. That's the guard this test is meant to cover. Also switch the test's cleanup to deleteRecursivelyWithoutFollowingLinks() (copied from ExtractZipToDirMergeTest), since the prior deleteRecursively() follows symlinks and would delete the escape target's contents if outsideDir happened to be cleaned up after destDir. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent e429548 commit dcda33c

5 files changed

Lines changed: 272 additions & 63 deletions

File tree

‎app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,14 @@ object AssetsInstallationHelper {
245245
Files.createDirectories(destDir)
246246
// Normalize and make destDir absolute for secure path validation
247247
val normalizedDestDir = destDir.toAbsolutePath().normalize()
248+
val realDestDir = normalizedDestDir.toRealPath()
249+
250+
// Zip entries are commonly clustered by directory (e.g. dozens of files
251+
// under the same build-tools/<version>/ prefix); cache the last-verified
252+
// parent so consecutive entries under it skip a redundant toRealPath() call.
253+
// Nothing below can turn an already-verified real directory into a symlink
254+
// mid-run, so caching by lexical parent equality is safe.
255+
var lastVerifiedParent: Path? = null
248256

249257
ZipInputStream(srcStream.buffered()).useEntriesEach { zipInput, entry ->
250258
// Validate entry name doesn't contain dangerous patterns
@@ -260,9 +268,28 @@ object AssetsInstallationHelper {
260268
throw IllegalStateException("Entry is outside of the target dir: ${entry.name}")
261269
}
262270

271+
// The checks above are lexical (entry name only) and don't catch a symlink
272+
// already present on disk (e.g. destDir merged/reused across installer
273+
// runs). Reject writing through an existing symlink up front, then
274+
// re-check containment against the real, on-disk path once created.
275+
if (Files.isSymbolicLink(destFile)) {
276+
throw IllegalStateException("Refusing to extract over an existing symlink: ${entry.name}")
277+
}
278+
263279
if (entry.isDirectory) {
264280
Files.createDirectories(destFile)
281+
if (!destFile.toRealPath().startsWith(realDestDir)) {
282+
throw IllegalStateException("Entry escapes the target dir via symlink: ${entry.name}")
283+
}
265284
} else {
285+
Files.createDirectories(destFile.parent)
286+
if (destFile.parent != lastVerifiedParent) {
287+
if (!destFile.parent.toRealPath().startsWith(realDestDir)) {
288+
throw IllegalStateException("Entry parent escapes the target dir via symlink: ${entry.name}")
289+
}
290+
lastVerifiedParent = destFile.parent
291+
}
292+
266293
Files.newOutputStream(destFile).use { dest ->
267294
zipInput.copyTo(dest)
268295
}

‎app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt‎

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import java.io.FileNotFoundException
2929
import java.io.IOException
3030
import java.nio.file.Files
3131
import java.nio.file.Path
32-
import java.util.zip.ZipInputStream
3332
import kotlin.io.path.ExperimentalPathApi
3433
import kotlin.io.path.deleteRecursively
3534

@@ -174,27 +173,7 @@ data object BundledAssetsInstaller : BaseAssetsInstaller() {
174173
val assetPath = ToolsManager.getCommonAsset("$entryName.br")
175174
assets.open(assetPath).use { assetStream ->
176175
BrotliInputStream(assetStream).use { brotliStream ->
177-
ZipInputStream(brotliStream).use { pluginZip ->
178-
var pluginEntry = pluginZip.nextEntry
179-
while (pluginEntry != null) {
180-
if (!pluginEntry.isDirectory) {
181-
val targetPath = pluginDirPath.resolve(pluginEntry.name).normalize()
182-
// Security check: prevent path traversal attacks
183-
if (!targetPath.startsWith(pluginDirPath)) {
184-
throw IllegalStateException(
185-
"Zip entry '${pluginEntry.name}' would escape target directory",
186-
)
187-
}
188-
val targetFile = targetPath.toFile()
189-
targetFile.parentFile?.mkdirs()
190-
logger.debug("Extracting '{}' to {}", pluginEntry.name, targetFile)
191-
targetFile.outputStream().use { output ->
192-
pluginZip.copyTo(output)
193-
}
194-
}
195-
pluginEntry = pluginZip.nextEntry
196-
}
197-
}
176+
AssetsInstallationHelper.extractZipToDir(brotliStream, pluginDirPath)
198177
}
199178
}
200179
logger.debug("Completed extracting plugin artifacts")

‎app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt‎

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import java.io.FileNotFoundException
2323
import java.nio.file.Files
2424
import java.nio.file.Path
2525
import java.util.zip.ZipFile
26-
import java.util.zip.ZipInputStream
2726
import kotlin.io.path.ExperimentalPathApi
2827
import kotlin.io.path.deleteRecursively
2928
import kotlin.system.measureTimeMillis
@@ -167,27 +166,7 @@ data object SplitAssetsInstaller : BaseAssetsInstaller() {
167166
}
168167
Files.createDirectories(pluginDirPath)
169168

170-
ZipInputStream(zipInput).use { pluginZip ->
171-
var pluginEntry = pluginZip.nextEntry
172-
while (pluginEntry != null) {
173-
if (!pluginEntry.isDirectory) {
174-
val targetPath = pluginDirPath.resolve(pluginEntry.name).normalize()
175-
// Security check: prevent path traversal attacks
176-
if (!targetPath.startsWith(pluginDirPath)) {
177-
throw IllegalStateException(
178-
"Zip entry '${pluginEntry.name}' would escape target directory",
179-
)
180-
}
181-
val targetFile = targetPath.toFile()
182-
targetFile.parentFile?.mkdirs()
183-
logger.debug("Extracting '{}' to {}", pluginEntry.name, targetFile)
184-
targetFile.outputStream().use { output ->
185-
pluginZip.copyTo(output)
186-
}
187-
}
188-
pluginEntry = pluginZip.nextEntry
189-
}
190-
}
169+
AssetsInstallationHelper.extractZipToDir(zipInput, pluginDirPath)
191170
logger.debug("Completed extracting plugin artifacts")
192171
}
193172

‎app/src/test/java/com/itsaky/androidide/assets/AssetsInstallationHelperTest.kt‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,23 @@ import io.mockk.every
77
import io.mockk.mockk
88
import io.mockk.mockkObject
99
import kotlinx.coroutines.runBlocking
10+
import org.junit.Assert.assertEquals
1011
import org.junit.Assert.assertFalse
12+
import org.junit.Assert.assertThrows
1113
import org.junit.Assert.assertTrue
1214
import org.junit.Before
1315
import org.junit.Test
16+
import java.io.ByteArrayInputStream
17+
import java.io.ByteArrayOutputStream
1418
import java.io.FileNotFoundException
19+
import java.io.IOException
20+
import java.nio.file.FileVisitResult
21+
import java.nio.file.Files
22+
import java.nio.file.Path
23+
import java.nio.file.SimpleFileVisitor
24+
import java.nio.file.attribute.BasicFileAttributes
25+
import java.util.zip.ZipEntry
26+
import java.util.zip.ZipOutputStream
1527

1628
class AssetsInstallationHelperTest {
1729
private val ctx: Context = mockk(relaxed = true)
@@ -48,4 +60,94 @@ class AssetsInstallationHelperTest {
4860
(failure.cause?.cause) is FileNotFoundException,
4961
)
5062
}
63+
64+
@Test
65+
fun `extractZipToDir creates parent directories for nested entries with no directory entries`() {
66+
val destDir = Files.createTempDirectory("extract-zip-to-dir-test")
67+
try {
68+
val content = "test notice content"
69+
val zipBytes =
70+
ByteArrayOutputStream().use { baos ->
71+
ZipOutputStream(baos).use { zos ->
72+
// No directory entries, matching how android-sdk.zip is packaged.
73+
zos.putNextEntry(ZipEntry("build-tools/35.0.0/NOTICE.txt"))
74+
zos.write(content.toByteArray())
75+
zos.closeEntry()
76+
}
77+
baos.toByteArray()
78+
}
79+
80+
AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir)
81+
82+
val extracted = destDir.resolve("build-tools/35.0.0/NOTICE.txt")
83+
assertTrue("Expected extracted file to exist", Files.exists(extracted))
84+
assertEquals(content, String(Files.readAllBytes(extracted)))
85+
} finally {
86+
destDir.toFile().deleteRecursively()
87+
}
88+
}
89+
90+
@Test
91+
fun `extractZipToDir rejects a file entry whose pre-existing symlinked grandparent escapes destDir`() {
92+
val destDir = Files.createTempDirectory("extract-zip-to-dir-test")
93+
val outsideDir = Files.createTempDirectory("extract-zip-to-dir-outside")
94+
try {
95+
Files.createSymbolicLink(destDir.resolve("linked"), outsideDir)
96+
97+
val content = "escaping content"
98+
val zipBytes =
99+
ByteArrayOutputStream().use { baos ->
100+
ZipOutputStream(baos).use { zos ->
101+
// Two levels below the symlink ("linked/sub/nested.txt", no directory
102+
// entries), not one: for a one-level entry ("linked/nested.txt"),
103+
// destFile.parent IS the symlink, so Files.createDirectories() throws
104+
// FileAlreadyExistsException (NOFOLLOW_LINKS rejects the existing
105+
// symlink-to-dir) before the toRealPath() guard below it ever runs. One
106+
// level deeper, createDirectories() silently traverses the symlink to
107+
// create "sub" for real inside outsideDir, and only then does the
108+
// toRealPath() check on destFile.parent fire -- which is what this test
109+
// exercises.
110+
zos.putNextEntry(ZipEntry("linked/sub/nested.txt"))
111+
zos.write(content.toByteArray())
112+
zos.closeEntry()
113+
}
114+
baos.toByteArray()
115+
}
116+
117+
assertThrows(IllegalStateException::class.java) {
118+
AssetsInstallationHelper.extractZipToDir(ByteArrayInputStream(zipBytes), destDir)
119+
}
120+
} finally {
121+
outsideDir.deleteRecursivelyWithoutFollowingLinks()
122+
destDir.deleteRecursivelyWithoutFollowingLinks()
123+
}
124+
}
125+
126+
// Deletes a directory tree without following symlinks it contains, unlike
127+
// File.deleteRecursively(). Files.walkFileTree() doesn't follow symlinks unless
128+
// FileVisitOption.FOLLOW_LINKS is passed (it isn't here), so a symlink is visited
129+
// as a leaf via visitFile() -- deleting it unlinks the link itself, never the
130+
// target it points to. Needed because the symlink test above symlinks out of destDir.
131+
private fun Path.deleteRecursivelyWithoutFollowingLinks() {
132+
Files.walkFileTree(
133+
this,
134+
object : SimpleFileVisitor<Path>() {
135+
override fun visitFile(
136+
file: Path,
137+
attrs: BasicFileAttributes,
138+
): FileVisitResult {
139+
Files.delete(file)
140+
return FileVisitResult.CONTINUE
141+
}
142+
143+
override fun postVisitDirectory(
144+
dir: Path,
145+
exc: IOException?,
146+
): FileVisitResult {
147+
Files.delete(dir)
148+
return FileVisitResult.CONTINUE
149+
}
150+
},
151+
)
152+
}
51153
}

0 commit comments

Comments
 (0)