diff --git a/src/tools/file_edit.rs b/src/tools/file_edit.rs index caed4a2..9d530bf 100644 --- a/src/tools/file_edit.rs +++ b/src/tools/file_edit.rs @@ -66,7 +66,7 @@ impl Tool for FileEditTool { if let Some(err) = super::check_protected_path(&path) { return Ok(err); } - if let Some(err) = super::check_sensitive_path(&path, super::SensitiveOp::Write) { + if let Some(err) = super::check_sensitive_path_resolved(&path, super::SensitiveOp::Write) { return Ok(err); } diff --git a/src/tools/file_read.rs b/src/tools/file_read.rs index 65d1aee..acbcb2d 100644 --- a/src/tools/file_read.rs +++ b/src/tools/file_read.rs @@ -61,7 +61,7 @@ impl Tool for FileReadTool { Err(e) => return Ok(ToolOutput::error(e.to_string())), }; - if let Some(err) = super::check_sensitive_path(&path, super::SensitiveOp::Read) { + if let Some(err) = super::check_sensitive_path_resolved(&path, super::SensitiveOp::Read) { return Ok(err); } diff --git a/src/tools/file_write.rs b/src/tools/file_write.rs index 1cd95ba..d846414 100644 --- a/src/tools/file_write.rs +++ b/src/tools/file_write.rs @@ -53,7 +53,7 @@ impl Tool for FileWriteTool { if let Some(err) = super::check_protected_path(&path) { return Ok(err); } - if let Some(err) = super::check_sensitive_path(&path, super::SensitiveOp::Write) { + if let Some(err) = super::check_sensitive_path_resolved(&path, super::SensitiveOp::Write) { return Ok(err); } diff --git a/src/tools/grep.rs b/src/tools/grep.rs index b1078fc..72c960a 100644 --- a/src/tools/grep.rs +++ b/src/tools/grep.rs @@ -159,6 +159,13 @@ async fn run_with_rg(input: &GrepInput, ctx: &ToolContext) -> Result args.push(format!("!**/{excl}/**")); } + // Same read deny-list the fallback backend and FileRead enforce. rg opens + // files itself, so exclusions have to be declared rather than checked. + for g in super::denied_read_globs() { + args.push("--glob".into()); + args.push(g); + } + args.push("--".into()); args.push(input.pattern.clone()); @@ -253,6 +260,15 @@ async fn run_with_regex(input: &GrepInput, ctx: &ToolContext) -> Result bool { /// Returns Some(error ToolOutput) if the path should be blocked for `op`. /// Read mode blocks private keys only. Write mode additionally blocks /// dotenv files, credential stores, and secrets directories. +/// Deny-listed read paths expressed as ripgrep exclusion globs. +/// +/// The Grep tool has two backends — a `ripgrep` subprocess and a pure-Rust +/// fallback — and both must honour the same rules. The fallback can check each +/// file as it opens it; `rg` opens files itself, so it has to be told up front. +pub fn denied_read_globs() -> Vec { + let mut g: Vec = PRIVATE_KEY_NAMES.iter().map(|n| format!("!**/{n}")).collect(); + g.extend(PRIVATE_KEY_SUFFIXES.iter().map(|s| format!("!**/*{s}"))); + g +} + +/// Resolve symlinks so [`check_sensitive_path`] sees the file that will actually +/// be touched, not the name the caller supplied. +/// +/// The deny-list matches on the *file name*, so without this every protection it +/// offers is defeated by a symlink with an innocuous name. Verified: a link +/// named `notes.md` pointing at `~/.ssh/id_rsa` returned the private key, and a +/// link named `config.json` pointing at `~/.aws/credentials` overwrote them — +/// while the same operations on the real names were correctly refused. +/// +/// That needs no unusual privileges: a repository can simply *ship* a symlink +/// called `README.md`, and asking the agent to read it exfiltrates the target. +/// +/// Falls back to the parent directory for paths that do not exist yet (a fresh +/// write), which also catches a symlinked parent, and to the input unchanged +/// when nothing can be resolved — a check on the literal path is never worse +/// than the old behaviour. +pub fn resolve_for_sensitivity_check(path: &std::path::Path) -> std::path::PathBuf { + if let Ok(real) = std::fs::canonicalize(path) { + return real; + } + if let (Some(parent), Some(name)) = (path.parent(), path.file_name()) + && let Ok(real_parent) = std::fs::canonicalize(parent) + { + return real_parent.join(name); + } + path.to_path_buf() +} + +/// [`check_sensitive_path`] applied to both the supplied path and its symlink +/// target. Either looking sensitive is a refusal. +/// +/// Use this at every filesystem entry point; the raw `check_sensitive_path` only +/// inspects the name it is given. +pub fn check_sensitive_path_resolved( + path: &std::path::Path, + op: SensitiveOp, +) -> Option { + if let Some(err) = check_sensitive_path(path, op) { + return Some(err); + } + let resolved = resolve_for_sensitivity_check(path); + if resolved != path { + return check_sensitive_path(&resolved, op); + } + None +} + pub fn check_sensitive_path(path: &std::path::Path, op: SensitiveOp) -> Option { let file_name = path.file_name().and_then(|n| n.to_str()).unwrap_or(""); diff --git a/src/tools/multi_edit.rs b/src/tools/multi_edit.rs index 5a9b01d..1c94a54 100644 --- a/src/tools/multi_edit.rs +++ b/src/tools/multi_edit.rs @@ -117,7 +117,7 @@ impl Tool for MultiEditTool { had_error = true; continue; } - if let Some(err) = super::check_sensitive_path(&path, super::SensitiveOp::Write) { + if let Some(err) = super::check_sensitive_path_resolved(&path, super::SensitiveOp::Write) { let msg = err .content .iter() diff --git a/tests/fs_tool_sensitivity_tests.rs b/tests/fs_tool_sensitivity_tests.rs new file mode 100644 index 0000000..303e05d --- /dev/null +++ b/tests/fs_tool_sensitivity_tests.rs @@ -0,0 +1,177 @@ +//! Phase 2: the filesystem tools must not become a way around the sensitive-path +//! deny-list. +//! +//! Two bypasses were live before these tests existed: +//! +//! 1. `check_sensitive_path` matches on the *file name* of the path it is +//! handed, so a symlink with an innocuous name defeated every protection it +//! offers. A link `notes.md` -> `~/.ssh/id_rsa` returned the private key, +//! and `config.json` -> `~/.aws/credentials` overwrote them, while the same +//! operations on the real names were correctly refused. A repository can +//! simply ship such a link — "read the README" is enough to exfiltrate. +//! +//! 2. Grep never consulted the deny-list at all, so searching for a string +//! inside a private key returned the key material verbatim — a read +//! primitive that walked straight past the guard on FileRead. +//! +//! Unix-only: symlink semantics. + +#![cfg(unix)] + +use rustyclaw::tools::{ + Tool, ToolContext, file_read::FileReadTool, file_write::FileWriteTool, grep::GrepTool, +}; +use serde_json::json; +use std::path::PathBuf; +use tempfile::TempDir; + +fn text(o: &rustyclaw::tools::ToolOutput) -> String { + o.content + .iter() + .map(|c| match c { + rustyclaw::api::types::ToolResultContent::Text { text } => text.as_str(), + }) + .collect::>() + .join("") +} + +/// Secrets outside the project, a project dir, and links pointing out of it. +fn fixture() -> (TempDir, PathBuf) { + let td = TempDir::new().unwrap(); + let ssh = td.path().join(".ssh"); + std::fs::create_dir_all(&ssh).unwrap(); + std::fs::write(ssh.join("id_rsa"), "PRIVATE-KEY-MATERIAL\n").unwrap(); + + let aws = td.path().join(".aws"); + std::fs::create_dir_all(&aws).unwrap(); + std::fs::write(aws.join("credentials"), "[default]\nreal=secret\n").unwrap(); + + let proj = td.path().join("proj"); + std::fs::create_dir_all(&proj).unwrap(); + std::os::unix::fs::symlink(ssh.join("id_rsa"), proj.join("notes.md")).unwrap(); + std::os::unix::fs::symlink(aws.join("credentials"), proj.join("config.json")).unwrap(); + (td, proj) +} + +#[tokio::test] +async fn symlink_cannot_be_used_to_read_a_private_key() { + let (_td, proj) = fixture(); + let ctx = ToolContext::new(proj); + let out = FileReadTool + .execute(json!({"file_path": "notes.md"}), &ctx) + .await + .unwrap(); + assert!(out.is_error, "reading a link to a private key must be refused"); + assert!( + !text(&out).contains("PRIVATE-KEY-MATERIAL"), + "key material must not appear in the result" + ); +} + +#[tokio::test] +async fn symlink_cannot_be_used_to_overwrite_credentials() { + let (td, proj) = fixture(); + let creds = td.path().join(".aws").join("credentials"); + let before = std::fs::read_to_string(&creds).unwrap(); + + let ctx = ToolContext::new(proj); + let out = FileWriteTool + .execute(json!({"file_path": "config.json", "content": "CLOBBERED\n"}), &ctx) + .await + .unwrap(); + + assert!(out.is_error, "writing through a link to credentials must be refused"); + assert_eq!( + std::fs::read_to_string(&creds).unwrap(), + before, + "the target file must be byte-identical" + ); +} + +/// Ordinary files must keep working — a guard that blocks real work gets +/// disabled, which is worse than no guard. +#[tokio::test] +async fn normal_files_are_unaffected() { + let (_td, proj) = fixture(); + let ctx = ToolContext::new(proj.clone()); + + let w = FileWriteTool + .execute(json!({"file_path": "src.rs", "content": "fn main() {}\n"}), &ctx) + .await + .unwrap(); + assert!(!w.is_error, "writing an ordinary file must work: {}", text(&w)); + + let r = FileReadTool + .execute(json!({"file_path": "src.rs"}), &ctx) + .await + .unwrap(); + assert!(!r.is_error && text(&r).contains("fn main"), "{}", text(&r)); + + // A symlink to a harmless file is still fine. + std::os::unix::fs::symlink(proj.join("src.rs"), proj.join("alias.rs")).unwrap(); + let a = FileReadTool + .execute(json!({"file_path": "alias.rs"}), &ctx) + .await + .unwrap(); + assert!(!a.is_error, "benign symlinks must not be blocked: {}", text(&a)); +} + +/// Grep returns matching lines verbatim, so it must honour the same read +/// deny-list as FileRead. Both backends are exercised: the `rg` subprocess when +/// ripgrep is installed, and the pure-Rust fallback when it is not — a fix +/// applied to only one of them leaves the other leaking. +#[tokio::test] +async fn grep_does_not_return_private_key_contents() { + let (_td, proj) = fixture(); + std::fs::write( + proj.join("server.pem"), + "-----BEGIN PRIVATE KEY-----\nPEMSECRET\n", + ) + .unwrap(); + std::fs::write(proj.join("app.rs"), "// PEMSECRET appears here legitimately\n").unwrap(); + + let ctx = ToolContext::new(proj); + let out = GrepTool + .execute(json!({"pattern": "PEMSECRET", "output_mode": "content"}), &ctx) + .await + .unwrap(); + let body = text(&out); + + assert!( + !body.contains("server.pem"), + "key-material file must not be searched: {body}" + ); + assert!( + body.contains("app.rs"), + "ordinary files must still be searched: {body}" + ); +} + +/// The same, forced down the non-ripgrep path. +#[tokio::test] +async fn grep_fallback_backend_also_honours_the_deny_list() { + let (_td, proj) = fixture(); + std::fs::write(proj.join("key.pem"), "FALLBACKSECRET\n").unwrap(); + std::fs::write(proj.join("ok.txt"), "FALLBACKSECRET\n").unwrap(); + + // An empty PATH removes `rg`, so the pure-Rust backend runs. + let ctx = ToolContext::new(proj); + let prev = std::env::var_os("PATH"); + // SAFETY: restored below; this test does not run concurrently with other + // PATH users in this binary. + unsafe { std::env::set_var("PATH", "") }; + let out = GrepTool + .execute(json!({"pattern": "FALLBACKSECRET", "output_mode": "content"}), &ctx) + .await + .unwrap(); + unsafe { + match prev { + Some(v) => std::env::set_var("PATH", v), + None => std::env::remove_var("PATH"), + } + } + + let body = text(&out); + assert!(!body.contains("key.pem"), "fallback leaked key material: {body}"); + assert!(body.contains("ok.txt"), "fallback must still search normal files: {body}"); +}