A two-reviewer audit of the AgentStack control plane — trust gating, policy intersection, secret resolution, and the new Docker sandbox + egress proxy. Independent findings, cross-checked against each other.
This is not security theater. The policy engine, trust re-gating, scoped secret resolution, and deterministic compiled ruleset show real security engineering. The central weakness is that the clean model does not yet survive contact with the runtime boundary — today's sandbox asks hostile code to cooperate with its proxy, when the whole premise is that hostile code must not be trusted to cooperate.
Every High finding lives at the sandbox/egress edge, and all five were confirmed by both reviewers. The theme is singular: the egress firewall is currently advisory, and traffic that does reach the proxy is filtered on attacker-controlled strings without canonicalization or resolved-address checks.
The sandbox's ProxyOnly mode uses Docker's default bridge (runtime/src/docker.rs:73) — full NAT internet access — and steers traffic to the proxy only via HTTPS_PROXY env vars (cli/commands/sandbox.rs:148) that any child process can unset. A hostile process reaches any host by direct TCP, raw socket, IPv6, or its own DNS, never touching the proxy or the recorder. The CLI and spec.rs:20 assert "no direct route out"; the code contradicts it. This breaks the primary sandbox-egress guarantee.
glob_match (core/manifest/model.rs:206) is case-sensitive and exact, and nothing normalizes the CONNECT authority before policy. CONNECT EVIL.EXAMPLE:443 or a trailing-dot evil.example. sails past a !evil.example deny — and equally defeats allowlists / default-deny configs, not just explicit denies. Fix: ASCII-lowercase and strip a trailing dot before egress_decision.
egress/src/sni.rs is implemented and tested but only exported — the proxy decides on the CONNECT authority (proxy.rs:85) and copies tunnel bytes untouched (proxy.rs:101). Yet egress/lib.rs:7 and ARCHITECTURE.md:227 advertise SNI name-filtering. The result is a domain-fronting gap: CONNECT to an allowed host, speak TLS to a denied one behind the same front. The defense that would catch it (assert SNI == CONNECT host) exists in the repo but isn't wired.
The proxy validates the hostname string (proxy.rs:85) then independently re-resolves and connects (proxy.rs:94) with no check on the resolved IP class. An allowed, attacker-controlled hostname that resolves to 169.254.169.254 (cloud metadata), loopback, or a private range is reachable — direct SSRF into host and internal networks; DNS-rebinding between check and connect is the secondary case. Fix: resolve once, reject forbidden IP classes by default, connect to the validated address.
The parser explicitly accepts IPv6 literals (connect.rs:39) and passes IPv4 literals as ordinary hosts. With no canonicalization or IP-class policy, a container can CONNECT 93.184.216.34:443 and skip name-based rules altogether; alternate textual IP encodings can also defeat exact denies. Neither reviewer caught this on the first pass — it surfaced only in cross-check, and it compounds H1, H2, and H4.
Documented guarantees the code doesn't yet provide, plus audit-integrity and exposure gaps. Several are latent today (in-process, single-user) but become live the moment Phase 2 crosses a process or trust boundary.
Found independently by both reviewers. policy/ruleset.rs:19 rules that an unknown version must deny everything; no decision method reads self.version (ruleset.rs:193). Unreachable in-process (always current version) — but this is the Phase-2 cross-process wire contract, and a future/tampered ruleset would be enforced with v1 semantics instead of deny-all. Add a central version gate at the decision entry points.
digest_for (trust/lib.rs:94) joins manifest ‖ local ‖ lock with a single 0x00 separator, so bytes can move across a file boundary without changing the digest — distinct pinned triples can collide. Exploitation needs control of adjacent files, "but that is precisely the consent surface." The byte-flip proptest structurally can't detect this class. Fix: length-prefix or tag each segment.
Two threads append to events.jsonl through separate O_APPEND handles, and writeln! is multi-syscall (recorder/lib.rs:288). Interleaved writes produce malformed lines that RunLog::read silently discards (lib.rs:314) — losing exactly the egress decisions the tool exists to record. The "O_APPEND-atomic per line" comment at sandbox.rs:122 is unjustified. Fix: format the whole line and issue one write_all, or serialize behind a mutex.
The proxy binds 0.0.0.0 (sandbox.rs:134) so the container can reach it, exposing it to the whole LAN for the run's duration; with allow-by-default policy (ruleset.rs:38) that's an open forward relay whose traffic is attributed to the harness. Codex adds a sibling both teams missed: proxy identity is the listener, never the peer (proxy.rs:38) — any process reaching a per-server listener inherits that server's egress privileges. Fix: bind the bridge gateway IP; authenticate the peer.
connect.rs:54 parses host:port, but egress_decision matches host only (ruleset.rs:208). An allowed host is reachable on any port — CONNECT allowed-web-host:22 passes. Medium where a policy intends HTTPS-only; at minimum the port-blindness should be explicit.
The request head is read one byte at a time with no timeout (proxy.rs:112) and the tunnel has no idle deadline. A container can open many connections and dribble bytes to pin async tasks indefinitely. Memory is bounded (8 KiB head cap) but connection count and lifetime are not.
collect_files accepts non-directory symlinks (core/digest.rs:55), then fs::read follows them (digest.rs:37) — during agentstack trust, before gating protects you. A hostile skill can link to /dev/zero (OOM/hang) or a FIFO (indefinite hang). The following-symlinks copy variant (fsx.rs:38) also lacks cycle detection (ancestor link → infinite recursion).
~/.ssh/id_rsa as info disclosure. Codex downgraded that specific vector: it's a digest oracle, not direct disclosure, unless another output compares digests. The DoS half stands; net severity Medium.Correctness debt and portability issues, plus two items both reviewers over-rated and codex pulled down to Low. None are exploitable in the shipped single-user in-process path.
dir_digest_cached (store.rs:413) returns the cached SHA when path + size + mtime match. Codex sharpened it: the freshness guard only gates cache insertion (line 425), not lookup — so a same-size, restored-mtime substitution against an existing entry is served from cache. Contained because the trust-critical drift check (consolidate.rs:219) uses the uncached dir_digest. Keep the cache off any verification path.
It hashes OS-native path separators and to_string_lossy on filenames (core/digest.rs:35). The lossy conversion is a same-platform collision class for non-UTF-8 names; the separator difference mainly causes portability / re-lock churn when a pin travels between a Mac and Windows — not a direct bypass. Normalize to / and hash raw path bytes.
Codex corrected the framing here: the proxy's socket I/O is properly async — it's the synchronous event-sink / log append at proxy.rs:86 that blocks the single-worker runtime. Move it to spawn_blocking or a logging channel (which the recorder doc already envisions).
No size caps on manifest / lockfile / store reads (bound attacker-reachable reads per rule 7) · unbounded recursion depth in the digest/copy walks (stack-overflow DoS; the symlink-following copy can elevate this) · a run proceeds silently if the run log can't be created (recorder/lib.rs:273) · refs.rs — the hostile-input ${REF} scanner — has zero unit tests (coverage debt, not a live bug).
The adapters crate doesn't actually depend on policy, though CLAUDE.md pins that edge — architecture drift, not independently reportable without a bypass. And trust carries off-allowlist anyhow + toml (both TODO-tracked) — a supply-chain-policy violation of the "reviewed line by line" crate, not a code vulnerability by itself.
What held up under scrutiny — read, not assumed. Both models were emphatic that the core is real security engineering.
All 7 non-cli crates carry #![forbid(unsafe_code)]; the entire libc surface is concentrated in one cli/sys.rs with SAFETY: notes — the rule-1 work marked "before Phase 2" is already done.
Intersection logic is correct and the proptests are non-tautological: bidirectional compiled-vs-live equivalence, an artifact-level restatement of "policy only narrows," IndexMap-order independence.
Policy denial is terminal and gates every ${REF} before any backing store is consulted; no raw-secret or placeholder path to disk was found. Bundle export encrypts opt-in via age.
trust::check runs before activate everywhere; only Trusted reaches gateway spawn. No command was found using bundle content before its trust check.
The run log lives host-side under ~/.agentstack/runs/, is never mounted into the container, is 0600/0700, and guards run-id path escape — a sandboxed process can't reach it.
fmt and clippy -D warnings clean, 146/146 tests pass, and the Phase-2 sandbox e2e tests were re-run live against real Docker — the "verified on Docker" claim is true.
Both reviewers independently reached the same sequencing: freeze feature work and make the runtime boundary non-bypassable before anything else.
A dedicated --internal Docker network with the proxy as the only reachable peer (or equivalent host firewall rules). Then add an adversarial test that unsets proxy vars and attempts direct TCP, direct DNS, host-gateway and metadata IPs — all must fail while a proxied allow succeeds. Until then, rename ProxyOnly and soften the "no direct route out" claim. (H1, H4, H5)
Resolve once, reject forbidden IP classes by default, apply policy to both the requested name and the resolved address, constrain ports, normalize the host (lowercase + trailing dot), and decide whether SNI ≠ CONNECT authority must be rejected. (H2, H3, H4, H5, M5)
Versioned, length-framed digest encoding with explicit symlink behavior (reject or canonicalize-and-bound); hostile tests for symlinks, non-UTF-8 paths, and size/mtime-preserving edits. Keep the stat cache off any verification path. (M2, M7, L1, L2)
One deny-all gate at the decision entry points, before the ruleset ever crosses a process boundary. Add the missing proptest. (M1)
A library API that turns a trusted, resolved bundle into an immutable plan (identities, checked policy, authorized refs, mounts, processes, endpoints, recorder context). Commands execute or display that plan instead of re-assembling the security model across 1,000–2,500-line files. (reduces the surface behind M3 and every "did this path skip a check?" question)