diff --git a/.cargo/mutants.toml b/.cargo/mutants.toml new file mode 100644 index 00000000..f8a1adcc --- /dev/null +++ b/.cargo/mutants.toml @@ -0,0 +1,27 @@ +# cargo-mutants configuration (Issue #673). +# +# Incident (2026-10-14, macOS): a `cargo mutants` run replaced +# src/bounded_process.rs: `replace signalable_process_group -> Option +# with Some(1)` +# and the mutated test binary then executed `kill(-1, SIGTERM)`, terminating +# every process the user owned (the launchd log recorded 102 SIGTERM exits). +# The product guard was correct; the mutant removed it. The same shape exists +# in `group_is_signalable` (src/bin/gui_server/trial_process.rs), where a +# `-> true` mutant turns `send_group_signal(1, Kill)` into `kill(-1, SIGKILL)`. +# +# Exclusion is the immediate mitigation (Issue #673 対処の方向 1). It is a +# *listing* filter: cargo-mutants applies `exclude_re` to the mutation +# description, and the command line `--re` does not override it. Removing these +# entries requires the isolated-container evidence from 対処の方向 4: the mutant +# must actually be enumerated and executed inside the container, the dangerous +# value must be refused by the last, non-mutatable defense in the `kill` +# wrapper, no dangerous syscall may be observed, the same-UID sentinel must +# survive, and only then may the exclusion be dropped (対処の方向 4, last item). +# +# The names match cargo-mutants' description text, for example +# `replace signalable_process_group -> Option with Some(1)` +# `replace group_is_signalable -> bool with true` +exclude_re = [ + "signalable_process_group", + "group_is_signalable", +] diff --git a/docs/dev/dangerous-commands.md b/docs/dev/dangerous-commands.md new file mode 100644 index 00000000..235bf52f --- /dev/null +++ b/docs/dev/dangerous-commands.md @@ -0,0 +1,243 @@ +# Dangerous commands: inventory, defenses, and detection limits + +This document is the human list behind +`tests/issue673_dangerous_command_inventory.rs`. The test greps the tree and +fails when a signal call or a recursive delete appears that is not declared in +its `DECLARED` table, or when a declared count changes. The list below explains +each site and, for the sites that are deliberately not defended, why. + +This is **detection for review, not isolation**. A green test means "no +undeclared site of the covered kinds appeared", not "no dangerous command can +run". The isolated-container verification of Issue #673 対処の方向 4 is what +bounds execution; that runs outside this repository's tests. + +Two incidents motivate this file: + +- 2026-10-14 (#673): a `cargo mutants` run replaced + `signalable_process_group -> Option` with `Some(1)`; the mutated test + binary executed `kill(-1, SIGTERM)` and terminated every process the user + owned. `.cargo/mutants.toml` excludes that selector and `group_is_signalable`. +- The same shape exists at every negative `kill`: a selector that decides the + target is separated from the `kill` by a function boundary, so a mutant can + cross the boundary with a dangerous value. + +## 1. Signal sites + +Column `Defense added` is the non-mutatable `assert!` written **directly in the +function that calls `kill`** (never in a wrapper helper: cargo-mutants replaces +the body of a `()`-returning helper with `()`, deleting a helper's check). The +existing `Err`/`bail!` paths are unchanged; the `assert!` is the last fortress +and never fires for a real value. + +| File | Function | Kind | Value source | Existing defense | Defense added | +| --- | --- | --- | --- | --- | --- | +| `src/bounded_process.rs` | `terminate_process_group_by_pid` | `kill(-pgid)` ×2 | `signalable_process_group(child.id())` | `signalable_process_group` (`pgid > 1`) + `Err` path | `assert!(-pgid < -1)` before each `kill` | +| `src/bounded_process.rs` | `terminate_process_group_with_grace` | `kill(-pgid)` ×2 | `signalable_process_group(child.id())` | `signalable_process_group` | `assert!(-pgid < -1)` before each `kill` | +| `src/bounded_process.rs` | `dangerous_sentinel_same_uid_other_group_survives` (`group_exists`) | signal 0 probe | spawned target pid | none | **none** — container-only sentinel (`#[ignore]`, `COMMANDAGENT_ISSUE673_CONTAINER=1`) | +| `src/bin/gui_server/trial_process.rs` | `send_group_signal` | `kill(-process_group)` | `ProcessIdentity.process_group` from `Child::id()` | `group_is_signalable` + `bail!` | `assert!(-process_group < -1)` | +| `src/bin/gui_server/trial_process.rs` | `process_group_exists` | signal 0 probe | `ProcessIdentity.process_group` | `group_is_signalable` | `assert!(-process_group < -1)` | +| `src/planner/runner/acceptance.rs` | `signal_dev_server_process_group` | `kill(-pid)` | dev-server `Child::id()` | none | `assert!(-pid < -1)` | +| `src/planner/runner/acceptance.rs` | `dev_server_process_group_exists` | signal 0 probe | dev-server `Child::id()` | none | `assert!(-pid < -1)` | +| `src/preflight.rs` | `SystemPortControl::kill` | positive pid | `lsof` owner pid | `Option` `Some` + `context` | `assert!(pid > 1)` and `assert!(pid != own pid)` | +| `src/planner/runner/tests/support/process.rs` | `process_group_exists` | signal 0 probe (test support) | spawned child pid | none | **none** — test-only helper, outside this Issue's file scope (see §4) | +| `tests/headless_summary.rs` | inline | positive pid | spawned `Child::id()` | none | **none** — test-only, outside this Issue's file scope | +| `tests/gui_server.rs` | inline | signal 0 probe | spawned child pid | none | **none** — test-only, outside this Issue's file scope | +| `tests/gui_read_only_guard.rs` | inline | none (string literal) | — | — | **none** — asserts the source text of `trial_process.rs` | + +`starts_with`-style work is not applicable here: the check is on the value that +is passed to `kill` (`-pgid < -1`), which is exact. + +### Selectors listed in `.cargo/mutants.toml` + +`signalable_process_group` and `group_is_signalable` return a value, so a mutant +can replace the whole body (`Some(1)` / `true`). They are excluded from +mutation, and removing the exclusion requires the isolated-container evidence of +対処の方向 4. The `assert!` in each `kill` caller is what makes the value safe +even before that evidence exists. + +### Functions to compare against `cargo mutants --list` + +Selectors and the functions that call `kill`, plus the functions that decide the +value (leader enumerates the list in the isolated container; the worker does not +run `cargo mutants`): + +- `signalable_process_group`, `group_is_signalable` +- `terminate_process_group_by_pid`, `terminate_process_group_with_grace`, + `wait_with_timeout_cancel_force_grace_and_clock`, + `wait_with_timeout_cancel_and_force`, `wait_with_timeout_and_cancel`, + `wait_with_timeout`, `run_with_timeout_cancel_and_force`, `run_with_timeout`, + `spawn_child`, `configure_child_session` +- `send_group_signal`, `process_group_exists`, `terminate_unregistered`, + `request_stop`, `start_force_watchdog`, `ProcessIdentity`, `register` +- `signal_dev_server_process_group`, `dev_server_process_group_exists`, + `cleanup_dev_server_child_unix`, `wait_for_dev_server_process_group_exit` +- `SystemPortControl::kill`, `handle_busy_port`, `run_for_goal_with`, + `parse_lsof_owner` + +## 2. Recursive deletes (`remove_dir_all`) + +Nine sites under `src` plus seven test helpers. Each is classified by whether a +mutation that moves the target could delete something the run does not own. The +guard is `crate::tools::dir_fd::strict_descendant_real_path(owned_root, target)`: +it `canonicalize`s an `owned_root` that is derived **independently of the target** +and the target, and returns the target's real path only when +`target != root && target.starts_with(root)`. That rejects the root itself, an +ancestor, a sibling, a path outside the root, a `..` that escapes, and a symlink +(even one that points back inside) — none of which `starts_with` alone rejects. +Each caller passes **the returned real path** to `remove_dir_all`, so the checked +value is the value that is removed; a `None` return deletes nothing. Independence +holds on the production `install` path. The test-only `install_for_test` seam is +the one exception: it derives the root from the delete target's parent (see the +note after the table). + +| File | Site | Kind | Owned root | Guard | +| --- | --- | --- | --- | --- | +| `src/bin/gui_server/session_paths.rs` | `rollback_unstarted` | product cleanup | the `runs` dir (`SessionPaths.runs_root`) | added | +| `src/model_probe.rs` | `ScratchWorkspace::cleanup` | product cleanup | `std::env::temp_dir()` (`ScratchWorkspace.root`) | added | +| `src/planner/pack/supply.rs` | `install` (failure path) | product staging | `packs/` (`packs_root`) | added | +| `src/planner/pack/supply.rs` | `install` (success path) | product staging | `packs/` | added | +| `src/tools/os_sandbox.rs` | `cleanup` | product temp | `/sandbox` (`Installed.run_tmp_root`) | added — production `install` only; see the note below | +| `src/tools/dir_fd.rs` | test swap seam (×2) | test helper | a synthetic tempdir | **none** — synthetic path | +| `src/tools/edit.rs` | test swap seam | test helper | a synthetic tempdir | **none** — synthetic path | +| `src/tools/write.rs` | test swap seam | test helper | a synthetic tempdir | **none** — synthetic path | +| `tests/gui_server.rs` (×4), `tests/issue505_write_toctou.rs` (×3) | test cleanup | test helper | synthetic tempdirs | **none** — synthetic paths | + +Note on `src/tools/os_sandbox.rs`: the production `install` (`install_impl`) +decides `/sandbox` independently of the run temp dir and stores it, so +`cleanup` compares against a reference a mutation of the run temp dir cannot +move. The test-only `install_for_test` seam is not independent — it builds the +root from `policy.run_tmp_dir.parent()`, i.e. from the delete target itself — and +is called only by tests, whose run temp dir is their own tempdir. Taking an +independent root there needs edits to files outside Issue #673's scope +(`src/lib.rs`, `tests/issue502_os_sandbox_network.rs`, +`tests/issue502_os_sandbox_read.rs`), so it is left as a follow-up. + +The product sites keep deleting the product location (pack staging stays under +`packs/`, the sandbox temp stays under `/sandbox`); none is redirected to +a generic temp dir. The product-owned delete targets are not optional for the +product, so the fix is a real-path check, not a relocation. + +`strict_descendant_real_path`'s own mutation is not defended by an `assert!`: +the Issue asks for a real-path check on deletion, and the check guards a delete, +not a syscall target. Returning `Option` keeps the mutation direction +safe — a body-replacement mutant yields `None` (the `Option` default), which +deletes nothing — and the guard is exercised by its unit tests in +`src/tools/dir_fd.rs` (fake root, sibling canary) and reviewed in the +isolated-container run. + +## 2a. Shell and Python sites + +The inventory also reads tracked `scripts/**/*.sh`, `scripts/**/*.py`, and +`tests/**/*.py`. The declared sites it found: + +| File | Token | Count | Notes | +| --- | --- | --- | --- | +| `scripts/build-release.sh` | `rm -rf` | 3 | build/publish dir cleanup | +| `scripts/install.sh` | `rm -rf` | 1 | temp dir cleanup | +| `scripts/cmate-pm-watch.sh` | `rm -rf` | 3 | stop-pattern string and its own grep | +| `scripts/demo/record_cli_demo.py` | `os.kill(` | 1 | terminate a recorded child | +| `scripts/eval_lib/heavy_lock.py` | `os.kill(` | 2 | liveness probe (`signal 0`) | +| `scripts/eval_lib/heavy_lock.py` | `shutil.rmtree(` | 2 | lock dir cleanup | +| `scripts/eval_lib/goal_verify_recovery_deterministic_pair.py` | `os.killpg(` | 2 | terminate a spawned process group | +| `scripts/eval_lib/goal_verify_recovery_exposure_corpus_pilot.py` | `os.killpg(` | 2 | terminate a spawned process group | +| `scripts/eval_lib/goal_verify_workspaces_v3.py` | `shutil.rmtree(` | 1 | destination cleanup | +| `scripts/eval_lib/goal_verify_live_v4.py` | `shutil.rmtree(` | 2 | destination cleanup | +| `scripts/nextjs_domain_oracle.py` | `shutil.rmtree(` | 1 | scratch cleanup | +| `tests/eval/test_heavy_lock.py` | `os.kill(` | 1 | liveness probe (`signal 0`) | + +No external `kill`/`pkill`/`killall` or `rm -r ` occurs anywhere in the scanned +set; every `rm -rf`, `os.kill(`, `os.killpg(`, and `shutil.rmtree(` site is +listed above. + +## 3. Detection limits + +The inventory test matches **direct API names and shell commands in tracked `.rs`, +`.sh`, and `.py` files**: + +- Rust (`src/**`, `tests/**`, `build.rs`): `libc::kill(`, `libc::killpg(`, + `nix::sys::signal::kill(`, `Command::new("kill"|"killall"|"pkill")`, and + `remove_dir_all(`. +- Shell (`scripts/**/*.sh`): external `kill `, `pkill`, `killall`, `rm -rf`, + `rm -r `. +- Python (`scripts/**/*.py`, `tests/**/*.py`): `os.kill(`, `os.killpg(`, + `shutil.rmtree(`. +- **Missed**: an alias or a re-export of a covered name; a wrapper function that + hides the token; an external `kill` not followed by a space (for example + `kill$pid` or a bare `kill` on its own line); a signal sent from inside `sh -c` + or a Python string (`python3 -c "import os; os.kill(...)"`); `kill` invoked + through a variable or `exec`; Node's `child.kill()` in `scripts/*.mjs`; and any + other language not scanned. +- `grep` counts string literals too (for example + `tests/gui_read_only_guard.rs`, which asserts the source text, and + `scripts/cmate-pm-watch.sh`, whose stop-pattern string contains `rm -rf`). + Those are declared, so the inventory stays honest about them. + +Out of the automated match, with the reason: + +- **Single-file delete (`remove_file`)**: many sites (test cleanup and product + temp files); the dangerous case at Issue #673 scale is the recursive delete, + and a single `remove_file` cannot remove a tree. Tracked for review, not + machine-matched. +- **Overwrite / truncate / rename (`File::create`, `O_TRUNC`, `fs::rename`)**: + high count, mostly correct-by-construction temp staging. +- **Permission changes (`set_permissions`)**: not present in the signal/delete + threat model of this Issue. +- **Terminal ioctl / termios / shared-fd flags**: measured by Issue #667's own + probes, not by this inventory. +- **`sh -c` and interpreter string bodies**: invisible to a token grep by design; + the lexical and OS guards cover the write side, the inventory cannot. +- **Node/`.mjs` scripts**: `scripts/issue45*_*.mjs` call `server.kill('SIGTERM')` + on a child the script itself spawned. The token set the Issue names does not + include Node's `ChildProcess.kill()`, so these are outside the match. + +The inventory is thus a tripwire for the two categories with the largest blast +radius (signalling every process, and recursive deletion), and an explicit +admission that the remaining categories are not machine-checked here. + +## 4. Sites outside this Issue's file scope + +`src/planner/runner/tests/support/process.rs`, `tests/headless_summary.rs`, and +`tests/gui_server.rs` contain signal calls but are not in Issue #673's +`## 対象ファイル`, so no `assert!` was added. They are test-only: the value comes +from a child the test itself spawned, and the signal is an existence probe or a +positive-pid `SIGINT`. They are listed so the inventory is complete, and so a +future Issue can decide whether to guard them too. + +## 5. Dangerous-value tests (mutated runs only in the isolated container) + +These pass dangerous values (0, 1, negative) to a function that calls `kill`. +They must not run on the host **under a mutation** — neither inside a `cargo +mutants` run nor on a tree where the defense was removed by hand; that is the +Mutation-testing rule in `parallel-dev-harness.md` §3a. On an unmutated tree they +do run on the host (CI and the verify gates do not skip them): the +`Err`/`bail!`/`return false` path returns before `libc::kill`, so no syscall +happens. A newly added `dangerous_value_` test is first confirmed once in the +isolated container on an unmutated tree, before it may run on the host. The +`dangerous_value_` tests deliberately do not check the selector, so under a +mutated selector the value reaches the non-mutatable `assert!` in front of `kill` +and panics there (the Issue #673 対処の方向 4 requirement), and no syscall is +made. + +| Test | Target | Notes | +| --- | --- | --- | +| `bounded_process::tests::process_group_guard_refuses_non_child_group_ids` | `terminate_process_group_by_pid` | existing; also checks the selector | +| `bounded_process::tests::dangerous_value_terminate_process_group_by_pid_refuses_init_and_caller_group` | `terminate_process_group_by_pid(0/1)` | new; skips the selector, so a mutated selector reaches the `assert!` | +| `preflight::tests::dangerous_value_system_port_control_kill_refuses_init_zero_and_own_pid` | `SystemPortControl::kill(0/1/own pid)` | new; on an unmutated tree the `bail!` paths return `Err` before `libc::kill` | +| `bounded_process::tests::dangerous_sentinel_same_uid_other_group_survives` | real `kill` scope | `#[ignore]`, needs `COMMANDAGENT_ISSUE673_CONTAINER=1` | +| `trial_process::tests::process_group_guard_rejects_init_and_caller_group` | `send_group_signal` / `process_group_exists` | existing; also checks the selector; `--features gui` | +| `trial_process::tests::dangerous_value_send_group_signal_refuses_init_caller_and_negative_groups` | `send_group_signal(0/1/-1)`, `process_group_exists(0/1/-1)` | new; skips the selector; `--features gui` | + +Docker commands (from a container that shares no host PID namespace, is not +`--privileged`, and shares no host HOME, terminal, or Docker socket): + +```sh +cargo test --lib -- --exact bounded_process::tests::process_group_guard_refuses_non_child_group_ids +cargo test --lib -- --exact bounded_process::tests::dangerous_value_terminate_process_group_by_pid_refuses_init_and_caller_group +cargo test --lib -- --exact preflight::tests::dangerous_value_system_port_control_kill_refuses_init_zero_and_own_pid +COMMANDAGENT_ISSUE673_CONTAINER=1 cargo test --lib -- --ignored --exact \ + bounded_process::tests::dangerous_sentinel_same_uid_other_group_survives +cargo test --features gui --bin gui_server -- --exact \ + trial_process::tests::process_group_guard_rejects_init_and_caller_group +cargo test --features gui --bin gui_server -- --exact \ + trial_process::tests::dangerous_value_send_group_signal_refuses_init_caller_and_negative_groups +``` diff --git a/docs/dev/parallel-dev-harness.md b/docs/dev/parallel-dev-harness.md index eff20fae..d5572309 100644 --- a/docs/dev/parallel-dev-harness.md +++ b/docs/dev/parallel-dev-harness.md @@ -301,6 +301,39 @@ Approval is always written, never implied: the PM's dispatch brief says (`gh pr list --state merged --head `), then `git push origin --delete `. +## 3a. Mutation-testing rules + +`cargo mutants` may run only under these rules. They exist because a macOS run +on 2026-10-14 mutated a guard (`signalable_process_group -> Some(1)`) and the +mutated test binary executed `kill(-1, SIGTERM)`, stopping every process the user +owned (102 SIGTERM exits in the launchd log; #673). + +- Scope the run to the functions named in the brief. Widening it needs the PM's + confirmation. +- Use a fresh run script and a fresh output directory for every run. Before + running, produce `cargo mutants --list` and have the PM match every mutant's + name and replacement value (a count alone is not enough). +- Mutations that touch signal, deletion, or the child session always run in an + isolated environment, whether or not the last, non-mutatable defense is in + place (Docker; a macOS-only test on a disposable VM or a dedicated account). + The Docker container is non-root (uid 1000) with `--network none`, no mounts, + and no `--privileged`, `--pid=host`, or Docker socket; the tree enters only + through the `docker build` context or `docker cp`. +- `cargo mutants`' `--timeout` does not reap processes a test left behind (for + example a child that ignores `TERM`), so each run uses a disposable container + and the whole container is discarded afterwards (`docker rm -f`). On the + 2026-10-15 confirmation run a `TERM`-ignoring child survived inside the + container after the timeout. +- Host tests run with the narrowest test-name filter, a dedicated secret-free + HOME and Git configuration, and known executables. Set `GITHUB_STEP_SUMMARY` + only to an unset value or a file the new run owns. The outer `GIT_*` is dropped + by the bounded child's allowlist, so verify the actual child environment. +- `--jobs 1`, the heavy lock, and the user being at the keyboard are not + substitutes for isolation or for reaping the processes a run started. + +The signal sites, their defenses, and the functions to compare against +`cargo mutants --list` are listed in `docs/dev/dangerous-commands.md`. + ## 4. auto-yes policy | Session | auto-yes | diff --git a/src/bin/gui_server/session_paths.rs b/src/bin/gui_server/session_paths.rs index db2491cc..d35ca5d8 100644 --- a/src/bin/gui_server/session_paths.rs +++ b/src/bin/gui_server/session_paths.rs @@ -46,6 +46,10 @@ pub(super) struct RunRecordPaths { pub(super) struct SessionPaths { run_root: PathBuf, + /// The `runs` directory that owns `run_root`. Kept separately from + /// `run_root` so a recursive delete can re-check the real path against an + /// independent reference (Issue #673). + runs_root: PathBuf, execution_workspace: PathBuf, selected_binding: Option, } @@ -73,8 +77,10 @@ impl SessionPaths { || workspace.join(SESSION_WORKSPACES_DIRECTORY).join(id), |binding| binding.canonical_path.clone(), ); + let runs_root = commandagent::runtime_paths::runs_dir(workspace); Ok(Self { - run_root: commandagent::runtime_paths::runs_dir(workspace).join(id), + run_root: runs_root.join(id), + runs_root, execution_workspace, selected_binding, }) @@ -105,6 +111,7 @@ impl SessionPaths { ); return Ok(Some(Self { run_root, + runs_root, execution_workspace, selected_binding, })); @@ -297,7 +304,34 @@ impl SessionPaths { } pub(super) fn rollback_unstarted(&self) -> anyhow::Result<()> { - match std::fs::remove_dir_all(&self.run_root) { + // Issue #673: re-check the recursive delete target against the real + // path of the runs root that owns it, so a mutation that moves + // `run_root` outside `runs/` (a `..`, an ancestor symlink, the root + // itself, or the filesystem root) is refused before anything is + // removed. The checked real path is the value that is removed. A + // missing target left nothing to roll back and stays `Ok`. + match std::fs::symlink_metadata(&self.run_root) { + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => { + return Err(error).with_context(|| { + format!( + "inspect unstarted session directory {}", + self.run_root.display() + ) + }); + } + Ok(_) => {} + } + let Some(checked) = commandagent::tools::dir_fd::strict_descendant_real_path( + &self.runs_root, + &self.run_root, + ) else { + anyhow::bail!( + "refusing to remove a session run outside the runs root: {}", + self.run_root.display() + ); + }; + match std::fs::remove_dir_all(&checked) { Ok(()) => Ok(()), Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), Err(error) => Err(error).with_context(|| { diff --git a/src/bin/gui_server/trial_process.rs b/src/bin/gui_server/trial_process.rs index a4d432ba..11c4a682 100644 --- a/src/bin/gui_server/trial_process.rs +++ b/src/bin/gui_server/trial_process.rs @@ -426,6 +426,11 @@ enum Signal { /// Guards the negative `kill` targets: group id 1 is init and 0 is the /// caller's own group, so only a real child group id may be signalled. +/// +/// This is a value-returning selector, so cargo-mutants can replace the whole +/// body with `true` (Issue #673). It is listed in `.cargo/mutants.toml`, and +/// every `kill` caller re-checks the converted target with a non-mutatable +/// `assert!(-process_group < -1)` in the function that calls `kill`. #[cfg(unix)] fn group_is_signalable(process_group: i32) -> bool { process_group > 1 @@ -436,6 +441,15 @@ fn send_group_signal(process_group: i32, signal: Signal) -> anyhow::Result<()> { if !group_is_signalable(process_group) { bail!("refusing to signal process group {process_group}"); } + // Last, non-mutatable defense (Issue #673), written directly in the + // function that calls `kill`: even a mutated selector + // (`group_is_signalable -> true`) panics here instead of executing + // `kill(-1, ...)` (every signalable process) or `kill(0, ...)` (the + // caller's own group). The `bail!` path above is unchanged. + assert!( + -process_group < -1, + "refusing to signal process group {process_group}" + ); let signal = match signal { Signal::Interrupt => libc::SIGINT, Signal::Kill => libc::SIGKILL, @@ -464,6 +478,12 @@ fn process_group_exists(process_group: i32) -> bool { if !group_is_signalable(process_group) { return false; } + // Last, non-mutatable defense (Issue #673), written directly in the + // function that calls `kill`. + assert!( + -process_group < -1, + "refusing to signal process group {process_group}" + ); // SAFETY: `process_group` is checked to be greater than 1 and is derived // from the delegated child PID; signal 0 only probes for its existence. let result = unsafe { libc::kill(-process_group, 0) }; @@ -517,4 +537,18 @@ mod tests { assert!(send_group_signal(1, Signal::Kill).is_err()); assert!(!process_group_exists(1)); } + + /// Issue #673 対処の方向 4: pass 0 (the caller's own group), 1 (init), and + /// -1 straight to the functions that call `kill`, skipping the selector's + /// own assertion, so a mutated `group_is_signalable` reaches the + /// non-mutatable `assert!` in front of `kill` and panics there instead of + /// signalling `kill(0)` / `kill(-1)`. Runs only in the isolated container. + #[cfg(unix)] + #[test] + fn dangerous_value_send_group_signal_refuses_init_caller_and_negative_groups() { + for process_group in [0, 1, -1] { + assert!(send_group_signal(process_group, Signal::Kill).is_err()); + assert!(!process_group_exists(process_group)); + } + } } diff --git a/src/bounded_process.rs b/src/bounded_process.rs index 3d7761ee..6455cfb4 100644 --- a/src/bounded_process.rs +++ b/src/bounded_process.rs @@ -401,6 +401,11 @@ pub fn terminate_process_group(child: &mut Child) { /// signal with a negative `kill` target. The child is spawned with /// `setsid()`, so its PID equals its group id, but `-1` would signal /// every signalable process and `0`/`1` would target the caller or init. +/// +/// This is a value-returning selector, so cargo-mutants can replace the whole +/// body with `Some(1)` (Issue #673). It is listed in `.cargo/mutants.toml`, and +/// every `kill` caller re-checks the converted target with a non-mutatable +/// `assert!(-pgid < -1)` in the function that calls `kill`. #[cfg(unix)] fn signalable_process_group(pid: u32) -> Option { let pgid = i32::try_from(pid).ok()?; @@ -415,6 +420,16 @@ pub fn terminate_process_group_by_pid(pid: u32) -> io::Result<()> { "refusing to signal a process group outside the child range", )); }; + // Last, non-mutatable defense (Issue #673). cargo-mutants does not + // rewrite comparisons inside `assert!`, and this is written directly in + // the function that calls `kill`, so a mutated selector (`Some(0)`, + // `Some(1)`, `Some(-1)`) panics here instead of executing `kill(0, ...)` + // (the caller's own group) or `kill(-1, ...)` (every signalable + // process). The `Err` path above is unchanged. + assert!( + -pgid < -1, + "refusing to signal a process group outside the child range" + ); // SAFETY: `pgid` is greater than 1 and derived from a child spawned // with `setsid()`, so `-pgid` names only that child's own // process group, never pid 0/1 or the caller's group. @@ -475,6 +490,12 @@ fn terminate_process_group_with_grace( #[cfg(unix)] { if let Some(pgid) = signalable_process_group(child.id()) { + // Last, non-mutatable defense (Issue #673), written directly in the + // function that calls `kill`. + assert!( + -pgid < -1, + "refusing to signal a process group outside the child range" + ); // SAFETY: `pgid` is greater than 1 and was created with // `configure_child_session()` for this child, so `-pgid` targets // only the child's process group, never pid 0/1 or the caller's. @@ -497,6 +518,12 @@ fn terminate_process_group_with_grace( #[cfg(unix)] { if let Some(pgid) = signalable_process_group(child.id()) { + // Last, non-mutatable defense (Issue #673), written directly in the + // function that calls `kill`. + assert!( + -pgid < -1, + "refusing to signal a process group outside the child range" + ); // SAFETY: the same checked child-derived group identifier as above. unsafe { libc::kill(-pgid, libc::SIGKILL); @@ -861,6 +888,95 @@ mod tests { assert!(terminate_process_group_by_pid(1).is_err()); } + /// Issue #673 対処の方向 4: pass 0 (the caller's own group) and 1 (init) + /// straight to the function that calls `kill`, skipping the selector's own + /// assertions, so a mutated `signalable_process_group` reaches the + /// non-mutatable `assert!` in front of `kill` and panics there instead of + /// signalling. Runs only in the isolated container. + #[cfg(unix)] + #[test] + fn dangerous_value_terminate_process_group_by_pid_refuses_init_and_caller_group() { + assert!(terminate_process_group_by_pid(0).is_err()); + assert!(terminate_process_group_by_pid(1).is_err()); + } + + /// Issue #673 sentinel test. It exercises a real `kill(-pgid, ...)`, so it + /// must run only inside the isolated container: it panics unless + /// `COMMANDAGENT_ISSUE673_CONTAINER=1` is set, and it is `#[ignore]`d so + /// `cargo test` never runs it in a host process. Docker command (from the + /// repository root of a container that shares no host PID namespace): + /// + /// ```text + /// COMMANDAGENT_ISSUE673_CONTAINER=1 \ + /// cargo test --lib -- --ignored --exact \ + /// bounded_process::tests::dangerous_sentinel_same_uid_other_group_survives + /// ``` + /// + /// It proves the scope of the signal: a same-UID process in a **different** + /// process group survives, while the target child's group and its + /// grandchild are terminated. + #[cfg(unix)] + #[test] + #[ignore = "run only inside the isolated container (COMMANDAGENT_ISSUE673_CONTAINER=1)"] + fn dangerous_sentinel_same_uid_other_group_survives() { + if std::env::var("COMMANDAGENT_ISSUE673_CONTAINER") + .ok() + .as_deref() + != Some("1") + { + panic!("run only inside the isolated container"); + } + use std::os::unix::process::CommandExt; + + // Sentinel: same UID, its own process group, and not the target group. + let mut sentinel = Command::new("sh"); + sentinel + .arg("-c") + .arg("sleep 30") + .process_group(0) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()); + let mut sentinel = sentinel.spawn().expect("spawn the sentinel"); + + // Target: a `setsid` child whose `sh` also leaves a grandchild in the + // same process group. + let mut target = Command::new("sh"); + target + .arg("-c") + .arg("sh -c 'sleep 30' & sleep 30") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()); + let mut target = spawn_child(&mut target).expect("spawn the target"); + let target_pid = target.id(); + thread::sleep(Duration::from_millis(300)); + + terminate_process_group_by_pid(target_pid).expect("reap the target group"); + let _ = target.wait(); + assert!( + !group_exists(target_pid), + "the target group must be gone after the reap" + ); + assert!( + sentinel.try_wait().expect("poll the sentinel").is_none(), + "the same-UID sentinel in another group must survive" + ); + + let _ = sentinel.kill(); + let _ = sentinel.wait(); + } + + #[cfg(unix)] + fn group_exists(pgid: u32) -> bool { + let Ok(pgid) = i32::try_from(pgid) else { + return false; + }; + // SAFETY: signal 0 only probes for the group's existence. + let rc = unsafe { libc::kill(-pgid, 0) }; + rc == 0 + } + struct TestEnvGuard { keys: Vec<&'static str>, } diff --git a/src/model_probe.rs b/src/model_probe.rs index 93fb6c06..fe028a08 100644 --- a/src/model_probe.rs +++ b/src/model_probe.rs @@ -212,16 +212,22 @@ enum ProbeTaskKind { } struct ScratchWorkspace { + /// The system temp directory that owns `path`. Kept separately from `path` + /// so the recursive delete in [`ScratchWorkspace::cleanup`] can re-check the + /// real path against an independent reference (Issue #673). + root: PathBuf, path: PathBuf, cleaned: bool, } impl ScratchWorkspace { fn create() -> anyhow::Result { - let path = std::env::temp_dir().join(format!("anvil-model-probe-{}", Uuid::now_v7())); + let root = std::env::temp_dir(); + let path = root.join(format!("anvil-model-probe-{}", Uuid::now_v7())); fs::create_dir_all(&path) .with_context(|| format!("failed to create scratch workspace {}", path.display()))?; Ok(Self { + root, path, cleaned: false, }) @@ -231,7 +237,27 @@ impl ScratchWorkspace { if self.cleaned { return true; } - self.cleaned = match fs::remove_dir_all(&self.path) { + // Issue #673: only remove a scratch directory that is a strict + // descendant of the temp dir it was created in, so a mutation that + // moves `path` cannot recursively delete an unrelated directory. + match std::fs::symlink_metadata(&self.path) { + Err(err) if err.kind() == std::io::ErrorKind::NotFound => { + self.cleaned = true; + return true; + } + Err(_) => { + self.cleaned = false; + return false; + } + Ok(_) => {} + } + let Some(checked) = + crate::tools::dir_fd::strict_descendant_real_path(&self.root, &self.path) + else { + self.cleaned = false; + return false; + }; + self.cleaned = match fs::remove_dir_all(&checked) { Ok(()) => true, Err(err) if err.kind() == std::io::ErrorKind::NotFound => true, Err(_) => false, diff --git a/src/planner/pack/supply.rs b/src/planner/pack/supply.rs index d18f9957..a438b677 100644 --- a/src/planner/pack/supply.rs +++ b/src/planner/pack/supply.rs @@ -687,7 +687,8 @@ impl SupplyRoot { fn install(&self, directory: &Path, members: &Members) -> Result<(), SupplyError> { let nonce = uuid::Uuid::now_v7().to_string(); - let staging_root = self.packs_root().join(STAGING_DIRECTORY); + let packs_root = self.packs_root(); + let staging_root = packs_root.join(STAGING_DIRECTORY); let session = staging_root.join(&nonce); let pending = session.join("pack"); self.create_directory(&pending)?; @@ -711,10 +712,23 @@ impl SupplyRoot { if let Some(previous) = previous { let _ = std::fs::rename(previous, directory); } - let _ = std::fs::remove_dir_all(&session); + // Issue #673: only remove the staging session when it is a strict + // descendant of `packs/`, so a mutation that moves `session` cannot + // recursively delete an unrelated directory. The checked real path + // is the value that is removed. + if let Some(checked) = + crate::tools::dir_fd::strict_descendant_real_path(&packs_root, &session) + { + let _ = std::fs::remove_dir_all(checked); + } return Err(SupplyError::io(directory, source)); } - let _ = std::fs::remove_dir_all(&session); + // Issue #673: the same strict-descendant check for the success path. + if let Some(checked) = + crate::tools::dir_fd::strict_descendant_real_path(&packs_root, &session) + { + let _ = std::fs::remove_dir_all(checked); + } // Leave no residue: the staging parent is only meaningful while a // rename is in flight, and other roots below it belong to other packs. let _ = std::fs::remove_dir(staging_root); diff --git a/src/planner/runner/acceptance.rs b/src/planner/runner/acceptance.rs index 397b9ef0..14d05db7 100644 --- a/src/planner/runner/acceptance.rs +++ b/src/planner/runner/acceptance.rs @@ -1322,6 +1322,13 @@ pub(super) fn signal_dev_server_process_group( ) -> std::io::Result<()> { let pid = i32::try_from(pid).map_err(|_| std::io::Error::other("child pid does not fit pid_t"))?; + // Last, non-mutatable defense (Issue #673). The group target comes from + // `std::process::Child::id()`, so `-pid < -1` always holds for the real + // value; the check is a fortress against a mutation that rewrites the + // converted value to 0 (the caller's own group) or 1 (init). cargo-mutants + // does not rewrite comparisons inside `assert!`, and this is written + // directly in the function that calls `kill`. + assert!(-pid < -1, "refusing to signal process group {pid}"); // SAFETY: `kill` is called with a process-group id derived from the child // pid returned by `std::process::Child` and a libc signal constant. let rc = unsafe { libc::kill(-pid, signal) }; @@ -1363,6 +1370,9 @@ pub(super) fn dev_server_process_group_exists(pgid: u32) -> bool { let Ok(pgid) = i32::try_from(pgid) else { return false; }; + // Last, non-mutatable defense (Issue #673), written directly in the + // function that calls `kill`. + assert!(-pgid < -1, "refusing to signal process group {pgid}"); // SAFETY: signal 0 performs existence/permission checking only, using // a process-group id derived from a child process spawned by this probe. let rc = unsafe { libc::kill(-pgid, 0) }; diff --git a/src/preflight.rs b/src/preflight.rs index 4b63f3a7..36b16daa 100644 --- a/src/preflight.rs +++ b/src/preflight.rs @@ -170,6 +170,28 @@ impl PortControl for SystemPortControl { #[cfg(unix)] { let pid = i32::try_from(pid).context("owning pid does not fit platform pid type")?; + // Normal path (Issue #673): a pid that must not be signalled returns + // `Err`, never a panic. lsof can report pid 1 under socket activation + // (launchd/systemd), and a user who chose kill must still get the + // pre-#673 `Err`. 0 (the caller's own group) and 1 (init) are refused + // here, then the caller's own pid. This is not an ownership proof for + // the port owner. + if pid <= 1 { + bail!("refusing to signal pid {pid}"); + } + if pid == std::process::id() as i32 { + bail!("refusing to signal the caller's own pid"); + } + // Last, non-mutatable defense (Issue #673): the `bail!` paths above + // are the normal result, and these `assert!`s are the fortress for a + // mutated tree. cargo-mutants does not rewrite comparisons inside + // `assert!`, and they are written directly in the function that calls + // `kill`. + assert!(pid > 1, "refusing to signal pid {pid}"); + assert!( + pid != std::process::id() as i32, + "refusing to signal the caller's own pid" + ); // SAFETY: sends SIGTERM to the concrete pid reported by lsof after // explicit user confirmation in the preflight prompt. let rc = unsafe { libc::kill(pid, libc::SIGTERM) }; @@ -411,4 +433,26 @@ mod tests { assert_eq!(owner.pid, Some(123)); assert_eq!(owner.command, "next-server"); } + + /// Issue #673 対処の方向 4: pass 0 (the caller's own group), 1 (init), and + /// the caller's own pid straight to the function that calls `kill`, so a + /// mutated tree reaches the last, non-mutatable `assert!`. On an unmutated + /// tree the `bail!` paths return `Err` before `libc::kill`, so the message + /// is `refusing …`, never `failed to kill owning process`. Under a mutation + /// it runs only in the isolated container; on an unmutated tree it also runs + /// in host CI and verify. + #[cfg(unix)] + #[test] + fn dangerous_value_system_port_control_kill_refuses_init_zero_and_own_pid() { + let ports = SystemPortControl; + for pid in [0, 1, std::process::id()] { + let error = ports.kill(pid).expect_err("must refuse"); + let message = error.to_string(); + assert!(message.contains("refusing"), "{message}"); + assert!( + !message.contains("failed to kill owning process"), + "the refusal must not reach libc::kill: {message}" + ); + } + } } diff --git a/src/tools/dir_fd.rs b/src/tools/dir_fd.rs index 0fca4d3f..2940ebb0 100644 --- a/src/tools/dir_fd.rs +++ b/src/tools/dir_fd.rs @@ -13,7 +13,7 @@ //! no component ever follows a symlink, a parent directory that is a symlink is //! refused even when it points back inside the workspace. -use std::path::Path; +use std::path::{Path, PathBuf}; #[cfg(unix)] mod unix_impl { @@ -282,6 +282,31 @@ pub(crate) use unix_impl::{ParentDir, open_parent}; #[cfg(unix)] pub use unix_impl::open_read_file; +/// Issue #673: the real path of `target`, returned only when it is a strict +/// descendant of `owned_root`. +/// +/// `canonicalize` resolves `.`, `..`, and every symlink, so the root itself, an +/// ancestor, a sibling, a path outside the root, and a symlink (even one that +/// points back inside) all return `None`. `starts_with` alone is not enough: +/// it accepts the root and is fooled by `..` and ancestor symlinks. The check +/// takes an `owned_root` that is derived independently of `target`, so a +/// mutation that moves `target` cannot also move the reference it is compared +/// against. A target that no longer exists returns `None`. +/// +/// Callers must pass the returned path to the delete, so the value that was +/// checked is the value that is removed. `None` means "do not delete" (missing +/// or refused); callers that treat a missing target as "nothing to do" check +/// for it first or treat `None` as a no-op. +pub fn strict_descendant_real_path(owned_root: &Path, target: &Path) -> Option { + let root = owned_root.canonicalize().ok()?; + let metadata = target.symlink_metadata().ok()?; + if metadata.file_type().is_symlink() { + return None; + } + let target = target.canonicalize().ok()?; + (target != root && target.starts_with(&root)).then_some(target) +} + #[cfg(not(unix))] pub fn open_read_file(root: &Path, path: &Path) -> anyhow::Result { use anyhow::{Context, bail}; @@ -379,6 +404,7 @@ mod tests { use super::open_parent; use super::open_read_file; + use super::strict_descendant_real_path; use super::test_hook::install; const PAYLOAD: &str = "PAYLOAD_505"; @@ -596,4 +622,78 @@ mod tests { let err = open_read_file(&root, Path::new("pipe.md")).unwrap_err(); assert!(err.to_string().contains("not a regular file"), "{err}"); } + + // Issue #673: the recursive-delete guard. Every case uses a synthetic + // tempdir with a fake `root` and a sibling `outside` (canary), never a real + // host path, and none of these calls deletes anything. The guard returns the + // real path to delete, so the asserted value is what a caller would remove. + + fn guard_fixture() -> (tempfile::TempDir, PathBuf, PathBuf) { + let hold = tempfile::tempdir().unwrap(); + let root = hold.path().join("root"); + let outside = hold.path().join("outside"); + fs::create_dir_all(root.join("child")).unwrap(); + fs::create_dir_all(&outside).unwrap(); + fs::write(outside.join("canary.txt"), "OUTSIDE_CANARY_673").unwrap(); + (hold, root, outside) + } + + #[test] + fn strict_descendant_real_path_accepts_a_real_child() { + let (_hold, root, _outside) = guard_fixture(); + assert_eq!( + strict_descendant_real_path(&root, &root.join("child")), + Some(root.join("child").canonicalize().unwrap()) + ); + } + + #[test] + fn strict_descendant_real_path_rejects_root_itself() { + let (_hold, root, _outside) = guard_fixture(); + assert_eq!(strict_descendant_real_path(&root, &root), None); + } + + #[test] + fn strict_descendant_real_path_rejects_an_ancestor_and_a_sibling() { + let (hold, root, outside) = guard_fixture(); + assert_eq!(strict_descendant_real_path(&root, hold.path()), None); + assert_eq!(strict_descendant_real_path(&root, &outside), None); + } + + #[test] + fn strict_descendant_real_path_rejects_traversal_outside() { + let (_hold, root, outside) = guard_fixture(); + let traversal = root.join("child").join("..").join("..").join("outside"); + assert_eq!(strict_descendant_real_path(&root, &traversal), None); + assert_eq!( + strict_descendant_real_path(&root, &outside.join("canary.txt")), + None + ); + } + + #[test] + fn strict_descendant_real_path_rejects_a_symlink_even_when_it_points_inside() { + let (_hold, root, _outside) = guard_fixture(); + std::os::unix::fs::symlink(root.join("child"), root.join("link")).unwrap(); + assert_eq!(strict_descendant_real_path(&root, &root.join("link")), None); + } + + #[test] + fn strict_descendant_real_path_rejects_an_outside_symlink() { + let (_hold, root, outside) = guard_fixture(); + std::os::unix::fs::symlink(&outside, root.join("escape")).unwrap(); + assert_eq!( + strict_descendant_real_path(&root, &root.join("escape")), + None + ); + } + + #[test] + fn strict_descendant_real_path_rejects_a_missing_target() { + let (_hold, root, _outside) = guard_fixture(); + assert_eq!( + strict_descendant_real_path(&root, &root.join("missing")), + None + ); + } } diff --git a/src/tools/os_sandbox.rs b/src/tools/os_sandbox.rs index 9bc7699a..be63abc5 100644 --- a/src/tools/os_sandbox.rs +++ b/src/tools/os_sandbox.rs @@ -239,6 +239,11 @@ struct Installed { policy: Option, mode: SandboxMode, availability: SandboxAvailability, + /// The directory that owns the run temp dir (`/sandbox`), stored + /// independently of `policy.run_tmp_dir` so the recursive cleanup can + /// re-check the real path against a reference a mutation of the run temp + /// dir cannot move (Issue #673). + run_tmp_root: Option, } static INSTALLED: Mutex> = Mutex::new(None); @@ -438,13 +443,12 @@ fn install_impl( offline: bool, dev_server_ports: Vec, ) -> SandboxStatus { - let run_tmp_dir = state_dir - .join("sandbox") - .join(format!("run-{}", uuid::Uuid::now_v7())); + let run_tmp_root = state_dir.join("sandbox"); + let run_tmp_dir = run_tmp_root.join(format!("run-{}", uuid::Uuid::now_v7())); if let Err(err) = std::fs::create_dir_all(&run_tmp_dir) { let availability = SandboxAvailability::Unavailable(format!("run temp directory is unavailable: {err}")); - return store(mode, None, availability); + return store(mode, None, availability, None); } let policy = SandboxPolicy::new(workspace_root, &run_tmp_dir) .with_offline(offline) @@ -454,7 +458,7 @@ fn install_impl( } else { SandboxAvailability::Unavailable(DISABLED_REASON.to_string()) }; - store(mode, Some(policy), availability) + store(mode, Some(policy), availability, Some(run_tmp_root)) } /// Install an explicit policy and availability. Used by tests to simulate an @@ -464,7 +468,8 @@ pub fn install_for_test( policy: SandboxPolicy, availability: SandboxAvailability, ) -> SandboxStatus { - store(mode, Some(policy), availability) + let run_tmp_root = policy.run_tmp_dir.parent().map(Path::to_path_buf); + store(mode, Some(policy), availability, run_tmp_root) } /// The reason recorded when a workflow child node runs in a workspace other than @@ -502,7 +507,7 @@ pub fn reconcile_workspace(workspace_root: &Path) -> Option { #[cfg(not(test))] fn reconcile_workspace_impl(workspace_root: &Path) -> Option { let mut installed = INSTALLED.lock().expect("sandbox state"); - let (mode, availability, policy, mismatched) = { + let (mode, availability, policy, run_tmp_root, mismatched) = { let state = installed.as_ref()?; let mismatched = state .policy @@ -512,6 +517,7 @@ fn reconcile_workspace_impl(workspace_root: &Path) -> Option { state.mode, state.availability.clone(), state.policy.clone(), + state.run_tmp_root.clone(), mismatched, ) }; @@ -541,6 +547,7 @@ fn reconcile_workspace_impl(workspace_root: &Path) -> Option { policy, mode, availability, + run_tmp_root, }); Some(status) } @@ -549,6 +556,7 @@ fn store( mode: SandboxMode, policy: Option, availability: SandboxAvailability, + run_tmp_root: Option, ) -> SandboxStatus { let read = read_isolation(policy.as_ref()); let offline = policy.as_ref().is_some_and(|policy| policy.offline); @@ -563,6 +571,7 @@ fn store( policy, mode, availability, + run_tmp_root, }); status } @@ -607,9 +616,20 @@ pub fn cleanup() { state.availability = SandboxAvailability::Unavailable(RUN_FINISHED_REASON.to_string()); } let run_tmp_dir = state.policy.take().map(|policy| policy.run_tmp_dir); + let run_tmp_root = state.run_tmp_root.take(); drop(installed); if let Some(run_tmp_dir) = run_tmp_dir { - let _ = std::fs::remove_dir_all(run_tmp_dir); + // Issue #673: only remove the run temp dir when it is a strict + // descendant of the `/sandbox` root stored at install time, so a + // mutation that moves `run_tmp_dir` cannot recursively delete an + // unrelated directory. The checked real path is the value that is + // removed. + if let Some(root) = run_tmp_root.as_deref() + && let Some(checked) = + crate::tools::dir_fd::strict_descendant_real_path(root, &run_tmp_dir) + { + let _ = std::fs::remove_dir_all(checked); + } } } @@ -925,6 +945,7 @@ mod tests { policy: None, mode: SandboxMode::Required, availability: SandboxAvailability::Available, + run_tmp_root: None, }); let error = wrap_command(Command::new("sh")).expect_err("required must refuse"); @@ -945,6 +966,7 @@ mod tests { policy: None, mode: SandboxMode::Auto, availability: SandboxAvailability::Available, + run_tmp_root: None, }); assert!(wrap_command(Command::new("sh")).is_ok()); diff --git a/tests/issue502_os_sandbox_write.rs b/tests/issue502_os_sandbox_write.rs index 832b6db6..2f95b960 100644 --- a/tests/issue502_os_sandbox_write.rs +++ b/tests/issue502_os_sandbox_write.rs @@ -2085,6 +2085,36 @@ fn applied_terminal_slave_write_is_denied() { assert_terminal_slave_write_is_denied(&fixture); } +/// Issue #673: a `git` command that cannot reach the user's HOME, global Git +/// configuration, system configuration, or template/hook directory. The host +/// pager test uses this instead of the ambient `git`, so a developer's +/// `~/.gitconfig` (hooks, `init.templateDir`, signing, pager) never applies. +fn isolated_git_command(home: &Path) -> Command { + let mut command = Command::new("git"); + command + .env("HOME", home) + .env("GIT_CONFIG_GLOBAL", home.join("gitconfig")) + .env("GIT_CONFIG_SYSTEM", "/dev/null") + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("XDG_CONFIG_HOME", home.join("xdg")) + .env("GIT_TERMINAL_PROMPT", "0") + .env_remove("GIT_DIR") + .env_remove("GIT_WORK_TREE") + .env_remove("GIT_INDEX_FILE") + .env_remove("GIT_OBJECT_DIRECTORY") + .env_remove("GIT_COMMON_DIR") + .env_remove("GIT_TEMPLATE_DIR") + .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES"); + command +} + +/// A secret-free HOME for the host git commands, inside the test tempdir. +fn isolated_git_home(fixture: &Fixture) -> PathBuf { + let home = fixture._dir.path().join("iso-git-home"); + std::fs::create_dir_all(&home).unwrap(); + home +} + /// Issue #667: a command that needs a terminal (a `git` pager) still works. /// `git` runs its pager only when stdout is a tty, so the fixture gives the /// wrapped command the pty slave as stdout; the pager (`cat > pager.out`) then @@ -2094,12 +2124,13 @@ fn applied_terminal_slave_write_is_denied() { #[cfg(any(target_os = "linux", target_os = "macos"))] fn assert_git_pager_runs_inside_sandbox(fixture: &Fixture) { let (_master, slave, _slave_path) = open_pty(); - std::process::Command::new("git") + let iso_home = isolated_git_home(fixture); + isolated_git_command(&iso_home) .args(["init", "-q"]) .current_dir(&fixture.root) .status() .expect("git init"); - std::process::Command::new("git") + isolated_git_command(&iso_home) .args([ "-c", "user.email=probe@example.invalid", @@ -2118,7 +2149,11 @@ fn assert_git_pager_runs_inside_sandbox(fixture: &Fixture) { process .arg("-c") .arg("git -c core.pager='cat > pager.out' --paginate log --oneline") - .current_dir(&fixture.root); + .current_dir(&fixture.root) + // The bounded child drops every `GIT_*`, but passes `HOME`; set it to + // the isolated HOME so the sandboxed `git` cannot read the user's + // global Git configuration either (Issue #673). + .env("HOME", &iso_home); let mut process = os_sandbox::wrap_command(process).expect("available sandbox wraps"); // Standard I/O is set after wrapping: the macOS backend rewrites the // command, so a `Stdio` set beforehand would not survive the transfer. @@ -2140,6 +2175,63 @@ fn assert_git_pager_runs_inside_sandbox(fixture: &Fixture) { ); } +/// Issue #673: the host git work does not reach the user's HOME or Git +/// configuration. It inspects the actual child environment (a bounded child +/// sees the isolated HOME and none of the outer `GIT_*`), and reads back a value +/// the isolated helper wrote, proving the helper uses the isolated config. +#[cfg(unix)] +#[test] +fn host_git_child_environment_is_isolated_from_the_user() { + let fixture = fixture(); + let iso_home = isolated_git_home(&fixture); + + let mut process = Command::new("sh"); + process + .arg("-c") + .arg("printf '%s\\n' \"$HOME\" \"${GIT_CONFIG_GLOBAL-unset}\" \"${GIT_DIR-unset}\"") + .current_dir(&fixture.root) + .env("HOME", &iso_home) + .env( + "GIT_CONFIG_GLOBAL", + "/nonexistent/issue673-canary-gitconfig", + ) + .env("GIT_DIR", "/nonexistent/issue673-canary-git-dir"); + process.stdout(Stdio::piped()).stderr(Stdio::piped()); + let output = bounded_process::run_with_timeout(&mut process, TIMEOUT).expect("run child"); + let text = String::from_utf8_lossy(&output.stdout); + let lines: Vec<&str> = text.lines().collect(); + assert_eq!( + lines.first().copied(), + Some(iso_home.to_string_lossy().as_ref()), + "the child must see the isolated HOME: {text:?}" + ); + assert_eq!( + lines.get(1).copied(), + Some("unset"), + "GIT_CONFIG_GLOBAL must be dropped by the bounded child allowlist: {text:?}" + ); + assert_eq!( + lines.get(2).copied(), + Some("unset"), + "GIT_DIR must be dropped by the bounded child allowlist: {text:?}" + ); + + std::fs::write( + iso_home.join("gitconfig"), + "[issue673]\n\tmarker = isolated\n", + ) + .unwrap(); + let out = isolated_git_command(&iso_home) + .args(["config", "--global", "--get", "issue673.marker"]) + .output() + .expect("run git config"); + assert_eq!( + String::from_utf8_lossy(&out.stdout).trim(), + "isolated", + "the isolated helper must read the isolated global config" + ); +} + #[cfg(target_os = "linux")] #[test] fn linux_applied_git_pager_still_runs_inside_the_sandbox() { diff --git a/tests/issue673_dangerous_command_inventory.rs b/tests/issue673_dangerous_command_inventory.rs new file mode 100644 index 00000000..46cb778d --- /dev/null +++ b/tests/issue673_dangerous_command_inventory.rs @@ -0,0 +1,285 @@ +//! Issue #673: a machine-checked inventory of the dangerous commands in this +//! repository, paired with a grep of the same tree. +//! +//! The inventory is detection, not isolation. It fails when a signal call or a +//! recursive delete appears that is not declared here (and therefore not in +//! `docs/dev/dangerous-commands.md`), or when a declared count changes. That +//! is what makes an added `kill` visible in review instead of silent. +//! +//! Limits (see the doc for the full list): this matches direct API names and +//! shell commands in tracked `.rs`, `.sh`, and `.py` files, so it misses +//! aliases, wrappers, code inside `sh -c` / a Python string, and other +//! languages (for example the Node `child.kill()` in `scripts/*.mjs`). It +//! covers in full the categories the Issue requires — signal (Rust +//! `libc::kill`/`libc::killpg`, a `nix` signal wrapper, the external +//! `kill`/`pkill`/`killall`, Python `os.kill(`/`os.killpg(`) and recursive +//! delete (`remove_dir_all`, `rm -rf`/`rm -r `, `shutil.rmtree(`) — and leaves +//! single-file delete, truncate/rename, permission changes, terminal ioctls, +//! and interpreter string bodies out of the automated match (documented). + +use std::collections::BTreeMap; +use std::fs; +use std::path::{Path, PathBuf}; + +/// Tokens that mark a dangerous site in a Rust source file. +const RUST_TOKENS: &[&str] = &[ + "libc::kill(", + "libc::killpg(", + "nix::sys::signal::kill(", + "Command::new(\"kill\")", + "Command::new(\"killall\")", + "Command::new(\"pkill\")", + "remove_dir_all(", +]; + +/// Tokens that mark a dangerous site in a shell script: the external `kill`, +/// `pkill`, `killall`, and the recursive `rm`. +const SHELL_TOKENS: &[&str] = &["kill ", "pkill", "killall", "rm -rf", "rm -r "]; + +/// Tokens that mark a dangerous site in a Python file: `os.kill`, `os.killpg`, +/// and the recursive `shutil.rmtree`. +const PYTHON_TOKENS: &[&str] = &["os.kill(", "os.killpg(", "shutil.rmtree("]; + +fn tokens_for(path: &str) -> &'static [&'static str] { + if path.ends_with(".rs") { + RUST_TOKENS + } else if path.ends_with(".sh") { + SHELL_TOKENS + } else if path.ends_with(".py") { + PYTHON_TOKENS + } else { + &[] + } +} + +/// Every signal / recursive-delete site declared in the inventory, with the +/// number of token occurrences. `(path, token, count)`. +/// +/// This file is excluded from the scan, so its own token literals do not count. +const DECLARED: &[(&str, &str, usize)] = &[ + // Rust: negative process-group targets, one positive pid, probes. + // `src/bounded_process.rs` carries 4 in the wrappers plus 1 in the + // `dangerous_sentinel_` test's `group_exists` probe. + ("src/bounded_process.rs", "libc::kill(", 5), + ("src/bin/gui_server/trial_process.rs", "libc::kill(", 2), + ("src/planner/runner/acceptance.rs", "libc::kill(", 2), + ("src/preflight.rs", "libc::kill(", 1), + ( + "src/planner/runner/tests/support/process.rs", + "libc::kill(", + 1, + ), + ("tests/gui_server.rs", "libc::kill(", 1), + ("tests/headless_summary.rs", "libc::kill(", 1), + // `tests/gui_read_only_guard.rs` holds these as string literals only. + ("tests/gui_read_only_guard.rs", "libc::kill(", 3), + // Rust recursive delete. + ("src/model_probe.rs", "remove_dir_all(", 1), + ("src/bin/gui_server/session_paths.rs", "remove_dir_all(", 1), + ("src/tools/dir_fd.rs", "remove_dir_all(", 2), + ("src/tools/edit.rs", "remove_dir_all(", 1), + ("src/tools/os_sandbox.rs", "remove_dir_all(", 1), + ("src/planner/pack/supply.rs", "remove_dir_all(", 2), + ("src/tools/write.rs", "remove_dir_all(", 1), + ("tests/gui_server.rs", "remove_dir_all(", 4), + ("tests/issue505_write_toctou.rs", "remove_dir_all(", 3), + // Shell recursive delete. `scripts/cmate-pm-watch.sh` also matches the + // `rm -rf` inside its stop-pattern string and its own grep of that string. + ("scripts/build-release.sh", "rm -rf", 3), + ("scripts/install.sh", "rm -rf", 1), + ("scripts/cmate-pm-watch.sh", "rm -rf", 3), + // Python signal and recursive delete. + ("scripts/demo/record_cli_demo.py", "os.kill(", 1), + ("scripts/eval_lib/heavy_lock.py", "os.kill(", 2), + ("scripts/eval_lib/heavy_lock.py", "shutil.rmtree(", 2), + ( + "scripts/eval_lib/goal_verify_recovery_deterministic_pair.py", + "os.killpg(", + 2, + ), + ( + "scripts/eval_lib/goal_verify_recovery_exposure_corpus_pilot.py", + "os.killpg(", + 2, + ), + ( + "scripts/eval_lib/goal_verify_workspaces_v3.py", + "shutil.rmtree(", + 1, + ), + ( + "scripts/eval_lib/goal_verify_live_v4.py", + "shutil.rmtree(", + 2, + ), + ("scripts/nextjs_domain_oracle.py", "shutil.rmtree(", 1), + ("tests/eval/test_heavy_lock.py", "os.kill(", 1), +]; + +/// Report every mismatch between the declared inventory and `corpus`: +/// undeclared sites, changed counts, and declared sites that disappeared. +fn inventory_violations(corpus: &[(String, String)]) -> Vec { + let mut found: BTreeMap<(String, String), usize> = BTreeMap::new(); + for (path, text) in corpus { + for token in tokens_for(path) { + let count = text.matches(token).count(); + if count > 0 { + found.insert((path.clone(), token.to_string()), count); + } + } + } + + let mut violations = Vec::new(); + for ((path, token), count) in &found { + match DECLARED.iter().find(|(declared_path, declared_token, _)| { + declared_path == path && declared_token == token + }) { + None => violations.push(format!("{path}: undeclared `{token}` site ({count})")), + Some((_, _, declared_count)) if declared_count != count => violations.push(format!( + "{path}: `{token}` count {count} does not match declared {declared_count}" + )), + Some(_) => {} + } + } + for (path, token, declared_count) in DECLARED { + if found.get(&(path.to_string(), token.to_string())) != Some(declared_count) { + violations.push(format!( + "{path}: declared `{token}` ({declared_count}) is no longer present" + )); + } + } + violations +} + +#[test] +fn dangerous_command_inventory_matches_the_tree() { + let corpus = scan_tree(); + let violations = inventory_violations(&corpus); + assert!( + violations.is_empty(), + "dangerous-command inventory is out of date:\n{}", + violations.join("\n") + ); +} + +/// The two-point measurement for the detector itself: an unlisted `kill` is a +/// violation. The synthetic site is never compiled or executed — it is a +/// string handed to the audit. +#[test] +fn dangerous_command_inventory_rejects_an_unlisted_signal() { + let corpus = vec![( + "src/new_signal.rs".to_string(), + "fn f(pid: i32) { unsafe { libc::kill(-pid, libc::SIGTERM); } }".to_string(), + )]; + let violations = inventory_violations(&corpus); + assert!( + violations + .iter() + .any(|violation| violation.contains("src/new_signal.rs") + && violation.contains("libc::kill(")), + "{violations:?}" + ); +} + +/// The two-point measurement for the recursive-delete detector. +#[test] +fn dangerous_command_inventory_rejects_an_unlisted_recursive_delete() { + let corpus = vec![( + "src/new_delete.rs".to_string(), + "fn f(p: &std::path::Path) { let _ = std::fs::remove_dir_all(p); }".to_string(), + )]; + let violations = inventory_violations(&corpus); + assert!( + violations + .iter() + .any(|violation| violation.contains("src/new_delete.rs") + && violation.contains("remove_dir_all(")), + "{violations:?}" + ); +} + +/// A change to a declared file (here a second signal in a declared file) is +/// caught by the count, not only by the file list. +#[test] +fn dangerous_command_inventory_rejects_an_extra_signal_in_a_declared_file() { + let corpus = vec![( + "src/preflight.rs".to_string(), + "fn a() { unsafe { libc::kill(1, 1); } } fn b() { unsafe { libc::kill(2, 2); } }" + .to_string(), + )]; + let violations = inventory_violations(&corpus); + assert!( + violations + .iter() + .any(|violation| violation.contains("src/preflight.rs") + && violation.contains("does not match declared")), + "{violations:?}" + ); +} + +/// The script surface is measured too: an unlisted external `kill` in a shell +/// script is a violation. +#[test] +fn dangerous_command_inventory_rejects_an_unlisted_shell_kill() { + let corpus = vec![( + "scripts/new_run.sh".to_string(), + "kill -TERM \"$pid\"\n".to_string(), + )]; + let violations = inventory_violations(&corpus); + assert!( + violations + .iter() + .any(|violation| violation.contains("scripts/new_run.sh")), + "{violations:?}" + ); +} + +fn scan_tree() -> Vec<(String, String)> { + let root = Path::new(env!("CARGO_MANIFEST_DIR")); + let mut corpus = Vec::new(); + for (dir, extensions) in [ + ("src", ["rs"].as_slice()), + ("tests", ["rs", "py"].as_slice()), + ("scripts", ["sh", "py"].as_slice()), + ] { + for path in files_with_extensions(&root.join(dir), extensions) { + push_relative(root, &path, &mut corpus); + } + } + push_relative(root, &root.join("build.rs"), &mut corpus); + // This file declares the tokens as string literals; it is not a site. + corpus.retain(|(path, _)| path != "tests/issue673_dangerous_command_inventory.rs"); + corpus +} + +fn push_relative(root: &Path, path: &Path, corpus: &mut Vec<(String, String)>) { + let relative = path + .strip_prefix(root) + .expect("relative source path") + .to_string_lossy() + .replace('\\', "/"); + let text = fs::read_to_string(path) + .unwrap_or_else(|error| panic!("failed to read {}: {error}", path.display())); + corpus.push((relative, text)); +} + +fn files_with_extensions(dir: &Path, extensions: &[&str]) -> Vec { + let mut out = Vec::new(); + collect_files(dir, extensions, &mut out); + out +} + +fn collect_files(dir: &Path, extensions: &[&str], out: &mut Vec) { + for entry in fs::read_dir(dir).unwrap_or_else(|error| panic!("failed to read {dir:?}: {error}")) + { + let entry = entry.expect("directory entry"); + let path = entry.path(); + if path.is_dir() { + collect_files(&path, extensions, out); + } else if let Some(extension) = path.extension().and_then(|ext| ext.to_str()) + && extensions.contains(&extension) + { + out.push(path); + } + } +}