From f7cd4e5256441f69ddb7a3e78167b200460a6601 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Thu, 29 Oct 2026 16:04:49 +0800 Subject: [PATCH] fix(adb): validate screen capture PNG headers (#7962) * fix(adb): validate screen capture PNG headers * fix(adb): state screen header validation limits * style(adb): format screen capture assertion Match the current formatter after integrating main. Co-authored-by: morluto --------- Co-authored-by: Cursor Agent Co-authored-by: morluto --- docs/adb-device-analysis.md | 4 + src/adb/AdbProvider.test.ts | 17 +++- src/adb/AdbProvider.ts | 9 +- src/adb/AdbShellOutput.ts | 23 ----- tests/boundary/cli/adbCli.test.ts | 91 ++++++++++++++++++- .../adbScreenPublicationCancellation.test.ts | 2 +- .../adb/adbSelectedLocalPaths.test.ts | 7 +- 7 files changed, 116 insertions(+), 37 deletions(-) diff --git a/docs/adb-device-analysis.md b/docs/adb-device-analysis.md index 1df083c5c7..634a004b0a 100644 --- a/docs/adb-device-analysis.md +++ b/docs/adb-device-analysis.md @@ -103,6 +103,10 @@ existing analysis families directly, for example | `start_adb_activity` | **mutates device** | Starts an intent (`am start -a` with optional URI, component, and up to 16 typed extras); preserves the activity manager's verdict | | `stop_adb_app` | **mutates device** | Force-stops every process of a package, ending a dynamic-analysis run | +Screen capture validates the PNG signature, IHDR framing, dimensions, and IHDR +checksum before publishing. It does not validate encoding fields or decode and +validate image contents; that limitation is retained in Evidence. + When `push_adb_file` receives an existing directory, adb places the local file inside it under the local basename; Evidence reports that effective file path while retaining the caller's directory path in the input parameters. The diff --git a/src/adb/AdbProvider.test.ts b/src/adb/AdbProvider.test.ts index 1bccde00bc..b5e6b77e7c 100644 --- a/src/adb/AdbProvider.test.ts +++ b/src/adb/AdbProvider.test.ts @@ -38,6 +38,11 @@ import type { AdbRequest } from "../domain/adb/adbDeviceAnalysis.js"; const it = test.skipIf(process.platform === "win32"); +const SCREEN_PNG = Buffer.from( + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAIAAACQd1PeAAAADElEQVR4nGP4z8AAAAMBAQDJ/pLvAAAAAElFTkSuQmCC", + "base64", +); + const VERSION_BLOCK = "printf 'Android Debug Bridge version 1.0.41\\nVersion 34.0.5-stub\\nInstalled as %s\\n' \"$0\""; @@ -1453,19 +1458,20 @@ exit 1`, it("captures the screen as a digested PNG with its dimensions", async () => { const { binary, root } = await writeAdbStub( - () => `if [ "$1" = "version" ]; then ${VERSION_BLOCK}; exit 0; fi + (root) => `if [ "$1" = "version" ]; then ${VERSION_BLOCK}; exit 0; fi if [ "$1" = "-s" ] && [ "$3" = "exec-out" ]; then - printf '\\211PNG\\r\\n\\032\\n\\0\\0\\0\\rIHDR\\0\\0\\0\\004\\0\\0\\0\\003' + cat "${join(root, "screen-fixture.png")}" exit 0 fi exit 1`, ); + await writeFile(join(root, "screen-fixture.png"), SCREEN_PNG); const output = join(root, "captures"); const result = await evidenceResult(providerOver(binary), { operation: "capture_adb_screen", input: { serial: "emulator-5554", output_directory: output }, }); - expect(result).toMatchObject({ width: 4, height: 3 }); + expect(result).toMatchObject({ width: 1, height: 1 }); expect( await readFile(join(output, "screen.png")).then( (bytes) => bytes.subarray(1, 4).toString("latin1") === "PNG", @@ -1476,13 +1482,14 @@ exit 1`, it("stops a screen capture aborted after adb returns but before publication", async () => { const { binary, root } = await writeAdbStub( - () => `if [ "$1" = "version" ]; then ${VERSION_BLOCK}; exit 0; fi + (root) => `if [ "$1" = "version" ]; then ${VERSION_BLOCK}; exit 0; fi if [ "$1" = "-s" ] && [ "$3" = "exec-out" ]; then - printf '\\211PNG\\r\\n\\032\\n\\0\\0\\0\\rIHDR\\0\\0\\0\\004\\0\\0\\0\\003' + cat "${join(root, "screen-fixture.png")}" exit 0 fi exit 1`, ); + await writeFile(join(root, "screen-fixture.png"), SCREEN_PNG); const output = join(root, "cancelled-capture"); const controller = new AbortController(); adbTestHooks.afterScreencap = () => controller.abort(); diff --git a/src/adb/AdbProvider.ts b/src/adb/AdbProvider.ts index 37eaac339c..c5a5ace10b 100644 --- a/src/adb/AdbProvider.ts +++ b/src/adb/AdbProvider.ts @@ -66,6 +66,7 @@ import { } from "../domain/analysisErrorCore.js"; import { AnalysisError } from "../domain/analysisErrorBase.js"; import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js"; +import { readPngHeaderDimensions } from "../domain/pngHeader.js"; import { err, ok, type Result } from "../domain/result.js"; import type { JsonValue } from "../domain/jsonValue.js"; import { jsonObjectSchema } from "../domain/jsonValue.js"; @@ -132,7 +133,6 @@ import { parseFeatureListOutput, parsePackageChangeOutput, parseProcessListOutput, - parsePngDimensions, parseResolveActivityOutput, parseServiceListOutput, parseSettingsGetOutput, @@ -3114,11 +3114,11 @@ export class AdbProvider { return err( new AnalysisCancelledError(operation, { cause: signal.reason }), ); - const dimensions = parsePngDimensions(captured.value.stdout); - if (dimensions.width === null) + const dimensions = readPngHeaderDimensions(captured.value.stdout); + if (dimensions === undefined) return err( new AnalysisProtocolError( - "screencap did not return a PNG payload; the device may not expose a display", + "screencap returned an invalid PNG header; the device may not expose a display", ), ); return this.#withRuntimeRoot( @@ -3182,6 +3182,7 @@ export class AdbProvider { { limitations: [ "The capture is one frame at read time; no recording or interaction occurred", + "PNG signature, IHDR framing, checksum, and dimensions are validated; encoding and image contents are not validated", ...(usedExclusiveFallback ? [ "The exclusive-create fallback makes the output path visible while its bytes are being written", diff --git a/src/adb/AdbShellOutput.ts b/src/adb/AdbShellOutput.ts index ed35d975a9..bca37332b2 100644 --- a/src/adb/AdbShellOutput.ts +++ b/src/adb/AdbShellOutput.ts @@ -609,26 +609,3 @@ export const parseAmStartOutput = ( (text === "" ? "am start returned no activity" : text.slice(0, 512)), }; }; - -const PNG_SIGNATURE = Buffer.from([ - 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, -]); - -/** Read image dimensions from a PNG header, or null for other payloads. */ -export const parsePngDimensions = ( - bytes: Uint8Array, -): { - width: number | null; - height: number | null; -} => { - if ( - bytes.length < 24 || - !Buffer.from(bytes.subarray(0, 8)).equals(PNG_SIGNATURE) - ) - return { width: null, height: null }; - const view = new DataView(bytes.buffer, bytes.byteOffset, bytes.byteLength); - return { - width: view.getUint32(16), - height: view.getUint32(20), - }; -}; diff --git a/tests/boundary/cli/adbCli.test.ts b/tests/boundary/cli/adbCli.test.ts index 877d4e39d3..23fccc5593 100644 --- a/tests/boundary/cli/adbCli.test.ts +++ b/tests/boundary/cli/adbCli.test.ts @@ -1,10 +1,12 @@ -import { chmod, lstat, readFile, unlink } from "node:fs/promises"; +import { chmod, lstat, readFile, unlink, writeFile } from "node:fs/promises"; import { createHash } from "node:crypto"; +import { crc32 } from "node:zlib"; import { basename, dirname, join } from "node:path"; import { expect, it, onTestFinished } from "vitest"; import { z } from "zod"; import { toolContract } from "../../../src/contracts/toolContracts.js"; +import { adbResultSchemas } from "../../../src/domain/adb/adbDeviceAnalysis.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { execFileOutput, @@ -19,6 +21,93 @@ import { removeTestWorkspace, } from "../../support/workspace/workspaceFixture.js"; +it + .skipIf(process.platform === "win32") + .each([ + "zero width", + "zero height", + "invalid chunk type", + "invalid checksum", + "truncated header", + "valid", + ] as const)( + "validates the screencap PNG header before publishing: %s", + async (kind) => { + const workspace = await createTestWorkspace("rea-adb-screen-cli-"); + onTestFinished(() => removeTestWorkspace(workspace.root)); + const png = Buffer.from( + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAIAAACQd1PeAAAADElEQVR4nGP4z8AAAAMBAQDJ/pLvAAAAAElFTkSuQmCC", + "base64", + ); + if (kind === "zero width") png.writeUInt32BE(0, 16); + if (kind === "zero height") png.writeUInt32BE(0, 20); + if (kind === "invalid chunk type") png[12] = 0xc9; + if (kind !== "invalid checksum") + png.writeUInt32BE(crc32(png.subarray(12, 29)), 29); + else png[29] = png[29]! ^ 0xff; + const frame = kind === "truncated header" ? png.subarray(0, 24) : png; + await writeFile(workspace.path("frame.png"), frame); + const adb = await workspace.write( + "adb", + `#!/bin/sh +if [ "$1" = version ]; then printf 'Android Debug Bridge version 1.0.41\\nVersion 34.0.5-cli\\n'; exit 0; fi +if [ "$1" = -s ] && [ "$3" = exec-out ]; then /bin/cat "$(dirname "$0")/frame.png"; exit 0; fi +exit 1 +`, + ); + await chmod(adb, 0o755); + const outputPath = workspace.path("captures"); + const result = await execFileOutput( + process.execPath, + [ + "scripts/rea.mjs", + "capture-adb-screen", + "device", + "--output-directory", + outputPath, + "--json", + ], + { + env: { ...process.env, REA_ADB_PATH: adb }, + timeout: 20_000, + stopSignal: "SIGTERM", + maxBuffer: 1024 * 1024, + }, + ).then( + (output) => ({ exitCode: 0, stdout: output.stdout }), + (cause: unknown) => { + const failure = execFileOutputFailure(cause); + if (failure === undefined) throw cause; + return { exitCode: failure.code, stdout: failure.stdout }; + }, + ); + if (kind === "valid") { + expect(result.exitCode).toBe(0); + const output = JSON.parse(result.stdout) as { + normalized_result: unknown; + limitations: string[]; + }; + expect( + adbResultSchemas.capture_adb_screen.parse(output.normalized_result), + ).toMatchObject({ width: 1, height: 1 }); + expect(output.limitations).toContain( + "PNG signature, IHDR framing, checksum, and dimensions are validated; encoding and image contents are not validated", + ); + expect( + (await readFile(join(outputPath, "screen.png"))).equals(frame), + ).toBe(true); + } else { + expect(result.exitCode).toBe(1); + expect(JSON.parse(result.stdout)).toMatchObject({ + code: "unreadable_output", + }); + await expect( + readFile(join(outputPath, "screen.png")), + ).rejects.toMatchObject({ code: "ENOENT" }); + } + }, +); + it.skipIf(process.platform === "win32").each([ { exit: 1, stdout: "", stderr: "adb: error: closed", overwrite: false }, { exit: 1, stdout: "", stderr: "adb: error: closed", overwrite: true }, diff --git a/tests/boundary/providers/adb/adbScreenPublicationCancellation.test.ts b/tests/boundary/providers/adb/adbScreenPublicationCancellation.test.ts index f8e018cb6e..e3b87cfa02 100644 --- a/tests/boundary/providers/adb/adbScreenPublicationCancellation.test.ts +++ b/tests/boundary/providers/adb/adbScreenPublicationCancellation.test.ts @@ -50,7 +50,7 @@ const fixture = async () => { root, provider, png: Buffer.from( - "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+jv4sAAAAASUVORK5CYII=", + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAIAAACQd1PeAAAADElEQVR4nGP4z8AAAAMBAQDJ/pLvAAAAAElFTkSuQmCC", "base64", ), }; diff --git a/tests/boundary/providers/adb/adbSelectedLocalPaths.test.ts b/tests/boundary/providers/adb/adbSelectedLocalPaths.test.ts index 4aa51dfad9..79c453e2b3 100644 --- a/tests/boundary/providers/adb/adbSelectedLocalPaths.test.ts +++ b/tests/boundary/providers/adb/adbSelectedLocalPaths.test.ts @@ -20,8 +20,8 @@ const sourceBytes = "selected native bytes"; const pulledBytes = "pulled fixture"; const bugreportBytes = "bugreport fixture"; const png = Buffer.from( - "89504e470d0a1a0a0000000d494844520000000400000003", - "hex", + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAIAAACQd1PeAAAADElEQVR4nGP4z8AAAAMBAQDJ/pLvAAAAAElFTkSuQmCC", + "base64", ); const digest = (bytes: string | Buffer) => createHash("sha256").update(bytes).digest("hex"); @@ -97,6 +97,7 @@ const fixture = async () => { await mkdir(join(physical, "nested"), { recursive: true }); await symlink(join(physical, "nested"), join(root, "alias")); const binary = join(root, "adb"); + await writeFile(join(root, "screen-fixture.png"), png); const quote = (value: string) => `'${value.replaceAll("'", "'\\''")}'`; await writeFile( binary, @@ -110,7 +111,7 @@ if [ "$3" = push ] || [ "$3" = install ]; then printf 'Success\\n'; exit 0 fi if [ "$3" = pull ]; then printf '${pulledBytes}' > "$5"; exit 0; fi -if [ "$3" = exec-out ]; then printf '\\211PNG\\r\\n\\032\\n\\0\\0\\0\\rIHDR\\0\\0\\0\\004\\0\\0\\0\\003'; exit 0; fi +if [ "$3" = exec-out ]; then /bin/cat ${quote(join(root, "screen-fixture.png"))}; exit 0; fi if [ "$3" = bugreport ]; then printf '${bugreportBytes}' > "$4/bugreport.zip" printf 'Bug report copied to %s/bugreport.zip\\n' "$4"; exit 0