From 70da0179a624312238eac7bc945b4c5885d8cdbd Mon Sep 17 00:00:00 2001 From: jcl-bit <336218001+jcl-bit@users.noreply.github.com> Date: Wed, 7 Oct 2026 18:12:32 -0700 Subject: [PATCH] Security: --root confinement leaks and escapes (#136, #137) (#164) * Automation: --root refuses every outside path the same way (#136) With a root set, `resolve` canonicalized a path before checking it against the root, so an existing outside path got "is outside the allowed directory" while a missing one got the OS error ("No such file or directory", "Not a directory", "invalid path"): a confined MCP client or script could probe which files and folders exist anywhere on the disk. Containment is now decided before anything about an outside path is reported: - `..` is resolved by name first (as Windows does), so a path can't pass through an outside folder on its way back in; - on Windows another network share or device namespace (`\\host\share`, `\\?\UNC\...`, `\\.\...`) is refused by name, without contacting it; - the deepest existing ancestor, links followed, must be inside the root before anything is said about the rest; - a broken link is refused the same way, so whether its target exists stays hidden too. Every outside path now gets ` is outside the allowed directory `, existing or not, file or folder, read or write. Filesystem errors such as "not found" are still reported for paths inside the root. Behaviour change: on Unix, `..` now means the parent by name, as on Windows (`link/../x` is `x` beside `link`), and a write such as `new/../x.pdf` that stays in the root now succeeds instead of failing with "'..' is not allowed here". Co-Authored-By: Claude Opus 5.5 * CLI: script images stay inside --root (#137) `pdfcraft-cli run --script steps.json --root DIR` wrote each step's `"out"` PNG with `std::fs::write`, never through the root check the tools use: an absolute `"out"` created or replaced any file the process could write, and a relative one landed in the working directory instead of the root. The image is now saved through the new `Automation::write_output`, which uses the same resolver and atomic write as a tool's own output: outside paths get the usual "is outside the allowed directory" refusal (the step fails, nothing is written), relative paths resolve inside the root, and without --root the path is written as given, in place as before (so `/dev/stdout` still works). `run --out` goes the same way. Found while testing this, the same "." problem in two tools: - a write path naming a folder, such as "." (the root itself), staged its temporary file beside that folder, i.e. outside the root for ".", overwriting and then deleting any file there called `.root.pdfcraft-tmp`. `write_atomic` now refuses a folder before writing anything; - `image_save` adds an extension to a path without one, so "." became `root.png` beside the root. It now refuses a folder first. Co-Authored-By: Claude Opus 5.5 * Automation: files named after a document stay in the output folder Found while reviewing the --root fixes for #136 and #137. Folder outputs name their files after the document and joined that name straight onto the resolved folder, without checking the result against the root: - `doc_export_images` and `doc_export_all_images` used the whole document name, so `doc_create` with `name: "../../escape"` followed by `doc_export_images` wrote `escape_page_1.png` outside the root (an absolute name went anywhere); - `page_extract` (separate files), `doc_split`, `ocr_recognize_files` and `action_run` took the last component, but on Windows a name such as `x/C:evil.pdf` gives `C:evil`, and `folder.join("C:evil-page1.pdf")` replaces the folder with a path relative to the current directory on C:. Every such name now goes through `child()`, which turns separators, colons and control characters into `_` (and prefixes a name of only dots), so the file is always a direct child of the folder. `with_root` also refuses a root that is not a folder. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- README.md | 2 + apps/pdfcraft-cli/src/main.rs | 17 +- apps/pdfcraft-cli/tests/run_root.rs | 92 ++++++++++ crates/automation/README.md | 3 +- crates/automation/src/a11y.rs | 6 +- crates/automation/src/lib.rs | 205 ++++++++++++++++++--- crates/automation/tests/automation.rs | 252 ++++++++++++++++++++++++++ parity/acrobat-features.toml | 4 +- 8 files changed, 539 insertions(+), 42 deletions(-) create mode 100644 apps/pdfcraft-cli/tests/run_root.rs diff --git a/README.md b/README.md index 8b04e28cf..e6d416d58 100644 --- a/README.md +++ b/README.md @@ -282,6 +282,8 @@ Every engine feature is reachable without the GUI, through one table of JSON-Sch pdfcraft-cli run --script steps.json --root ./work # several steps in one session ``` + With `--root`, every file the script's steps read or write stays in that directory, including the PNG a step saves with `"out"` (the script itself is read from wherever you name it). + - **An MCP server**, for AI agents such as Claude. **It is opt-in:** PdfCraft never starts it on its own, and it opens no network port. It runs only while an agent launches `pdfcraft-cli mcp`, talks over stdin/stdout, and stops when the agent disconnects. To enable it, add it to your agent's MCP configuration: ```json diff --git a/apps/pdfcraft-cli/src/main.rs b/apps/pdfcraft-cli/src/main.rs index 6e79ffe38..cb8cdc828 100644 --- a/apps/pdfcraft-cli/src/main.rs +++ b/apps/pdfcraft-cli/src/main.rs @@ -20,7 +20,9 @@ //! //! `run` and `mcp` drive the same tool table (`pdfcraft-automation`). In `run`, values parse as //! JSON when they can (`pages=[1,3]`, `degrees=90`) and are strings otherwise. A script runs its -//! steps in one session, so `doc_open` returns id 1, the next document id 2, and so on. +//! steps in one session, so `doc_open` returns id 1, the next document id 2, and so on. A step's +//! `"out"` (like `--out`) saves its image there. With `--root DIR`, every path a tool or step +//! names, `"out"` included, must be inside DIR, and relative paths resolve inside it. //! //! The MCP server never starts on its own: it runs only when this command is launched (normally //! by an agent configured to use it), talks only over stdio, and exits when stdin closes. @@ -414,15 +416,16 @@ fn tools() -> Result<(), String> { Ok(()) } -/// Print a tool's result: JSON as JSON; images go to `--out` (or are summarised). -fn print_output(content: Vec, out: Option<&str>) -> Result<(), String> { +/// Print a tool's result: JSON as JSON; images go to `--out` (or are summarised). The image is +/// written like a tool's own output, so `--root` confines it too. +fn print_output(auto: &pdfcraft_automation::Automation, content: Vec, out: Option<&str>) -> Result<(), String> { for c in content { match c { pdfcraft_automation::Content::Json(v) => println!("{}", serde_json::to_string_pretty(&v).unwrap_or_default()), pdfcraft_automation::Content::Png { data, width, height } => match out { Some(path) => { - std::fs::write(path, &data).map_err(|e| format!("{path}: {e}"))?; - println!("{}", serde_json::json!({ "image": path, "width": width, "height": height })); + let written = auto.write_output(path, &data).map_err(|e| e.to_string())?; + println!("{}", serde_json::json!({ "image": written.to_string_lossy(), "width": width, "height": height })); } None => println!( "{}", @@ -442,7 +445,7 @@ fn run(args: &[String]) -> Result<(), String> { for (i, step) in steps.iter().enumerate() { let tool = step["tool"].as_str().ok_or(format!("step {}: missing \"tool\"", i + 1))?; let content = auto.call(tool, &step["args"]).map_err(|e| format!("step {} ({tool}): {e}", i + 1))?; - print_output(content, step["out"].as_str())?; + print_output(&auto, content, step["out"].as_str()).map_err(|e| format!("step {} ({tool}): {e}", i + 1))?; } return Ok(()); } @@ -454,7 +457,7 @@ fn run(args: &[String]) -> Result<(), String> { obj.insert(k.to_string(), value); } let content = auto.call(tool, &serde_json::Value::Object(obj)).map_err(|e| e.to_string())?; - print_output(content, flag(args, "--out")) + print_output(&auto, content, flag(args, "--out")) } #[cfg(feature = "mcp")] diff --git a/apps/pdfcraft-cli/tests/run_root.rs b/apps/pdfcraft-cli/tests/run_root.rs new file mode 100644 index 000000000..2f9fb69b6 --- /dev/null +++ b/apps/pdfcraft-cli/tests/run_root.rs @@ -0,0 +1,92 @@ +//! `pdfcraft-cli run --root`: a script's image output stays inside the root (#137). + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +const PNG: &[u8] = b"\x89PNG\r\n\x1a\n"; + +/// `root/` next to `outside/` (holding `existing.txt`) in a fresh temporary directory. +fn sandbox(test: &str) -> PathBuf { + let base = std::env::temp_dir().join(format!("pdfcraft-cli-{test}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(base.join("root")).unwrap(); + std::fs::create_dir_all(base.join("outside")).unwrap(); + std::fs::write(base.join("outside/existing.txt"), "SENTINEL\n").unwrap(); + base +} + +/// Run a two-step script (a blank page, then render it to `out`) from the working directory +/// `cwd`, with `--root root` when `root` is given. +fn render_to(base: &Path, cwd: &Path, root: Option<&Path>, out: &str) -> Output { + let script = base.join("steps.json"); + let steps = serde_json::json!([ + { "tool": "doc_create", "args": { "from": "blank", "width": 100, "height": 100 } }, + { "tool": "page_render", "args": { "doc": 1, "page": 1, "dpi": 10 }, "out": out }, + ]); + std::fs::write(&script, steps.to_string()).unwrap(); + let mut cmd = Command::new(env!("CARGO_BIN_EXE_pdfcraft-cli")); + cmd.current_dir(cwd).arg("run").arg("--script").arg(&script); + if let Some(root) = root { + cmd.arg("--root").arg(root); + } + cmd.output().unwrap() +} + +fn is_png(path: &Path) -> bool { + std::fs::read(path).is_ok_and(|b| b.starts_with(PNG)) +} + +#[test] +fn script_image_output_is_confined_to_the_root() { + let base = sandbox("run-root"); + let root = base.join("root"); + let abs = |rel: &str| base.join(rel).to_str().unwrap().to_owned(); + + #[cfg_attr(not(windows), allow(unused_mut))] + let mut refused = vec![abs("outside/escaped.png"), abs("outside/existing.txt"), "../outside/escaped.png".into(), abs("outside/new/dir/x.png")]; + #[cfg(windows)] + { + refused.push(r"..\outside\escaped.png".into()); + refused.push(r"\\pdfcraft-test.invalid\share\x.png".into()); + if let Some(drive) = (b'D'..=b'Z').rev().map(|d| format!("{}:\\", d as char)).find(|d| !Path::new(d).exists()) { + refused.push(format!("{drive}x.png")); + } + } + for out in &refused { + let o = render_to(&base, &base, Some(&root), out); + let stderr = String::from_utf8_lossy(&o.stderr); + assert!(!o.status.success(), "{out}: expected a refusal"); + assert!(stderr.contains("step 2 (page_render)") && stderr.contains("is outside the allowed directory"), "{out}: {stderr}"); + } + assert!(!base.join("outside/escaped.png").exists() && !base.join("outside/new").exists()); + assert_eq!(std::fs::read_to_string(base.join("outside/existing.txt")).unwrap(), "SENTINEL\n"); + + // "." is the root itself: refused, without staging a temporary file beside the root. + std::fs::write(base.join(".root.pdfcraft-tmp"), "SENTINEL\n").unwrap(); + let o = render_to(&base, &base, Some(&root), "."); + assert!(!o.status.success() && String::from_utf8_lossy(&o.stderr).contains("is a folder"), "{}", String::from_utf8_lossy(&o.stderr)); + assert_eq!(std::fs::read_to_string(base.join(".root.pdfcraft-tmp")).unwrap(), "SENTINEL\n"); + + // A relative path resolves inside the root, not in the working directory. + let o = render_to(&base, &base, Some(&root), "relative.png"); + assert!(o.status.success(), "{}", String::from_utf8_lossy(&o.stderr)); + assert!(is_png(&root.join("relative.png")) && !base.join("relative.png").exists()); + // So does an absolute one inside the root, and a new folder in it. + for (out, file) in [(abs("root/inside.png"), "inside.png"), ("pages/one.png".into(), "pages/one.png")] { + let o = render_to(&base, &base, Some(&root), &out); + assert!(o.status.success(), "{out}: {}", String::from_utf8_lossy(&o.stderr)); + assert!(is_png(&root.join(file)), "{out}"); + } + + // Without --root, the path is used as given. + let o = render_to(&base, &base, None, "free.png"); + assert!(o.status.success(), "{}", String::from_utf8_lossy(&o.stderr)); + assert!(is_png(&base.join("free.png"))); + // Written in place, not through a temporary file beside it, so a device still works. + #[cfg(unix)] + { + let o = render_to(&base, &base, None, "/dev/stdout"); + assert!(o.status.success() && o.stdout.windows(PNG.len()).any(|w| w == PNG), "{}", String::from_utf8_lossy(&o.stderr)); + } + let _ = std::fs::remove_dir_all(&base); +} diff --git a/crates/automation/README.md b/crates/automation/README.md index f680ae80d..977cf74e8 100644 --- a/crates/automation/README.md +++ b/crates/automation/README.md @@ -10,6 +10,7 @@ Agent control for PdfCraft: a headless tool table over the engine, and an opt-in ```rust let mut a = Automation::new().with_root("/work")?; // optional: confine all paths let doc = a.call("doc_open", &json!({ "path": "in.pdf" }))?; // Vec +a.write_output("page1.png", &png)?; // save a result yourself, under the same root rules tools() -> Vec // name, title, description, input_schema, read_only, destructive, command mcp::McpServer::new(a).serve(stdin, stdout)? // newline-delimited JSON-RPC 2.0 ``` @@ -23,7 +24,7 @@ mcp::McpServer::new(a).serve(stdin, stdout)? // newline-delimited JS - Unknown arguments are rejected, so typos fail loudly. - Tools that change a document return its summary (`doc`, `pages`, `dirty`, `undo`, `redo`, …). - `doc_close` refuses to drop unsaved changes unless `discard_changes: true`. `doc_save` writes atomically, incrementally in place, and in full for a new path. -- With a root set, every read and write path must resolve inside it (symlinks and `..` included). +- With a root set, every read and write path must resolve inside it (symlinks and `..` included). Relative paths resolve inside the root, and `..` is resolved by name before the check. Every path outside the root gets the same refusal (` is outside the allowed directory `), whether or not it exists, so a confined agent can't probe the rest of the disk; on Windows another network share or device path (`\\host\share`, `\\?\UNC\…`, `\\.\…`) is refused without being contacted. "Not found" and other filesystem errors are reported only for paths inside the root. Tools that write several files into a folder name them after the document or input file, with separators, colons and control characters replaced by `_`, so a name can't lead them out of the folder. The root itself must be a folder. ## MCP server diff --git a/crates/automation/src/a11y.rs b/crates/automation/src/a11y.rs index 2c015623e..aa9ad55d5 100644 --- a/crates/automation/src/a11y.rs +++ b/crates/automation/src/a11y.rs @@ -3,7 +3,7 @@ use pdfcraft_engine::a11y::{Category, Options, Report, Rule, Status}; use serde_json::{Value, json}; -use crate::{Args, Automation, Result, ToolError, failed, write_atomic}; +use crate::{Args, Automation, Result, ToolError, child, failed, write_atomic}; fn status_id(s: Status) -> &'static str { match s { @@ -167,7 +167,7 @@ impl Automation { }); out.push(match result { Ok(r) => { - let target = folder.join(&name); + let target = child(&folder, &name); write_atomic(&target, &r.bytes)?; let skipped: Vec = r.pages.iter().filter(|p| p.skipped.is_some()).map(|p| p.page + 1).collect(); json!({ "path": p, "output": target.to_string_lossy(), "words": r.words(), "skipped_pages": skipped }) @@ -339,7 +339,7 @@ impl Automation { .and_then(|bytes| run_on(&action, &name, std::sync::Arc::new(bytes), |_, _| {})); out.push(match result { Ok(r) => { - let target = folder.join(&name); + let target = child(&folder, &name); write_atomic(&target, &r.bytes)?; json!({ "path": p, "output": target.to_string_lossy(), "log": r.log }) } diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index 9adde658d..776f9238a 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -100,7 +100,11 @@ impl Automation { /// Confine every path the tools read or write to `root` (relative paths resolve inside it). pub fn with_root(mut self, root: impl Into) -> std::io::Result { - self.root = Some(root.into().canonicalize()?); + let root = root.into().canonicalize()?; + if !root.is_dir() { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "the root must be a folder")); + } + self.root = Some(root); Ok(self) } @@ -114,6 +118,20 @@ impl Automation { &self.session } + /// Save `bytes` that a caller writes on the session's behalf, such as `pdfcraft-cli run` + /// saving a rendered page. With a root, it is confined like a tool's own output (a relative + /// path resolves inside it) and written atomically. Without one, the path is written as + /// given, so `/dev/stdout` and the like still work. Returns where the file went. + pub fn write_output(&self, path: &str, bytes: &[u8]) -> Result { + let target = self.resolve(path, true)?; + if self.root.is_some() { + write_atomic(&target, bytes)?; + } else { + std::fs::write(&target, bytes).map_err(|e| failed(format!("{path}: {e}")))?; + } + Ok(target) + } + /// Run the tool `name` with JSON `args` (an object; `null` means no arguments). pub fn call(&mut self, name: &str, args: &Value) -> Result> { let empty = json!({}); @@ -326,6 +344,11 @@ impl Automation { if name == "image_save" { let (ext, bytes) = self.doc(&a)?.page_image_file(page, index).map_err(failed)?; let mut path = self.resolve(a.str("path")?, true)?; + if path.is_dir() { + // Adding the extension to a folder's name would write beside it: "." is the + // root, so that would land outside it. + return Err(failed(format!("{}: is a folder, not a file", path.display()))); + } if path.extension().is_none() { path.set_extension(ext); } @@ -905,7 +928,7 @@ impl Automation { let out = pdfcraft_engine::export::extract_images(&doc.export_source(), &pages, min).map_err(failed)?; let mut files = Vec::new(); for (k, img) in out.images.iter().enumerate() { - let path = folder.join(pdfcraft_engine::export::image_file_name(&stem, img, k + 1)); + let path = child(&folder, &pdfcraft_engine::export::image_file_name(&stem, img, k + 1)); write_atomic(&path, &img.data)?; files.push(json!({ "path": path.to_string_lossy(), "page": img.page + 1, "width": img.width, "height": img.height })); } @@ -923,7 +946,7 @@ impl Automation { let mut files = Vec::new(); for p in pages { let img = ex.image(p, dpi, format).map_err(failed)?; - let path = folder.join(format!("{stem}_page_{}.{}", p + 1, format.extension())); + let path = child(&folder, &format!("{stem}_page_{}.{}", p + 1, format.extension())); write_atomic(&path, &img)?; files.push(path.to_string_lossy().into_owned()); } @@ -1098,7 +1121,7 @@ impl Automation { let mut files = Vec::new(); for &p in &pages { let bytes = self.session.extract(id, &[p]).map_err(failed)?; - let path = dir.join(format!("{stem}-page{}.pdf", p + 1)); + let path = child(&dir, &format!("{stem}-page{}.pdf", p + 1)); write_atomic(&path, &bytes)?; files.push(path.to_string_lossy().into_owned()); } @@ -1177,7 +1200,7 @@ impl Automation { Some((_, t)) => format!("{stem}-{}.pdf", safe(t)), None => format!("{stem}-part{}.pdf", i + 1), }; - let path = dir.join(file); + let path = child(&dir, &file); write_atomic(&path, bytes)?; files.push(json!({ "path": path.to_string_lossy(), "first_page": first, "last_page": last })); } @@ -1322,37 +1345,89 @@ impl Automation { /// Resolve a user-supplied path, enforcing the root (if any). `for_write` allows a file that /// does not exist yet (its nearest existing ancestor must be inside the root). + /// + /// Every path outside the root gets the same refusal, so a confined client can't learn what + /// exists out there (#136): `..` is resolved by name first, another network share or device + /// namespace is refused without touching it, and the deepest existing ancestor (links + /// followed) must be inside the root before anything about the rest is reported. fn resolve(&self, path: &str, for_write: bool) -> Result { let p = Path::new(path); - let joined = match &self.root { - Some(root) if p.is_relative() => root.join(p), - _ => p.to_path_buf(), - }; - let Some(root) = &self.root else { return Ok(joined) }; - let real = if for_write { - // Canonicalize the deepest existing ancestor, then re-append the rest. - let mut existing = joined.as_path(); - let mut rest = Vec::new(); - while !existing.exists() { - rest.push(existing.file_name().ok_or_else(|| failed(format!("{path}: invalid path")))?); - existing = existing.parent().ok_or_else(|| failed(format!("{path}: invalid path")))?; - } - if rest.iter().any(|c| *c == "..") { - return Err(failed(format!("{path}: '..' is not allowed here"))); - } - let mut real = existing.canonicalize().map_err(|e| failed(format!("{path}: {e}")))?; - real.extend(rest.iter().rev()); - real - } else { - joined.canonicalize().map_err(|e| failed(format!("{path}: {e}")))? - }; - if !real.starts_with(root) { - return Err(failed(format!("{path} is outside the allowed directory {}", root.display()))); + let Some(root) = &self.root else { return Ok(p.to_path_buf()) }; + let outside = || failed(format!("{path} is outside the allowed directory {}", root.display())); + let joined = lexical(&root.join(p)).ok_or_else(outside)?; + if foreign_share(&joined, root) { + return Err(outside()); } + let mut existing = joined.as_path(); + let mut rest = Vec::new(); + while !existing.exists() { + if existing.symlink_metadata().is_ok_and(|m| m.file_type().is_symlink()) { + return Err(outside()); // a broken link: going on would tell whether its target exists + } + match (existing.file_name(), existing.parent()) { + (Some(name), Some(parent)) => { + rest.push(name); + existing = parent; + } + _ => return Err(outside()), // not even a drive or share that exists + } + } + let mut real = existing.canonicalize().map_err(|_| outside())?; + if !real.starts_with(root) { + return Err(outside()); + } + if !rest.is_empty() && !for_write { + // Missing, below a folder inside the root: say why, as the system reports it. + real = joined.canonicalize().map_err(|e| failed(format!("{path}: {e}")))?; + if !real.starts_with(root) { + return Err(outside()); // it appeared, as a link out, since the check above + } + return Ok(real); + } + real.extend(rest.iter().rev()); Ok(real) } } +/// `path` with `.` and `..` resolved by name, before the filesystem is consulted (Windows does +/// the same, and so does joining onto a canonical root there). `None` if a `..` would climb +/// above the start of the path. +fn lexical(path: &Path) -> Option { + use std::path::Component; + let mut parts: Vec = Vec::new(); + for c in path.components() { + match c { + Component::CurDir => {} + Component::ParentDir => match parts.last() { + Some(Component::Normal(_)) => { + parts.pop(); + } + _ => return None, + }, + other => parts.push(other), + } + } + Some(parts.iter().collect()) +} + +/// Whether `path` names another network share or device namespace than `root` (Windows +/// `\\host\share`, `\\?\UNC\…`, `\\.\…`, `\\?\…`). Those are refused by name: even checking +/// that one exists would contact the host. Drive letters are left to the normal check. +fn foreign_share(path: &Path, root: &Path) -> bool { + use std::path::{Component, Prefix}; + fn share(p: &Path) -> Option { + let Some(Component::Prefix(prefix)) = p.components().next() else { return None }; + let (kind, a, b) = match prefix.kind() { + Prefix::Disk(_) | Prefix::VerbatimDisk(_) => return None, + Prefix::UNC(host, share) | Prefix::VerbatimUNC(host, share) => ("unc", host, share), + Prefix::DeviceNS(name) => ("device", name, std::ffi::OsStr::new("")), + Prefix::Verbatim(name) => ("verbatim", name, std::ffi::OsStr::new("")), + }; + Some(format!("{kind}\\{}\\{}", a.to_string_lossy(), b.to_string_lossy()).to_lowercase()) + } + share(path).is_some_and(|s| share(root).as_ref() != Some(&s)) +} + // ---- JSON helpers ---------------------------------------------------------------------------- /// Typed access to validated arguments. @@ -1577,8 +1652,24 @@ fn extract_parallel(bytes: &Arc>, password: Option>, pages: &[u out } +/// The file `name` inside `dir`, where `name` comes from a document or an argument: separators, +/// colons (a Windows drive or stream) and control characters become `_`, and a name of only dots +/// gets a leading `_`, so it is one plain file name and can't lead out of `dir`. +fn child(dir: &Path, name: &str) -> PathBuf { + let mut safe: String = name.chars().map(|c| if matches!(c, '/' | '\\' | ':') || c.is_control() { '_' } else { c }).collect(); + if safe.chars().all(|c| c == '.') { + safe.insert(0, '_'); + } + dir.join(safe) +} + /// Write via a temporary file in the same directory, then rename over the target. fn write_atomic(path: &Path, bytes: &[u8]) -> Result<()> { + // A folder can't be replaced, and staging beside it could land outside the root: "." names + // the root itself, whose parent isn't ours to write in. + if path.is_dir() { + return Err(failed(format!("{}: is a folder, not a file", path.display()))); + } let dir = path.parent().filter(|p| !p.as_os_str().is_empty()).unwrap_or(Path::new(".")); let name = path.file_name().ok_or_else(|| failed(format!("{}: not a file path", path.display())))?; std::fs::create_dir_all(dir).map_err(|e| failed(format!("{}: {e}", dir.display())))?; @@ -1589,3 +1680,59 @@ fn write_atomic(path: &Path, bytes: &[u8]) -> Result<()> { failed(format!("{}: {e}", path.display())) }) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn dot_dot_resolves_by_name_and_never_climbs_above_the_start() { + let base = std::env::temp_dir(); + let l = |rel: &str| lexical(&base.join(rel)); + assert_eq!(l("a/./b/../c"), Some(base.join("a").join("c"))); + assert_eq!(l("missing/../../x"), lexical(&base.join("..").join("x"))); + assert_eq!(l("a/.."), Some(lexical(&base).unwrap())); + let deep = "../".repeat(base.components().count() + 1); + assert_eq!(lexical(&base.join(&deep)), None); + assert_eq!(lexical(Path::new("..")), None); + } + + #[test] + fn names_from_documents_become_one_plain_file_name() { + let dir = std::env::temp_dir().join("out"); + for name in ["../../x.png", "/abs/x.png", r"..\..\x.png", "C:x.png", r"\\host\share\x.png", "a:stream", "..", ".", "", "tab\there", "ok.png"] + { + let p = child(&dir, name); + assert_eq!(p.parent(), Some(dir.as_path()), "{name:?} -> {}", p.display()); + assert_eq!(p.components().count(), dir.components().count() + 1, "{name:?} -> {}", p.display()); + } + assert_eq!(child(&dir, "ok.png"), dir.join("ok.png")); + assert_eq!(child(&dir, "../x.png"), dir.join(".._x.png")); + } + + #[cfg(windows)] + #[test] + fn other_shares_and_device_paths_are_refused_by_name() { + let local = Path::new(r"\\?\C:\work\root"); + for p in [ + r"\\host\share\x.pdf", + "//host/share/x.pdf", + r"\/host/share/x.pdf", + r"\\?\UNC\host\share\x.pdf", + r"\\.\pipe\x", + r"\\.\C:\work\root\x.pdf", + r"\\?\GLOBALROOT\Device\x", + ] { + assert!(foreign_share(Path::new(p), local), "{p}"); + } + for p in [r"C:\work\root\x.pdf", r"c:\elsewhere\x.pdf", r"\\?\C:\work\root\x.pdf", r"D:\x.pdf", r"C:x.pdf"] { + assert!(!foreign_share(Path::new(p), local), "{p}"); + } + // A root on a share accepts that share, however it is spelled, and nothing else. + let shared = Path::new(r"\\?\UNC\Server\Docs\root"); + assert!(!foreign_share(Path::new(r"\\server\docs\root\x.pdf"), shared)); + assert!(!foreign_share(Path::new(r"\\?\UNC\SERVER\DOCS\x.pdf"), shared)); + assert!(foreign_share(Path::new(r"\\server\other\x.pdf"), shared)); + assert!(foreign_share(Path::new(r"\\attacker\docs\x.pdf"), shared)); + } +} diff --git a/crates/automation/tests/automation.rs b/crates/automation/tests/automation.rs index 13e35ea18..e200bfd45 100644 --- a/crates/automation/tests/automation.rs +++ b/crates/automation/tests/automation.rs @@ -218,6 +218,258 @@ fn errors_are_specific_and_safe() { let _ = std::fs::remove_file(outside); } +/// `root/` (with `inside.pdf`) next to `outside/` (with the file `secret.pdf` and the folder +/// `sub`), all in a fresh temporary directory. Returns (base, canonical root). +fn sandbox(test: &str) -> (PathBuf, PathBuf) { + let base = std::env::temp_dir().join(format!("pdfcraft-automation-{test}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(base.join("root")).unwrap(); + std::fs::create_dir_all(base.join("outside/sub")).unwrap(); + std::fs::write(base.join("root/inside.pdf"), fixture(1)).unwrap(); + std::fs::write(base.join("outside/secret.pdf"), fixture(1)).unwrap(); + let root = base.join("root").canonicalize().unwrap(); + (base, root) +} + +/// A link `root/` to the directory `target`: a symlink on Unix, a junction on Windows +/// (which needs no privilege). +fn link_dir(root: &Path, name: &str, target: &Path) { + #[cfg(unix)] + std::os::unix::fs::symlink(target, root.join(name)).unwrap(); + #[cfg(windows)] + { + // Rebuilt from components so every separator is `\` (cmd reads `/x` as a switch). + let (link, target): (PathBuf, PathBuf) = (root.join(name).components().collect(), target.components().collect()); + let status = std::process::Command::new("cmd").arg("/C").arg("mklink").arg("/J").arg(link).arg(target).output().unwrap(); + assert!(status.status.success(), "mklink /J failed: {}", String::from_utf8_lossy(&status.stderr)); + } +} + +#[test] +fn root_refusals_do_not_reveal_what_exists_outside() { + // Regression test for #136: every path outside the root gets the same refusal, whether it + // exists, is a file or a folder, or passes through a missing folder. + let (base, root) = sandbox("root-oracle"); + let mut a = auto(&root); + let refusal = |p: &str| ToolError::Failed(format!("{p} is outside the allowed directory {}", root.display())); + let abs = |rel: &str| base.join(rel).to_str().unwrap().to_owned(); + + #[cfg_attr(not(windows), allow(unused_mut))] + let mut reads = vec![ + "../outside/secret.pdf".to_owned(), + "../outside/nope.pdf".into(), + "../outside/secret.pdf/x".into(), + "../outside/sub".into(), + "../outside/sub/x".into(), + "../nowhere/at/all.pdf".into(), + "missing/../../outside/secret.pdf".into(), + "missing/../../outside/nope.pdf".into(), + "../../../../../../../../../../../../../../../../../../../../../../../../x.pdf".into(), + abs("outside/secret.pdf"), + abs("outside/nope.pdf"), + abs("outside/sub/x"), + abs("nowhere/x.pdf"), + abs("nowhere/../outside/nope.pdf"), + abs("outside/../outside/secret.pdf"), + abs("root/../outside/secret.pdf"), + ]; + #[cfg(windows)] + { + reads.extend([ + r"..\outside\secret.pdf".to_owned(), + r"..\outside/nope.pdf".into(), + "../outside/secret.pdf.".into(), + "../outside/secret.pdf ".into(), + // Another network share or device namespace is refused by name, without contacting + // it (`.invalid` never resolves, so a regression fails instead of reaching a host). + r"\\pdfcraft-test.invalid\share\secret.pdf".into(), + "//pdfcraft-test.invalid/share/secret.pdf".into(), + r"\\?\UNC\pdfcraft-test.invalid\share\secret.pdf".into(), + r"\\.\pipe\pdfcraft-test".into(), + r"\\?\GLOBALROOT\Device\Null".into(), + ]); + let other = base.join("outside/secret.pdf").canonicalize().unwrap(); + reads.push(other.to_str().unwrap().to_owned()); // the verbatim \\?\C:\… form + if let Some(drive) = (b'D'..=b'Z').rev().map(|d| format!("{}:\\", d as char)).find(|d| !Path::new(d).exists()) { + reads.push(format!("{drive}secret.pdf")); // a drive that doesn't exist + } + } + for p in &reads { + assert_eq!(a.call("doc_open", &json!({ "path": p })).unwrap_err(), refusal(p), "reading {p}"); + } + + let writes = [ + "../outside/sub/../y.pdf".to_owned(), + "../outside/nosub/../y.pdf".into(), + "../outside/y.pdf".into(), + "../outside/nosub/y.pdf".into(), + "../outside/secret.pdf".into(), + "../outside/secret.pdf/y.pdf".into(), + "new/../../escape.pdf".into(), + abs("outside/nosub/deeper/y.pdf"), + ]; + let doc = ok(&mut a, "doc_open", json!({ "path": "inside.pdf" }))["doc"].as_u64().unwrap(); + for p in &writes { + assert_eq!(a.call("doc_save", &json!({ "doc": doc, "path": p })).unwrap_err(), refusal(p), "writing {p}"); + } + #[cfg(windows)] + { + // In a verbatim path `/` is not a separator, so `x/../..` can't climb out of it either. + for tail in [r"\x/../../outside/v.pdf", r"\x/../../outside/secret.pdf"] { + let p = format!("{}{tail}", root.display()); + if let Ok(c) = a.call("doc_save", &json!({ "doc": doc, "path": p })) { + let Content::Json(v) = &c[0] else { panic!("{p}: expected JSON") }; + assert!(Path::new(v["path"].as_str().unwrap()).starts_with(&root), "{p} wrote {v}"); + } + // (What lands outside the root, if anything, is checked below.) + } + } + + // A link inside the root that leads out of it is refused the same way, below it too. + link_dir(&base.join("root"), "link", &base.join("outside")); + for p in ["link/secret.pdf", "link/nope.pdf", "link/sub/x", "link"] { + assert_eq!(a.call("doc_open", &json!({ "path": p })).unwrap_err(), refusal(p), "reading {p}"); + } + for p in ["link/new.pdf", "link/nosub/new.pdf", "link/secret.pdf"] { + assert_eq!(a.call("doc_save", &json!({ "doc": doc, "path": p })).unwrap_err(), refusal(p), "writing {p}"); + } + // So is a link whose target is gone: whether a link's target exists stays hidden too. + std::fs::create_dir_all(base.join("outside/gone")).unwrap(); + link_dir(&base.join("root"), "broken", &base.join("outside/gone")); + std::fs::remove_dir(base.join("outside/gone")).unwrap(); + for p in ["broken", "broken/x.pdf", "broken/x/y.pdf"] { + assert_eq!(a.call("doc_open", &json!({ "path": p })).unwrap_err(), refusal(p), "reading {p}"); + assert_eq!(a.call("doc_save", &json!({ "doc": doc, "path": p })).unwrap_err(), refusal(p), "writing {p}"); + } + + // Nothing was written outside the root, and the outside files are untouched. + let mut left: Vec = + std::fs::read_dir(base.join("outside")).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); + left.sort(); + assert_eq!(left, ["secret.pdf", "sub"]); + assert_eq!(std::fs::read(base.join("outside/secret.pdf")).unwrap(), fixture(1)); + assert!(!base.join("escape.pdf").exists() && !base.join("y.pdf").exists()); + + // Inside the root nothing changes: missing files say so, and existing ones open and save, + // however the path is spelled. + let missing = a.call("doc_open", &json!({ "path": "missing-inside.pdf" })).unwrap_err(); + assert!(matches!(&missing, ToolError::Failed(m) if m.starts_with("missing-inside.pdf: ") && !m.contains("outside")), "{missing:?}"); + let not_dir = a.call("doc_open", &json!({ "path": "inside.pdf/x" })).unwrap_err(); + assert!(matches!(¬_dir, ToolError::Failed(m) if !m.contains("outside")), "{not_dir:?}"); + #[cfg_attr(not(windows), allow(unused_mut))] + let mut opens = vec![ + "inside.pdf".to_owned(), + "./inside.pdf".into(), + "sub/../inside.pdf".into(), + "../root/inside.pdf".into(), + "../outside/../root/inside.pdf".into(), + "../nowhere/../root/inside.pdf".into(), + abs("root/inside.pdf"), + abs("outside/../root/inside.pdf"), + abs("nowhere/../root/inside.pdf"), + root.join("inside.pdf").to_str().unwrap().to_owned(), + ]; + #[cfg(windows)] + { + let plain = abs("root/inside.pdf"); + opens.extend([plain.to_lowercase(), plain.to_uppercase(), r"..\root\inside.pdf".into()]); + } + for p in &opens { + ok(&mut a, "doc_open", json!({ "path": p })); + } + for p in ["new.pdf", "fresh/dir/new.pdf", "fresh/../also-new.pdf"] { + ok(&mut a, "doc_save", json!({ "doc": doc, "path": p })); + } + ok(&mut a, "doc_save", json!({ "doc": doc, "path": abs("root/abs-new.pdf") })); + for f in ["new.pdf", "fresh/dir/new.pdf", "also-new.pdf", "abs-new.pdf"] { + assert!(root.join(f).is_file(), "{f} was written inside the root"); + } + + // Without a root, paths are used as given. + let mut free = Automation::new(); + ok(&mut free, "doc_open", json!({ "path": abs("outside/secret.pdf") })); + let _ = std::fs::remove_dir_all(&base); +} + +#[test] +fn writing_to_a_folder_touches_nothing_beside_it() { + // "." names the root itself. Saving there used to stage its temporary file next to the + // root, outside it, overwriting and then deleting any file of that name. + let (base, root) = sandbox("root-itself"); + let mut a = auto(&root); + let beside = base.join(".root.pdfcraft-tmp"); + std::fs::write(&beside, "SENTINEL").unwrap(); + std::fs::create_dir_all(root.join("folder")).unwrap(); + let doc = ok(&mut a, "doc_open", json!({ "path": "inside.pdf" }))["doc"].as_u64().unwrap(); + for p in [".", "", "folder", "folder/"] { + let e = a.call("doc_save", &json!({ "doc": doc, "path": p })).unwrap_err(); + assert!(matches!(&e, ToolError::Failed(m) if m.contains("is a folder")), "{p:?}: {e:?}"); + } + let png = vec![1, 2, 3]; + assert!(a.write_output(".", &png).is_err()); + assert_eq!(std::fs::read_to_string(&beside).unwrap(), "SENTINEL"); + assert!(!root.join(".folder.pdfcraft-tmp").exists()); + // `image_save` adds an extension when the path has none, which turned "." into `root.png` + // beside the root. + ok(&mut a, "doc_export_images", json!({ "doc": doc, "folder": "src", "dpi": 18 })); + let pic = ok(&mut a, "doc_create", json!({ "from": "images", "paths": ["src/inside_page_1.png"] }))["doc"].as_u64().unwrap(); + for p in [".", "", "folder", "src/.."] { + let e = a.call("image_save", &json!({ "doc": pic, "page": 1, "image": 1, "path": p })).unwrap_err(); + assert!(matches!(&e, ToolError::Failed(m) if m.contains("is a folder")), "{p:?}: {e:?}"); + } + assert!(!base.join("root.png").exists() && !root.join("folder.png").exists()); + ok(&mut a, "image_save", json!({ "doc": pic, "page": 1, "image": 1, "path": "folder/picture" })); + assert!(root.join("folder/picture.png").is_file()); + // Folder outputs may still name the root. + ok(&mut a, "doc_split", json!({ "doc": doc, "every": 1, "out_dir": "." })); + let _ = std::fs::remove_dir_all(&base); +} + +#[test] +fn file_names_from_documents_stay_in_the_output_folder() { + // Folder outputs name their files after the document. A document name with separators, + // `..` or (on Windows) a drive letter used to take those files out of the folder, and out + // of the root. + let (base, root) = sandbox("doc-names"); + let mut a = auto(&root); + let names = ["../../escape", "../../escape.pdf", "x/../../../escape.pdf", "/tmp/escape.pdf", r"x\C:escape.pdf", "C:escape.pdf", "..", "."]; + for (i, name) in names.iter().enumerate() { + let doc = ok(&mut a, "doc_create", json!({ "from": "blank", "pages": 2, "name": name }))["doc"].as_u64().unwrap(); + let out = format!("out{i}"); + let mut files: Vec = Vec::new(); + let r = ok(&mut a, "doc_export_images", json!({ "doc": doc, "folder": out, "dpi": 10 })); + files.extend(r["files"].as_array().unwrap().iter().map(|f| f.as_str().unwrap().to_owned())); + let r = ok(&mut a, "page_extract", json!({ "doc": doc, "pages": [1], "separate": true, "out_dir": out })); + files.extend(r["files"].as_array().unwrap().iter().map(|f| f.as_str().unwrap().to_owned())); + let r = ok(&mut a, "doc_split", json!({ "doc": doc, "every": 1, "out_dir": out })); + files.extend(r["files"].as_array().unwrap().iter().map(|f| f["path"].as_str().unwrap().to_owned())); + assert_eq!(files.len(), 5, "{name:?}: {files:?}"); + for f in &files { + let f = Path::new(f); + assert_eq!(f.parent(), Some(root.join(&out).as_path()), "{name:?} wrote {}", f.display()); + assert!(f.is_file(), "{name:?}: {} exists", f.display()); + } + } + // The same for the images a page uses, with a document that has one. + let doc = ok(&mut a, "doc_open", json!({ "path": "inside.pdf" }))["doc"].as_u64().unwrap(); + ok(&mut a, "doc_export_images", json!({ "doc": doc, "folder": "src", "dpi": 18 })); + let pic = + ok(&mut a, "doc_create", json!({ "from": "images", "paths": ["src/inside_page_1.png"], "name": "../../escape" }))["doc"].as_u64().unwrap(); + let r = ok(&mut a, "doc_export_all_images", json!({ "doc": pic, "folder": "all" })); + let files = r["files"].as_array().unwrap(); + assert_eq!(files.len(), 1, "{r}"); + for f in files { + assert_eq!(Path::new(f["path"].as_str().unwrap()).parent(), Some(root.join("all").as_path()), "{r}"); + } + let mut left: Vec = std::fs::read_dir(&base).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); + left.sort(); + assert_eq!(left, ["outside", "root"]); + + // The root itself must be a folder. + assert!(Automation::new().with_root(root.join("inside.pdf")).is_err()); + let _ = std::fs::remove_dir_all(&base); +} + #[test] fn command_list_reports_enablement_and_tools() { let dir = workdir("commands"); diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 258b17702..fcf3d75a6 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -9350,8 +9350,8 @@ milestone = "M13" status = "shipped" commands = [] tools = [] -evidence = ["crates/automation/tests/automation.rs::errors_are_specific_and_safe"] -notes = "" +evidence = ["crates/automation/tests/automation.rs::errors_are_specific_and_safe", "crates/automation/tests/automation.rs::root_refusals_do_not_reveal_what_exists_outside", "apps/pdfcraft-cli/tests/run_root.rs::script_image_output_is_confined_to_the_root", "crates/automation/tests/automation.rs::file_names_from_documents_stay_in_the_output_folder"] +notes = "One refusal for every path outside the root, existing or not (#136). `run --script` images (`out`) are confined too (#137). Check-then-write, not openat-style: a process racing the tool on the same disk could swap a folder for a link between the check and the write." [[feature]] id = "misc.mcp-resources"