Skip to content

fix: harden execution-control decision boundary (WorkspaceWrite write-target bypasses + truthful sandbox diagnosis)#46

Merged
XuebinMa merged 3 commits into
mainfrom
security/exec-control-hardening
Jun 3, 2026
Merged

fix: harden execution-control decision boundary (WorkspaceWrite write-target bypasses + truthful sandbox diagnosis)#46
XuebinMa merged 3 commits into
mainfrom
security/exec-control-hardening

Conversation

@XuebinMa
Copy link
Copy Markdown
Owner

@XuebinMa XuebinMa commented Jun 3, 2026

Summary

Two execution-control hardening fixes found during a code audit of the guard pipeline. Both are in the security-critical decision/attestation surface.

HIGH-1 — fix(validators): close WorkspaceWrite write-target extraction bypasses

In WorkspaceWrite mode, validate_paths collected write targets only from the tokens after the command word and recognised writes via a closed command allowlist. Three classes of out-of-workspace write slipped through as Allow (and, in a default build where the sandbox is a passthrough shell, actually executed):

  1. Leading redirection>/etc/passwd echo x parsed > as the command word, so the target was never scanned.
  2. dd of=PATH — an =-joined operand, invisible to both the redirection and positional scans.
  3. tar createtar -cf /etc/evil.tar . wrote an archive outside the workspace.

Fix: scan the whole segment (after unwrapping one sudo) for >/>>/>& and <, stripping redirection operators and their targets out of the positional stream so command-word detection is robust to a leading redirect (symmetric fix for read targets); extract dd of=; add tar_archive_write_target for create/append forms (-cf, dashless czf, --create, --file=). Extract/list -f stays a read so legitimate extracts aren't false-positived.

HIGH-2 — fix(sdk): stop reporting seccomp when no syscall filter is compiled in

On Linux, resolve_default_sandbox unconditionally returned SeccompSandbox with selected_name="seccomp", fallback_to_noop=false — even when the seccomp Cargo feature was not compiled in, in which case SeccompSandbox silently runs an unfiltered sh -c compat shell. Operators reading the doctor/diagnosis output — and execution receipts, which record sandbox_type() — were told syscall isolation was active when it was not.

Fix: split the Linux (non-Landlock) diagnosis on cfg(feature = "seccomp"). Feature on → seccomp as before; feature off → NoopSandbox with a truthful selected="none", fallback_to_noop=true, and a reason pointing at the rebuild flags. Makes both the diagnosis and the execution-receipt backend honest.

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Docs
  • CI / tooling

Checklist

  • ./scripts/verify.sh full passes locally — ran the Rust scope only (workspace tests, clippy -D warnings, fmt); did not run the Python/Node/docs legs.
  • Tests added or updated for the behavior change
  • Security-sensitive changes include regression coverage — 13 new tests in the validator unit module (crates/agent-guard-validators/src/bash.rs) plus the platform-selection gate (tests/release_gate.rs). A reviewer may want these mirrored into tests/security_regression.rs.
  • Cross-language parity preserved — no public type or decision surface changed (private functions / internal diagnosis only).
  • Commits follow Conventional Commits
  • Docs updated — none yet; if any guide states "Linux defaults to seccomp," it is now contradicted by the honest behavior and should be revisited (flagged, not done here).
  • No secrets, tokens, or internal-only material added

Test plan

  • cargo test --workspace --exclude agent-guard-python --all-features — all suites green.
  • cargo test -p agent-guard-validators --lib181 passed (incl. 13 new HIGH-1 regressions).
  • cargo clippy --all-features -- -D warnings clean; cargo fmt -- --check clean.
  • Independent PoC harness against the real validator before/after: all 6 out-of-workspace writes flipped from ALLOWBLOCK; controls (in-workspace leading redirect, in-workspace dd of=, tar -cf, and tar -xf reading an outside archive) behave correctly.

Reviewer caveat

HIGH-2's edited branch is cfg(target_os = "linux"). It was developed on macOS with a Homebrew toolchain that has no Linux std/rustup, so that arm could not be compiled locally — Linux CI is the authoritative check for it. The change is a simple cfg-split with both arms yielding the same (Box<dyn Sandbox>, DefaultSandboxDiagnosis) tuple, and NoopSandbox is already used elsewhere in the same function.

In WorkspaceWrite mode, `validate_paths` collected write targets only from
the tokens after the command word, and recognised writes via a closed
command allowlist. Three classes of out-of-workspace write slipped through
as Allow:

  1. a redirection placed before the command word (`>/etc/passwd echo x`):
     `>` was treated as the command word, so the target was never scanned;
  2. `dd of=PATH`: an `=`-joined operand, invisible to both the redirection
     and positional scans;
  3. `tar` creating an archive outside the workspace (`tar -cf /etc/x.tar`).

Fix:
  - scan the whole segment (after unwrapping one `sudo`) for `>`/`>>`/`>&`
    and `<`, stripping redirection operators and their targets out of the
    positional stream so command-word detection is robust to a leading
    redirect (symmetric fix for read targets);
  - extract `dd of=PATH` as a write target;
  - add `tar_archive_write_target` covering create/append forms (`-cf`,
    dashless `czf`, `--create`, `--file=`); extract/list `-f` stays a read
    so legitimate extracts are not false-positived.

