sami/vdm
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

daemon: fs/safepath — the saveDir/filename path-traversal boundary (security)

Browse Source 
veloxd is the one process that turns an untrusted string into a
filesystem destination, and via capture.offer that string can come
from a web page. CLAUDE.md §4 and the M1 DoD both name this.

daemon/docs/safepath-adversarial.md is the spec, written before the
code the way EXT did for shouldCapture: 21 rows — .. traversal
(A1/A2), absolute-outside-roots (A3), prefix-match confusion (A4),
symlink-out (A7), TOCTOU on a created tail (A8), NUL/control bytes in
the leaf that CORE's fuzzer hit through Content-Disposition (A9/A10),
degenerate and overlong leaves (A11/A13), overlong dir component
(A14), symlinked root (A16), destination-is-a-file (A17), and the
legitimate cases that must still pass — non-ASCII (A18), redundant "."
(A19), trailing space/dot trimming (A20).

fs/safepath.cpp:
- sanitize_leaf: strip <0x20 and 0x7F, trim ws, strip trailing dots,
  reject ""/"."/".."/contains-'/', cap 255 UTF-8 bytes on a codepoint
  boundary. Mirrors core/src/net/content_disposition.cpp.
- canonicalize_root: expand ~ and realpath each allowedRoots entry
  once, so a symlinked root resolves to its target.
- resolve_target: reject relative saveDir and any ".." component
  lexically; if the dir exists, realpath + component-wise containment
  (a symlink that escapes is caught, one that stays inside passes); if
  a tail is missing, realpath+check the deepest existing ancestor then
  create the tail via an openat/mkdirat O_NOFOLLOW walk and re-derive
  the final path from the fd. Every failure is -32011 with data.path =
  the *original* saveDir (never the resolved path). Residual TOCTOU on
  a pre-existing intermediate dir is documented and closed by CORE's
  O_NOFOLLOW open of the file.

