From e15c023204e36989ad8c46778ea98cd2d7075dab Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 2 Sep 2026 05:11:09 -0500 Subject: [PATCH 01/16] fix(scenario-verify): withhold recorded_replay pending isolation evidence-boundary repair External review of the merged tree (ab415be6c) found the evidence boundary scenario-verify rests recorded_replay on was itself unproven: the unshare/ bwrap launcher binaries are resolved through inherited PATH (a fake launcher can be selected), and the unshare mechanism's --rbind submounts only get their top mount remounted read-only, leaving nested mounts under a ro bind writable. Either gap lets recorded_replay: PASS print without the OS-level isolation that claim asserts. Add isolationEvidenceBoundaryProven to evaluateClaimEligibility's inputs, gating recorded_replay on top of namespace-activity alone, and wire it to a hardcoded false in scenario-verify.ts so every intermediate state of this repair stays honest (diagnostic_replay only) until the trusted-launcher and recursive-read-only fixes land and this literal is flipped to a real proof. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../bin/scenario-verify-strict.test.ts | 66 +++++++++++++++++++ .../bin/scenario-verify.ts | 10 +++ .../src/scenario/claims.ts | 25 +++++++ 3 files changed, 101 insertions(+) diff --git a/packages/polyfill-connectors/bin/scenario-verify-strict.test.ts b/packages/polyfill-connectors/bin/scenario-verify-strict.test.ts index eadfbdc70..d4a1d298d 100644 --- a/packages/polyfill-connectors/bin/scenario-verify-strict.test.ts +++ b/packages/polyfill-connectors/bin/scenario-verify-strict.test.ts @@ -857,6 +857,7 @@ function eligibleDigestObservations(): { currentSourceDigestComputed: boolean; observedUnsupportedEvidenceSurface: boolean; driverEvidenceSatisfied: boolean; + isolationEvidenceBoundaryProven: boolean; } { return { capturedDeclarationDigestPresent: true, @@ -865,6 +866,7 @@ function eligibleDigestObservations(): { currentSourceDigestComputed: true, observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, + isolationEvidenceBoundaryProven: true, }; } @@ -954,6 +956,7 @@ test("evaluateClaimEligibility negative control: source-only historical scenario currentSourceDigestComputed: true, observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, + isolationEvidenceBoundaryProven: true, isNamespaceIsolationActive: true, }); assert.equal(decision.claim, "diagnostic_replay"); @@ -971,6 +974,7 @@ test("evaluateClaimEligibility negative control: declaration-only scenario (sour currentSourceDigestComputed: true, observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, + isolationEvidenceBoundaryProven: true, isNamespaceIsolationActive: true, }); assert.equal(decision.claim, "diagnostic_replay"); @@ -988,6 +992,7 @@ test("evaluateClaimEligibility negative control: missing current manifest (decla currentSourceDigestComputed: true, observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, + isolationEvidenceBoundaryProven: true, isNamespaceIsolationActive: true, }); assert.equal(decision.claim, "diagnostic_replay"); @@ -1005,6 +1010,7 @@ test("evaluateClaimEligibility negative control: missing current connector sourc currentSourceDigestComputed: false, observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, + isolationEvidenceBoundaryProven: true, isNamespaceIsolationActive: true, }); assert.equal(decision.claim, "diagnostic_replay"); @@ -1029,6 +1035,7 @@ test("evaluateClaimEligibility negative control: legacy top-level digests only ( currentSourceDigestComputed: true, observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, + isolationEvidenceBoundaryProven: true, isNamespaceIsolationActive: true, }); assert.equal(decision.claim, "diagnostic_replay"); @@ -1111,6 +1118,7 @@ test("evaluateClaimEligibility: multiple failing conditions are all reported at observedUnsupportedEvidenceSurface: true, driverEvidenceSatisfied: false, isNamespaceIsolationActive: false, + isolationEvidenceBoundaryProven: false, }); assert.equal(decision.claim, "diagnostic_replay"); assert.ok(decision.claim === "diagnostic_replay"); @@ -1152,6 +1160,64 @@ test("evaluateClaimEligibility: with every other condition eligible, the claim t } }); +// ─── Bounded P1 repair (external review of ab415be6c): isolation evidence +// boundary — launcher trust + recursive read-only, on TOP of namespace +// activity alone ──────────────────────────────────────────────────────────── +// +// The review found that `isNamespaceIsolationActive: true` (the OS +// namespaces genuinely exist) was being treated as sufficient for +// `recorded_replay`, even though two separate defects meant the FILESYSTEM +// half of that isolation could be unproven: the `unshare`/`bwrap` launcher +// binaries were resolved through the caller's inherited `$PATH` (a +// PATH-prepended fake launcher could be selected), and the unshare +// mechanism's `--rbind` submounts only had their top mount remounted +// read-only, leaving nested mounts under a `ro` bind writable. These tests +// pin that namespace-active alone can never reach `recorded_replay` — the +// new `isolationEvidenceBoundaryProven` observation must ALSO be true. + +test("evaluateClaimEligibility: namespace isolation active but isolationEvidenceBoundaryProven false withholds recorded_replay (launcher trust / recursive-ro not proven)", () => { + const decision = evaluateClaimEligibility({ + scenario: eligibleScenario(), + isEntrypointOverride: false, + ...eligibleDigestObservations(), + isNamespaceIsolationActive: true, + isolationEvidenceBoundaryProven: false, + }); + assert.equal(decision.claim, "diagnostic_replay"); + assert.ok(decision.claim === "diagnostic_replay"); + assert.deepEqual(decision.limitations, [ + "network isolation: launcher trust or recursive read-only filesystem closure not proven for this run", + ]); +}); + +test("evaluateClaimEligibility: namespace isolation active AND isolationEvidenceBoundaryProven true — recorded_replay is reachable again", () => { + const decision = evaluateClaimEligibility({ + scenario: eligibleScenario(), + isEntrypointOverride: false, + ...eligibleDigestObservations(), + isNamespaceIsolationActive: true, + isolationEvidenceBoundaryProven: true, + }); + assert.deepEqual(decision, { claim: "recorded_replay" }); +}); + +test("evaluateClaimEligibility: namespace isolation NOT active reports only the coarser process-local limitation, never BOTH isolation limitations at once", () => { + // When isolation isn't active at all, the boundary-proof limitation is + // redundant with (and would be confusing alongside) the coarser + // process-local-only limitation — evaluateClaimEligibility's `else if` + // must report exactly one of the two, never both. + const decision = evaluateClaimEligibility({ + scenario: eligibleScenario(), + isEntrypointOverride: false, + ...eligibleDigestObservations(), + isNamespaceIsolationActive: false, + isolationEvidenceBoundaryProven: false, + }); + assert.equal(decision.claim, "diagnostic_replay"); + assert.ok(decision.claim === "diagnostic_replay"); + assert.deepEqual(decision.limitations, ["network isolation: process-local only - descendant escape not excluded"]); +}); + // ─── Repair wave 6, P1-1: driver-evidence prerequisite ───────────────────── // // `wire-registry.ts`'s `DRIVER_EVIDENCE_POLICIES` map — `recorded-http`'s diff --git a/packages/polyfill-connectors/bin/scenario-verify.ts b/packages/polyfill-connectors/bin/scenario-verify.ts index 8a38b2562..a19dd4ba3 100644 --- a/packages/polyfill-connectors/bin/scenario-verify.ts +++ b/packages/polyfill-connectors/bin/scenario-verify.ts @@ -1844,6 +1844,16 @@ function printCoverageReport( currentDeclarationDigestComputed: digestObservation.currentDeclarationDigestComputed, currentSourceDigestComputed: digestObservation.currentSourceDigestComputed, isNamespaceIsolationActive: isolationCapability.available, + // WITHHELD PENDING BOUNDED P1 REPAIR (external review of ab415be6c): + // hardcoded false until the trusted-launcher resolution and the + // recursive-read-only post-pivot verification are both wired in and + // proven — see claims.ts's `isolationEvidenceBoundaryProven` doc comment. + // Every intermediate state of this repair must stay honest: a build that + // has the launcher-trust or recursive-ro fix applied only partially must + // still print `diagnostic_replay`, never `recorded_replay`, until this + // literal is flipped to the real, wired-in proof in the commit that + // completes both fixes. + isolationEvidenceBoundaryProven: false, observedUnsupportedEvidenceSurface: observedUnsupportedEvidenceSurfaceFlag, driverEvidenceSatisfied: driverEvidenceOk, }); diff --git a/packages/polyfill-connectors/src/scenario/claims.ts b/packages/polyfill-connectors/src/scenario/claims.ts index e6a68a7a6..4e0ff2201 100644 --- a/packages/polyfill-connectors/src/scenario/claims.ts +++ b/packages/polyfill-connectors/src/scenario/claims.ts @@ -100,6 +100,7 @@ export type ClaimLimitation = | "non-recorded-http driver - canonical replay is defined only for recorded-http" | "legacy scenario without protocol trace" | "network isolation: process-local only - descendant escape not excluded" + | "network isolation: launcher trust or recursive read-only filesystem closure not proven for this run" | "connector exercised an evidence surface the oracle cannot observe (ASSISTANCE)" | "no recorded provider interaction - driver evidence for recorded-http not satisfied" | "no recorded HAR entries - driver evidence for recorded-browser not satisfied" @@ -166,6 +167,26 @@ export interface ClaimEligibilityInput { * `isNamespaceIsolationAvailable()`) was ACTIVE for this replay, as * opposed to the weaker process-local-only fallback (condition f). */ isNamespaceIsolationActive: boolean; + /** + * External review of the merged tree (ab415be6c) found the evidence + * boundary this claim rests on was itself unproven in two ways: (1) the + * `unshare`/`bwrap` launcher binaries were resolved through the CALLER's + * inherited `$PATH` rather than a trusted absolute path, so a + * PATH-prepended fake launcher could be selected in place of the real one; + * (2) the unshare mechanism's `--rbind` submounts only had their TOP mount + * remounted read-only — Linux does not apply `remount,ro,bind` recursively + * — so a nested mount under a `ro` bind (e.g. `REPO_ROOT`) stayed writable. + * Either defect lets `isNamespaceIsolationActive` read `true` (namespaces + * genuinely exist) while the filesystem/launcher half of the OS-isolation + * claim does not actually hold. `isNamespaceIsolationActive` alone is no + * longer sufficient for `recorded_replay`: this field must ALSO be true, + * set by `bin/scenario-verify.ts` only once both the trusted-launcher + * resolution (isolation.ts's `resolveTrustedLauncherPath`) and the + * recursive-read-only post-pivot verification + * (`postPivotVerificationStatements`'s per-submount check) are wired in and + * this replay's own isolated child was verified against them. + */ + isolationEvidenceBoundaryProven: boolean; /** Repair wave 4 (P1-2, FIX 2d): true when this run's messages included at * least one kind `TRACE_POLICY` (verify.ts) dispositions * `"unsupported_claim_withheld"` — today, ASSISTANCE or ASSISTANCE_STATUS. @@ -250,6 +271,10 @@ export function evaluateClaimEligibility(input: ClaimEligibilityInput): ClaimDec } if (!input.isNamespaceIsolationActive) { limitations.push("network isolation: process-local only - descendant escape not excluded"); + } else if (!input.isolationEvidenceBoundaryProven) { + limitations.push( + "network isolation: launcher trust or recursive read-only filesystem closure not proven for this run" + ); } if (input.observedUnsupportedEvidenceSurface) { limitations.push("connector exercised an evidence surface the oracle cannot observe (ASSISTANCE)"); From af22b479ebc4d57798f1019f6db9d2042981dc0d Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 2 Sep 2026 05:19:48 -0500 Subject: [PATCH 02/16] fix(isolation): resolve unshare/bwrap launchers from a trusted absolute path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit External review of ab415be6c found the unshare/bwrap launcher binaries were resolved through the caller's inherited $PATH during both the capability probe and real execution, so a PATH-prepended fake launcher was selected over the real one — the project's own PATH-shadowing test infrastructure demonstrated exactly this. Add resolveTrustedLauncherPath, which walks a fixed allowlist of trusted system directories (/usr/sbin, /usr/bin, /sbin, /bin — the same set TRUSTED_SETUP_PATH already uses for the isolated child's own setup commands), never the inherited PATH, and use the one resolved path for both probeUnshare/probeBwrap and the real spawnWithNetworkIsolation calls. Fails closed (throws, never falls back to a bare name) when a trusted directory doesn't have the binary. Existing tests that PATH-shadowed unshare/bwrap to inject fakes no longer reach anything once this lands (proving the fix), so they're rewritten to bind-mount their shims directly over the real trusted-path binaries instead. Added a new end-to-end test proving a PATH-prepended fake `unshare` is never selected by either the probe or a real isolated spawn. Full isolation-mechanism suite verified under both mechanisms in a privileged container (bwrap native, unshare via container): 52 pass, 0 fail, 10 skipped (pre-existing $HOME/.ssh/agent and /run/user/ fixture preconditions, unrelated to this fix). Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../src/scenario/isolation-mechanism.test.ts | 586 ++++++++++-------- .../src/scenario/isolation.ts | 133 +++- 2 files changed, 471 insertions(+), 248 deletions(-) diff --git a/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts b/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts index 2e64052fa..d9b371e13 100644 --- a/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts +++ b/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts @@ -21,6 +21,7 @@ import { isNamespaceIsolationAvailable, postPivotVerificationStatements, requiredFilesystemBinds, + resolveTrustedLauncherPath, spawnWithNetworkIsolation, } from "./isolation.ts"; @@ -33,6 +34,37 @@ const unshareUsable = process.platform === "linux" && spawnSync("unshare", ["-r", "-n", "-m", "true"], { stdio: "ignore", timeout: 5000 }).status === 0; +/** True when this test process can actually shadow a real trusted-path + * binary via a host-level bind mount (root or an equivalent capability) — + * the injection mechanism the test below needs now that the prelude + * resolves its setup commands through a fixed `TRUSTED_SETUP_PATH` + * (P1-1, ninth review) rather than the caller's inherited `$PATH`. Probed + * by attempting the exact bind-then-unbind sequence the real test performs, + * against a scratch file, so the skip condition matches the test's actual + * requirement rather than a proxy for it (e.g. `process.getuid() === 0` + * would be wrong inside a rootless-but-capable container). */ +function canBindMountOverAFile(): boolean { + if (process.platform !== "linux") { + return false; + } + const probeSource = mkdtempSync(join(tmpdir(), "pdpp-bindmount-probe-src-")); + const probeTarget = mkdtempSync(join(tmpdir(), "pdpp-bindmount-probe-dst-")); + const srcFile = join(probeSource, "a"); + const dstFile = join(probeTarget, "b"); + writeFileSync(srcFile, ""); + writeFileSync(dstFile, ""); + const bound = spawnSync("mount", ["--bind", srcFile, dstFile], { stdio: "ignore" }).status === 0; + if (bound) { + spawnSync("umount", [dstFile], { stdio: "ignore" }); + } + rmSync(probeSource, { recursive: true, force: true }); + rmSync(probeTarget, { recursive: true, force: true }); + return bound; +} + +const bindMountCapable = unshareUsable && canBindMountOverAFile(); + + test("a host that denies `unshare` but ships a working bwrap still reports isolation AVAILABLE", { skip: !bwrapUsable, }, () => { @@ -84,56 +116,60 @@ test("an isolated child has NO outbound network — the property, not the mechan // use) rather than trusted by reading the source, so a future edit that // reverts to a bare/simplified probe argv is caught mechanically. +// INJECTION MECHANISM (P1, external review of ab415be6c — trusted launcher +// resolution): this test used to shadow `bwrap` via a PATH-prepended shim +// directory. Now that the probe resolves the launcher through +// `resolveTrustedLauncherPath` (a fixed allowlist of trusted directories, +// never the caller's inherited `$PATH`), a PATH-prepended shim is no longer +// selected — proving the fix works, but also meaning a test that still +// relied on PATH-shadowing to intercept the launcher would silently stop +// exercising anything. The injection is done the only way that still +// reaches a trusted-path binary: bind-mounting the logging shim DIRECTLY +// OVER the real trusted-path `bwrap` binary for the duration of the test, +// via `withShimmedTrustedBinary` (defined below — a hoisted function +// declaration, callable here despite the later textual position). test("[bwrap] the fixed probeBwrap() invokes bwrap with the SAME production argv shape bwrapArgvForFilesystemClosure builds — not a bare --dev-bind / / check", { - skip: !bwrapUsable, -}, () => { + skip: !bwrapUsable || !bindMountCapable, +}, async () => { const logDir = mkdtempSync(join(tmpdir(), "pdpp-isolation-probe-argv-log-")); const logPath = join(logDir, "invocations.log"); writeFileSync(logPath, ""); - // A real, delegating shim (not a bare "exit 0") — the probe's own - // production-equivalence is only meaningful if the shim still actually - // RUNS bwrap for real (so a genuinely broken derived-bind shape would - // still be caught), it just additionally logs the argv it was given. - const realBwrapPath = spawnSync("which", ["bwrap"], { encoding: "utf8" }).stdout.trim(); - const shimDir = mkdtempSync(join(tmpdir(), "pdpp-isolation-probe-argv-shim-")); - writeFileSync( - join(shimDir, "bwrap"), - ["#!/bin/sh", `echo "$*" >> ${JSON.stringify(logPath)}`, `exec ${realBwrapPath} "$@"`].join("\n"), - { mode: 0o755 } - ); - const realPath = process.env.PATH; - process.env.PATH = `${shimDir}:${realPath ?? ""}`; try { - const cap = isNamespaceIsolationAvailable(); - // Only meaningful when bwrap is genuinely what got selected (on a host - // where unshare is denied but bwrap works — this suite's own - // AppArmor-restricted dev sandbox is exactly that shape); skip the argv - // assertion (but still ran the probe) otherwise. - const invocations = readFileSync(logPath, "utf8").trim().split("\n").filter(Boolean); - if (cap.available && cap.mechanism === "bwrap") { - assert.equal( - invocations.length, - 1, - `expected exactly one bwrap probe invocation; got ${JSON.stringify(invocations)}` - ); - const probeArgv = invocations[0] ?? ""; - assert.ok( - probeArgv.includes("--tmpfs") && !probeArgv.includes("--dev-bind"), - `probe argv must use the empty --tmpfs / root, never --dev-bind / /; got ${JSON.stringify(probeArgv)}` - ); - assert.ok( - probeArgv.includes("--unshare-pid"), - `probe argv must include --unshare-pid, matching production's derived closure; got ${JSON.stringify(probeArgv)}` - ); - assert.ok( - probeArgv.includes("--ro-bind") || probeArgv.includes("--bind"), - `probe argv must include the derived requiredFilesystemBinds() entries, not just namespace flags; got ${JSON.stringify(probeArgv)}` - ); - } + await withShimmedTrustedBinary( + "bwrap", + (realBwrapPath) => + ["#!/bin/sh", `echo "$*" >> ${JSON.stringify(logPath)}`, `exec ${realBwrapPath} "$@"`].join("\n"), + async () => { + const cap = isNamespaceIsolationAvailable(); + // Only meaningful when bwrap is genuinely what got selected (on a + // host where unshare is denied but bwrap works — this suite's own + // AppArmor-restricted dev sandbox is exactly that shape); skip the + // argv assertion (but still ran the probe) otherwise. + const invocations = readFileSync(logPath, "utf8").trim().split("\n").filter(Boolean); + if (cap.available && cap.mechanism === "bwrap") { + assert.equal( + invocations.length, + 1, + `expected exactly one bwrap probe invocation; got ${JSON.stringify(invocations)}` + ); + const probeArgv = invocations[0] ?? ""; + assert.ok( + probeArgv.includes("--tmpfs") && !probeArgv.includes("--dev-bind"), + `probe argv must use the empty --tmpfs / root, never --dev-bind / /; got ${JSON.stringify(probeArgv)}` + ); + assert.ok( + probeArgv.includes("--unshare-pid"), + `probe argv must include --unshare-pid, matching production's derived closure; got ${JSON.stringify(probeArgv)}` + ); + assert.ok( + probeArgv.includes("--ro-bind") || probeArgv.includes("--bind"), + `probe argv must include the derived requiredFilesystemBinds() entries, not just namespace flags; got ${JSON.stringify(probeArgv)}` + ); + } + } + ); } finally { - process.env.PATH = realPath; rmSync(logDir, { recursive: true, force: true }); - rmSync(shimDir, { recursive: true, force: true }); } }); @@ -157,141 +193,101 @@ test("[bwrap] the fixed probeBwrap() invokes bwrap with the SAME production argv // logic-level proof of the fix, complementary to (not a replacement for) the // live container reproduction recorded in the review notes. -/** Writes a fake `unshare` that mimics the exact advertise-vs-honor failure - * shape: any invocation whose argv contains `mount -t proc proc /proc` (the - * probe's own mount-and-verify dry run, or the real prelude's) exits 32 with - * a stderr line matching the real kernel refusal, `mount: /proc: permission +// INJECTION MECHANISM (P1, external review of ab415be6c — trusted launcher +// resolution): these two tests used to shadow `unshare`/`bwrap` via a +// PATH-prepended fake-bin directory. Now that both the probe and the real +// execution resolve the launcher through `resolveTrustedLauncherPath` (a +// fixed allowlist, never the caller's `$PATH`), that injection no longer +// reaches anything — proving the fix, but also meaning a test still relying +// on it would silently stop exercising the fallback logic. Both fakes are +// now installed via bind-mounting DIRECTLY OVER the real trusted-path +// `unshare`/`bwrap` binaries (`withShimmedTrustedBinary`, same technique the +// setup-command forced-failure tests below already use), nested so both +// binaries are shimmed for the duration of each test. + +/** Shim body mimicking the exact advertise-vs-honor failure shape: any + * invocation whose argv contains `mount -t proc proc /proc` (the probe's + * own mount-and-verify dry run, or the real prelude's) exits 32 with a + * stderr line matching the real kernel refusal, `mount: /proc: permission * denied.` — every other invocation shape (a bare capability probe like * `-r -n true`) exits 0, so namespace CREATION still looks available; only * the procfs mount specifically is refused, mirroring the CAP_SYS_ADMIN - * -only container shape exactly. */ -function fakeUnshareRefusingProcMount(dir: string): void { - const scriptPath = join(dir, "unshare"); - writeFileSync( - scriptPath, - [ - "#!/bin/sh", - 'case "$*" in', - ' *"mount -t proc proc /proc"*)', - ' echo "mount: /proc: permission denied." 1>&2', - " exit 32", - " ;;", - " *)", - " exit 0", - " ;;", - "esac", - ].join("\n"), - { mode: 0o755 } - ); + * -only container shape exactly. Ignores `realUnsharePath` (unlike the + * setup-command shims elsewhere in this file, this fake never delegates — + * the whole point is to mimic the OLD probe's blind spot, not to actually + * run real `unshare`). */ +function unshareShimRefusingProcMount(_realUnsharePath: string): string { + return [ + "#!/bin/sh", + 'case "$*" in', + ' *"mount -t proc proc /proc"*)', + ' echo "mount: /proc: permission denied." 1>&2', + " exit 32", + " ;;", + " *)", + " exit 0", + " ;;", + "esac", + ].join("\n"); } -/** Writes a fake `bwrap` that always succeeds — used to prove the fallback - * path is taken (not just that `unshare` was correctly rejected). */ -function fakeBwrapAlwaysAvailable(dir: string): void { - const scriptPath = join(dir, "bwrap"); - writeFileSync(scriptPath, "#!/bin/sh\nexit 0\n", { mode: 0o755 }); +/** Shim body that always succeeds — used to prove the fallback path is + * taken (not just that `unshare` was correctly rejected). */ +function bwrapShimAlwaysAvailable(_realBwrapPath: string): string { + return "#!/bin/sh\nexit 0\n"; } -/** Writes a fake `bwrap` that always fails — used so the "no fallback - * available" test is not accidentally rescued by a REAL, working bwrap - * elsewhere on this host's PATH; PATH-shadowing alone isn't enough because - * omitting a bwrap entry from the fake dir doesn't hide a real one located - * later in the (still-inherited) PATH string. */ -function fakeBwrapAlwaysUnavailable(dir: string): void { - const scriptPath = join(dir, "bwrap"); - writeFileSync( - scriptPath, - '#!/bin/sh\necho "bwrap: Creating new namespace failed: Operation not permitted" 1>&2\nexit 1\n', - { - mode: 0o755, - } - ); +/** Shim body that always fails — used so the "no fallback available" test + * is not accidentally rescued by a real, working bwrap. */ +function bwrapShimAlwaysUnavailable(_realBwrapPath: string): string { + return '#!/bin/sh\necho "bwrap: Creating new namespace failed: Operation not permitted" 1>&2\nexit 1\n'; } -test("probe reports UNAVAILABLE (not available-then-crash) when unshare's PID-ns procfs mount is refused, with no working bwrap fallback", () => { - const fakeBinDir = mkdtempSync(join(tmpdir(), "pdpp-isolation-fake-refusal-")); - fakeUnshareRefusingProcMount(fakeBinDir); - // A fake bwrap that ALSO fails — not just an absent one — because the fake - // dir is only PREPENDED to PATH; a real, working bwrap later in the - // inherited PATH would otherwise rescue this "no fallback" scenario and - // make the test assert something false about this host. - fakeBwrapAlwaysUnavailable(fakeBinDir); - const realPath = process.env.PATH; - process.env.PATH = `${fakeBinDir}:${realPath ?? ""}`; - try { - const cap = isNamespaceIsolationAvailable(); - assert.equal( - cap.available, - false, - "a host where namespace creation succeeds but the PID-namespace procfs mount is refused must report UNAVAILABLE, not available-then-crash-later" - ); - if (!cap.available) { - assert.ok( - /permission denied|mount/i.test(cap.reason), - `the diagnostic must name the kernel-level mount refusal, not just 'unavailable'; got ${JSON.stringify(cap.reason)}` +test("probe reports UNAVAILABLE (not available-then-crash) when unshare's PID-ns procfs mount is refused, with no working bwrap fallback", { + skip: !bindMountCapable, +}, async () => { + await withShimmedTrustedBinary("unshare", unshareShimRefusingProcMount, () => + withShimmedTrustedBinary("bwrap", bwrapShimAlwaysUnavailable, () => { + const cap = isNamespaceIsolationAvailable(); + assert.equal( + cap.available, + false, + "a host where namespace creation succeeds but the PID-namespace procfs mount is refused must report UNAVAILABLE, not available-then-crash-later" ); - } - } finally { - process.env.PATH = realPath; - rmSync(fakeBinDir, { recursive: true, force: true }); - } + if (!cap.available) { + assert.ok( + /permission denied|mount/i.test(cap.reason), + `the diagnostic must name the kernel-level mount refusal, not just 'unavailable'; got ${JSON.stringify(cap.reason)}` + ); + } + return Promise.resolve(); + }) + ); }); -test("probe falls back to bwrap when unshare's procfs mount is refused but bwrap genuinely works", () => { - const fakeBinDir = mkdtempSync(join(tmpdir(), "pdpp-isolation-fake-refusal-fallback-")); - fakeUnshareRefusingProcMount(fakeBinDir); - fakeBwrapAlwaysAvailable(fakeBinDir); - const realPath = process.env.PATH; - process.env.PATH = `${fakeBinDir}:${realPath ?? ""}`; - try { - const cap = isNamespaceIsolationAvailable(); - assert.equal( - cap.available, - true, - "a host where unshare's procfs mount is refused but bwrap genuinely works must still report AVAILABLE — the fallback exists precisely for this shape" - ); - if (cap.available) { +test("probe falls back to bwrap when unshare's procfs mount is refused but bwrap genuinely works", { + skip: !bindMountCapable, +}, async () => { + await withShimmedTrustedBinary("unshare", unshareShimRefusingProcMount, () => + withShimmedTrustedBinary("bwrap", bwrapShimAlwaysAvailable, () => { + const cap = isNamespaceIsolationAvailable(); assert.equal( - cap.mechanism, - "bwrap", - "must select bwrap, not unshare — unshare demonstrably cannot honor the isolation it would advertise on this (simulated) host" + cap.available, + true, + "a host where unshare's procfs mount is refused but bwrap genuinely works must still report AVAILABLE — the fallback exists precisely for this shape" ); - } - } finally { - process.env.PATH = realPath; - rmSync(fakeBinDir, { recursive: true, force: true }); - } + if (cap.available) { + assert.equal( + cap.mechanism, + "bwrap", + "must select bwrap, not unshare — unshare demonstrably cannot honor the isolation it would advertise on this (simulated) host" + ); + } + return Promise.resolve(); + }) + ); }); -/** True when this test process can actually shadow a real trusted-path - * binary via a host-level bind mount (root or an equivalent capability) — - * the injection mechanism the test below needs now that the prelude - * resolves its setup commands through a fixed `TRUSTED_SETUP_PATH` - * (P1-1, ninth review) rather than the caller's inherited `$PATH`. Probed - * by attempting the exact bind-then-unbind sequence the real test performs, - * against a scratch file, so the skip condition matches the test's actual - * requirement rather than a proxy for it (e.g. `process.getuid() === 0` - * would be wrong inside a rootless-but-capable container). */ -function canBindMountOverAFile(): boolean { - if (process.platform !== "linux") { - return false; - } - const probeSource = mkdtempSync(join(tmpdir(), "pdpp-bindmount-probe-src-")); - const probeTarget = mkdtempSync(join(tmpdir(), "pdpp-bindmount-probe-dst-")); - const srcFile = join(probeSource, "a"); - const dstFile = join(probeTarget, "b"); - writeFileSync(srcFile, ""); - writeFileSync(dstFile, ""); - const bound = spawnSync("mount", ["--bind", srcFile, dstFile], { stdio: "ignore" }).status === 0; - if (bound) { - spawnSync("umount", [dstFile], { stdio: "ignore" }); - } - rmSync(probeSource, { recursive: true, force: true }); - rmSync(probeTarget, { recursive: true, force: true }); - return bound; -} - -const bindMountCapable = unshareUsable && canBindMountOverAFile(); test("a forced PID-ns procfs-mount refusal inside the real unshare-mechanism prelude fails the spawn closed, never silently proceeds", { skip: !bindMountCapable, @@ -374,6 +370,111 @@ test("a forced PID-ns procfs-mount refusal inside the real unshare-mechanism pre } }); +// ─── Trusted launcher resolution (P1, external review of ab415be6c) ─────── +// +// The review's exact finding: `probeUnshare()`/`probeBwrap()` and +// `spawnWithNetworkIsolation`'s real execution both spawned the launcher via +// a BARE command name (`spawnSync("unshare", ...)`, `spawn("bwrap", ...)`), +// which `node:child_process` resolves through the CALLING process's own +// inherited `$PATH` — so a PATH-prepended fake `unshare`/`bwrap` earlier in +// `$PATH` than the real, trusted one gets selected instead. These tests +// prove the fix: (1) at the unit level, `resolveTrustedLauncherPath` finds +// the REAL binary regardless of what `$PATH` says, even with a fake +// prepended; (2) at the end-to-end level, a PATH-prepended fake `unshare` +// is never invoked by either the probe or a real isolated spawn. + +test("resolveTrustedLauncherPath: resolves the real trusted-location binary, ignoring a fake earlier in $PATH", { + skip: process.platform !== "linux", +}, () => { + const fakeBinDir = mkdtempSync(join(tmpdir(), "pdpp-isolation-fake-launcher-path-")); + const fakeMarkerPath = join(fakeBinDir, "fake-unshare-ran"); + writeFileSync( + join(fakeBinDir, "unshare"), + ["#!/bin/sh", `touch ${JSON.stringify(fakeMarkerPath)}`, "exit 0"].join("\n"), + { mode: 0o755 } + ); + const realPath = process.env.PATH; + process.env.PATH = `${fakeBinDir}:${realPath ?? ""}`; + try { + const resolved = resolveTrustedLauncherPath("unshare"); + assert.ok( + !resolved.startsWith(fakeBinDir), + `resolveTrustedLauncherPath must never return the PATH-prepended fake; got ${JSON.stringify(resolved)}` + ); + assert.ok( + ["/usr/sbin/unshare", "/usr/bin/unshare", "/sbin/unshare", "/bin/unshare"].includes(resolved), + `expected a real trusted-directory path; got ${JSON.stringify(resolved)}` + ); + // Actually running the resolved binary must not be the fake — the fake + // would touch its own marker file the instant it started. + spawnSync(resolved, ["-r", "-n", "true"], { stdio: "ignore" }); + assert.ok( + !existsSync(fakeMarkerPath), + "the fake unshare's marker file must NOT exist — resolveTrustedLauncherPath's return value must never invoke the fake" + ); + } finally { + process.env.PATH = realPath; + rmSync(fakeBinDir, { recursive: true, force: true }); + } +}); + +test("a PATH-prepended fake `unshare` is never selected by the probe or by a real isolated spawn", { + skip: !unshareUsable, +}, async () => { + const fakeBinDir = mkdtempSync(join(tmpdir(), "pdpp-isolation-fake-launcher-e2e-")); + const fakeMarkerPath = join(fakeBinDir, "fake-unshare-ran"); + // The fake always "succeeds" instantly (exit 0, no real namespace, no real + // isolation) — if it were ever selected, both the probe and a real spawn + // would misreport success while providing NO isolation at all. Also + // touches its own marker so this test can prove, directly, that the fake + // was never invoked (not just that isolation happened to still work). + writeFileSync( + join(fakeBinDir, "unshare"), + ["#!/bin/sh", `touch ${JSON.stringify(fakeMarkerPath)}`, "exit 0"].join("\n"), + { mode: 0o755 } + ); + const realPath = process.env.PATH; + process.env.PATH = `${fakeBinDir}:${realPath ?? ""}`; + try { + const cap = isNamespaceIsolationAvailable(); + assert.ok( + !existsSync(fakeMarkerPath), + "the fake unshare's marker must NOT exist after the capability probe — the probe must resolve the real trusted-path binary, never the PATH-prepended fake" + ); + if (cap.available && cap.mechanism === "unshare") { + // Only meaningful when the probe genuinely selected unshare (true on + // this suite's own host, where unshare works natively) — prove a real + // spawn under `isolate: true` (which re-derives the mechanism itself, + // exercising the SAME resolution path as production) also never + // touches the fake, and that the child is genuinely isolated (no + // outbound network) rather than the fake's instant, unisolated exit 0. + const exitCode = await new Promise((resolveExit) => { + const child = spawnWithNetworkIsolation( + process.execPath, + [ + "-e", + 'require("http").get("http://1.1.1.1",()=>process.exit(9)).on("error",()=>process.exit(0));setTimeout(()=>process.exit(0),4000)', + ], + { isolate: true, stdio: "ignore" } + ); + child.on("close", resolveExit); + }); + assert.ok( + !existsSync(fakeMarkerPath), + "the fake unshare's marker must NOT exist after a real isolated spawn — spawnWithNetworkIsolation must resolve the real trusted-path binary, never the PATH-prepended fake" + ); + assert.equal( + exitCode, + 0, + "the spawn must still be genuinely network-isolated (exit 9 would mean the fake ran instead and no real isolation happened)" + ); + } + } finally { + process.env.PATH = realPath; + rmSync(fakeBinDir, { recursive: true, force: true }); + } +}); + // ─── Fail-closed setup — forced-failure controls for every mandatory step ── // // P1-1 (ninth review): the OLD `filesystemClosureShellPrelude` discarded the @@ -839,110 +940,109 @@ test("[unshare] a genuinely successful filesystem closure passes post-pivot veri // `detectMechanism()`, which re-runs the ENTIRE probe (spawning `unshare`, // and — if that's denied — `bwrap`) from scratch on every single spawn, // contradicting the "probe once, reuse everywhere" contract callers rely -// on. This test proves that contract mechanically: with fake `unshare`/ -// `bwrap` binaries on PATH that log every invocation, passing the -// already-known mechanism directly must invoke the probe binaries ZERO -// times, while passing a bare `true` must invoke them (the regression this -// test exists to catch if a caller — or this function itself — regresses -// back to re-probing). - -/** Writes a fake `unshare`/`bwrap` shell shim to `dir/name` that appends its - * full argv (one line, space-joined) to `logPath` and exits 0, then - * returns `dir` prepended onto `PATH` so a child process resolves the fake - * binary instead of the real one. */ -function fakeIsolationBinDir(logPath: string): string { - const dir = mkdtempSync(join(tmpdir(), "pdpp-isolation-fake-bin-")); - for (const name of ["unshare", "bwrap"]) { - const scriptPath = join(dir, name); - writeFileSync(scriptPath, `#!/bin/sh\necho "${name} $*" >> ${JSON.stringify(logPath)}\nexit 0\n`, { mode: 0o755 }); - } - return dir; +// on. This test proves that contract mechanically: with logging shims +// covering both trusted-path binaries, passing the already-known mechanism +// directly must invoke the probe binaries ZERO times, while passing a bare +// `true` must invoke them (the regression this test exists to catch if a +// caller — or this function itself — regresses back to re-probing). +// +// INJECTION MECHANISM (P1, external review of ab415be6c — trusted launcher +// resolution): these two tests used to PATH-prepend fake `unshare`/`bwrap` +// binaries. Now that both the probe and the real execution resolve the +// launcher through `resolveTrustedLauncherPath` (never the caller's `$PATH`), +// that no longer reaches anything — both binaries are shimmed via +// bind-mount-over-the-real-trusted-path binary instead +// (`withShimmedTrustedBinary`, nested so both are covered at once). + +/** Shim body that appends its full argv (one line, space-joined) to + * `logPath` and exits 0 WITHOUT delegating to the real binary — unlike + * the setup-command shims elsewhere in this file, these two tests need to + * observe exactly which binary/argv shape `spawnWithNetworkIsolation` + * invokes, not exercise a real isolated spawn. */ +function loggingShim(name: string, logPath: string): (realBinaryPath: string) => string { + return (_realBinaryPath: string) => `#!/bin/sh\necho "${name} $*" >> ${JSON.stringify(logPath)}\nexit 0\n`; +} + +async function withBothLaunchersLoggingShimmed(logPath: string, fn: () => Promise): Promise { + return withShimmedTrustedBinary("unshare", loggingShim("unshare", logPath), () => + withShimmedTrustedBinary("bwrap", loggingShim("bwrap", logPath), fn) + ); } -test("spawnWithNetworkIsolation given an already-resolved mechanism does NOT re-probe (no unshare/bwrap probe invocation)", async () => { +test("spawnWithNetworkIsolation given an already-resolved mechanism does NOT re-probe (no unshare/bwrap probe invocation)", { + skip: !bindMountCapable, +}, async () => { const logDir = mkdtempSync(join(tmpdir(), "pdpp-isolation-probe-log-")); const logPath = join(logDir, "invocations.log"); writeFileSync(logPath, ""); - const fakeBinDir = fakeIsolationBinDir(logPath); - const fakePath = `${fakeBinDir}:${process.env.PATH ?? ""}`; - // detectMechanism()'s isNamespaceIsolationAvailable() probe (when it runs - // at all — the whole point of this test is that it must NOT) runs - // spawnSync in THIS process using THIS process's env/PATH, not the - // spawned child's — so the fake binaries must be resolvable from here too. - const realPath = process.env.PATH; - process.env.PATH = fakePath; try { - const exitCode = await new Promise((resolve) => { - const child = spawnWithNetworkIsolation("node", ["-e", "process.exit(0)"], { - isolate: "bwrap", - stdio: "ignore", - env: { ...process.env, PATH: fakePath }, + await withBothLaunchersLoggingShimmed(logPath, async () => { + const exitCode = await new Promise((resolveExit) => { + const child = spawnWithNetworkIsolation("node", ["-e", "process.exit(0)"], { + isolate: "bwrap", + stdio: "ignore", + }); + child.on("close", resolveExit); }); - child.on("close", resolve); + assert.equal(exitCode, 0); + + const invocations = readFileSync(logPath, "utf8").trim().split("\n").filter(Boolean); + // Exactly one bwrap call: the ACTUAL wrapped spawn (`bwrap --unshare-net + // ... -- sh -c ...`), never a probe call (`bwrap --unshare-net + // --dev-bind / / true`, no trailing `-- sh -c`) and never an `unshare` + // call at all — proving detectMechanism()'s `isNamespaceIsolationAvailable()` + // re-probe path was never taken when the mechanism was already known. + assert.equal( + invocations.length, + 1, + `expected exactly one fake-binary invocation (the real spawn, no probe); got ${JSON.stringify(invocations)}` + ); + assert.ok( + invocations[0]?.startsWith("bwrap "), + `expected the one invocation to be bwrap; got ${JSON.stringify(invocations)}` + ); + assert.ok( + invocations[0]?.includes("-- sh -c"), + `expected the real wrapped-spawn argv shape, not a probe; got ${JSON.stringify(invocations)}` + ); }); - assert.equal(exitCode, 0); - - const invocations = readFileSync(logPath, "utf8").trim().split("\n").filter(Boolean); - // Exactly one bwrap call: the ACTUAL wrapped spawn (`bwrap --unshare-net - // ... -- sh -c ...`), never a probe call (`bwrap --unshare-net - // --dev-bind / / true`, no trailing `-- sh -c`) and never an `unshare` - // call at all — proving detectMechanism()'s `isNamespaceIsolationAvailable()` - // re-probe path was never taken when the mechanism was already known. - assert.equal( - invocations.length, - 1, - `expected exactly one fake-binary invocation (the real spawn, no probe); got ${JSON.stringify(invocations)}` - ); - assert.ok( - invocations[0]?.startsWith("bwrap "), - `expected the one invocation to be bwrap; got ${JSON.stringify(invocations)}` - ); - assert.ok( - invocations[0]?.includes("-- sh -c"), - `expected the real wrapped-spawn argv shape, not a probe; got ${JSON.stringify(invocations)}` - ); } finally { - process.env.PATH = realPath; rmSync(logDir, { recursive: true, force: true }); - rmSync(fakeBinDir, { recursive: true, force: true }); } }); -test("spawnWithNetworkIsolation given a bare `true` DOES re-probe (documents the boolean fallback path's cost, for contrast)", async () => { +test("spawnWithNetworkIsolation given a bare `true` DOES re-probe (documents the boolean fallback path's cost, for contrast)", { + skip: !bindMountCapable, +}, async () => { const logDir = mkdtempSync(join(tmpdir(), "pdpp-isolation-probe-log-")); const logPath = join(logDir, "invocations.log"); writeFileSync(logPath, ""); - const fakeBinDir = fakeIsolationBinDir(logPath); - const fakePath = `${fakeBinDir}:${process.env.PATH ?? ""}`; - const realPath = process.env.PATH; - process.env.PATH = fakePath; try { - const exitCode = await new Promise((resolve) => { - const child = spawnWithNetworkIsolation("node", ["-e", "process.exit(0)"], { - isolate: true, - stdio: "ignore", - env: { ...process.env, PATH: fakePath }, + await withBothLaunchersLoggingShimmed(logPath, async () => { + const exitCode = await new Promise((resolveExit) => { + const child = spawnWithNetworkIsolation("node", ["-e", "process.exit(0)"], { + isolate: true, + stdio: "ignore", + }); + child.on("close", resolveExit); }); - child.on("close", resolve); + assert.equal(exitCode, 0); + + const invocations = readFileSync(logPath, "utf8").trim().split("\n").filter(Boolean); + // A bare `true` forces detectMechanism() to call isNamespaceIsolationAvailable(), + // which probes `unshare` first (the fake shim reports success, so the + // probe reports mechanism "unshare" without ever trying bwrap — but the + // point is a probe call happens AT ALL, unlike the resolved-mechanism + // case above) before the real wrapped spawn. + assert.ok( + invocations.length >= 2, + `expected at least a probe call plus the real spawn call; got ${JSON.stringify(invocations)}` + ); }); - assert.equal(exitCode, 0); - - const invocations = readFileSync(logPath, "utf8").trim().split("\n").filter(Boolean); - // A bare `true` forces detectMechanism() to call isNamespaceIsolationAvailable(), - // which probes `unshare` first (the fake shim reports success, so the - // probe reports mechanism "unshare" without ever trying bwrap — but the - // point is a probe call happens AT ALL, unlike the resolved-mechanism - // case above) before the real wrapped spawn. - assert.ok( - invocations.length >= 2, - `expected at least a probe call plus the real spawn call; got ${JSON.stringify(invocations)}` - ); } finally { - process.env.PATH = realPath; rmSync(logDir, { recursive: true, force: true }); - rmSync(fakeBinDir, { recursive: true, force: true }); } }); diff --git a/packages/polyfill-connectors/src/scenario/isolation.ts b/packages/polyfill-connectors/src/scenario/isolation.ts index 6a8856fa8..a214f476e 100644 --- a/packages/polyfill-connectors/src/scenario/isolation.ts +++ b/packages/polyfill-connectors/src/scenario/isolation.ts @@ -209,7 +209,17 @@ */ import { type ChildProcess, type SpawnOptions, spawn, spawnSync } from "node:child_process"; -import { existsSync, lstatSync, mkdirSync, mkdtempSync, readlinkSync, rmSync } from "node:fs"; +import { + accessSync, + constants, + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readlinkSync, + realpathSync, + rmSync, +} from "node:fs"; import { homedir, tmpdir } from "node:os"; import { dirname, join, resolve, sep } from "node:path"; import { fileURLToPath } from "node:url"; @@ -235,6 +245,97 @@ export type NamespaceIsolationCapability = | { available: true; mechanism: IsolationMechanism } | { available: false; reason: string }; +/** + * The same fixed, absolute directory list as `TRUSTED_SETUP_PATH`, as an + * array — used by `resolveTrustedLauncherPath` below to find the LAUNCHER + * binaries themselves (`unshare`, `bwrap`), not the setup commands the + * launched shell script runs. Kept as a literal array (not derived by + * splitting `TRUSTED_SETUP_PATH`) so the two stay independently readable at + * their own call sites, but the values are the same list for the same + * reason: this is the operating system's own set of locations for trusted, + * privileged system binaries, nothing caller- or environment-specific. + */ +const TRUSTED_LAUNCHER_DIRECTORIES: readonly string[] = ["/usr/sbin", "/usr/bin", "/sbin", "/bin"]; + +/** + * TRUSTED LAUNCHER RESOLUTION (P1, external review of ab415be6c) — resolves + * the `unshare`/`bwrap` binary this module actually spawns to ONE fixed, + * absolute path, found by walking `TRUSTED_LAUNCHER_DIRECTORIES` in order, + * rather than letting `node:child_process`'s `spawn`/`spawnSync` resolve a + * bare command NAME through the calling process's own inherited `$PATH`. + * + * WHAT THIS CLOSES: before this fix, `probeUnshare()`/`probeBwrap()` (the + * CAPABILITY CHECK) and `spawnWithNetworkIsolation` (the REAL EXECUTION) + * both called `spawnSync("unshare", ...)`/`spawn("bwrap", ...)` — a bare + * command name. Node resolves a bare command name by searching the + * CALLING PROCESS's own `PATH` environment variable, entry by entry, and + * uses the FIRST match — exactly the same "attacker prepends a directory + * ahead of the real one" shape `TRUSTED_SETUP_PATH` already closes for the + * commands INSIDE the isolated child's own setup script (`mount`, + * `pivot_root`, ...), but that fix never touched the launcher itself: a + * caller (or a compromised connector's own environment mutation, since this + * package's own subprocess env construction is caller-controlled) could + * still prepend a directory containing a same-named `unshare` or `bwrap` to + * `process.env.PATH` before this module ran, and that fake binary — not the + * real, trusted one — is what actually got spawned with this process's own + * privileges. Both the capability PROBE and the real EXECUTION resolved the + * bare name independently, so a caller could even see a probe report + * `available: true` against the REAL binary, then have the real spawn moments + * later silently run the FAKE one instead (or vice versa) if `PATH` changed + * in between — this fix removes that gap entirely by resolving once, from a + * `PATH`-independent source of truth, and threading the SAME resolved + * absolute path through both call sites. + * + * RESOLUTION: walks `TRUSTED_LAUNCHER_DIRECTORIES` in the FIXED order given + * — never the caller's `$PATH`, never any other environment-derived list — + * and returns the first `${dir}/${name}` that exists and is executable + * (`X_OK`). A symlink at that path (e.g. a merged-usr host's `/bin/unshare` + * pointing into `/usr/bin/unshare`, or vice versa) is followed to its real + * target via `realpathSync` before being returned, so the value callers spawn + * is always a concrete file, not a path whose target could be swapped out + * from under a cached lookup by re-pointing a symlink. Throws (fails closed, + * never silently falls back to a bare, PATH-resolved name) when NO trusted + * directory has the binary — a host missing `unshare`/`bwrap` entirely from + * every trusted location cannot isolate, and this module must say so loudly + * rather than let `spawn` fall through to an unaudited `$PATH` lookup as an + * implicit fallback. + * + * CACHED per (name), computed once per process — the trusted directories are + * fixed, real filesystem locations, not expected to change during a single + * run, and this resolution runs on every probe and every isolated spawn, so + * memoizing avoids repeating four `existsSync`+`accessSync` checks (up to + * eight, across both binaries) on every single replay run in a scenario with + * many runs. + */ +const trustedLauncherPathCache = new Map(); + +/** Exported for `isolation-mechanism.test.ts`'s direct unit-level proof that + * resolution is `$PATH`-independent — production code never needs to call + * this from outside the module, every internal call site already does. */ +export function resolveTrustedLauncherPath(name: "unshare" | "bwrap"): string { + const cached = trustedLauncherPathCache.get(name); + if (cached !== undefined) { + return cached; + } + for (const dir of TRUSTED_LAUNCHER_DIRECTORIES) { + const candidate = join(dir, name); + if (!existsSync(candidate)) { + continue; + } + try { + accessSync(candidate, constants.X_OK); + } catch { + continue; + } + const resolved = realpathSync(candidate); + trustedLauncherPathCache.set(name, resolved); + return resolved; + } + throw new Error( + `pdpp isolation: trusted launcher '${name}' not found in any trusted location (${TRUSTED_LAUNCHER_DIRECTORIES.join(", ")}) — refusing to fall back to a PATH-resolved lookup` + ); +} + /** * Shell statements that mount and verify a fresh procfs, shared verbatim * between the real `unshare`-mechanism prelude @@ -311,7 +412,13 @@ function unshareProcMountProbeArgv(): string[] { * in well under a second so the cost of asking is negligible. */ function probeUnshare(): NamespaceIsolationCapability { - const probe = spawnSync("unshare", unshareProcMountProbeArgv(), { + let unsharePath: string; + try { + unsharePath = resolveTrustedLauncherPath("unshare"); + } catch (err) { + return { available: false, reason: err instanceof Error ? err.message : String(err) }; + } + const probe = spawnSync(unsharePath, unshareProcMountProbeArgv(), { stdio: ["ignore", "pipe", "pipe"], timeout: 5000, }); @@ -405,9 +512,15 @@ export function isNamespaceIsolationAvailable(): NamespaceIsolationCapability { * invokes. */ function probeBwrap(): NamespaceIsolationCapability { + let bwrapPath: string; + try { + bwrapPath = resolveTrustedLauncherPath("bwrap"); + } catch (err) { + return { available: false, reason: err instanceof Error ? err.message : String(err) }; + } const probeWorkspace = mkdtempSync(join(tmpdir(), "pdpp-isolation-bwrap-probe-")); try { - const probe = spawnSync("bwrap", bwrapArgvForFilesystemClosure("true", [], probeWorkspace), { + const probe = spawnSync(bwrapPath, bwrapArgvForFilesystemClosure("true", [], probeWorkspace), { stdio: ["ignore", "ignore", "pipe"], timeout: 5000, }); @@ -814,6 +927,7 @@ function requiredFhsCompatSymlinks(): readonly FhsCompatSymlink[] { */ const TRUSTED_SETUP_PATH = "/usr/sbin:/usr/bin:/sbin:/bin"; + /** * The single fixed exit code every mandatory setup step in * `filesystemClosureShellPrelude` uses on failure (distinct from `97`, the @@ -1221,7 +1335,11 @@ export function spawnWithNetworkIsolation( ensureSandboxScratchDirs(filesystemBindPath); const mechanism = isolate === true ? detectMechanism() : isolate; if (mechanism === "bwrap") { - return spawn("bwrap", bwrapArgvForFilesystemClosure(cmd, args, filesystemBindPath), spawnOpts); + // TRUSTED LAUNCHER (P1, external review of ab415be6c): resolved via + // resolveTrustedLauncherPath, never a bare "bwrap" name that node: + // child_process would otherwise resolve through this process's own + // inherited $PATH — see that function's doc comment. + return spawn(resolveTrustedLauncherPath("bwrap"), bwrapArgvForFilesystemClosure(cmd, args, filesystemBindPath), spawnOpts); } const innerCommand = [cmd, ...args].map(shQuote).join(" "); // `spawnOpts.cwd` (a `string | URL | undefined` per `SpawnOptions`) is @@ -1242,8 +1360,13 @@ export function spawnWithNetworkIsolation( // does not alter), so it intentionally stays a best-effort step, not // wrapped in `req`. const shScript = `PATH=${TRUSTED_SETUP_PATH}; ip link set lo up >/dev/null 2>&1; ${closurePrelude}; exec ${innerCommand}`; + // TRUSTED LAUNCHER (P1, external review of ab415be6c): resolved via + // resolveTrustedLauncherPath, never a bare "unshare" name — see that + // function's doc comment. Note this is the LAUNCHER binary itself; the + // commands INSIDE the shell script it runs (mount, pivot_root, ...) are + // separately trusted via TRUSTED_SETUP_PATH above. return spawn( - "unshare", + resolveTrustedLauncherPath("unshare"), ["--map-root-user", "--net", "--mount", "--pid", "--ipc", "--uts", "--fork", "--", "sh", "-c", shScript], spawnOpts ); From 639afa9f8617f1e405146879dfd6834ffcf14e9b Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 2 Sep 2026 05:30:58 -0500 Subject: [PATCH 03/16] fix(isolation): remount every submount of a ro bind read-only, not just the top MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit External review of ab415be6c found the unshare mechanism binds submounts recursively via --rbind but only remounts the TOP mount read-only via classic remount,ro,bind — Linux does not apply that operation recursively, so a nested mount that existed under a ro bind's source directory at spawn time (e.g. Docker's own /etc/resolv.conf-style injected submounts, or any other nested mount under REPO_ROOT) stayed writable inside the isolated child even though the parent directory correctly reported read-only. Reproduced live: an isolated child could append to /etc/resolv.conf and /etc/hostname despite /etc itself being genuinely ro. Add recursiveReadOnlyRemountCommand, which walks /proc/self/mountinfo after each ro bind's top-level remount and individually remounts every descendant mount point found under it, wrapped in the existing req fail-closed helper so a submount that refuses to go read-only halts the whole prelude. Extend postPivotVerificationStatements the same way — the post-pivot verification now probes every submount of every ro bind, not just the parent, so a future regression is caught at verification time too. Fixing this exposed a related, pre-existing bug in dedupeBinds: it kept whichever entry was declared FIRST in requiredFilesystemBinds()'s array even when a later, broader entry (e.g. /usr) would have covered an earlier, narrower one (e.g. the Node binary's own directory, /usr/local/bin in a container's default install) — harmless under the old top-level-only remount, but it left a redundant nested nested nested mount that the new recursive-remount walk then tried to remount twice. dedupeBinds now sorts by path depth before deduping, so the broadest ancestor always wins regardless of declaration order. Verified live in a privileged container: an isolated unshare child can no longer write into Docker's real /etc/resolv.conf/hostname submounts. New regression tests create a real nested bind mount under REPO_ROOT and prove EACCES/EROFS on write, both for the real spawn (mutation-tested: the [unshare] variant genuinely fails without the fix, bwrap's own mechanism already closed this case) and for postPivotVerificationStatements directly (mutation-tested: exit 0 without the submount probe, exit 91 with it). Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../src/scenario/isolation-mechanism.test.ts | 151 +++++++++++++++- .../src/scenario/isolation.ts | 168 ++++++++++++++++-- 2 files changed, 307 insertions(+), 12 deletions(-) diff --git a/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts b/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts index d9b371e13..72e53f0dd 100644 --- a/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts +++ b/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts @@ -10,7 +10,7 @@ // under it has no outbound network. import assert from "node:assert/strict"; import { spawn, spawnSync } from "node:child_process"; -import { cpSync, existsSync, mkdtempSync, readFileSync, readlinkSync, rmSync, writeFileSync } from "node:fs"; +import { cpSync, existsSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, writeFileSync } from "node:fs"; import { createServer } from "node:http"; import { homedir, tmpdir } from "node:os"; import { join, resolve as resolvePath } from "node:path"; @@ -911,6 +911,88 @@ test("[bwrap sandbox] postPivotVerificationStatements FAILS when /oldroot is non ); }); +test("[bwrap sandbox] postPivotVerificationStatements FAILS when a NESTED submount under a genuinely-read-only ro bind is writable (P1, external review of ab415be6c)", { + skip: !bwrapUsable || !bindMountCapable, +}, () => { + // Unlike scenario (b) above (a bind whose OWN top mount never went + // read-only), this proves the EXTENDED property: the top-level bind IS + // genuinely read-only, but a real, separate mount point nested underneath + // it is not — the exact gap --rbind + a single top-level remount,ro,bind + // leaves open (see recursiveReadOnlyRemountCommand's doc comment). + const roDir = mkdtempSync(join(tmpdir(), "pdpp-isolation-ro-parent-")); + const nestedSource = mkdtempSync(join(tmpdir(), "pdpp-isolation-nested-source-")); + const nestedMountPoint = join(roDir, "nested"); + try { + mkdirSync(nestedMountPoint, { recursive: true }); + const mountResult = spawnSync("mount", ["--bind", nestedSource, nestedMountPoint], { stdio: "inherit" }); + assert.equal(mountResult.status, 0, "sanity check: creating the real nested mount point must itself succeed"); + try { + // The sandbox's OWN --ro-bind of roDir is genuinely read-only (bwrap's + // own mechanism, proven elsewhere in this file to close this + // specific case) — so this test targets the VERIFICATION FUNCTION's + // own submount-probing logic directly, independent of which + // mechanism's setup produced the nested mount. + const setup = ["touch /pdpp-isolation-canary", "mkdir -p /oldroot"].join("; "); + const script = `${setup}; ${postPivotVerificationStatements([{ path: "/ro-parent", mode: "ro" }]).join("; ")}; echo PDPP_VERIFY_PASSED`; + const result = spawnSync( + "bwrap", + [ + "--unshare-net", + "--tmpfs", + "/", + "--proc", + "/proc", + "--dev", + "/dev", + "--ro-bind", + "/usr", + "/usr", + "--ro-bind", + "/etc", + "/etc", + "--ro-bind", + roDir, + "/ro-parent", + "--bind", + nestedMountPoint, + "/ro-parent/nested", + "--symlink", + "usr/bin", + "/bin", + "--symlink", + "usr/sbin", + "/sbin", + "--symlink", + "usr/lib", + "/lib", + "--symlink", + "usr/lib64", + "/lib64", + "--", + "sh", + "-c", + script, + ], + { encoding: "utf8" } + ); + assert.equal( + result.status, + 91, + `expected POST_PIVOT_VERIFICATION_FAILURE_EXIT_CODE (91) when a nested submount under a ro bind is genuinely writable; stderr: ${result.stderr}` + ); + assert.ok( + /writable-ro-binds=\[[\s\S]*\/ro-parent\/nested[\s\S]*\]/.test(result.stderr), + `expected the diagnostic to name the specific writable NESTED path; got ${JSON.stringify(result.stderr)}` + ); + } finally { + spawnSync("umount", ["-l", nestedMountPoint], { stdio: "ignore" }); + } + } finally { + rmSync(roDir, { recursive: true, force: true }); + rmSync(nestedSource, { recursive: true, force: true }); + } +}); + test("[unshare] a genuinely successful filesystem closure passes post-pivot verification and the target command DOES run", { skip: !unshareUsable, }, async () => { @@ -1729,6 +1811,73 @@ for (const mechanism of ["bwrap", "unshare"] as const) { }); } +// ─── Recursive read-only — nested submount under a ro bind (P1, external +// review of ab415be6c) ────────────────────────────────────────────────────── +// +// `--rbind` (recursive bind) pulls in every submount that exists under a +// `ro` bind's source directory at bind time — but the classic +// `mount -o remount,ro,bind ` step that follows only remounts the +// TOP mount; Linux does not apply that operation recursively to the +// submounts `--rbind` carried along. Before this fix, a nested mount point +// that existed under REPO_ROOT (or any other declared `ro` bind) at spawn +// time stayed WRITABLE inside the isolated child even though the parent +// directory correctly reported read-only. This test proves the fix by +// creating a REAL nested bind mount under REPO_ROOT on the host (requiring +// genuine bind-mount capability — the same `canBindMountOverAFile`/ +// `bindMountCapable` gate the setup-command forced-failure tests above use), +// spawning an isolated child, and attempting a write specifically INSIDE +// that nested mount — asserting EACCES/EROFS, not just checking the parent +// directory. + +for (const mechanism of ["bwrap", "unshare"] as const) { + const usable = (mechanism === "bwrap" ? bwrapUsable : unshareUsable) && bindMountCapable; + + test(`[${mechanism}] a nested bind mount under REPO_ROOT (a ro bind) stays read-only inside isolation — not just the parent directory`, { + skip: !usable, + }, async () => { + // A real, separate mount point INSIDE REPO_ROOT — a scratch directory + // bind-mounted onto ANOTHER scratch directory that itself lives under + // REPO_ROOT, mirroring the real-world shape this fix targets (Docker's + // own /etc/resolv.conf-style injected submounts, or any nested mount + // that happens to exist under a ro bind's real path at spawn time). + const nestedSource = mkdtempSync(join(tmpdir(), "pdpp-nested-ro-source-")); + writeFileSync(join(nestedSource, "seed.txt"), "seed"); + const nestedMountPoint = join(TEST_REPO_ROOT, `.pdpp-nested-ro-probe-${String(process.pid)}`); + rmSync(nestedMountPoint, { recursive: true, force: true }); + mkdirSync(nestedMountPoint, { recursive: true }); + const mountResult = spawnSync("mount", ["--bind", nestedSource, nestedMountPoint], { stdio: "inherit" }); + assert.equal( + mountResult.status, + 0, + `sanity check: bind-mounting a real nested mount point under REPO_ROOT must itself succeed for this test's injection to mean anything` + ); + try { + const probeFileName = `.pdpp-nested-ro-write-probe-${String(process.pid)}`; + const probePath = join(nestedMountPoint, probeFileName); + const { stdout, exitCode } = await runIsolatedProbe( + mechanism, + `const fs=require("fs");try{fs.writeFileSync(${JSON.stringify(probePath)},"x");console.log("WRITE_SUCCEEDED");}catch(e){console.log("WRITE_BLOCKED:"+e.code);}` + ); + if (existsSync(probePath)) { + rmSync(probePath, { force: true }); + } + assert.equal(exitCode, 0, `probe child must exit cleanly; stdout was ${JSON.stringify(stdout)}`); + assert.ok( + stdout.startsWith("WRITE_BLOCKED:"), + `an isolated child under ${mechanism} must NOT be able to write into a NESTED mount under REPO_ROOT (only remounting the top-level ro bind, not its submounts, is exactly the P1 this test guards) — got ${JSON.stringify(stdout)}` + ); + assert.ok( + ["EROFS", "EACCES", "EPERM"].includes(stdout.slice("WRITE_BLOCKED:".length)), + `expected a genuine read-only-filesystem error, got ${JSON.stringify(stdout)}` + ); + } finally { + spawnSync("umount", ["-l", nestedMountPoint], { stdio: "ignore" }); + rmSync(nestedMountPoint, { recursive: true, force: true }); + rmSync(nestedSource, { recursive: true, force: true }); + } + }); +} + // ─── cwd survives the filesystem closure (R9) ────────────────────────────── // // Node's `spawn(cmd, args, { cwd })` only sets the working directory of the diff --git a/packages/polyfill-connectors/src/scenario/isolation.ts b/packages/polyfill-connectors/src/scenario/isolation.ts index a214f476e..5353c1f15 100644 --- a/packages/polyfill-connectors/src/scenario/isolation.ts +++ b/packages/polyfill-connectors/src/scenario/isolation.ts @@ -722,22 +722,56 @@ export function requiredFilesystemBinds(): readonly FilesystemBind[] { return dedupeBinds(binds); } -/** Drops any bind whose path is identical to, or a filesystem descendant of, - * an earlier entry in the list — binding both would either be a harmless - * redundant mount or (worse) a `rw` ancestor accidentally masking a - * narrower `ro` intent. Order-preserving: the FIRST occurrence wins. */ +/** + * Drops any bind whose path is identical to, or a filesystem descendant of, + * ANOTHER entry in the list, regardless of which was declared first — + * binding both would either be a harmless redundant mount or (worse) a `rw` + * ancestor accidentally masking a narrower `ro` intent. + * + * SORTED BY DEPTH FIRST (P1, external review of ab415be6c's recursive-ro + * fix): the old version was order-preserving ("first occurrence wins" — + * whichever entry appeared EARLIER in `requiredFilesystemBinds()`'s literal + * array kept its bind, even if a LATER, broader ancestor entry would have + * covered it). That was harmless under the old top-level-only `remount,ro, + * bind`, where a redundant nested bind was merely wasteful. It stopped being + * harmless once `recursiveReadOnlyRemountCommand` started walking + * `/proc/self/mountinfo` for submounts of each `ro` bind: a real, concrete + * case (confirmed empirically in a privileged test container) is `nodeDir` + * (`dirname(process.execPath)`, e.g. `/usr/local/bin` under a container's + * default Node install) being declared BEFORE `/usr` in + * `requiredFilesystemBinds()`'s literal array — the old dedup kept BOTH as + * separate top-level binds, so `/usr`'s own `--rbind` then ALSO recursively + * picked up the already-separately-staged `/usr/local/bin` mount as one of + * its own submounts, and the new recursive-remount walk tried to remount + * that same mount point a second time, which failed outright ("mount point + * not mounted or bad option" — the first remount had already changed its + * mount ID out from under the second). Sorting shortest-path-first before + * deduping means the BROADEST ancestor (`/usr`) is always considered first + * regardless of declaration order, so a narrower descendant (`nodeDir`) is + * correctly absorbed into it rather than staying a separate, redundant bind + * that the parent's own recursive walk then double-processes. + */ function dedupeBinds(binds: readonly FilesystemBind[]): FilesystemBind[] { - const kept: FilesystemBind[] = []; - for (const bind of binds) { - const normalized = resolve(bind.path); + const normalized = binds.map((bind, index) => ({ ...bind, index, path: resolve(bind.path) })); + // Depth-sorted only to DECIDE which entries survive — the broadest + // ancestor must be considered first regardless of declaration order (see + // this function's doc comment). The final return value is re-sorted back + // to original declaration order below, which is what every caller + // (the bind loop in filesystemClosureShellPrelude, bwrap's argv builder) + // expects for stable, readable generated output. + const sortedByDepth = [...normalized].sort((a, b) => a.path.length - b.path.length); + const kept: typeof normalized = []; + for (const bind of sortedByDepth) { const alreadyCovered = kept.some( - (existing) => normalized === existing.path || normalized.startsWith(`${existing.path}${sep}`) + (existing) => bind.path === existing.path || bind.path.startsWith(`${existing.path}${sep}`) ); if (!alreadyCovered) { - kept.push({ path: normalized, mode: bind.mode }); + kept.push(bind); } } - return kept; + return kept + .sort((a, b) => a.index - b.index) + .map(({ path, mode }) => ({ path, mode })); } /** @@ -1055,7 +1089,84 @@ function reqStatement(label: string, command: string): string { * like from inside the isolated child, and is a strict superset of what a * non-nested source (e.g. `REPO_ROOT`, which has no sub-mounts on any tested * host) needs — so it is used uniformly for every entry, not conditionally. + * + * RECURSIVE READ-ONLY (P1, external review of ab415be6c): `--rbind` pulls in + * every submount under a `ro` bind's source directory, but the classic + * `mount -o remount,ro,bind ` step that follows it ONLY remounts the + * TOP mount at `` — Linux does not apply `remount,ro,bind` + * recursively to the submounts `--rbind` carried along, confirmed + * empirically: a nested bind mount created under a source directory (e.g. + * Docker's own `/etc/resolv.conf`-style injected submounts, or any other + * mount point that happens to exist under a `ro` bind's real path) stays + * WRITABLE after the parent's remount succeeds and reports `available: true` + * — the exact false-success shape this hardening closes. See + * `recursiveReadOnlyRemountCommand` below for the fix: after each `ro` + * bind's top-level remount, walk `/proc/self/mountinfo` (still readable at + * this point — it's pre-pivot, so this reads the CURRENT namespace's own + * view) for every mount point that is a descendant of that bind's staged + * path, and remount each ONE individually. Wrapped in `req` like every + * other mandatory step, so a submount that refuses to go read-only halts + * the whole prelude rather than silently leaving it writable. + */ + +/** + * Builds a shell command that finds every mount point strictly UNDER + * `stagedPathShQuoted` (the already-quoted staged path of a `ro` bind whose + * OWN top mount was just remounted read-only) by walking + * `/proc/self/mountinfo`, and remounts EACH ONE, individually, + * `ro,bind` — closing the gap `--rbind` (recursive bind) plus a single + * top-level `remount,ro,bind` leaves open: Linux does not propagate a + * `remount` operation to submounts the way `--rbind`/`--rprivate` propagate + * at BIND/PROPAGATION time, so any mount point that existed under a `ro` + * bind's source directory at bind time (e.g. Docker's own + * `/etc/resolv.conf`-style injected submounts under `/etc`, or any other + * nested mount a future derived bind might carry) stays writable unless + * remounted on its own. + * + * `/proc/self/mountinfo` FIELD FORMAT: whitespace-separated, field 5 (1 + * -indexed) is the mount point — `awk` splits on runs of whitespace by + * default, matching the format's own separator, so no custom field + * separator is needed. The kernel encodes literal space/tab/backslash/ + * newline bytes IN a mount point path as octal escapes (`\040` for space, + * etc.) specifically so single-whitespace-separated parsing like this stays + * unambiguous — every path this module's own derived bind set can ever + * contain (`REPO_ROOT`, `/usr`, `/etc`, the Node binary's directory, the + * Playwright cache, `filesystemBindPath`, which is always a `mkdtemp` + * result) is a plain filesystem path with no such bytes, so this function + * does not attempt to decode those escapes — a submount path that DID + * contain one would fail the subsequent `mount -o remount,ro,bind` step + * with a diagnosable "not mounted" error (caught by `req`, fail-closed) as + * an unescaped octal sequence, rather than silently matching or missing. + * + * ORDER: newest mounts appear later in `/proc/self/mountinfo`, so a mount + * nested two levels deep (a submount of a submount) is naturally remounted + * AFTER its own parent submount, in the same top-to-bottom order the file + * already lists them — `mount -o remount,ro,bind` on a path does not + * require any particular order relative to a SEPARATE mount point beneath + * it (each remount only affects its own mount, never cascades to a child + * the way the original bind/rbind did), so no explicit sort is needed + * beyond the file's own natural order. + * + * FAILS CLOSED: the caller wraps this in `reqStatement`, so if EVEN ONE + * submount refuses `remount,ro,bind` (a filesystem type that genuinely + * cannot be remounted read-only, a kernel refusal, or the path awk parsed + * simply not existing as a real mountpoint) the whole prelude halts before + * `exec ` is ever reached — never silently leaves that one submount + * writable and proceeds. */ +function recursiveReadOnlyRemountCommand(stagedPathShQuoted: string): string { + const findSubmounts = + `awk -v staged=${stagedPathShQuoted} -v stagedslash=${stagedPathShQuoted}"/" ` + + `'{ mp = $5; if (mp == staged) next; if (index(mp, stagedslash) == 1) print mp }' /proc/self/mountinfo`; + const loop = `for __submount in $(${findSubmounts}); do mount -o remount,ro,bind "$__submount" || exit 1; done`; + // `req` (see REQ_FUNCTION_DEFINITION's doc comment) executes its wrapped + // command as `"$@"` — a single command name plus argv entries, not a + // shell snippet — so a compound statement like this `for` loop must be + // handed to `req` as ONE argv entry, itself run via `sh -c`, rather than + // being split on whitespace the way a plain `mount ...` command is. + return `sh -c ${shQuote(loop)}`; +} + function filesystemClosureShellPrelude(filesystemBindPath: string | undefined, cwd: string | undefined): string { const newroot = "/tmp/pdpp-scenario-isolation-newroot"; const oldroot = `${newroot}/oldroot`; @@ -1082,6 +1193,7 @@ function filesystemClosureShellPrelude(filesystemBindPath: string | undefined, c statements.push(reqStatement(`bind ${bind.path}`, `mount --rbind ${shQuote(bind.path)} ${staged}`)); if (bind.mode === "ro") { statements.push(reqStatement(`remount ${bind.path} read-only`, `mount -o remount,ro,bind ${staged}`)); + statements.push(reqStatement(`remount ${bind.path} submounts read-only`, recursiveReadOnlyRemountCommand(staged))); } } if (filesystemBindPath !== undefined) { @@ -1237,12 +1349,46 @@ function filesystemClosureShellPrelude(filesystemBindPath: string | undefined, c export function postPivotVerificationStatements(binds: readonly FilesystemBind[]): string[] { const statements: string[] = []; const roBindPaths = binds.filter((b) => b.mode === "ro").map((b) => b.path); - const roCheck = roBindPaths + // Property 3 checks the TOP of each ro bind, at its real, post-pivot path + // (`/usr`, `/etc`, ... — the new root's OWN view, not the pre-pivot + // staging prefix `postPivotVerificationStatements`'s caller uses). + const topLevelRoCheck = roBindPaths .map((path) => { const probe = shQuote(`${path}/.pdpp-isolation-ro-probe-$$`); return `if touch ${probe} 2>/dev/null; then rm -f ${probe} 2>/dev/null; echo ${shQuote(path)}; fi`; }) .join("; "); + // RECURSIVE READ-ONLY, property 3 EXTENDED (P1, external review of + // ab415be6c): the top-level check above only proves the PARENT mount of + // each ro bind is read-only — it says nothing about a nested submount + // `--rbind` carried along underneath it (see `recursiveReadOnlyRemountCommand`'s + // doc comment for the full defect this closes). This verification must + // catch the same class of false-success the setup-time fix targets: if a + // FUTURE edit reintroduces the old single-level remount (or the + // recursive-remount loop silently skips a submount added after this + // function was written), the top-level-only check above would still + // report every ro bind's PARENT correctly read-only while a submount + // stayed writable — exactly invisible to that check alone. This walks + // `/proc/self/mountinfo` (POST-pivot, so it reflects the NEW root's own + // mount table — the real, live view the isolated child actually has, not + // the pre-pivot staging tree) for every mount point strictly under each ro + // bind's real path, and probes each one with the SAME touch-then-remove + // technique as the top-level check. + const submountRoCheck = roBindPaths + .map((path) => { + const quotedPath = shQuote(path); + const findSubmounts = + `awk -v staged=${quotedPath} -v stagedslash=${quotedPath}"/" ` + + `'{ mp = $5; if (mp == staged) next; if (index(mp, stagedslash) == 1) print mp }' /proc/self/mountinfo`; + return ( + `for __sm in $(${findSubmounts}); do ` + + `__smprobe="$__sm/.pdpp-isolation-ro-probe-$$"; ` + + `if touch "$__smprobe" 2>/dev/null; then rm -f "$__smprobe" 2>/dev/null; echo "$__sm"; fi; ` + + "done" + ); + }) + .join("; "); + const roCheck = [topLevelRoCheck, submountRoCheck].filter(Boolean).join("; "); statements.push( "__canary_ok=1; [ -e /pdpp-isolation-canary ] || __canary_ok=0; " + `__oldroot_leftover=$(set -- /oldroot/*; if [ -e "$1" ]; then echo "$1"; fi); ` + From 073454b842988c0cbbb526b6a7d103f2c3d718a3 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 2 Sep 2026 05:54:00 -0500 Subject: [PATCH 04/16] fix(isolation): reconcile the repository-UDS exception with a bounded pre-spawn scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit External review of ab415be6c flagged the known repository-UDS exception: a ro bind (REPO_ROOT included) blocks writes, not reads/dials, so a Unix domain socket that already existed under a ro bind at spawn time stays dialable from inside an isolated child. Confirmed live: curl reached a real REPO_ROOT-internal socket even with the trusted-launcher and recursive-ro fixes from the two prior commits applied. Recursive read-only closes the other half of this: once every submount of a ro bind is genuinely read-only, a connector cannot CREATE a new socket there during a run. That turns the exception into a finite, checkable precondition instead of an open-ended gap: findPreexistingSocketsUnderReadOnlyBinds() walks every user-writable ro bind (REPO_ROOT, the Node install dir, the Playwright cache — /usr and /etc are excluded, root-owned paths outside this module's DAC threat model and, measured, 4x the cost of REPO_ROOT alone for a check that can't find anything real there) immediately before spawn. bin/scenario-verify.ts runs the scan once per scenario (nothing new can appear mid-run) and feeds the result into evaluateClaimEligibility, which withholds recorded_replay and names every socket path found when the scan isn't empty. Wires isolationEvidenceBoundaryProven to isolationCapability.available now that the trusted-launcher and recursive-ro fixes are unconditionally baked into every isolated spawn (not an optional mode a caller can bypass) — recorded_replay is reachable again once all three isolation sub-conditions hold: namespace active, evidence boundary proven, no pre-existing sockets. New tests: findPreexistingSocketsUnderReadOnlyBinds finds a real socket planted under REPO_ROOT and stops finding it once removed (mutation-tested: returns [] without the walk); does not descend into symlinks. claims.ts gets dedicated eligibility tests for a non-empty scan result (withholds, names the path(s)), an empty result (does not withhold on this condition), and the priority ordering against the coarser isolation-inactive limitation. Correction (external independent review, local/isolation-r10-independent-0902.md): this commit originally claimed "Full package test suite (pnpm test, all bin/connectors/src tests): 0 failures." That claim was false and should have been scoped to the isolation-relevant files. The reviewer ran the full suite independently and found 19 failures (46 cancelled) across exactly three files unrelated to isolation by name or location: src/auto-login/ venmo.test.ts (one hung-Promise cascade cancelling 46 others), connectors/heb/index.test.ts (11 fails, browser-fixture tests), and connectors/codex/coverage-truthful.test.ts (2 fails, file-read-error coverage accounting). Each reproduced identically on d250afcf6, this branch's own parent commit, with none of the four isolation commits present — pre-existing, environment-sensitive failures (resource contention / browser-launch flakiness under 5300+ concurrently running tests), not caused by this repair. The isolation-specific surface is what actually matters here and is unambiguously clean: isolation-mechanism.test.ts 67 total / 57 pass / 0 fail / 10 pre-existing skips (23 bwrap-tagged, 24 unshare-tagged, both mechanisms 0 fail), scenario-cli.test.ts 51/51, scenario-verify-strict.test.ts 86/86 — independently reconfirmed after rebasing this branch onto current origin/main (PR #269, #272 merged since), run in a privileged Docker container with bwrap/unshare installed. Rerunning the full pnpm test suite under concurrent full-suite load also surfaces isolation-mechanism.test.ts and connectors/slack/slackdump-runtime.test.ts as occasional failures alongside codex/coverage-truthful.test.ts; both reran clean (0 fail) every time in isolation, confirming resource contention under ~5300 concurrent tests in a shared container, not a code regression — codex/coverage-truthful.test.ts is the one file that also fails standalone on the pre-isolation parent commit, confirming it alone is genuinely pre-existing and unrelated to this branch's code. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../bin/scenario-verify-strict.test.ts | 80 +++++++++++ .../bin/scenario-verify.ts | 57 ++++++-- .../src/scenario/claims.ts | 56 +++++++- .../src/scenario/isolation-mechanism.test.ts | 84 ++++++++++- .../src/scenario/isolation.ts | 136 +++++++++++++++++- 5 files changed, 386 insertions(+), 27 deletions(-) diff --git a/packages/polyfill-connectors/bin/scenario-verify-strict.test.ts b/packages/polyfill-connectors/bin/scenario-verify-strict.test.ts index d4a1d298d..7209c9e31 100644 --- a/packages/polyfill-connectors/bin/scenario-verify-strict.test.ts +++ b/packages/polyfill-connectors/bin/scenario-verify-strict.test.ts @@ -858,6 +858,7 @@ function eligibleDigestObservations(): { observedUnsupportedEvidenceSurface: boolean; driverEvidenceSatisfied: boolean; isolationEvidenceBoundaryProven: boolean; + preexistingSocketsUnderReadOnlyBinds: readonly string[]; } { return { capturedDeclarationDigestPresent: true, @@ -867,6 +868,7 @@ function eligibleDigestObservations(): { observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, isolationEvidenceBoundaryProven: true, + preexistingSocketsUnderReadOnlyBinds: [], }; } @@ -957,6 +959,7 @@ test("evaluateClaimEligibility negative control: source-only historical scenario observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, isolationEvidenceBoundaryProven: true, + preexistingSocketsUnderReadOnlyBinds: [], isNamespaceIsolationActive: true, }); assert.equal(decision.claim, "diagnostic_replay"); @@ -975,6 +978,7 @@ test("evaluateClaimEligibility negative control: declaration-only scenario (sour observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, isolationEvidenceBoundaryProven: true, + preexistingSocketsUnderReadOnlyBinds: [], isNamespaceIsolationActive: true, }); assert.equal(decision.claim, "diagnostic_replay"); @@ -993,6 +997,7 @@ test("evaluateClaimEligibility negative control: missing current manifest (decla observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, isolationEvidenceBoundaryProven: true, + preexistingSocketsUnderReadOnlyBinds: [], isNamespaceIsolationActive: true, }); assert.equal(decision.claim, "diagnostic_replay"); @@ -1011,6 +1016,7 @@ test("evaluateClaimEligibility negative control: missing current connector sourc observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, isolationEvidenceBoundaryProven: true, + preexistingSocketsUnderReadOnlyBinds: [], isNamespaceIsolationActive: true, }); assert.equal(decision.claim, "diagnostic_replay"); @@ -1036,6 +1042,7 @@ test("evaluateClaimEligibility negative control: legacy top-level digests only ( observedUnsupportedEvidenceSurface: false, driverEvidenceSatisfied: true, isolationEvidenceBoundaryProven: true, + preexistingSocketsUnderReadOnlyBinds: [], isNamespaceIsolationActive: true, }); assert.equal(decision.claim, "diagnostic_replay"); @@ -1119,6 +1126,7 @@ test("evaluateClaimEligibility: multiple failing conditions are all reported at driverEvidenceSatisfied: false, isNamespaceIsolationActive: false, isolationEvidenceBoundaryProven: false, + preexistingSocketsUnderReadOnlyBinds: [], }); assert.equal(decision.claim, "diagnostic_replay"); assert.ok(decision.claim === "diagnostic_replay"); @@ -1197,6 +1205,7 @@ test("evaluateClaimEligibility: namespace isolation active AND isolationEvidence ...eligibleDigestObservations(), isNamespaceIsolationActive: true, isolationEvidenceBoundaryProven: true, + preexistingSocketsUnderReadOnlyBinds: [], }); assert.deepEqual(decision, { claim: "recorded_replay" }); }); @@ -1218,6 +1227,77 @@ test("evaluateClaimEligibility: namespace isolation NOT active reports only the assert.deepEqual(decision.limitations, ["network isolation: process-local only - descendant escape not excluded"]); }); +// ─── Repository-UDS exception, reconciled (P1, external review of ab415be6c) +// ──────────────────────────────────────────────────────────────────────────── +// +// Recursive read-only closes the ability to CREATE a socket under a ro +// bind, but not the ability to DIAL one that already existed at spawn time +// — see claims.ts's `preexistingSocketsUnderReadOnlyBinds` doc comment. +// These tests pin the eligibility gate's own handling of the scan result: +// a non-empty result withholds recorded_replay and names every path found; +// an empty result does not withhold on this condition at all. + +test("evaluateClaimEligibility: a non-empty preexistingSocketsUnderReadOnlyBinds withholds recorded_replay and names the socket path", () => { + const decision = evaluateClaimEligibility({ + scenario: eligibleScenario(), + isEntrypointOverride: false, + ...eligibleDigestObservations(), + isNamespaceIsolationActive: true, + isolationEvidenceBoundaryProven: true, + preexistingSocketsUnderReadOnlyBinds: ["/repo/root/.leftover.sock"], + }); + assert.equal(decision.claim, "diagnostic_replay"); + assert.ok(decision.claim === "diagnostic_replay"); + assert.deepEqual(decision.limitations, [ + "pre-existing socket(s) found under a read-only bind at spawn time, dialable despite recursive read-only: /repo/root/.leftover.sock", + ]); +}); + +test("evaluateClaimEligibility: multiple preexistingSocketsUnderReadOnlyBinds are all named in one limitation, comma-joined", () => { + const decision = evaluateClaimEligibility({ + scenario: eligibleScenario(), + isEntrypointOverride: false, + ...eligibleDigestObservations(), + isNamespaceIsolationActive: true, + isolationEvidenceBoundaryProven: true, + preexistingSocketsUnderReadOnlyBinds: ["/repo/root/a.sock", "/repo/root/nested/b.sock"], + }); + assert.ok(decision.claim === "diagnostic_replay"); + assert.deepEqual(decision.limitations, [ + "pre-existing socket(s) found under a read-only bind at spawn time, dialable despite recursive read-only: /repo/root/a.sock, /repo/root/nested/b.sock", + ]); +}); + +test("evaluateClaimEligibility: an empty preexistingSocketsUnderReadOnlyBinds does not withhold on this condition — recorded_replay reachable", () => { + const decision = evaluateClaimEligibility({ + scenario: eligibleScenario(), + isEntrypointOverride: false, + ...eligibleDigestObservations(), + isNamespaceIsolationActive: true, + isolationEvidenceBoundaryProven: true, + preexistingSocketsUnderReadOnlyBinds: [], + }); + assert.deepEqual(decision, { claim: "recorded_replay" }); +}); + +test("evaluateClaimEligibility: the socket-scan limitation only fires when isolation is active AND the evidence boundary is proven (not a fourth, independent gate)", () => { + // If isolation isn't active at all, the coarser process-local limitation + // must fire instead — a non-empty socket scan result is meaningless + // (and, in bin/scenario-verify.ts's real wiring, always empty) when + // isolation was never active for this run. + const decision = evaluateClaimEligibility({ + scenario: eligibleScenario(), + isEntrypointOverride: false, + ...eligibleDigestObservations(), + isNamespaceIsolationActive: false, + isolationEvidenceBoundaryProven: false, + preexistingSocketsUnderReadOnlyBinds: ["/repo/root/.leftover.sock"], + }); + assert.equal(decision.claim, "diagnostic_replay"); + assert.ok(decision.claim === "diagnostic_replay"); + assert.deepEqual(decision.limitations, ["network isolation: process-local only - descendant escape not excluded"]); +}); + // ─── Repair wave 6, P1-1: driver-evidence prerequisite ───────────────────── // // `wire-registry.ts`'s `DRIVER_EVIDENCE_POLICIES` map — `recorded-http`'s diff --git a/packages/polyfill-connectors/bin/scenario-verify.ts b/packages/polyfill-connectors/bin/scenario-verify.ts index a19dd4ba3..f597371a8 100644 --- a/packages/polyfill-connectors/bin/scenario-verify.ts +++ b/packages/polyfill-connectors/bin/scenario-verify.ts @@ -92,6 +92,7 @@ import { import { evaluateClaimEligibility } from "../src/scenario/claims.ts"; import type { ConnectorScenario, ScenarioUserInteraction } from "../src/scenario/format.ts"; import { + findPreexistingSocketsUnderReadOnlyBinds, type IsolationMechanism, isNamespaceIsolationAvailable, type NamespaceIsolationCapability, @@ -1387,6 +1388,27 @@ function resolveIsolationMechanism(capability: NamespaceIsolationCapability): fa return capability.available ? capability.mechanism : false; } +/** + * REPOSITORY-UDS EXCEPTION, RECONCILED (P1, external review of ab415be6c) — + * recursive read-only (isolation.ts's `recursiveReadOnlyRemountCommand`) + * closes the ability to CREATE a socket under a `ro` bind during a run, so + * scanning ONCE, here, before any run's subprocess spawns, covers the whole + * scenario: nothing new can appear under a `ro` bind while replay runs (a + * connector cannot write there at all). Only meaningful when isolation is + * actually active for this replay — an empty scan result under + * process-local-only isolation would be misleading (the escape this scan + * closes is specific to the OS-isolation boundary), so this returns an empty + * array without scanning otherwise, which is also the CORRECT input for + * `evaluateClaimEligibility` in that case — the coarser + * `isNamespaceIsolationActive: false` limitation already fires and takes + * priority, per that function's `else if` chain. Split out of `main` purely + * to keep `main`'s own cognitive complexity under this package's lint + * ceiling — behavior is unchanged from the inline version. + */ +function scanPreexistingSocketsIfIsolated(isolationCapability: NamespaceIsolationCapability): readonly string[] { + return isolationCapability.available ? findPreexistingSocketsUnderReadOnlyBinds() : []; +} + async function main(): Promise { const args = parseArgs(process.argv.slice(2)); const connectorPath = resolveConnectorPath(args); @@ -1455,6 +1477,7 @@ async function main(): Promise { ? "network isolation: os-namespace" : `network isolation: process-local only (${isolationCapability.reason})`; process.stdout.write(` ${isolationLine}\n`); + const preexistingSocketsUnderReadOnlyBinds = scanPreexistingSocketsIfIsolated(isolationCapability); // Every replayed response is served from the recording, not a live // provider, so a connector's own pacing/backoff timers (governor pacing, // an inline PAGE_DELAY sleep, anything else built on setTimeout/ @@ -1716,7 +1739,8 @@ async function main(): Promise { isolationLine, digestObservation, isolationCapability, - observedUnsupportedEvidenceSurface(allRunMessages) + observedUnsupportedEvidenceSurface(allRunMessages), + preexistingSocketsUnderReadOnlyBinds ); process.exitCode = 0; } @@ -1748,7 +1772,8 @@ function printCoverageReport( isolationLine: string, digestObservation: CaptureSourceDigestObservation, isolationCapability: NamespaceIsolationCapability, - observedUnsupportedEvidenceSurfaceFlag: boolean + observedUnsupportedEvidenceSurfaceFlag: boolean, + preexistingSocketsUnderReadOnlyBinds: readonly string[] ): void { const capturedAt = scenario.capture.captured_at; // state_seeded_second_run_with_changed_requests (formerly named @@ -1844,16 +1869,24 @@ function printCoverageReport( currentDeclarationDigestComputed: digestObservation.currentDeclarationDigestComputed, currentSourceDigestComputed: digestObservation.currentSourceDigestComputed, isNamespaceIsolationActive: isolationCapability.available, - // WITHHELD PENDING BOUNDED P1 REPAIR (external review of ab415be6c): - // hardcoded false until the trusted-launcher resolution and the - // recursive-read-only post-pivot verification are both wired in and - // proven — see claims.ts's `isolationEvidenceBoundaryProven` doc comment. - // Every intermediate state of this repair must stay honest: a build that - // has the launcher-trust or recursive-ro fix applied only partially must - // still print `diagnostic_replay`, never `recorded_replay`, until this - // literal is flipped to the real, wired-in proof in the commit that - // completes both fixes. - isolationEvidenceBoundaryProven: false, + // BOUNDED P1 REPAIR COMPLETE (external review of ab415be6c): the + // trusted-launcher resolution (isolation.ts's `resolveTrustedLauncherPath`) + // and the recursive-read-only filesystem closure + // (`recursiveReadOnlyRemountCommand`/`postPivotVerificationStatements`'s + // per-submount check) are both now UNCONDITIONALLY wired into every + // isolated spawn `spawnWithNetworkIsolation` performs — not an optional + // mode a caller can bypass — so whenever OS-namespace isolation is + // active at all, these two proofs were necessarily also in effect for + // this replay. `isNamespaceIsolationActive` (immediately above) already + // reflects whether isolation was active; this field is therefore the + // same fact restated for `evaluateClaimEligibility`'s own, separately + // named condition — kept as a distinct field (rather than collapsed into + // `isNamespaceIsolationActive`) so the eligibility gate's three isolation + // sub-conditions (namespace-active, evidence-boundary-proven, + // no-preexisting-sockets) stay independently readable in + // `claims.ts`, matching every other split condition on this interface. + isolationEvidenceBoundaryProven: isolationCapability.available, + preexistingSocketsUnderReadOnlyBinds, observedUnsupportedEvidenceSurface: observedUnsupportedEvidenceSurfaceFlag, driverEvidenceSatisfied: driverEvidenceOk, }); diff --git a/packages/polyfill-connectors/src/scenario/claims.ts b/packages/polyfill-connectors/src/scenario/claims.ts index 4e0ff2201..675b76fda 100644 --- a/packages/polyfill-connectors/src/scenario/claims.ts +++ b/packages/polyfill-connectors/src/scenario/claims.ts @@ -6,7 +6,9 @@ * binding split and the ASSISTANCE-withholding condition added repair wave * 4, P1-1/P1-2; driver-evidence prerequisite added repair wave 6, P1-1; * recorded-browser driver support and its mandatory staleness limitation - * added alongside `browser-har-replay.ts`). + * added alongside `browser-har-replay.ts`; isolation-evidence-boundary and + * repository-UDS-socket-scan conditions added by the bounded P1 repair + * following external review of the merged tree ab415be6c). * * `bin/scenario-verify.ts` used to print `recorded_replay: PASS` the moment * every per-run comparison passed — but a passing comparison only proves the @@ -90,6 +92,23 @@ import type { ConnectorScenario } from "./format.ts"; export type ScenarioStalenessLimitation = `recorded-browser: verified against capture of ${string}; asserts nothing about the live provider`; +/** + * The repository-UDS-socket withholding limitation, PARAMETERIZED on the + * comma-joined list of pre-existing socket paths `findPreexistingSocketsUnderReadOnlyBinds()` + * found under a `ro` bind for THIS run — a TypeScript template-literal type, + * matching `ScenarioStalenessLimitation`'s own reasoning: the path(s) are + * genuinely per-run data, not a closed enum value. See + * `buildPreexistingSocketLimitation` below for the single place this string + * is constructed, and this module's doc comment ("REPOSITORY-UDS EXCEPTION, + * RECONCILED") for the reasoning: recursive read-only (isolation.ts's + * `recursiveReadOnlyRemountCommand`) closes the ability to CREATE a socket + * under a `ro` bind, so the only sockets an isolated child can ever dial + * through one are sockets that ALREADY EXISTED at spawn time — a finite, + * checkable fact about this specific run, not an open-ended exception. + */ +export type PreexistingSocketLimitation = + `pre-existing socket(s) found under a read-only bind at spawn time, dialable despite recursive read-only: ${string}`; + export type ClaimLimitation = | "unbound entrypoint replay" | "no capture-time declaration digest" @@ -104,8 +123,20 @@ export type ClaimLimitation = | "connector exercised an evidence surface the oracle cannot observe (ASSISTANCE)" | "no recorded provider interaction - driver evidence for recorded-http not satisfied" | "no recorded HAR entries - driver evidence for recorded-browser not satisfied" + | PreexistingSocketLimitation | ScenarioStalenessLimitation; +/** + * Builds the exact repository-UDS-socket limitation string for a run whose + * pre-existing-socket scan found at least one match. `socketPaths` must be + * non-empty — callers only invoke this when + * `preexistingSocketsUnderReadOnlyBinds.length > 0` (see + * `evaluateClaimEligibility`). + */ +export function buildPreexistingSocketLimitation(socketPaths: readonly string[]): PreexistingSocketLimitation { + return `pre-existing socket(s) found under a read-only bind at spawn time, dialable despite recursive read-only: ${socketPaths.join(", ")}`; +} + /** * Builds the exact staleness limitation string for a scenario carrying at * least one `recorded-browser` run — see `ScenarioStalenessLimitation`'s doc @@ -194,6 +225,27 @@ export interface ClaimEligibilityInput { * oracle cannot observe, so even an otherwise-fully-eligible run must not * print the unqualified `recorded_replay: PASS` claim. */ observedUnsupportedEvidenceSurface: boolean; + /** + * REPOSITORY-UDS EXCEPTION, RECONCILED (P1, external review of ab415be6c): + * a `ro` bind (e.g. `REPO_ROOT`) blocks WRITES, not reads/dials — a Unix + * domain socket file that already existed under a `ro` bind at spawn time + * stays dialable from inside the isolated child regardless of recursive + * read-only, confirmed empirically (a `curl --unix-socket` against a real + * `REPO_ROOT`-internal socket succeeds with both the trusted-launcher and + * recursive-ro fixes applied). Recursive read-only DOES close the other + * half: a connector cannot CREATE a new socket under a `ro` bind once + * every submount is genuinely read-only, so the only sockets reachable + * this way are ones that existed BEFORE the spawn — a finite, checkable + * precondition, not an open-ended gap. `bin/scenario-verify.ts` populates + * this from `isolation.ts`'s `findPreexistingSocketsUnderReadOnlyBinds()`, + * run immediately before spawning. An EMPTY array means the scan found + * nothing — combined with `isolationEvidenceBoundaryProven`, this is what + * justifies `recorded_replay`'s OS-isolation claim being airtight for this + * specific run; a NON-EMPTY array withholds the strong claim and names + * every path found (see `buildPreexistingSocketLimitation`), rather than + * silently accepting the old, undocumented, open-ended exception. + */ + preexistingSocketsUnderReadOnlyBinds: readonly string[]; scenario: ConnectorScenario; } @@ -275,6 +327,8 @@ export function evaluateClaimEligibility(input: ClaimEligibilityInput): ClaimDec limitations.push( "network isolation: launcher trust or recursive read-only filesystem closure not proven for this run" ); + } else if (input.preexistingSocketsUnderReadOnlyBinds.length > 0) { + limitations.push(buildPreexistingSocketLimitation(input.preexistingSocketsUnderReadOnlyBinds)); } if (input.observedUnsupportedEvidenceSurface) { limitations.push("connector exercised an evidence surface the oracle cannot observe (ASSISTANCE)"); diff --git a/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts b/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts index 72e53f0dd..05e1c80f8 100644 --- a/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts +++ b/packages/polyfill-connectors/src/scenario/isolation-mechanism.test.ts @@ -10,7 +10,17 @@ // under it has no outbound network. import assert from "node:assert/strict"; import { spawn, spawnSync } from "node:child_process"; -import { cpSync, existsSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, writeFileSync } from "node:fs"; +import { + cpSync, + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + readlinkSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { createServer } from "node:http"; import { homedir, tmpdir } from "node:os"; import { join, resolve as resolvePath } from "node:path"; @@ -18,6 +28,7 @@ import test from "node:test"; import { fileURLToPath } from "node:url"; import { bwrapArgvForFilesystemClosure, + findPreexistingSocketsUnderReadOnlyBinds, isNamespaceIsolationAvailable, postPivotVerificationStatements, requiredFilesystemBinds, @@ -64,7 +75,6 @@ function canBindMountOverAFile(): boolean { const bindMountCapable = unshareUsable && canBindMountOverAFile(); - test("a host that denies `unshare` but ships a working bwrap still reports isolation AVAILABLE", { skip: !bwrapUsable, }, () => { @@ -129,7 +139,7 @@ test("an isolated child has NO outbound network — the property, not the mechan // via `withShimmedTrustedBinary` (defined below — a hoisted function // declaration, callable here despite the later textual position). test("[bwrap] the fixed probeBwrap() invokes bwrap with the SAME production argv shape bwrapArgvForFilesystemClosure builds — not a bare --dev-bind / / check", { - skip: !bwrapUsable || !bindMountCapable, + skip: !(bwrapUsable && bindMountCapable), }, async () => { const logDir = mkdtempSync(join(tmpdir(), "pdpp-isolation-probe-argv-log-")); const logPath = join(logDir, "invocations.log"); @@ -139,7 +149,7 @@ test("[bwrap] the fixed probeBwrap() invokes bwrap with the SAME production argv "bwrap", (realBwrapPath) => ["#!/bin/sh", `echo "$*" >> ${JSON.stringify(logPath)}`, `exec ${realBwrapPath} "$@"`].join("\n"), - async () => { + () => { const cap = isNamespaceIsolationAvailable(); // Only meaningful when bwrap is genuinely what got selected (on a // host where unshare is denied but bwrap works — this suite's own @@ -166,6 +176,7 @@ test("[bwrap] the fixed probeBwrap() invokes bwrap with the SAME production argv `probe argv must include the derived requiredFilesystemBinds() entries, not just namespace flags; got ${JSON.stringify(probeArgv)}` ); } + return Promise.resolve(); } ); } finally { @@ -288,7 +299,6 @@ test("probe falls back to bwrap when unshare's procfs mount is refused but bwrap ); }); - test("a forced PID-ns procfs-mount refusal inside the real unshare-mechanism prelude fails the spawn closed, never silently proceeds", { skip: !bindMountCapable, }, async () => { @@ -912,7 +922,7 @@ test("[bwrap sandbox] postPivotVerificationStatements FAILS when /oldroot is non }); test("[bwrap sandbox] postPivotVerificationStatements FAILS when a NESTED submount under a genuinely-read-only ro bind is writable (P1, external review of ab415be6c)", { - skip: !bwrapUsable || !bindMountCapable, + skip: !(bwrapUsable && bindMountCapable), }, () => { // Unlike scenario (b) above (a bind whose OWN top mount never went // read-only), this proves the EXTENDED property: the top-level bind IS @@ -1045,7 +1055,7 @@ function loggingShim(name: string, logPath: string): (realBinaryPath: string) => return (_realBinaryPath: string) => `#!/bin/sh\necho "${name} $*" >> ${JSON.stringify(logPath)}\nexit 0\n`; } -async function withBothLaunchersLoggingShimmed(logPath: string, fn: () => Promise): Promise { +function withBothLaunchersLoggingShimmed(logPath: string, fn: () => Promise): Promise { return withShimmedTrustedBinary("unshare", loggingShim("unshare", logPath), () => withShimmedTrustedBinary("bwrap", loggingShim("bwrap", logPath), fn) ); @@ -1878,6 +1888,66 @@ for (const mechanism of ["bwrap", "unshare"] as const) { }); } +// ─── Repository-UDS exception, reconciled: findPreexistingSocketsUnderReadOnlyBinds +// (P1, external review of ab415be6c) ────────────────────────────────────── +// +// Recursive read-only (proven above) closes the ability to CREATE a socket +// under a ro bind, but a socket that already existed at spawn time stays +// dialable — a ro bind blocks writes, not reads/dials. These tests prove +// the reconciling scan itself: it finds a real, nested socket under +// REPO_ROOT (no root/bind-mount capability needed — creating a UDS file is +// an ordinary, unprivileged filesystem operation), and stops finding it the +// moment it's removed — closing the loop the claim-eligibility gate depends +// on (see the `evaluateClaimEligibility` tests in bin/scenario-verify-strict.test.ts +// for the claim-text side of this same reconciliation). + +test("findPreexistingSocketsUnderReadOnlyBinds: finds a real socket nested under REPO_ROOT, and stops finding it once removed", async () => { + const nestedDir = join(TEST_REPO_ROOT, `.pdpp-socket-scan-probe-${String(process.pid)}`); + mkdirSync(nestedDir, { recursive: true }); + const socketPath = join(nestedDir, "leftover.sock"); + const server = createServer(); + try { + await new Promise((resolveListen, rejectListen) => { + server.once("error", rejectListen); + server.listen(socketPath, resolveListen); + }); + + const foundWhilePresent = findPreexistingSocketsUnderReadOnlyBinds(); + assert.ok( + foundWhilePresent.includes(socketPath), + `expected the scan to find the real socket at ${JSON.stringify(socketPath)}; got ${JSON.stringify(foundWhilePresent)}` + ); + + await new Promise((resolveClose) => server.close(() => resolveClose())); + rmSync(socketPath, { force: true }); + + const foundAfterRemoval = findPreexistingSocketsUnderReadOnlyBinds(); + assert.ok( + !foundAfterRemoval.includes(socketPath), + `expected the scan to stop finding the socket once removed; got ${JSON.stringify(foundAfterRemoval)}` + ); + } finally { + server.close(); + rmSync(nestedDir, { recursive: true, force: true }); + } +}); + +test("findPreexistingSocketsUnderReadOnlyBinds: does NOT descend into symlinks (avoids an unbounded/cyclic walk)", () => { + const nestedDir = join(TEST_REPO_ROOT, `.pdpp-socket-scan-symlink-probe-${String(process.pid)}`); + mkdirSync(nestedDir, { recursive: true }); + const symlinkPath = join(nestedDir, "self-loop"); + try { + // A symlink pointing back at its own parent directory — if the scan + // followed symlinks, this would recurse forever (or at least far beyond + // the bounded, finite walk this function's own doc comment promises). + symlinkSync(nestedDir, symlinkPath, "dir"); + const result = findPreexistingSocketsUnderReadOnlyBinds(); + assert.ok(Array.isArray(result), "the scan must complete (not hang/throw) against a self-referential symlink"); + } finally { + rmSync(nestedDir, { recursive: true, force: true }); + } +}); + // ─── cwd survives the filesystem closure (R9) ────────────────────────────── // // Node's `spawn(cmd, args, { cwd })` only sets the working directory of the diff --git a/packages/polyfill-connectors/src/scenario/isolation.ts b/packages/polyfill-connectors/src/scenario/isolation.ts index 5353c1f15..43f40042f 100644 --- a/packages/polyfill-connectors/src/scenario/isolation.ts +++ b/packages/polyfill-connectors/src/scenario/isolation.ts @@ -119,6 +119,19 @@ * widening the sandbox — the opposite failure direction from a mask list, * where a missed entry fails open and invisibly. * + * REMAINING GAP, RECONCILED (P1, external review of ab415be6c): default-deny + * closes reachability for a FOREIGN path outside the derived set, but a `ro` + * bind (the derived set's own entries, including `REPO_ROOT`) only blocks + * WRITES — a socket file that ALREADY EXISTS somewhere under `REPO_ROOT` (or + * any other `ro` bind) at spawn time stays dialable, `connect()` needing no + * write permission. Recursive read-only (`recursiveReadOnlyRemountCommand`) + * closes the ability to CREATE a new one during a run, turning this into a + * finite, checkable precondition rather than an open-ended exception: + * `findPreexistingSocketsUnderReadOnlyBinds()` scans for exactly this + * before every spawn, and `bin/scenario-verify.ts` withholds + * `recorded_replay` — naming the exact path — whenever the scan finds one. + * See that function's own doc comment for the full mechanism. + * * CAPABILITY DETECTION: unprivileged user-namespace creation is not * guaranteed available. It can be disabled at the kernel level * (`kernel.unprivileged_userns_clone=0`, some hardened distros/containers) @@ -151,7 +164,12 @@ * host process enumeration or `/proc//cmdline` read, no `kill(pid, 0)` * reachability), mount/filesystem (default-deny: only the derived allowlist * plus `filesystemBindPath` are visible, closing pathname-UDS dials to any - * foreign socket outside that set — see PATHNAME-UDS ESCAPE above), SysV IPC + * FOREIGN socket outside that set — see PATHNAME-UDS ESCAPE above — AND, + * recursively, every submount under a `ro` bind, not just its top mount — + * see `recursiveReadOnlyRemountCommand`; a pre-existing socket already + * inside a `ro` bind at spawn time is a separate, RECONCILED case, checked + * per-run by `findPreexistingSocketsUnderReadOnlyBinds()`, not something + * this static filesystem view alone closes), SysV IPC * (no `/proc/sysvipc/*` enumeration of host shared memory/semaphores/message * queues), UTS (hostname/domainname). Deliberately NOT isolated, by * conscious scope decision rather than oversight: the CGROUP namespace (an @@ -212,10 +230,12 @@ import { type ChildProcess, type SpawnOptions, spawn, spawnSync } from "node:chi import { accessSync, constants, + type Dirent, existsSync, lstatSync, mkdirSync, mkdtempSync, + readdirSync, readlinkSync, realpathSync, rmSync, @@ -722,6 +742,105 @@ export function requiredFilesystemBinds(): readonly FilesystemBind[] { return dedupeBinds(binds); } +/** + * PRE-EXISTING-SOCKET SCAN — reconciles the repository-UDS exception (P1, + * external review of ab415be6c). Recursive read-only (see + * `recursiveReadOnlyRemountCommand`) closes writes into any `ro` bind's + * submounts, but a `ro` bind only blocks WRITES, not reads/dials: a Unix + * domain socket file that ALREADY EXISTS somewhere under a `ro` bind at + * spawn time (most concretely, anywhere under `REPO_ROOT`) stays perfectly + * DIALABLE from inside the isolated child — `connect()` to an existing UDS + * needs no write permission on the socket or its containing directory, + * confirmed empirically (a `curl --unix-socket` against a real + * `REPO_ROOT`-internal socket succeeds even with both the trusted-launcher + * and recursive-ro fixes applied). Recursive read-only genuinely closes the + * other half of this: a CONNECTOR CANNOT CREATE a new socket anywhere under + * a `ro` bind once every submount is genuinely read-only — so the only + * sockets an isolated child can ever dial through a `ro` bind are ones that + * were ALREADY THERE before the spawn. That turns an open-ended "the + * closure isn't universal inside the repo bind" gap into a FINITE, checkable + * precondition: scan every `ro` bind for a socket inode immediately before + * spawning, and if the scan finds ANY, that specific run cannot honestly + * claim the OS-isolation boundary is airtight — the finding is fed into the + * `recorded_replay` eligibility decision (see + * `bin/scenario-verify.ts`'s `isolationEvidenceBoundaryProven` wiring), + * withholding the strong claim and naming the exact socket path, rather than + * silently accepting an unbounded, undocumented exception forever. + * + * BOUNDED, not a mask list: this is the opposite shape from the + * `worldWritableTempDirs` mask-list architecture this module's own module + * doc comment explains was proven unable to terminate — that list tried to + * enumerate every directory a FOREIGN socket might live under, which is + * unbounded in principle. This scan instead enumerates every socket that + * ALREADY EXISTS under the module's OWN finite, derived bind set right now, + * a concrete, checkable fact about THIS run, not a guess about what a + * foreign process might place somewhere in the future. + * + * Does not follow symlinks (`Dirent.isSymbolicLink()` entries are skipped + * entirely, neither descended into nor stat'd as a socket themselves) — + * matches every other traversal in this module's own filesystem-closure + * logic, which never follows a symlink outside the bind it was found under, + * and avoids a symlink cycle turning this bounded scan unbounded. + * + * Read-metadata only (`Dirent`'s own type flags from `readdirSync`, no + * `lstatSync` call per entry, no file content read) — confirmed empirically + * to complete in a couple hundred milliseconds against this repository's + * full `node_modules` tree (~130k entries), negligible next to a scenario + * replay's own runtime. + * + * SCOPED TO USER-WRITABLE `ro` BINDS, NOT EVERY `requiredFilesystemBinds()` + * ENTRY: `/usr` and `/etc` are excluded — confirmed empirically, walking + * `/usr` alone costs ~700ms (690k entries) on a typical dev host, more than + * 4x `REPO_ROOT`'s own cost, for a check that cannot find anything real. A + * socket under `/usr`/`/etc` requires root (or an OS package/container-build + * step) to plant — this module's own DAC reasoning elsewhere already treats + * that as outside its threat model ("no new privilege, since DAC still + * applies... the isolated child runs as the same real UID as the parent" — + * see `FilesystemBind`'s doc comment), the same way a root-capable attacker + * could defeat this whole isolation boundary by many other means. `REPO_ROOT` + * (the repo checkout, writable by the calling user's own build/checkout + * process — the ACTUAL exception the external review named), `nodeDir` (a + * per-user version-manager install, e.g. under `~/.local/share/mise/...`), + * and the Playwright browser cache (also under the user's `$HOME`) are all + * scanned — every bind ordinarily writable by the SAME user this process + * itself runs as, which is the set that could plausibly have a socket + * planted under it without root. + */ +const SOCKET_SCAN_EXCLUDED_SYSTEM_PATHS: readonly string[] = ["/usr", "/etc"]; + +export function findPreexistingSocketsUnderReadOnlyBinds(): readonly string[] { + const found: string[] = []; + for (const bind of requiredFilesystemBinds()) { + if (SOCKET_SCAN_EXCLUDED_SYSTEM_PATHS.includes(bind.path)) { + continue; + } + walkForSockets(bind.path, found); + } + return found; +} + +function walkForSockets(dir: string, found: string[]): void { + let entries: Dirent[]; + try { + entries = readdirSync(dir, { withFileTypes: true }); + } catch { + return; + } + for (const entry of entries) { + if (entry.isSymbolicLink()) { + continue; + } + const entryPath = join(dir, entry.name); + if (entry.isDirectory()) { + walkForSockets(entryPath, found); + continue; + } + if (entry.isSocket()) { + found.push(entryPath); + } + } +} + /** * Drops any bind whose path is identical to, or a filesystem descendant of, * ANOTHER entry in the list, regardless of which was declared first — @@ -769,9 +888,7 @@ function dedupeBinds(binds: readonly FilesystemBind[]): FilesystemBind[] { kept.push(bind); } } - return kept - .sort((a, b) => a.index - b.index) - .map(({ path, mode }) => ({ path, mode })); + return kept.sort((a, b) => a.index - b.index).map(({ path, mode }) => ({ path, mode })); } /** @@ -961,7 +1078,6 @@ function requiredFhsCompatSymlinks(): readonly FhsCompatSymlink[] { */ const TRUSTED_SETUP_PATH = "/usr/sbin:/usr/bin:/sbin:/bin"; - /** * The single fixed exit code every mandatory setup step in * `filesystemClosureShellPrelude` uses on failure (distinct from `97`, the @@ -1193,7 +1309,9 @@ function filesystemClosureShellPrelude(filesystemBindPath: string | undefined, c statements.push(reqStatement(`bind ${bind.path}`, `mount --rbind ${shQuote(bind.path)} ${staged}`)); if (bind.mode === "ro") { statements.push(reqStatement(`remount ${bind.path} read-only`, `mount -o remount,ro,bind ${staged}`)); - statements.push(reqStatement(`remount ${bind.path} submounts read-only`, recursiveReadOnlyRemountCommand(staged))); + statements.push( + reqStatement(`remount ${bind.path} submounts read-only`, recursiveReadOnlyRemountCommand(staged)) + ); } } if (filesystemBindPath !== undefined) { @@ -1485,7 +1603,11 @@ export function spawnWithNetworkIsolation( // resolveTrustedLauncherPath, never a bare "bwrap" name that node: // child_process would otherwise resolve through this process's own // inherited $PATH — see that function's doc comment. - return spawn(resolveTrustedLauncherPath("bwrap"), bwrapArgvForFilesystemClosure(cmd, args, filesystemBindPath), spawnOpts); + return spawn( + resolveTrustedLauncherPath("bwrap"), + bwrapArgvForFilesystemClosure(cmd, args, filesystemBindPath), + spawnOpts + ); } const innerCommand = [cmd, ...args].map(shQuote).join(" "); // `spawnOpts.cwd` (a `string | URL | undefined` per `SpawnOptions`) is From 8dc7f06ab254a10cbd0da6e62a51d8ccbc74538c Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 2 Sep 2026 07:19:00 -0500 Subject: [PATCH 05/16] fix(scenario-verify): state recorded_replay's preconditions inline The R10 independent review (local/isolation-r10-independent-0902.md) flagged that a bare `recorded_replay: PASS (captured ...)` line told a reader the strong claim was earned but not what was checked to earn it: the three preconditions (trusted launcher path, recursive read-only submounts, no pre-existing sockets under writable binds) only ever appeared as named limitations on the WITHHELD path, never inline on PASS. Since resolveTrustedLauncherPath and the recursive-read-only post-pivot checks are unconditionally wired into every isolated spawn (not an optional mode a caller can bypass), reaching the recorded_replay branch already proves all three preconditions hold. Restate them in the PASS line itself instead of requiring the reader to go read claims.ts to learn what PASS means. Verified: scenario-verify-strict.test.ts (86/86) and scenario-cli.test.ts (51/51) both pass unchanged with this edit, run in a privileged Docker container with bwrap/unshare installed. Assisted-by: AI Signed-off-by: Tim Nunamaker --- packages/polyfill-connectors/bin/scenario-verify.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/polyfill-connectors/bin/scenario-verify.ts b/packages/polyfill-connectors/bin/scenario-verify.ts index f597371a8..f33b566b0 100644 --- a/packages/polyfill-connectors/bin/scenario-verify.ts +++ b/packages/polyfill-connectors/bin/scenario-verify.ts @@ -1891,7 +1891,17 @@ function printCoverageReport( driverEvidenceSatisfied: driverEvidenceOk, }); if (decision.claim === "recorded_replay") { - process.stdout.write(`\nrecorded_replay: PASS (captured ${capturedAt})\n`); + // A bare "PASS (captured ...)" line let a reader tell THAT the strong + // claim was earned but not WHAT was checked — the preconditions only + // ever surfaced as named limitations on the WITHHELD path. Since the + // trusted-launcher and recursive-read-only checks are unconditionally + // wired into every isolated spawn (see isolationEvidenceBoundaryProven + // above), reaching this branch already proves all three; restate them + // inline instead of making the reader go read claims.ts. + process.stdout.write( + `\nrecorded_replay: PASS (captured ${capturedAt}; preconditions: trusted absolute launcher path, ` + + "every ro-bind submount verified read-only, no pre-existing sockets under writable-bound paths)\n" + ); } else { process.stdout.write(`\ndiagnostic_replay: PASS (captured ${capturedAt})\n`); process.stdout.write("recorded_replay: WITHHELD\n"); From 2ea95ca0920f9cfd2e7084860e9ed66a6fbca553 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Wed, 2 Sep 2026 16:37:03 -0500 Subject: [PATCH 06/16] fix(isolation): resolve the sh interpreter through the trusted absolute allowlist, never a bare name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit External review of ced8300be: the trusted-launcher fix closed how unshare/ bwrap themselves are resolved, but never touched the sh those launchers exec their closure script into. unshareProcMountProbeArgv() (the probe) and spawnWithNetworkIsolation's unshare branch (the real execution) both passed the bare string "sh" as an argv entry to the already-trusted unshare binary (unshare ... -- sh -c