13 regression tests cover all three bypass classes plus no-false-positive
controls (in-workspace leading redirect, in-workspace dd/tar, tar extract
reading an outside archive) and one end-to-end validate_bash_command case.
On Linux, `resolve_default_sandbox` unconditionally returned
`SeccompSandbox` with `selected_name="seccomp"`, `selected_sandbox_type=
"linux-seccomp"`, `fallback_to_noop=false` — even when the `seccomp` Cargo
feature was not compiled in. In that configuration `SeccompSandbox` silently
runs an unfiltered `sh -c` compatibility shell (linux.rs
`execute_compat_shell`), so operators reading the doctor/diagnosis output —
and execution receipts, which record `sandbox_type()` — were told syscall
isolation was active when it was not.

Split the Linux (non-Landlock) diagnosis on `cfg(feature = "seccomp")`:
  - feature on  → seccomp as before (filter actually loads before exec);
  - feature off → NoopSandbox with a truthful `selected="none"`,
    `fallback_to_noop=true`, and a reason pointing at the rebuild flags.

This makes both the diagnosis and the execution-receipt backend honest.
Update the platform-selection release gate to assert "linux-seccomp" only
when the feature is present and "none" otherwise.
…ol audit

Capture the configuration-dependent behaviors surfaced by the audit as
operator guidance rather than changing tested security semantics:

  - default build has no OS-level isolation; the decision layer is the only
    boundary unless a platform sandbox feature is compiled in (confirm via
    default_sandbox_diagnosis);
  - working_directory must be set for ReadOnly/WorkspaceWrite or the implicit
    workspace fence is absent (deny_paths/allow_paths still apply);
  - Untrusted callers ignore tool-level `mode` by design (escalation-proof);
    restrict via default_mode / trust override / deny rules instead;
  - `allow` rules match by substring — anchor with prefix:/regex:;
  - check_destructive is a best-effort ask, not an enforcement boundary.

Adds three matching items to the hardening checklist and bumps the doc's
review metadata (v2.3, 2026-06-03). No code or behavior change.
@XuebinMa
Copy link
Copy Markdown
Owner Author

XuebinMa commented Jun 3, 2026

Heads-up for review: the red Node Binding & Framework Tests check is a pre-existing failure on main (loopback HTTP execute test vs. the SSRF deny-list), not introduced by this PR. Tracked separately in #47. All checks this PR actually touches — Rust workspace, Linux seccomp integration, clippy/fmt, cross-language parity, docs — are green.

@XuebinMa
Copy link
Copy Markdown
Owner Author

XuebinMa commented Jun 3, 2026

Post-merge note (no npm republish needed): the published agent-guard-plugin npm package only ships JS glue + the outbound policy and builds guard-hook from git source via cargo install --git, so fresh installs pick up these fixes automatically once this lands on main. However, cargo install --git is not version-pinned, so users who already installed guard-hook before this merge must refresh manually to get the HIGH-1/HIGH-2 fixes:

cargo install --git https://github.com/XuebinMa/agent-guard guard-hook --force

@agent-guard/node (the napi binding that embeds the Rust decision logic) is not published to npm, so nothing to republish there.

@XuebinMa XuebinMa merged commit a2b5a95 into main Jun 3, 2026
12 of 14 checks passed
@XuebinMa XuebinMa deleted the security/exec-control-hardening branch June 3, 2026 16:41
XuebinMa added a commit that referenced this pull request Jun 3, 2026
… audit (#49)

Address the HIGH/MEDIUM findings the weekly deep audit surfaced that PR #46
did not cover (PR #46 only closed the two top security findings in bash.rs).
Windows AppContainer findings are left for a follow-up issue.

- anomaly.rs (HIGH): report_denial recovers the poisoned state mutex via
  into_inner() and logs, so a denial is still recorded and the Deny Fuse can
  trip; previously a poison silently returned, letting an actor exhaust the
  lock threshold. check() now also logs on poison for parity.
- enforce.rs (HIGH): surface ledger.decide() failures when marking an expired
  approval Expired instead of discarding them; AlreadyDecided stays benign.
- enforce.rs (MEDIUM): replace the dead `let _ = stderr_present` no-op with a
  debug log so handoff stderr omission is observable.
- content_filter.rs (MEDIUM): warn when a tool payload fails to parse as JSON
  so a skipped scan is distinguishable from a clean one.
- executors.rs (MEDIUM): extend the SSRF deny-list with RFC 6598 CGNAT
  (100.64.0.0/10), RFC 2544 benchmark (198.18.0.0/15), 0.0.0.0/8, and 6to4
  (2002::/16) embedded-IPv4 unwrapping. Adds regression tests.
- decision.rs (MEDIUM x2): drop Deserialize from GuardDecision so untrusted
  JSON can't synthesize an Allow; mark DecisionReason #[non_exhaustive] so
  cross-crate callers go through the constructors that guarantee a message.

Co-authored-by: Xuebin Ma <ma_xuebin@hotmail.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.

2 participants