veloxd_fs static lib; veloxd_rpc links it for the download.add wiring
next. Test veloxd.safepath is the adversarial table, on a real temp
tree. ASan+UBSan and TSan clean; 33 daemon/cli tests green.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Upd9WhG9oppieig5nRDLig
This commit is contained in:
sami
2026-09-10 19:37:01 +04:00
co-authored by Claude Sonnet 5
parent 1914eed7db
commit ab479e7885
8 changed files with 528 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
daemon/docs/deferrals.md
+1 -1
View File
@@ -6,7 +6,7 @@ close. Kept here (not buried in commit messages) so the next pass can see them a
| # | What | Where | Why deferred | Closes when |
|---|---|---|---|---|
| D1 | Pairing prompt is `EnvAutoApprover` (needs `VELOX_PAIR_AUTO=1`) | `rpc/pairing.hpp`, `main.cpp` | A GUI dialog / `org.freedesktop.Notifications` approver is integration work | Build step 7 (systemd + notifications) |
| D2 | `download.add` → `-32603`, `download.probe` → `-32603` | `rpc/dispatcher.cpp` | Need path canonicalisation + allowed-root check (`-32011`) and the probe path (`-32013`); those need the engine link | `download.add` glue (after `sched/`) |
| D2 | `download.add` → `-32603`, `download.probe` → `-32603` | `rpc/dispatcher.cpp` | The path boundary (`-32011`) is built and tested (`fs/safepath`, `daemon/docs/safepath-adversarial.md`); still need it wired into the `download.add` handler with the store behind it, plus the probe path (`-32013`) which needs the engine | `download.add` glue (dispatcher ↔ store ↔ `fs/safepath`); probe with the engine link |
| D3 | Stub handlers for everything except `session.*`, `download.list`, `download.get` | `rpc/dispatcher.cpp` | No store behind them yet | Per method, as the store/scheduler wire in |
| D4 | `sched/` is the pure `Governor` + schedule window only; no `Scheduler` wiring to store/engine/timer | `sched/` | `Engine` bodies land in CORE stage 8; `Scheduler` needs the UUID↔`vdm::TaskId` map, a store query layer, and a timer | After CORE stage 8 lands `Engine::start()` |
| D5 | `event.*` fan-out not implemented; `session.subscribe` accepts and echoes but nothing is emitted | `rpc/uds_server.cpp`, `rpc/ws_server.cpp` | No task state to broadcast until the engine is wired | With the callback → `event.*` projection |
daemon/docs/safepath-adversarial.md
+77
View File
@@ -0,0 +1,77 @@
# `saveDir` / `filename` → filesystem destination: the adversarial table
`veloxd` is the only process that turns an untrusted string into a place bytes get
written. `capture.offer` means that string can originate from a web page, and
`download.add` over the Unix socket is reachable by any same-UID process. CLAUDE.md §4
("paths are canonicalized and checked against allowed roots before any write") and the M1
DoD ("no path traversal in `saveDir` … → `-32011`") make this a security boundary, not a
formatting nicety.
This table is written **before** `fs/safepath.cpp`, the way EXT did for `shouldCapture`.
Every row is a test in `daemon/tests/safepath_test.cpp`.
Roots for the examples: `allowedRoots = ["/home/u/Downloads", "/data/dl"]`, already
`realpath`-resolved and stored canonical at load time. `$HOME = /home/u`.
| # | Input (`saveDir`, `filename`) | Attack | Required outcome |
|---|---|---|---|
| A1 | `/home/u/Downloads/../.ssh`, `authorized_keys` | `..` climbs out of the root | `-32011`, `data.path` = the input `saveDir`. No dir created. |
| A2 | `/home/u/Downloads/a/b/../../../etc`, `x` | `..` chain escaping after descending | `-32011`. |
| A3 | `/etc`, `cron.d-payload` | absolute path, simply outside every root | `-32011`. |
| A4 | `/home/u/Downloads-evil`, `x` | prefix-match confusion with `/home/u/Downloads` | `-32011` — containment is component-wise, not `starts_with`. |
| A5 | `/home/u/Downloads`, `../.bashrc` | `..` in the **leaf**, not the dir | leaf rejected → `-32011` (or `InvalidParams`); a leaf is one component, never a path. |
| A6 | `/home/u/Downloads`, `sub/dir/file` | `/` in the leaf | leaf rejected — `filename` names a file, not a subpath. |
| A7 | `/home/u/Downloads/link-out` where `link-out` → `/etc` (pre-existing symlink) | symlink component points outside a root | `realpath` resolves it to `/etc`; `-32011`. |
| A8 | `/home/u/Downloads/goodsub`, `iso.img` — but between our check and CORE's `open`, `goodsub` is swapped for a symlink to `/etc` | **TOCTOU** on a directory component | Defense: resolve + create with `openat`/`mkdirat` from an `O_NOFOLLOW|O_DIRECTORY` fd walk, then `realpath` the final dir **again** and re-assert containment. A component that is a symlink at walk time → `-32011`. |
| A9 | `/home/u/Downloads`, `file<NUL>.iso` (`0x00` in the leaf) | NUL truncation — the write path sees `file`, logs/UI see more; CORE's fuzzer hit exactly this via `Content-Disposition` | NUL and every `< 0x20` byte and `0x7F` stripped from the leaf before use (mirrors `core/src/net/content_disposition.cpp` `sanitize_leaf`). If the leaf is empty after stripping → reject. |
| A10 | `/home/u/Downloads`, `"\r\nSet-Cookie: x".iso` | CR/LF injection into logs / downstream | control bytes stripped as A9. |
| A11 | `/home/u/Downloads`, `.` / `..` / `` (empty) | degenerate leaf | rejected. |
| A12 | `/home/u/Downloads`, `con` / `aux` / `nul` | Windows device names | **allowed** on Linux — we are not Windows; do not over-reject. (Noted so a future "harden" pass doesn't add it thinking it was missed.) |
| A13 | `/home/u/Downloads`, `<260 chars>` | overlong leaf, `ENAMETOOLONG` at `open` | leaf capped at 255 **bytes of UTF-8**, never splitting a codepoint (docs/04 §2). |
| A14 | `/home/u/Downloads/<260 chars>/x`, `y` | overlong directory component | `mkdirat` / `realpath` returns `ENAMETOOLONG` → mapped `-32011`, not a crash. |
| A15 | `saveDir` empty / null | no destination given | caller substitutes `saveTo.defaultDir`; `resolve_target` itself rejects an empty dir rather than defaulting silently. |
| A16 | root `/home/u/Downloads` is itself a symlink to `/mnt/big/dl` | a symlinked root | `canonicalize_root` `realpath`s every configured root at load; the stored root is `/mnt/big/dl`, and a `saveDir` resolving there passes. A `saveDir` of the literal `/home/u/Downloads/x` also passes because it `realpath`s to `/mnt/big/dl/x`. |
| A17 | `/home/u/Downloads` exists as a **file**, not a directory | destination is not a directory | `-32011` (`not_a_dir`), no write attempt. |
| A18 | `/home/u/Downloads/新しい/フォルダ`, `映画.mkv` | non-ASCII, legitimate | **succeeds** — UTF-8 is fine; only control bytes and the structural checks apply. |
| A19 | `/home/u/Downloads/./sub/.`, `x` | redundant `.` segments, no escape | normalized away; **succeeds** at `/home/u/Downloads/sub`. |
| A20 | `/home/u/Downloads`, ` trailing-spaces.iso ` / `dots...` | trailing space/dot (Windows-hostile, and confuses "same file" checks) | trimmed: leading/trailing whitespace and trailing dots removed before use. Empty after trim → reject. |
| A21 | relative `saveDir` (`Downloads/x`, `./x`, `x`) | a relative path has no well-defined base and invites cwd games | rejected — `resolve_target` requires an absolute `saveDir`. The GUI/CLI resolve against the default dir before calling. |
## Implementation (`fs/safepath.cpp`, as built)
1. **Sanitize the leaf first**, in isolation: strip `[0x00,0x20) ∪ {0x7F}`, trim
whitespace, strip trailing dots and spaces, reject `.`/`..`/empty/`contains '/'`, cap
255 UTF-8 bytes on a codepoint boundary. (A5, A6, A9–A13, A20)
2. **Require `saveDir` absolute; reject any `..` component lexically.** A legitimate
client never sends `..`; a web-origin path with `..` is an attack, so it does not even
reach `realpath`. (A1, A2, A21)
3. **If the directory already exists:** `realpath(saveDir)` — this follows every symlink,
so a symlinked root or component resolves to where it *really* points — then assert the
resolved path is inside a canonical root, component-wise (`d == root || d starts with
root + "/"`). A symlink that escapes is caught here (A7); one that stays inside passes
(A16). Open the resolved dir `O_PATH|O_DIRECTORY` for the leaf check. (A3, A4, A7, A16,
A17, A19)
4. **If a tail is missing (`mkdir -p` case):** find the deepest existing ancestor,
`realpath` + root-check *that*, then create the missing components through an
`openat/mkdirat` walk with `O_NOFOLLOW|O_DIRECTORY` from the ancestor's fd — the tail
has no symlinks because it had no entries; a race that plants one trips `ELOOP` →
`-32011`. Then re-derive the final dir's path from its fd (`/proc/self/fd/N`) and
re-assert containment. (A8 for the created tail, A14)
5. **Best-effort leaf check:** `fstatat(dir_fd, leaf, AT_SYMLINK_NOFOLLOW)` — refuse if it
is already a symlink. The real close on the create-after-check race is CORE opening the
file `O_NOFOLLOW|O_EXCL` (or `O_NOFOLLOW` + explicit resume); that is CORE's contract,
stated in `daemon/docs/engine-api-review.md`.
6. **Every failure is `-32011`, `data.path` = the *original* `saveDir`** — never the
resolved path, which would leak where the roots actually live. The one exception is a
`filename` that violates the schema's own `maxLength`, which is `-32602` at the param
layer before this code runs.
### Residual, accepted for M1
An **existing intermediate directory** swapped for an out-of-root symlink *between* our
`realpath` and CORE's `open` is not caught by this code (step 3 trusts `realpath` for the
pre-existing prefix; a full `O_NOFOLLOW` chase would reject legitimate symlinked
directories mid-path, which A16 requires us to allow). It is closed in practice by CORE's
`O_NOFOLLOW` open of the final file and by the download dir living under a `0700`
`~/.local/share` / `~/Downloads` the attacker would already need write access to. A
per-step "resolve, re-validate against roots" chase is the post-M1 hardening.