Commit Graph
2 Commits
Author SHA1 Message Date
f70c4caac9 Security: create save staging files with create_new (follow-up to #164) (#319)
* Security: create the --root staging file with create_new

Follow-up to #164. write_atomic staged every save at the fixed name
.<name>.pdfcraft-tmp and wrote it with fs::write, which follows a
symbolic link and writes through a hard link. So a link planted there,
in a folder someone else can write to, redirected the bytes to any file
the link pointed at.

The staging file now gets a random 64-bit suffix (std's RandomState,
keyed from the operating system's random source) and is opened with
OpenOptions::create_new, which fails if anything already has the name
(a file, a hard link, a symbolic link even when dangling), on Windows
(CREATE_NEW) as on Unix (O_EXCL). A taken name is skipped, never
opened; Windows reports a folder at the name as "access denied", which
counts as taken when something is there. After 16 taken names the save
fails and nothing is overwritten. A failed write or rename removes only
the file this save created. At most 128 bytes of the target's name go
into the staging name, so it fits every system's name limit.

The #164 tests that planted a sentinel at the old fixed name now also
compare folder listings, and say that the "is a folder" refusal is what
they check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Security: create the desktop app's save staging file with create_new

The same fix as the previous commit, for the desktop app.
editing::write_atomically, which Save, Save As, Export, Print to PDF,
signing and crash recovery all use, staged at
.<name>.pdfcraft-<pid>.tmp and opened it with File::create, which
follows a symbolic link and writes through a hard link. A process id is
easy to guess, so a link planted beside a document in a shared folder
could redirect a save into another file.

The staging file now gets a random 64-bit suffix and is opened with
OpenOptions::create_new; a taken name (on Windows, a folder reports
"access denied") is skipped, never opened, and after 16 the save fails
without overwriting anything. At most 128 bytes of the document's name
go into the staging name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Brandon Thomas <echelon@gmail.com>
2026-10-08 15:19:16 -04:00
jcl-bitandClaude Opus 5.5 70da0179a6 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 `<path> is outside the allowed directory
<root>`, 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 <noreply@anthropic.com>

* 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 <tool> --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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-07 21:12:32 -04:00