mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
72fea93d7f
## Summary
The iOS native profiler reached `/bin/sh` by string-interpolating values
into shell-executed commands — the classic command-injection shape. This
PR removes the shell from **every** such call site in the iOS-profiler
subsystem, converting them to argv-array `execFileSync`/`execFile` so no
caller- or path-derived value is ever re-parsed by a shell.
### 1. `device_id` interpolation — gated behind the UDID-shape
classifier (`native-profiler/platforms/ios.ts`)
The profiler interpolated the caller-supplied `device_id` (`udid`)
directly into shell-interpreted `execSync` template strings:
- `execSync(`xcrun simctl spawn ${udid} launchctl list`)`
- `execSync(`xcrun simctl listapps ${udid} | plutil -convert json -o -
-`)`
`device_id` is declared `z.string()` with no format constraint and
`resolveDevice()` returns it unchanged. **However, this sink is not
directly reachable with a hostile `device_id`** (thanks @hubgan for the
catch): before the iOS platform module runs, `classifyDevice()`
(`utils/device-info.ts`) routes by *shape* against an anchored hex-UDID
regex
`^[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{12}$`.
Any `device_id` carrying a shell metacharacter fails that (anchored,
hex-only) match, classifies as **Android**, and takes the argv-based
`adb -s <serial> shell …` path — it never reaches this iOS function.
Conversely, any value that *does* classify as iOS is, by the regex,
metacharacter-free. So on `main` the original `execSync` was **dormant /
defense-in-depth**, gated behind the classifier invariant, not an
exploitable path from a tool parameter.
The conversion is still worthwhile: it removes the code's reliance on
that classifier invariant (if the shape regex were ever loosened, or the
sink reached by a future refactor with a synthetic `platform: "ios"`,
the shell would be back). Fixed by converting both call sites to
`execFileSync` arg-arrays — the `simctl listapps | plutil` pipe is split
into two in-process stages so nothing is shell-interpreted.
This anti-pattern previously had a fix on an old branch that was lost
when the code moved from
`tools/profiler/ios-profiler/ios-profiler-start.ts` to
`native-profiler/platforms/ios.ts`.
### 2. Trace-export hardening — same anti-pattern
(`utils/ios-profiler/export.ts`)
Six `xctrace export` / `xctrace version` commands were built as
`/bin/sh` strings via `execSync`, interpolating the trace-file and
output paths:
- `execSync(`xctrace export --input "${traceFile}" --output "${outPath}"
--xpath '...'`)` (×5, incl. the CPU/leaks/hangs exports and `--toc`
discovery)
- `execSync("xctrace version 2>&1 || true")`
`traceFile` is internally generated today (a timestamped path under the
debug dir, only ever passed in by `native-profiler/platforms/ios.ts`),
so this is **not currently reachable from a tool parameter** — but it is
the identical anti-pattern sitting in the same subsystem, and #1's
interpolation was likewise dormant. Converted the shared
`execSyncWithTimeout` helper to an argv-based `execFileAsyncWithTimeout`
(`execFile`, no shell) and pass every `xctrace` invocation as a discrete
argv array.
### 3. `xcodebuild` version probe — same shell-string shape
(`capture-strategy/select.ts`)
`readActiveXcodeVersion()` ran `execSync("xcodebuild -version")`. The
command is a constant with no interpolated input, so this is **not an
injection** — but it is the same `/bin/sh`-string shape this PR removes
everywhere else in the subsystem. Converted to
`execFileSync("xcodebuild", ["-version"])` so the iOS-profiler subsystem
is uniformly shell-free.
## Audit
Swept the whole tool-server for the same class (caller-supplied tool
parameter → shell-interpreted sink). **No other reachable injection
found:**
- Host commands all use argv `execFile`/`spawn` (`adb`, `xctrace`,
`emulator`, `simctl`) — serial / udid / avdName / bundleId are discrete
argv elements.
- Device-shell commands that interpolate a caller value (`open-url` URL,
`launch-app` / `restart-app` bundleId+component) are escaped through
`shellQuote()` / single-quote escaping.
- The `command -v ${dep}` PATH probes in `check-deps.ts` /
`android-binary.ts` interpolate only the closed `ToolDependency` enum
(`adb | xcrun | emulator`), never a request value.
## Buffering note (post-review)
Splitting the `simctl listapps | plutil` shell pipe into two in-process
`execFileSync` stages moved stdout buffering from the OS to Node, where
each stage inherited Node's 1 MiB default `maxBuffer` — a large
`listapps` could throw `ENOBUFS` (`NATIVE_PROFILER_APP_LIST_FAILED`)
where the pipe streamed unbounded. All three iOS app-enumeration
captures now pass the subsystem's `DEFAULT_EXEC_MAX_BUFFER` (256 MiB,
from `run-with-timeout.ts`), matching how the exporter already avoids
this class.
## Verification
- `npm run build` (tsc) ✓ · `typecheck:tests` ✓ · `prettier --check` ✓ ·
`eslint --max-warnings 0` ✓
- Regression tests assert hostile values are only ever discrete argv
elements, that no `execSync` is used in the subsystem, and that every
app-enumeration stage passes the raised `maxBuffer`. Reverting a fix
back to `execSync` (or dropping a `maxBuffer`) makes its test fail —
confirming each is a real guard.
61 lines
2.3 KiB
TypeScript
61 lines
2.3 KiB
TypeScript
import { describe, it, expect, vi } from "vitest";
|
|
|
|
// Records the options execFile is actually invoked with, so the test can assert
|
|
// the wrapper's timeout/maxBuffer guards cannot be silently weakened by a caller.
|
|
const rec = vi.hoisted(() => ({
|
|
calls: [] as { options?: { timeout?: number; maxBuffer?: number; encoding?: string } }[],
|
|
}));
|
|
|
|
vi.mock("child_process", () => ({
|
|
// promisify(execFile) calls this callback form: (file, args, options, cb).
|
|
execFile: (_file: string, _args: string[], ...rest: unknown[]) => {
|
|
const options = rest.find((r) => typeof r === "object" && r !== null) as
|
|
| { timeout?: number; maxBuffer?: number; encoding?: string }
|
|
| undefined;
|
|
rec.calls.push({ options });
|
|
const cb = rest.find((r) => typeof r === "function") as
|
|
| ((e: unknown, r: { stdout: string; stderr: string }) => void)
|
|
| undefined;
|
|
cb?.(null, { stdout: "", stderr: "" });
|
|
return undefined;
|
|
},
|
|
}));
|
|
|
|
import {
|
|
execFileAsyncWithTimeout,
|
|
DEFAULT_EXEC_MAX_BUFFER,
|
|
DEFAULT_EXEC_TIMEOUT_MS,
|
|
} from "../src/utils/ios-profiler/run-with-timeout";
|
|
|
|
describe("execFileAsyncWithTimeout: safety guards can't be silently weakened", () => {
|
|
it("applies the timeout and 256 MiB maxBuffer when the caller passes no options", async () => {
|
|
rec.calls.length = 0;
|
|
await execFileAsyncWithTimeout("xctrace", ["version"]);
|
|
|
|
expect(rec.calls).toHaveLength(1);
|
|
expect(rec.calls[0].options?.timeout).toBe(DEFAULT_EXEC_TIMEOUT_MS);
|
|
expect(rec.calls[0].options?.maxBuffer).toBe(DEFAULT_EXEC_MAX_BUFFER);
|
|
});
|
|
|
|
it("ignores a caller trying to shrink maxBuffer or drop the timeout", async () => {
|
|
rec.calls.length = 0;
|
|
// A caller that passed these would otherwise re-open the ENOBUFS /
|
|
// event-loop-freeze holes the wrapper exists to close.
|
|
await execFileAsyncWithTimeout("xctrace", ["version"], {
|
|
maxBuffer: 1024,
|
|
timeout: 0,
|
|
});
|
|
|
|
expect(rec.calls[0].options?.timeout).toBe(DEFAULT_EXEC_TIMEOUT_MS);
|
|
expect(rec.calls[0].options?.maxBuffer).toBe(DEFAULT_EXEC_MAX_BUFFER);
|
|
});
|
|
|
|
it("lets a caller RAISE maxBuffer above the guard (floor, not a cap)", async () => {
|
|
rec.calls.length = 0;
|
|
const bigger = 512 * 1024 * 1024;
|
|
await execFileAsyncWithTimeout("xctrace", ["version"], { maxBuffer: bigger });
|
|
|
|
expect(rec.calls[0].options?.maxBuffer).toBe(bigger);
|
|
});
|
|
});
|