diff --git a/src/tools/bash/read_guard.rs b/src/tools/bash/read_guard.rs index 672a2f14..4e3d07b6 100644 --- a/src/tools/bash/read_guard.rs +++ b/src/tools/bash/read_guard.rs @@ -132,7 +132,12 @@ fn inspect_text(text: &str, cr_mode: CrMode) -> ReadVerdict { .push(ReadCandidate::Expand(word.text.clone())); continue; } - push_static(&mut verdict.candidates, word, index >= leading); + push_static( + &mut verdict.candidates, + word, + index >= leading, + index >= leading && !echo, + ); collect_relative_targets(&mut verdict.relative_words, word, index >= leading); } } @@ -240,7 +245,12 @@ fn join_base(base: &str, path: &str) -> String { } } -fn push_static(candidates: &mut Vec, word: &Word, equals_rhs_is_candidate: bool) { +fn push_static( + candidates: &mut Vec, + word: &Word, + equals_rhs_is_candidate: bool, + parent_home_is_candidate: bool, +) { let mut produced = Vec::new(); if word.text.starts_with('/') { // A quoted operator or bracket is part of the actual filename. Inspect @@ -264,6 +274,18 @@ fn push_static(candidates: &mut Vec, word: &Word, equals_rhs_is_c if equals_rhs_is_candidate && let Some(value) = equals_right_hand_side(&word.text) { produced.push(ReadCandidate::Literal(value.to_string())); } + // Issue #628: a bare `..` or a `~`-prefixed `/`-less word names the + // parent or home directory, which the existing literal read proof + // rejects. It carries no `/`, so it was never a literal-path candidate + // before. The right-hand side of a non-assignment `NAME=value` + // (`make PREFIX=~`, `cat --file=..`) is the same. A leading assignment + // and an `echo` display segment are excluded by the caller. + if parent_home_is_candidate && is_parent_or_home_literal(&word.text) { + produced.push(ReadCandidate::Literal(word.text.clone())); + } + if parent_home_is_candidate && let Some(value) = parent_home_right_hand_side(&word.text) { + produced.push(ReadCandidate::Literal(value.to_string())); + } produced.extend( super::absolute_path_candidates(&word.text) .map(|path| ReadCandidate::Expand(path.to_owned())), @@ -273,6 +295,21 @@ fn push_static(candidates: &mut Vec, word: &Word, equals_rhs_is_c candidates.extend(produced); } +/// Whether a static word names the parent directory (`..` exactly) or a +/// `~`-prefixed home spelling without a directory separator (Issue #628). A +/// word with a `/` keeps its existing literal-path handling. +fn is_parent_or_home_literal(word: &str) -> bool { + word == ".." || (word.starts_with('~') && !word.contains('/')) +} + +/// The right-hand side of a `NAME=value` word when it is a parent/home literal +/// (`make PREFIX=~`, `cat --file=..`); see [`is_parent_or_home_literal`] +/// (Issue #628). +fn parent_home_right_hand_side(word: &str) -> Option<&str> { + let (_, value) = word.split_once('=')?; + is_parent_or_home_literal(value).then_some(value) +} + /// The right-hand side of a `NAME=value` word when it is a static path spelling. /// An empty or absolute right side yields `None`: the embedded absolute-path scan /// already inspects an absolute one (Issue #603 design 3). diff --git a/src/tools/bash_write_guard.rs b/src/tools/bash_write_guard.rs index 33ed451a..a338e1c4 100644 --- a/src/tools/bash_write_guard.rs +++ b/src/tools/bash_write_guard.rs @@ -135,7 +135,10 @@ pub(super) fn confinement_rejection( }); } if target.path == "/dev/null" - && matches!(target.operation.as_str(), "output redirection" | "tee") + && matches!( + target.operation.as_str(), + "output redirection" | "tee" | "dd" + ) { continue; } @@ -504,6 +507,21 @@ fn collect_program_targets(program: &str, arguments: &[&str], targets: &mut Vec< operation: program.to_string(), })); } + // Issue #628: `dd` names its output file with `of=`, which the write + // table did not recognize. Every non-empty `of=` argument is a write + // destination, exactly like a redirection or `tee` operand. + "dd" => { + targets.extend( + arguments + .iter() + .filter_map(|argument| argument.strip_prefix("of=")) + .filter(|value| !value.is_empty()) + .map(|value| WriteTarget { + path: value.to_string(), + operation: program.to_string(), + }), + ); + } "chmod" | "chown" => { targets.extend(operands.into_iter().skip(1).map(|path| WriteTarget { path: path.to_string(), diff --git a/tests/corpus/apps/issue582-bash-read-candidates/fixtures/decision-cases.jsonl b/tests/corpus/apps/issue582-bash-read-candidates/fixtures/decision-cases.jsonl index 6b124f68..43c19793 100644 --- a/tests/corpus/apps/issue582-bash-read-candidates/fixtures/decision-cases.jsonl +++ b/tests/corpus/apps/issue582-bash-read-candidates/fixtures/decision-cases.jsonl @@ -39,7 +39,7 @@ {"id": "equals-parent-escape", "command": "dd if=../outside/secret", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} {"id": "allow-quoted-space-inside", "command": "cat \"docs/my notes.md\"", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} {"id": "allow-escaped-space-inside", "command": "cat docs/my\\ notes.md", "R": false, "A": false, "W": false, "P": false, "S": false, "V": false} -{"id": "allow-dd-nonpath", "command": "dd if=in.bin of=out.bin", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} +{"id": "allow-dd-nonpath", "command": "dd if=in.bin of=out.bin", "R": false, "A": true, "W": false, "P": false, "S": false, "V": false} {"id": "allow-grep-file-nonpath", "command": "grep --file=patterns.txt src", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} {"id": "allow-features", "command": "cargo test --features=gui", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} {"id": "allow-target-dir", "command": "cargo build --target-dir=target/h05", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} @@ -101,3 +101,29 @@ {"id": "allow-crlf-git", "command": "git status\r\ngit diff\r\n", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} {"id": "allow-crlf-cd-cat", "command": "cd sub\r\ncat f\r\n", "R": false, "A": false, "W": false, "P": false, "S": false, "V": false} {"id": "allow-crlf-echo-out", "command": "echo x > out.txt\r\n", "R": false, "A": true, "W": false, "P": false, "S": false, "V": false} +{"id": "parent-dir-read", "command": "ls ..", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "parent-dir-option-read", "command": "ls -la ..", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "parent-dir-quoted-read", "command": "ls '..'", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "parent-dir-cat-read", "command": "cat ..", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "home-read", "command": "ls ~", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "home-user-read", "command": "ls ~root", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "home-plus-read", "command": "ls ~+", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "home-minus-read", "command": "ls ~-", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "equals-home-arg", "command": "make PREFIX=~", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "equals-parent-arg", "command": "cat --file=..", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "option-c-parent-tar", "command": "tar -C .. -cf - x", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "option-c-parent-git", "command": "git -C .. status", "R": true, "A": false, "W": false, "P": false, "S": false, "V": true, "operation": "path reference"} +{"id": "dd-out-symlink", "command": "dd if=in of=lf", "R": true, "A": true, "W": true, "P": false, "S": false, "V": false, "operation": "dd"} +{"id": "dd-out-parent", "command": "dd if=in of=../outside/x", "R": true, "A": true, "W": true, "P": false, "S": false, "V": false, "operation": "dd"} +{"id": "dd-out-home", "command": "dd if=in of=~/f", "R": true, "A": true, "W": true, "P": false, "S": false, "V": false, "operation": "dd"} +{"id": "dd-out-devnull", "command": "dd if=in of=/dev/null", "R": false, "A": true, "W": false, "P": false, "S": false, "V": false} +{"id": "dd-protected-path", "command": "dd if=in of=tests/spec.rs", "R": false, "A": true, "W": false, "P": true, "S": false, "V": false} +{"id": "dd-no-output", "command": "dd if=in", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} +{"id": "dd-multi-output", "command": "dd if=in of=a of=b", "R": false, "A": true, "W": false, "P": false, "S": false, "V": false} +{"id": "dd-redirect-write", "command": "dd if=in > out", "R": false, "A": true, "W": false, "P": false, "S": false, "V": false} +{"id": "echo-home-unchanged", "command": "echo ~", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} +{"id": "echo-parent-unchanged", "command": "echo ..", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} +{"id": "assignment-home-unchanged", "command": "X=~ cargo test", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} +{"id": "git-range-unchanged", "command": "git diff main..feature", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} +{"id": "git-tilde-unchanged", "command": "git log HEAD~1", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} +{"id": "ls-dot-unchanged", "command": "ls .", "R": false, "A": false, "W": false, "P": false, "S": false, "V": true} diff --git a/tests/issue628_bash_parent_home_dd.rs b/tests/issue628_bash_parent_home_dd.rs new file mode 100644 index 00000000..0df76eb0 --- /dev/null +++ b/tests/issue628_bash_parent_home_dd.rs @@ -0,0 +1,266 @@ +#![cfg(unix)] + +//! Issue #628: the second-stage Bash confinement missed two shapes found while +//! scoping #613. +//! +//! 1. A `..`-only word and a `~`-prefixed `/`-less word were never literal read +//! candidates: `is_literal_path` requires a `/`, the `/`-less symlink check +//! deliberately excludes `..` and `~`, and so `ls ..` and `ls ~` read the +//! workspace parent or the home directory with `R=0`. The `=` right-hand side +//! (`make PREFIX=~`, `cat --file=..`) and the value of an option that changes +//! the directory (`tar -C ..`, `git -C ..`) had the same gap. +//! 2. `dd`'s `of=` was not a write target, so `dd if=in of=out` was not a +//! recognized mutation (`A=0`), was auto-approved by `bash:verify` (`V=1`), +//! and no protected-path write was detected (`P=0`). +//! +//! This file drives the public second-stage predicate `R` +//! (`path_confinement_rejection`) and the verify auto-approval `V`. The +//! crate-private `A`/`W`/`P` (`has_recognized_mutation`, `confinement_rejection`, +//! `protected_path_mutation`) are pinned by the unit tests in +//! `src/tools/bash/read_guard.rs` and `src/tools/bash_write_guard.rs`, and by the +//! corpus table `tests/corpus/apps/issue582-bash-read-candidates/fixtures/decision-cases.jsonl`. +//! +//! ## False-rejection estimate +//! +//! The 436 distinct agent Bash commands recorded under `dev-reports/` and +//! `workspace/management/` contain no `..`-only word, no `~`-only word, no `dd`, +//! and no `-C` option, so this change moves 0 of them. (Reproduced from the +//! pre-work investigation recorded for Issue #628.) +//! +//! ## Newly refused forms, and how to rewrite them +//! +//! * `ls ..` — list the workspace root instead (`ls`). +//! * `cd sub && ls ..` — the parent component is refused regardless of the +//! working directory; list a workspace-relative path instead. +//! * `ls "~"` (a file named `~`) — the lexical reading cannot tell the quotes +//! apart, so prefix the name: `ls ./~`. +//! * `make PREFIX=~` — the value points outside the workspace; pass a +//! workspace-relative value instead. +//! * `dd if=in of=out` — the write is now a recognized mutation, so it is no +//! longer auto-approved by `bash:verify`; run it through the normal write +//! authorization. + +use std::path::PathBuf; + +use commandagent::mode::ExecutionMode; +use commandagent::tools::allow_policy::bash_verify_command_is_auto_approvable; +use commandagent::tools::bash::path_confinement_rejection; +use commandagent::tools::registry::tool_error_kind; +use commandagent::tools::registry::{ToolContext, ToolRegistry}; +use commandagent::tools::workspace_policy::WorkspacePolicy; +use serde_json::json; + +/// A fake canary written into the outside file; a refused command never reads +/// it, so it must not appear in any error, event, or report output. +const CANARY: &str = "OUTER628_FAKE_CANARY_4d1a"; + +struct Fixture { + _dir: tempfile::TempDir, + root: PathBuf, + outside: PathBuf, +} + +/// A tempdir outside the prefix `is_system_prefix_allowed` accepts (`/usr`, +/// `/bin`, `/opt`, `/etc`, `/tmp`). `tempfile::tempdir()` lives under `/tmp` on +/// Linux, which would let a fixture path outside the workspace be admitted as a +/// system path and fail only on Linux; `CARGO_TARGET_TMPDIR` is under the target +/// directory instead. +fn tempdir() -> tempfile::TempDir { + let dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).expect("create tempdir"); + assert!( + !["/usr", "/bin", "/opt", "/etc", "/tmp"] + .iter() + .any(|prefix| dir.path().starts_with(prefix)), + "the fixture tempdir must not sit under a system prefix: {}", + dir.path().display() + ); + dir +} + +/// A workspace with a regular file, an inside directory, and three outward +/// symlinks (`lf`, `sub/link`, `linked-outside`) an absolute or relative `of=` +/// can name. +fn fixture() -> Fixture { + let dir = tempdir(); + let root = dir.path().join("ws"); + std::fs::create_dir_all(root.join("sub")).unwrap(); + std::fs::write(root.join("a.txt"), "x").unwrap(); + let outside = dir.path().join("outside"); + std::fs::create_dir_all(&outside).unwrap(); + std::fs::write(outside.join("secret"), CANARY).unwrap(); + std::os::unix::fs::symlink(&outside, root.join("lf")).unwrap(); + std::os::unix::fs::symlink(&outside, root.join("sub/link")).unwrap(); + std::os::unix::fs::symlink(&outside, root.join("linked-outside")).unwrap(); + let root = root.canonicalize().unwrap(); + Fixture { + _dir: dir, + root, + outside, + } +} + +/// Issue #628 problem 1: a `..`-only word, a `~`-prefixed `/`-less word, the `=` +/// right-hand side, and an option value that names the parent are all refused, +/// and the read refusal is the fixed `path reference` operation that carries no +/// file contents. Every one of them keeps `V=1`, so the second-stage refusal +/// rejects them before the shell starts. +#[test] +fn issue628_parent_and_home_words_are_refused() { + let fixture = fixture(); + let root = &fixture.root; + for command in [ + "ls ..", + "ls -la ..", + "du -sh ..", + "find .. -name x", + "grep -r foo ..", + "cat ..", + "ls '..'", + "ls ~", + "cat ~", + "ls ~root", + "ls ~+", + "ls ~-", + "make PREFIX=~", + "cat --file=..", + "tar -C .. -cf - x", + "git -C .. status", + ] { + let rejection = path_confinement_rejection(command, root) + .unwrap_or_else(|| panic!("expected rejection: {command:?}")); + assert_eq!(rejection.operation, "path reference", "{command:?}"); + assert!( + !rejection.reason.contains(CANARY) && !rejection.message.contains(CANARY), + "the canary leaked into the reason: {command:?}" + ); + assert!( + bash_verify_command_is_auto_approvable(command, root), + "the verify allow-list stays unchanged for {command:?}" + ); + } + // A `cd` before the parent word does not change the decision: the parent + // component is refused regardless of the working directory. + assert!(path_confinement_rejection("cd sub && ls ..", root).is_some()); + // A `/`-containing spelling already reached the literal proof before this + // change (`../x`, `~/x`, `sub/..`); its value is unchanged. + for command in ["ls ../", "cat ../x", "ls ~/", "cat ~/x", "ls sub/.."] { + assert!( + path_confinement_rejection(command, root).is_some(), + "expected rejection: {command:?}" + ); + } +} + +/// Issue #628 problem 2: `dd`'s `of=` is a write target. An inside write stays +/// allowed (`R=0`) but is a recognized mutation, so it loses `V`. An outward +/// `of=` is refused with the `dd` operation, and an absolute outward `of=` is +/// refused too. +#[test] +fn issue628_dd_of_is_a_write_and_loses_verify_auto_approval() { + let fixture = fixture(); + let root = &fixture.root; + for command in [ + "dd if=in.bin of=out.bin", + "dd if=in of=/dev/null", + "dd if=in", + ] { + assert!( + path_confinement_rejection(command, root).is_none(), + "R must stay 0: {command:?}" + ); + } + for command in [ + "dd if=in.bin of=out.bin".to_string(), + "dd if=in of=/dev/null".to_string(), + "dd if=in of=../outside/x".to_string(), + "dd if=in of=~/f".to_string(), + format!("dd if=in of={}", fixture.outside.join("out").display()), + ] { + assert!( + !bash_verify_command_is_auto_approvable(&command, root), + "V must be 0: {command:?}" + ); + } + for command in [ + "dd if=in of=lf".to_string(), + "dd if=in of=../outside/x".to_string(), + "dd if=in of=~/f".to_string(), + format!("dd if=in of={}", fixture.outside.join("out").display()), + ] { + let rejection = path_confinement_rejection(&command, root) + .unwrap_or_else(|| panic!("expected rejection: {command:?}")); + assert_eq!(rejection.operation, "dd", "{command:?}"); + } + // `dd if=in` names no write, so it keeps its existing auto-approval. + assert!(bash_verify_command_is_auto_approvable("dd if=in", root)); +} + +/// Acceptance criterion: workspace-inside reads and writes, the `echo` display +/// forms, a leading assignment, git ranges, and a `~` that is not a whole word +/// keep their value. `cd ..` keeps its working-directory refusal. +#[test] +fn issue628_inside_and_display_forms_are_unchanged() { + let fixture = fixture(); + let root = &fixture.root; + for command in [ + "ls", + "ls src", + "ls -la", + "ls .", + "cat a.txt", + "echo ~", + "echo ..", + "git diff main..feature", + "git log HEAD~1", + "X=~ cargo test", + "dd if=in of=out.bin", + "dd if=in of=a of=b", + "dd if=in of=", + ] { + assert!( + path_confinement_rejection(command, root).is_none(), + "expected allow: {command:?}" + ); + } + // `cd ..` is refused by the working-directory write side, unchanged. + let rejection = path_confinement_rejection("cd ..", root).expect("cd .. is refused"); + assert_eq!(rejection.operation, "working directory"); + assert!(path_confinement_rejection("cd ~", root).is_some()); +} + +/// A rejected parent/home read and a rejected `dd` write stop before the shell +/// starts: the registry returns `bash_path_confinement_error` and the fake +/// canary never reaches stdout, stderr, or the event log. +#[test] +fn issue628_refusals_stop_before_the_shell_and_leak_no_canary() { + let fixture = fixture(); + let events = fixture._dir.path().join("events.jsonl"); + let context = ToolContext { + root: fixture.root.clone(), + mode: ExecutionMode::Act, + auto_approve: true, + interactive_approval: false, + offline: true, + workspace_policy: WorkspacePolicy::NormalTask, + eval_events_path: Some(events.clone()), + expected_paths: Vec::new(), + protected_paths: Vec::new(), + }; + let registry = ToolRegistry::default(); + for command in ["ls ..", "cat ~", "dd if=in of=lf"] { + let error = registry + .execute("Bash", &json!({ "command": command }), &context) + .unwrap_err(); + assert_eq!( + tool_error_kind(&error), + "bash_path_confinement_error", + "{command}" + ); + assert!(!error.to_string().contains(CANARY), "{command}"); + } + let log = std::fs::read_to_string(&events).unwrap_or_default(); + assert!( + !log.contains(CANARY), + "the canary must not appear in the event log" + ); +}