Summary
wrap_argv()'s nested-sandbox passthrough is tier-blind: the KIROCREW_SANDBOX_ACTIVE marker check sits above the sandbox_level derivation, so an in-sandbox caller asking for a stricter tier than its host sandbox silently gets the host's tier instead.
# src/kiro_crew/sandbox.py (main)
1709: if _inside_kirocrew_sandbox(): # marker check — tier not computed yet
... # audit + return argv unchanged
1766: sandbox_level = "strict" # derived only AFTER the passthrough
This affects both platforms and predates the macOS marker work in #686 (which does not touch these lines — git diff origin/main -- src/kiro_crew/sandbox.py | grep -c sandbox_level is 0).
Where it bites today
Dev Fleet runs inside an app backend spawned at standard (apps/backend.py:701), and asks for strict in six spawns. None of them has ever received it:
| site |
spawn |
dev_fleet/server.py rebase (×2) |
_run_cmd(..., mode="strict") |
dev_fleet/server.py sync raw_steps (×3) |
sandboxed_spawn_argv(argv, "strict") — ff-merge, pip install -e ., npm ci / npm run build |
dev_fleet/server.py pod provision |
sandboxed_spawn_argv(..., "strict") |
At standard the credential directories are deliberately visible, so repo-controlled code executed by those steps (a worktree's pyproject.toml / package.json lifecycle scripts, a rebase applying its commits) runs without the hides the caller asked for. Partial mitigations exist and are unchanged: _GIT_ENV_NEUTRALIZERS pins core.hooksPath to /dev/null and clears core.fsmonitor / credential.helper, and the build steps get _build_env() without the operator's credential helpers. The tier is the gap, not the env.
Note for anyone grepping: a search for mode="strict" finds only two of the six — the other four pass the tier positionally as sandboxed_spawn_argv(argv, "strict"). An AST walk over Constant nodes finds all of them.
Why this is not a one-line fix
A nested wrap is impossible by design on both backends — Linux seccomp denies unshare, macOS Seatbelt refuses sandbox_apply with EPERM — so a sandboxed process cannot tighten the sandbox it is already inside. That leaves exactly three states, and each has a cost:
- Declare
standard honestly. The exposure remains, but stops being a silent lie; both tiers can be recorded in the passthrough audit event. Requires correcting the six sites, and decoupling one thing mode was doing double duty for in _run_cmd: mode == "standard" also gated whether the operator's git credential helpers were injected, so changing the tier alone would hand those helpers to exactly the rebase that must not have them.
- Declare
strict and fail closed on a proven downgrade. Isolation intent is honoured, but Sync and Provision stop working on both platforms — sandboxed_spawn_argv in the raw_steps loop has no except RuntimeError, so the refusal escapes as an HTTP 500 and leaks the already-collected cleanups temp profiles.
- Make
strict actually reachable. Repo-controlled builds are spawned by a process that is not inside the app-backend sandbox — i.e. the gateway wraps them at strict on the app's behalf. This is the only option that delivers the isolation the call sites believe they have. It needs an app → gateway spawn-delegation channel; today the relay runs the other way (gateway → app), and the app backend cannot be spawned at strict itself because Dev Fleet legitimately needs ~/.ssh and the git credential helpers for fetch and PR queries.
Prior art in the review history
#613 implemented (2), then (1), and each was blocked in turn by the same reviewer:
| revision |
finding |
remedy requested |
aac58ebf |
passthrough permits a proven tier downgrade |
refuse the downgrade |
872d65db |
the refusal breaks Dev Fleet Sync / Provision |
preserve the passthrough |
974beebf |
repo-controlled builds now run at the credential-readable standard tier |
restore strict |
Worth reading before re-attempting either: both (1) and (2) have been written and reviewed, and the objections to each are on record in that PR.
Suggested scope for a fix
- Decide between (1) and (3) as a design question, ideally with the Dev Fleet owner — (3) is the real fix, (1) is an honest interim that makes the state auditable.
- Whichever is chosen, add a guard so the class of mistake cannot recur silently. An AST-based test over
dev_fleet/server.py that rejects any "cc"/"strict" constant outside a docstring works, and catches the positional spelling that a mode="strict" grep misses.
- If (1): record
active_tier and requested_tier in the passthrough SEL audit event so a downgrade is visible in the audit log rather than inferred.
Related
Summary
wrap_argv()'s nested-sandbox passthrough is tier-blind: theKIROCREW_SANDBOX_ACTIVEmarker check sits above thesandbox_levelderivation, so an in-sandbox caller asking for a stricter tier than its host sandbox silently gets the host's tier instead.This affects both platforms and predates the macOS marker work in #686 (which does not touch these lines —
git diff origin/main -- src/kiro_crew/sandbox.py | grep -c sandbox_levelis0).Where it bites today
Dev Fleet runs inside an app backend spawned at
standard(apps/backend.py:701), and asks forstrictin six spawns. None of them has ever received it:dev_fleet/server.pyrebase (×2)_run_cmd(..., mode="strict")dev_fleet/server.pysyncraw_steps(×3)sandboxed_spawn_argv(argv, "strict")— ff-merge,pip install -e .,npm ci/npm run builddev_fleet/server.pypod provisionsandboxed_spawn_argv(..., "strict")At
standardthe credential directories are deliberately visible, so repo-controlled code executed by those steps (a worktree'spyproject.toml/package.jsonlifecycle scripts, a rebase applying its commits) runs without the hides the caller asked for. Partial mitigations exist and are unchanged:_GIT_ENV_NEUTRALIZERSpinscore.hooksPathto/dev/nulland clearscore.fsmonitor/credential.helper, and the build steps get_build_env()without the operator's credential helpers. The tier is the gap, not the env.Note for anyone grepping: a search for
mode="strict"finds only two of the six — the other four pass the tier positionally assandboxed_spawn_argv(argv, "strict"). An AST walk overConstantnodes finds all of them.Why this is not a one-line fix
A nested wrap is impossible by design on both backends — Linux seccomp denies
unshare, macOS Seatbelt refusessandbox_applywith EPERM — so a sandboxed process cannot tighten the sandbox it is already inside. That leaves exactly three states, and each has a cost:standardhonestly. The exposure remains, but stops being a silent lie; both tiers can be recorded in the passthrough audit event. Requires correcting the six sites, and decoupling one thingmodewas doing double duty for in_run_cmd:mode == "standard"also gated whether the operator's git credential helpers were injected, so changing the tier alone would hand those helpers to exactly the rebase that must not have them.strictand fail closed on a proven downgrade. Isolation intent is honoured, but Sync and Provision stop working on both platforms —sandboxed_spawn_argvin theraw_stepsloop has noexcept RuntimeError, so the refusal escapes as an HTTP 500 and leaks the already-collectedcleanupstemp profiles.strictactually reachable. Repo-controlled builds are spawned by a process that is not inside the app-backend sandbox — i.e. the gateway wraps them atstricton the app's behalf. This is the only option that delivers the isolation the call sites believe they have. It needs an app → gateway spawn-delegation channel; today the relay runs the other way (gateway → app), and the app backend cannot be spawned atstrictitself because Dev Fleet legitimately needs~/.sshand the git credential helpers for fetch and PR queries.Prior art in the review history
#613 implemented (2), then (1), and each was blocked in turn by the same reviewer:
aac58ebf872d65db974beebfstandardtierstrictWorth reading before re-attempting either: both (1) and (2) have been written and reviewed, and the objections to each are on record in that PR.
Suggested scope for a fix
dev_fleet/server.pythat rejects any"cc"/"strict"constant outside a docstring works, and catches the positional spelling that amode="strict"grep misses.active_tierandrequested_tierin the passthrough SEL audit event so a downgrade is visible in the audit log rather than inferred.Related