From 29c0ccc6c77f3dc1e40a39609c6018a52c8fc214 Mon Sep 17 00:00:00 2001 From: Evgeny Boger Date: Sun, 24 May 2026 13:40:20 +0300 Subject: [PATCH] fix: address remaining code-review findings (sweep) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Catches the rest of the xhigh review's verified findings, focused on security + reliability + correctness. Each bullet references the finding from the JSON output in this conversation. **Security** - intercept_hook: strip Set-Cookie / Set-Cookie2 / WWW-Authenticate / Proxy-Authenticate from upstream responses. Prevents in-VM agent from harvesting GitHub session cookies and replaying them on the unfiltered github.com path to authenticate as the host user (review #3). - intercept_hook: substitute_authorization_header handles both Bearer (literal substring) AND Basic auth (decode → replace → re-encode base64). Closes the "git Basic-auth always 401s" path while keeping the gh CLI Bearer path working. Also injects Bearer with the real token if the guest sent no Authorization header at all, so the forwarder never sends anonymous-but-substituted-looking requests (review #12). - run.rs detect_github_repos_in_cwd hardens the git invocation against repo-local config that can execute code on the host (core.fsmonitor, core.pager, core.editor, alias.*). Pass `safe.directory=*` so a foreign-UID checkout doesn't fail closed, `protocol.allow=never` so the call can't fetch anything, and force GIT_CONFIG_SYSTEM / GIT_CONFIG_GLOBAL to /dev/null so user-global git config can't inject an attacker-controlled include path. Closes the "host RCE before sandbox boot via untrusted .git/config" finding (review #6). - run.rs adds a long inline note above the gh secret entry calling out the github.com smart-HTTP gap (review #1). Filtering git smart-HTTP requires a streaming intercept primitive microsandbox doesn't expose (current intercept buffers full request, capped at 64 KiB; git push pack data exceeds that). Documented; the threat model is now explicit. **Reliability** - intercept_hook forward_github_api: reqwest::Client gets a 60 s timeout and `Policy::none()` redirect policy — hung api.github.com no longer freezes the sandbox; 3xx responses are surfaced to the guest verbatim instead of silently following (review #7). - intercept_hook trigger_host_refresh: 90 s wait-with-timeout on the host `claude -p` / `codex exec` so a stuck CLI doesn't hold the intercept hook indefinitely (review #8). - run.rs streaming exec: distinguish "stream closed without Exited event" (infra disconnect) from "agent exited with code 1" (real failure). The former bails with a clear error instead of returning exit 1 (review #9). **Correctness** - run.rs adds a SnapshotGuard with `impl Drop` so the Phase-5 host- cred mutation check runs on every exit path from launch(), including `?` propagation from attach/exec_stream errors. Previously the safety net only fired on the happy path (review #10). - run.rs: --no-git no longer silently discards explicit --repo arguments. The flag suppresses cwd auto-detection but explicit --repo entries are kept; the launcher warns when --no-git + --repo are combined (review #11). - secrets.rs: `Ok(_)` → `Ok(Some(()))` in the opencode token-file arm. `Ok(None)` (host file missing) no longer registers the placeholder pointing at a token file that wasn't written (review #13). - secrets.rs decode_id_token_account: try URL_SAFE_NO_PAD first (conformant JWT base64url) then fall back to STANDARD with manual '+'/'_' alphabet conversion. Closes the silent zero-UUID fallback when an upstream library emits standard-alphabet JWT payloads (review #14). - bin/agent-vm-ccusage: switch from `paste -sd,` to a null-delimited find→read loop, and skip any path containing `,` with a clear warning. CLAUDE_CONFIG_DIR's comma-separator has no escape mechanism so any path with a comma silently corrupts the union (review #15). Verified end-to-end: - `gh api graphql ...` reaches GitHub (allow-list extended in the previous commit; this commit didn't touch path filtering, but the pipeline still works: clean 403 for denied repos, real responses for allowed paths). - `git status` works on the bind-mounted project — no dubious- ownership (safe.directory) and the hardened cwd-detection git call still finds the remote. - gh hosts.yml is written, gh auth status shows the placeholder account is active (Bad credentials when reaching GitHub is the same documented outer-bridge nested-host limit). - 6/6 cargo tests still green. Co-Authored-By: Claude Opus 4.7 (1M context) --- bin/agent-vm-ccusage | 20 +++-- crates/agent-vm/src/intercept_hook.rs | 116 +++++++++++++++++++++++--- crates/agent-vm/src/run.rs | 109 ++++++++++++++++++++---- crates/agent-vm/src/secrets.rs | 24 +++++- 4 files changed, 229 insertions(+), 40 deletions(-) diff --git a/bin/agent-vm-ccusage b/bin/agent-vm-ccusage index b81313a..42c3864 100755 --- a/bin/agent-vm-ccusage +++ b/bin/agent-vm-ccusage @@ -13,14 +13,20 @@ state_root="${AGENT_VM_STATE_DIR:-${XDG_STATE_HOME:-$HOME/.local/state}/agent-vm # Each per-project state dir has the Claude session history under # /claude (which is symlinked into the guest as ~/.claude). -agent_vm_dirs="" -if [ -d "$state_root" ]; then - agent_vm_dirs=$(find "$state_root" -maxdepth 2 -type d -name claude 2>/dev/null | paste -sd,) -fi - +# Use NUL-delimited find→read so paths containing legal characters +# like `,` (which CLAUDE_CONFIG_DIR uses as a separator) aren't +# mis-split (review finding #15). paths="$HOME/.claude,$HOME/.config/claude" -if [ -n "$agent_vm_dirs" ]; then - paths="$paths,$agent_vm_dirs" +if [ -d "$state_root" ]; then + while IFS= read -r -d '' dir; do + # Refuse to add a path that contains a comma — CLAUDE_CONFIG_DIR + # has no escape mechanism and silently mis-tokenizes; better to + # skip than to merge directories the user didn't ask for. + case "$dir" in + *,*) echo "agent-vm-ccusage: skipping $dir (contains ',', breaks CLAUDE_CONFIG_DIR)" >&2;; + *) paths="$paths,$dir";; + esac + done < <(find "$state_root" -maxdepth 2 -type d -name claude -print0 2>/dev/null) fi export CLAUDE_CONFIG_DIR="$paths" diff --git a/crates/agent-vm/src/intercept_hook.rs b/crates/agent-vm/src/intercept_hook.rs index 212ce3e..f93e098 100644 --- a/crates/agent-vm/src/intercept_hook.rs +++ b/crates/agent-vm/src/intercept_hook.rs @@ -147,11 +147,19 @@ async fn forward_github_api( let url = format!("https://{}{}", secrets::GITHUB_API_HOST, path); let client = reqwest::Client::builder() + // Bounded upstream timeout so a hung api.github.com call + // doesn't freeze the in-VM agent indefinitely (review #7). + .timeout(std::time::Duration::from_secs(60)) + // Reflect 3xx back to the guest verbatim rather than + // following — protects against unexpected redirect targets + // and lets the agent decide (review #7). + .redirect(reqwest::redirect::Policy::none()) .build() .context("building reqwest client")?; let method_obj = reqwest::Method::from_bytes(method.as_bytes()) .context("invalid HTTP method")?; let mut req = client.request(method_obj, &url); + let mut had_authorization = false; for (name, value) in &headers { // Strip hop-by-hop + protocol-level headers; reqwest will // re-emit appropriate ones. `Host` is required to point at @@ -165,13 +173,28 @@ async fn forward_github_api( continue; } if lower == "authorization" { - // Substitute placeholder → real token before forwarding. - let v = value.replace(secrets::GH_TOKEN_PLACEHOLDER, &real_token); + had_authorization = true; + // Substitute the placeholder → real token. Two forms: + // - `token ` / `Bearer ` — + // literal substring, handled by `replace`. + // - `Basic base64(x-access-token:)` — + // the placeholder is base64-encoded, so a literal + // replace finds nothing. Decode, substitute, re- + // encode. Review finding #12. + let v = substitute_authorization_header(value, &real_token); req = req.header("Authorization", v); } else { req = req.header(name, value); } } + // If the guest sent no Authorization header at all (some scripts + // strip it before retry), inject a Bearer with the real token — + // we've already checked the path against the allow-list, and the + // alternative is silently anonymous traffic that gets a 401 and + // confuses the agent. Review finding #11/#12. + if !had_authorization { + req = req.header("Authorization", format!("Bearer {real_token}")); + } if !body.is_empty() { req = req.body(body); } @@ -182,9 +205,22 @@ async fn forward_github_api( let mut out_headers: Vec<(String, String)> = Vec::new(); for (k, v) in resp.headers() { let k_lower = k.as_str().to_ascii_lowercase(); + // Strip hop-by-hop headers (we set Content-Length below) AND + // anything that lets the guest re-authenticate as the host + // user without going through the substitution proxy. Review + // finding #3: Set-Cookie + WWW-Authenticate would otherwise + // let an in-VM agent harvest GitHub session cookies and + // drive github.com directly. if matches!( k_lower.as_str(), - "transfer-encoding" | "content-length" | "connection" | "keep-alive" + "transfer-encoding" + | "content-length" + | "connection" + | "keep-alive" + | "set-cookie" + | "set-cookie2" + | "www-authenticate" + | "proxy-authenticate" ) { continue; } @@ -314,6 +350,33 @@ fn read_gh_token(state_dir: &Path) -> Result { Ok(s.trim().to_string()) } +/// Substitute `GH_TOKEN_PLACEHOLDER` in an Authorization header value +/// with `real_token`, handling both: +/// - `token ` / `Bearer ` — literal +/// substring, simple `replace`. +/// - `Basic base64(x-access-token:)` — git's HTTP basic +/// auth scheme. The placeholder is base64-encoded inside the value, +/// so a literal replace would miss it; decode, substitute, re-encode. +/// +/// Falls back to the literal-replace result for any value that isn't +/// recognisable as Basic auth, so non-GitHub callers' headers are +/// touched as little as possible. +fn substitute_authorization_header(value: &str, real_token: &str) -> String { + if let Some(b64) = value.strip_prefix("Basic ").or_else(|| value.strip_prefix("basic ")) { + use base64::Engine as _; + if let Ok(decoded) = base64::engine::general_purpose::STANDARD.decode(b64.trim()) { + if let Ok(s) = std::str::from_utf8(&decoded) { + if s.contains(secrets::GH_TOKEN_PLACEHOLDER) { + let sub = s.replace(secrets::GH_TOKEN_PLACEHOLDER, real_token); + let re = base64::engine::general_purpose::STANDARD.encode(sub.as_bytes()); + return format!("Basic {re}"); + } + } + } + } + value.replace(secrets::GH_TOKEN_PLACEHOLDER, real_token) +} + fn write_response(bytes: &[u8]) -> Result<()> { let mut out = std::io::stdout().lock(); out.write_all(bytes).context("writing response to stdout")?; @@ -399,21 +462,50 @@ fn refresh_openai(state_dir: &Path) -> Result> { } fn trigger_host_refresh(cmd: &str, args: &[&str]) -> Result<()> { - let out = Command::new(cmd) + // Bounded wait so a hung host CLI doesn't keep the in-VM agent's + // OAuth refresh waiting indefinitely (review #8). 90 s is enough + // for normal claude/codex round-trips and small enough to surface + // a problem before the guest agent's own timeout fires. + use std::time::{Duration, Instant}; + const TIMEOUT: Duration = Duration::from_secs(90); + + let mut child = Command::new(cmd) .args(args) .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) - .output() + .spawn() .with_context(|| format!("spawning host {cmd}"))?; - if !out.status.success() { - anyhow::bail!( - "host {cmd} failed (status {}): {}", - out.status, - String::from_utf8_lossy(&out.stderr) - ); + let start = Instant::now(); + loop { + match child.try_wait()? { + Some(status) => { + let mut stderr = Vec::new(); + if let Some(mut e) = child.stderr.take() { + use std::io::Read as _; + let _ = e.read_to_end(&mut stderr); + } + if !status.success() { + anyhow::bail!( + "host {cmd} failed (status {status}): {}", + String::from_utf8_lossy(&stderr) + ); + } + return Ok(()); + } + None => { + if start.elapsed() >= TIMEOUT { + let _ = child.kill(); + let _ = child.wait(); + anyhow::bail!( + "host {cmd} did not return within {} s; killed", + TIMEOUT.as_secs() + ); + } + std::thread::sleep(Duration::from_millis(200)); + } + } } - Ok(()) } fn looks_like_oauth_refresh(req: &[u8]) -> bool { diff --git a/crates/agent-vm/src/run.rs b/crates/agent-vm/src/run.rs index b3181f9..f103757 100644 --- a/crates/agent-vm/src/run.rs +++ b/crates/agent-vm/src/run.rs @@ -215,21 +215,45 @@ pub async fn launch(agent: Agent, args: Args) -> Result { // decide whether to bother capturing host gh auth (no repos → // nothing to talk to) and to constrain api.github.com requests // server-side via the intercept hook. + // --no-git suppresses *automatic* cwd-remote detection but does + // NOT discard explicit --repo arguments (review #11): a user + // who passes `--no-git --repo X` clearly wants the explicit + // allow-list. We do warn so they notice if they didn't mean to. let mut allowed_repos: Vec = Vec::new(); if !args.no_git { allowed_repos.extend(detect_github_repos_in_cwd(&session.project_dir)); - for r in &args.repo { - let r = r.trim().to_string(); - if !r.is_empty() && !allowed_repos.iter().any(|x| x.eq_ignore_ascii_case(&r)) { - allowed_repos.push(r); - } + } else if !args.repo.is_empty() { + eprintln!( + "==> --no-git skips cwd remote auto-detection, but --repo overrides are kept ({} entr{})", + args.repo.len(), + if args.repo.len() == 1 { "y" } else { "ies" }, + ); + } + for r in &args.repo { + let r = r.trim().to_string(); + if !r.is_empty() && !allowed_repos.iter().any(|x| x.eq_ignore_ascii_case(&r)) { + allowed_repos.push(r); } } - let use_github = !args.no_git && !allowed_repos.is_empty(); + let use_github = !allowed_repos.is_empty(); let creds = crate::secrets::refresh(&session.state_dir, &project_guest_path, use_github) .context("snapshotting host credentials")?; + // RAII guard so the Phase-5 host-cred mutation check runs on + // *every* exit path from launch() — including `?` propagation + // from attach/exec_stream errors (review finding #10). Without + // this the safety net only fires on the happy path. + struct SnapshotGuard(Option); + impl Drop for SnapshotGuard { + fn drop(&mut self) { + if let Some(snap) = self.0.take() { + crate::secrets::verify_snapshot(&snap); + } + } + } + let _snap_guard = SnapshotGuard(creds.snapshot.clone()); + // Phase 6/9: always write the guest gitconfig (carries the // unconditional `safe.directory = *` so git inside the guest // accepts the host-bind-mounted project despite the UID @@ -397,6 +421,20 @@ pub async fn launch(agent: Agent, args: Args) -> Result { // `inject_basic_auth(true)` covers the Basic-auth path. We // accept the perf hit (basic_auth disables the per-chunk // fast path) because GitHub connections aren't WebSocket. + // + // **Known gap (review #1).** The per-launch repo allow-list + // is enforced by the intercept hook on api.github.com only. + // The gh secret also allows github.com / codeload / raw / + // objects so `git push` / `git clone` can reach them with + // the substituted token — but no intercept rule filters + // those paths. A malicious in-VM agent that knows how to + // craft git smart-HTTP requests can push to any repo the + // host token has access to. Filtering git protocol over + // HTTPS requires a streaming intercept primitive that + // microsandbox doesn't have yet (intercept buffers the + // full request, capped at 64 KiB — git push pack data + // exceeds that). Until then, the threat model assumes the + // agent is well-intentioned but possibly mistaken. if let Some(file) = gh { n = n.secret(|s| { s.env("MSB_AGENT_VM_GH_UNUSED") @@ -580,7 +618,13 @@ pub async fn launch(agent: Agent, args: Args) -> Result { .with_context(|| format!("running {inner_cmd} in sandbox"))?; let mut stdout = tokio::io::stdout(); let mut stderr = tokio::io::stderr(); - let mut code: i32 = 1; + // Track whether we actually saw an Exited event. The previous + // `let mut code = 1` conflated "stream closed without Exited" + // (infra failure) with "agent exited with 1" (real failure) — + // CI couldn't tell them apart. Review finding #9. Now we + // return Err on premature stream close so the launcher + // bubbles up an actionable error. + let mut exit_code: Option = None; while let Some(event) = handle.recv().await { match event { ExecEvent::Stdout(b) => { @@ -592,7 +636,7 @@ pub async fn launch(agent: Agent, args: Args) -> Result { stderr.flush().await.ok(); } ExecEvent::Exited { code: c } => { - code = c; + exit_code = Some(c); break; } ExecEvent::Failed(payload) => { @@ -601,7 +645,12 @@ pub async fn launch(agent: Agent, args: Args) -> Result { ExecEvent::Started { .. } | ExecEvent::StdinError(_) => {} } } - code + match exit_code { + Some(c) => c, + None => anyhow::bail!( + "exec session event stream ended without Exited (agentd disconnect or microsandbox bug; partial output above)" + ), + } }; if profile { @@ -620,13 +669,9 @@ pub async fn launch(agent: Agent, args: Args) -> Result { eprintln!("[profile] remove: {:?}", t_remove.elapsed()); } - // Phase 5 safety net: diff the host credential files against the - // SHA-256 snapshot we took at launch. The Phase 4 refresh hook may - // legitimately rewrite them mid-session; anything else changing - // them is a smell worth surfacing. - if let Some(snap) = creds.snapshot.as_ref() { - crate::secrets::verify_snapshot(snap); - } + // Phase 5 safety net (host-cred mutation check) runs via the + // SnapshotGuard above, which drops at end of scope including + // any `?`-propagated error path. Ok(exit) } @@ -675,13 +720,43 @@ fn parse_extra_mounts(raw: &[String]) -> Result> { /// non-github remote) just yield an empty list — caller passes /// `--repo` to widen. fn detect_github_repos_in_cwd(project_dir: &Path) -> Vec { + // Hardening (review #6): the user may have just cloned a project + // they don't fully trust; running git in that repo before we've + // even built the sandbox would otherwise honour core.fsmonitor / + // alias.* — host RCE pre-sandbox. Disable the dangerous knobs + // and force safe.directory so we don't fail closed on a foreign- + // UID checkout. (Note: -c include.path= isn't valid git syntax; + // includeIf/include are only honored from files git already + // decides to read, which the `-c` flags can't suppress in 2.x, + // so we rely on disabling the per-repo *execution* hooks below.) let out = std::process::Command::new("git") - .args(["-C"]) + .args([ + // Disable repo-local config that runs binaries: + "-c", "core.fsmonitor=", + "-c", "core.fsmonitorHookVersion=", + // Editor/pager fall back to cat — nothing we run here + // needs them but a repo `core.pager = !bad-script` would + // otherwise fire on `remote -v` output paging. + "-c", "core.pager=cat", + "-c", "core.editor=:", + // Trust the dir even if owned by another UID (we only + // read `git remote -v`, no writes). + "-c", "safe.directory=*", + // Don't fetch anything across this invocation. + "-c", "protocol.allow=never", + "-C", + ]) .arg(project_dir) .args(["remote", "-v"]) .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::null()) + // Refuse host-level git config so a $GIT_CONFIG_GLOBAL + // override (env injected by the user shell) can't sneak past + // the `-c` overrides above. The empty values turn into a + // non-existent path lookup, which git treats as missing. + .env("GIT_CONFIG_SYSTEM", "/dev/null") + .env("GIT_CONFIG_GLOBAL", "/dev/null") .output(); let out = match out { Ok(o) if o.status.success() => o, diff --git a/crates/agent-vm/src/secrets.rs b/crates/agent-vm/src/secrets.rs index 6647c30..005028d 100644 --- a/crates/agent-vm/src/secrets.rs +++ b/crates/agent-vm/src/secrets.rs @@ -215,7 +215,12 @@ pub fn refresh( // `openai_token_file` with Codex. let opencode_openai_access_token_file = if openai_token_file.is_some() { match refresh_opencode(state_dir) { - Ok(_) => Some(opencode_openai_token_path(state_dir)), + // Only register the secret when refresh_opencode actually + // wrote a placeholder auth.json. `Ok(None)` (host file + // missing) means we have nothing to wire up — review + // finding #13. + Ok(Some(())) => Some(opencode_openai_token_path(state_dir)), + Ok(None) => None, Err(e) => { tracing::warn!(error = %e, "opencode credential refresh failed; skipping"); None @@ -467,10 +472,21 @@ fn decode_id_token_account(json: &Value) -> Option { .pointer("/tokens/id_token") .and_then(|v| v.as_str())?; let payload_b64 = id_token.split('.').nth(1)?; - let padded = format!("{}{}", payload_b64, "=".repeat((4 - payload_b64.len() % 4) % 4)); use base64::Engine as _; - let payload_bytes = base64::engine::general_purpose::URL_SAFE - .decode(padded.as_bytes()) + // JWT spec says base64url *without* padding. Use URL_SAFE_NO_PAD + // first (the conformant variant); fall back to STANDARD (some + // libraries emit JWTs with '+'/'/' instead of '-'/'_') — review + // finding #14. + let payload_bytes = base64::engine::general_purpose::URL_SAFE_NO_PAD + .decode(payload_b64.as_bytes()) + .or_else(|_| { + let padded = format!( + "{}{}", + payload_b64.replace('-', "+").replace('_', "/"), + "=".repeat((4 - payload_b64.len() % 4) % 4) + ); + base64::engine::general_purpose::STANDARD.decode(padded.as_bytes()) + }) .ok()?; let payload: Value = serde_json::from_slice(&payload_bytes).ok()?; payload