diff --git a/crates/automation/src/file_access.rs b/crates/automation/src/file_access.rs index dfc33b4e7d..9ffdea083a 100644 --- a/crates/automation/src/file_access.rs +++ b/crates/automation/src/file_access.rs @@ -15,8 +15,8 @@ //! - A path on another network share or in a device namespace (`\\host\share`, `\\.\pipe\x`) //! than the root's is refused by name, before anything is resolved: resolving it would contact //! the host. -//! - The result must lie inside the root, compared by whole components (`/work2` isn't inside -//! `/work`), without regard to case on Windows. +//! - The result must lie inside the root, compared by whole canonical components (`/work2` isn't +//! inside `/work`), with the drive letter compared without regard to case on Windows. //! - Read and write access are separate: a root given alone grants only its own kind of access. //! //! Without roots nothing is checked. A path is checked, then opened: a process that can change the @@ -204,22 +204,23 @@ pub(crate) fn follow(shown: &str, path: &Path, root: &Path) -> Toward { if inside(&at, root) { Toward::Inside } else { Toward::Outside } } -/// Is `path` (resolved) `root` or inside it, component by component (without regard to case on -/// Windows, whose file systems ignore it)? +/// Is `path` (resolved) `root` or inside it, component by component (the drive letter without +/// regard to case on Windows)? fn inside(path: &Path, root: &Path) -> bool { let mut path = path.components(); root.components().all(|r| path.next().is_some_and(|p| same_component(p, r))) } fn same_component(a: Component, b: Component) -> bool { + use std::path::Prefix; if a == b { return true; } - if !cfg!(windows) { - return false; - } - match (a.as_os_str().to_str(), b.as_os_str().to_str()) { - (Some(a), Some(b)) => a.to_lowercase() == b.to_lowercase(), + match (a, b) { + (Component::Prefix(a), Component::Prefix(b)) => match (a.kind(), b.kind()) { + (Prefix::Disk(a) | Prefix::VerbatimDisk(a), Prefix::Disk(b) | Prefix::VerbatimDisk(b)) => a.eq_ignore_ascii_case(&b), + _ => false, + }, _ => false, } } @@ -466,6 +467,37 @@ mod tests { let _ = std::fs::remove_dir_all(base); } + /// #4003: canonical path components are compared as the file system gave them, with only the + /// Windows drive letter matched ASCII case-insensitively, so a sibling whose name `to_lowercase` + /// folds onto the root's (`K` U+212A → `k`) or a differently-cased canonical component is not + /// taken as inside the root. + #[test] + fn canonical_components_are_compared_exactly() { + let (base, _) = folders("canonical-exact"); + std::fs::create_dir_all(base.join("ink")).unwrap(); + let kelvin = base.join("in\u{212A}"); + std::fs::create_dir_all(&kelvin).unwrap(); + std::fs::write(kelvin.join("secret.pdf"), "x").unwrap(); + // U+212A folds to K. A case-insensitive volume stores both names as one directory, so + // `check` would be asked to reject a path inside its own root. The component comparison + // itself does not need that volume. + let distinct = base.join("ink").canonicalize().ok() != kelvin.canonicalize().ok(); + if distinct { + let roots = AutomationRoots::new(Some(&base.join("ink")), None).unwrap().unwrap(); + let e = roots.check(&s(kelvin.join("secret.pdf")), Access::Read).unwrap_err(); + assert!(e.starts_with("automation path rejected: outside the read root"), "{e}"); + } + assert!(!inside(Path::new("/work/in\u{212A}/secret.pdf"), Path::new("/work/ink"))); + assert!(!inside(Path::new(r"\\?\C:\work\IN\a.pdf"), Path::new(r"\\?\C:\work\in"))); + #[cfg(windows)] + { + assert!(inside(Path::new(r"\\?\c:\work\in\a.pdf"), Path::new(r"\\?\C:\work\in"))); + assert!(inside(Path::new(r"c:\work\in\a.pdf"), Path::new(r"\\?\C:\work\in"))); + assert!(!inside(Path::new(r"\\?\D:\work\in\a.pdf"), Path::new(r"\\?\C:\work\in"))); + } + let _ = std::fs::remove_dir_all(base); + } + /// Every file a tool reads or writes goes through `Automation::resolve`, which holds it to the /// roots. A function that reaches for the file system without resolving a path first would /// bypass them, so each use of `std::fs` or `File::open` must come after a `resolve` call in diff --git a/crates/automation/tests/it/hostile_args.rs b/crates/automation/tests/it/hostile_args.rs index c017ff78d3..9835231c63 100644 --- a/crates/automation/tests/it/hostile_args.rs +++ b/crates/automation/tests/it/hostile_args.rs @@ -11,9 +11,10 @@ //! //! 1. **Nothing panics.** A panic in a tool is a crashed MCP server or CLI (AGENTS.md §4), so //! every call must come back as `Ok` or as a `ToolError` the agent can read. -//! 2. **Nothing escapes `--root`.** Every path argument is tried with `..` traversal, absolute -//! paths, Windows drive and UNC paths, and a planted symbolic link. The call must fail, and -//! afterwards nothing outside the root may have been created. +//! 2. **Nothing escapes `--root` or the automation roots.** Every path argument is tried with `..` +//! traversal, absolute paths, Windows drive and UNC paths, and a planted symbolic link, under +//! both `--root` and `--automation-read-root`/`--automation-write-root` (#4003). The call must +//! fail, and afterwards nothing outside the root may have been created. //! //! The second is the mechanized form of the D3 audit. Reading the code says every handler resolves //! its own paths and that no tool dispatches another internally; this keeps that true as tools are @@ -22,6 +23,7 @@ use std::panic::{AssertUnwindSafe, catch_unwind}; use std::path::{Path, PathBuf}; +use pdfcraft_automation::file_access::AutomationRoots; use pdfcraft_automation::{Automation, ToolError, tools}; use serde_json::{Value, json}; @@ -84,6 +86,7 @@ fn zoo() -> Vec { /// bug can only touch what the test made. `linked`: the root holds `link-out`, a symbolic link /// to `base`. fn escapes(base: &std::path::Path, linked: bool) -> Vec { + let root = base.join("root"); let mut out: Vec = vec![ "../outside.pdf".into(), "../../outside.pdf".into(), @@ -93,13 +96,16 @@ fn escapes(base: &std::path::Path, linked: bool) -> Vec { base.join("outside.pdf").to_string_lossy().into_owned(), base.join("new-outside.pdf").to_string_lossy().into_owned(), base.join("sub").join("new-outside.pdf").to_string_lossy().into_owned(), + root.join("..").join("outside.pdf").to_string_lossy().into_owned(), ]; if linked { out.push("link-out/outside.pdf".into()); + out.push(root.join("link-out").join("outside.pdf").to_string_lossy().into_owned()); } // Backslashes separate only on Windows; elsewhere this is an ordinary file name in the root. if cfg!(windows) { out.push(r"..\..\outside.pdf".into()); + out.push(r"\\.\pipe\pdfcraft-hostile-escape.pdf".into()); } out } @@ -220,6 +226,20 @@ fn empty_dir(dir: &Path) { } } +/// `action_run` keeps going when one input path is refused and reports that refusal on the file. +/// That is still a confinement refusal: every file has an error naming the boundary, and none +/// has an output. +fn per_file_confinement_refusal(content: &[pdfcraft_automation::Content]) -> bool { + let Some(files) = content.iter().find_map(|c| match c { + pdfcraft_automation::Content::Json(v) => v.get("files").and_then(Value::as_array), + pdfcraft_automation::Content::Png { .. } => None, + }) else { + return false; + }; + !files.is_empty() + && files.iter().all(|file| file.get("output").is_none() && file.get("error").and_then(Value::as_str).is_some_and(|e| e.contains("outside"))) +} + /// Real values for the arguments a tool checks before the others, so a hostile value reaches the /// argument it is meant for instead of failing on a placeholder. `sign_document` checks its /// digital ID first and its document second: this ID opens with its password, and each call gets @@ -234,6 +254,12 @@ fn fixture(a: &mut Automation, root: &Path, tool: &str) -> serde_json::Map { + let args = json!({ "steps": [{ "step": "set_title", "arg": "Confined" }] }); + args.as_object().cloned().unwrap_or_default() + } _ => serde_json::Map::new(), } } @@ -313,28 +339,34 @@ fn no_tool_escapes_the_root() { } }; - let mut a = Automation::new().with_root(&root).expect("root"); - - // A real document, so tools reach their path arguments instead of failing on `doc` first. - let doc = blank_doc(&mut a); + let roots = AutomationRoots::new(Some(&root), Some(&root)).expect("roots"); + let modes = [("--root", Automation::new().with_root(&root).expect("root")), ("automation roots", Automation::new().with_automation_roots(roots))]; let mut leaked = Vec::new(); let mut tried = 0usize; let mut refused = 0usize; - for def in tools() { - for (prop, is_list) in path_props(&def) { - for escape in escapes(&base, linked) { - let mut obj = plausible_with(&def, Some(doc)); - obj.insert(prop.clone(), if is_list { json!([escape]) } else { json!(escape) }); - let args = Value::Object(obj); - tried += 1; - match catch_unwind(AssertUnwindSafe(|| a.call(def.name, &args))) { - // A path that leaves the root must be refused. Succeeding means the tool - // either used it or ignored an argument it declares — both are faults. - Ok(Ok(_)) => leaked.push(format!("{} accepted {prop}={escape:?}", def.name)), - Ok(Err(_)) => refused += 1, - Err(_) => leaked.push(format!("{} panicked on {prop}={escape:?}", def.name)), + for (mode, mut a) in modes { + // A real document, so tools reach their path arguments instead of failing on `doc` first. + let doc = blank_doc(&mut a); + + for def in tools() { + for (prop, is_list) in path_props(&def) { + for escape in escapes(&base, linked) { + let mut obj = plausible_with(&def, Some(doc)); + obj.insert(prop.clone(), if is_list { json!([escape]) } else { json!(escape) }); + let args = Value::Object(obj); + tried += 1; + match catch_unwind(AssertUnwindSafe(|| a.call(def.name, &args))) { + // A path that leaves the root must be refused. Succeeding means the tool + // either used it or ignored an argument it declares — both are faults. + // action_run reports a refused input inside an otherwise successful + // result, one error per file, and writes nothing for that file. + Ok(Ok(content)) if per_file_confinement_refusal(&content) => refused += 1, + Ok(Ok(_)) => leaked.push(format!("{mode}: {} accepted {prop}={escape:?}", def.name)), + Ok(Err(_)) => refused += 1, + Err(_) => leaked.push(format!("{mode}: {} panicked on {prop}={escape:?}", def.name)), + } } } }