close
Skip to content

fix: detect and audit tier downgrades in nested-sandbox passthrough (#691) - #2406

Merged
iamwhatever merged 1 commit into
mainfrom
fix/nested-passthrough-tier-691
Aug 10, 2026
Merged

fix: detect and audit tier downgrades in nested-sandbox passthrough (#691)#2406
iamwhatever merged 1 commit into
mainfrom
fix/nested-passthrough-tier-691

Conversation

@bolichen97

Copy link
Copy Markdown
Collaborator

Summary

The nested-sandbox passthrough in wrap_argv() was tier-blind: the KIROCREW_SANDBOX_ACTIVE marker proves a Kiro Crew sandbox is active but not which tier built it, so an in-sandbox caller requesting strict under a standard outer sandbox silently ran at standard — invisible in logs and in the SEL passthrough audit (which recorded only mode). Closes #691.

What changed

  • Tier recorded at sandbox entry. Both launcher sites export a companion KIROCREW_SANDBOX_LEVEL=<standard|cc|strict> beside the ACTIVE marker, with the same non-droppable placement: the Linux launcher sets it immediately after its env-scrub loop; the macOS env prefix carries it after the -u flags. cli.main() drops an inherited copy at the same site where it drops the marker itself (a stale ancestor value would corrupt a descendant's downgrade audit).
  • Requested vs active comparison in the passthrough. A shared _mode_to_level() helper (also replacing the inline mapping at the backend-wrap site) resolves the requested tier; ordinals standard(1) < cc(2) < strict(3). Absent/unrecognized level (older-build outer tree) reads as unknown — no ordinal claim, passthrough unaffected, never crashes.
  • Audit visibility. The SEL nested_sandbox_passthrough metadata now carries requested_tier, active_tier, tier_known, and tier_downgrade, so a downgrade is visible in the audit log rather than inferred (tier_known separates proven no downgrade from unprovable).
  • Loud warning + enforceable slice on proven downgrade. A per-call SECURITY: warning names both tiers and argv[0], and the passthrough argv is prefixed with the requested tier's env -u scrub — the one part of strict enforceable without a nested wrap (agent-denied credential keys a standard launcher never scrubbed). The env binary is resolved only at a trusted absolute path (_unset_env_argv); if none exists the scrub is skipped with a warning rather than resolving env through a caller-influenced PATH.
  • Deliberately NOT fail-closed. Refusing the downgrade is known to break Dev Fleet Sync/Provision (prior attempt in fix(sandbox): treat macOS nesting EPERM as nesting, not a missing backend #613); the passthrough return stays. The gateway spawn-delegation design (option 3 on the issue) remains out of scope.

Tests

  • test/test_sandbox_nested_tier.py (new, 19 tests): launcher templates the level per tier; level export sits after the scrub loop; macOS env assignment positioned after -u flags; downgrade detected + audited + warned + scrubbed via absolute env path; no-downgrade / same-tier / unknown / unrecognized-value / SEL-failure / no-trusted-env-binary paths.
  • test/test_cli.py: marker-drop test extended to pin the LEVEL pop.
  • test/test_terminal_commands.py: probe env allowlist learns the new sandbox-injected name.
  • Docs: docs/system-specs/modules/security.md passthrough section updated in the same commit.

Verification

  • isort / flake8 / mypy / brand gate: clean.
  • Full suite: 39498 passed; the 79 failures in a 32-way parallel run were all verified pre-existing on pristine main (sandbox-probe load flakes + host env), none from this change.
  • Pre-push dual-model review fleet ran: 1 High (bare env PATH lookup — fixed via trusted absolute resolver), 1 Blocking (inherited LEVEL not dropped in cli.main() — fixed), 4 advisories applied. One advisory (apply scrub on unknown tier) declined: the issue scope specifies the passthrough must be unaffected when no downgrade can be proven; tier_known metadata added instead.

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 3478fd78b9cde8c48cc459e0049e7a9fab170cf2 — this comment is updated in place on each push.

Review details

The changed logic is sound: dict lookups are all guarded (requested_level is always a valid tier from _mode_to_level; active_level is normalized to "unknown" before the ordinal comparison, which short-circuits on active_level in _TIER_ORDINALS). The env scrub goes through the trusted-absolute-path _unset_env_argv and never fails closed. No credential exposure, no crash path, no removed guard. No AUTOSDE rule is touched or weakened.

No findings.

[OPUS-REVIEWED] 3478fd7

Verdict parsed from the review's SHA-scoped output markers for commit 3478fd78b9cde8c48cc459e0049e7a9fab170cf2.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 3478fd78b9cde8c48cc459e0049e7a9fab170cf2: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3478fd78b9cde8c48cc459e0049e7a9fab170cf2 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3478fd7

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 3478fd78b9cde8c48cc459e0049e7a9fab170cf2: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 3478fd78b9cde8c48cc459e0049e7a9fab170cf2 — updated in place on each push; does not block merge.

Design-Verdict: PASS

A real audit-visibility gap closed with a proportionate mitigation: tier recorded, downgrade audited/warned, partial scrub enforced, fail-closed correctly rejected with cited evidence.

Suggestions

  • Dev Fleet Sync/Provision make strict-under-standard a routine legit pattern, so the per-call SECURITY: warning will fire constantly for expected flows — consider deduping per (argv[0], tier-pair) so the log signal survives; the SEL record already gives per-call fidelity.

[DESIGN-REVIEWED] 3478fd7

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 9, 2026
@bolichen97
bolichen97 force-pushed the fix/nested-passthrough-tier-691 branch from 6789888 to 743789d Compare August 9, 2026 21:36
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 9, 2026
…691)

The nested-sandbox passthrough in wrap_argv() returned argv unchanged
regardless of the tier the caller requested, and the in-sandbox marker
recorded only that a sandbox was active, not which tier built it. An
in-sandbox caller requesting strict under a standard outer sandbox
therefore silently ran at standard, invisible in logs and in the SEL
passthrough audit event.

Both launcher sites now export KIROCREW_SANDBOX_LEVEL beside the ACTIVE
marker, with the same non-droppable placement (after the Linux launcher
env-scrub loop; after the macOS env -u flags). The passthrough resolves
the requested mode through a shared _mode_to_level() helper, compares it
against the active tier (standard < cc < strict; absent/unrecognized
reads as unknown, which proves nothing and never crashes), and:

- records requested_tier, active_tier, and tier_downgrade in the SEL
  nested_sandbox_passthrough metadata so a downgrade is visible in the
  audit log rather than inferred
- emits a per-call SECURITY warning naming both tiers and the executable
  on a proven downgrade
- prefixes the passthrough argv with the requested tier's env-scrub
  delta (env -u agent-denied credential keys) — the one slice of the
  stricter tier enforceable without a nested wrap

The passthrough itself stays: a nested re-wrap is impossible by design
(Linux seccomp denies unshare; macOS Seatbelt refuses sandbox_apply
with EPERM), and refusing the downgrade is known to break Dev Fleet
Sync/Provision.

Closes #691
@bolichen97
bolichen97 force-pushed the fix/nested-passthrough-tier-691 branch from 743789d to 3478fd7 Compare August 9, 2026 21:50
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 9, 2026
@iamwhatever
iamwhatever merged commit 3f1d502 into main Aug 10, 2026
51 checks passed
@iamwhatever
iamwhatever deleted the fix/nested-passthrough-tier-691 branch August 10, 2026 04:54
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 10, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
…irodotdev#691) (kirodotdev#2406)

The nested-sandbox passthrough in wrap_argv() returned argv unchanged
regardless of the tier the caller requested, and the in-sandbox marker
recorded only that a sandbox was active, not which tier built it. An
in-sandbox caller requesting strict under a standard outer sandbox
therefore silently ran at standard, invisible in logs and in the SEL
passthrough audit event.

Both launcher sites now export KIROCREW_SANDBOX_LEVEL beside the ACTIVE
marker, with the same non-droppable placement (after the Linux launcher
env-scrub loop; after the macOS env -u flags). The passthrough resolves
the requested mode through a shared _mode_to_level() helper, compares it
against the active tier (standard < cc < strict; absent/unrecognized
reads as unknown, which proves nothing and never crashes), and:

- records requested_tier, active_tier, and tier_downgrade in the SEL
  nested_sandbox_passthrough metadata so a downgrade is visible in the
  audit log rather than inferred
- emits a per-call SECURITY warning naming both tiers and the executable
  on a proven downgrade
- prefixes the passthrough argv with the requested tier's env-scrub
  delta (env -u agent-denied credential keys) — the one slice of the
  stricter tier enforceable without a nested wrap

The passthrough itself stays: a nested re-wrap is impossible by design
(Linux seccomp denies unshare; macOS Seatbelt refuses sandbox_apply
with EPERM), and refusing the downgrade is known to break Dev Fleet
Sync/Provision.

Closes kirodotdev#691

Co-authored-by: bolichen97 <bolichen97@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sandbox: nested passthrough is tier-blind — an in-sandbox strict request silently runs at the host tier

2 participants