samiandClaude Sonnet 5
08d7ee9263
CORE stage 8 merged, so vdm::Engine is linkable. This closes D4a and narrows D4b: `velox add <url>` now actually downloads. - sched/engine_port_core.hpp — the real EnginePort: forwards to a live vdm::Engine, keeps the DownloadHandle per task for pause/resume/ cancel/provide_auth/decide/refresh_url, drives set_task_order / set_max_active_segments / set_host_segment_cap via engine.segment_budget(). CORE confirmed the admission model: DAEMON decides when to start(); the engine's own download_task calls register_task/set_want internally — DAEMON never touches per-task budget calls. EnginePort gains release(TaskId) so the port drops a handle when the task goes terminal. - rpc/event_loop — EventLoop::post(fn): thread-safe, runs fn on the loop thread next iteration. The marshaller for engine-thread callbacks. - main.cpp — constructs vdm::Engine + EnginePortCore + Scheduler (post_to_loop = loop.post). At startup: reconcile_after_restart() (ADR 0013 §5), reload_config(), tick(). A 1 s timerfd on the loop re-runs tick() (schedule windows, missed nudges); download.add nudges via dispatcher.set_on_mutation. End-to-end verified against tools/testserver: `velox add http://127.0.0.1:.../file/512K` -> task queued -> scheduler admits -> engine downloads 524288 bytes -> complete, file on disk. First byte-path all the way through the project. safepath-adversarial.md: re-verified per its own note — CORE landed O_NOFOLLOW on the target open (core/src/io/sparse_file.cpp), so the leaf-symlink TOCTOU is now closed; residual is down to one intermediate-dir gap (documented post-M1 chase). 36 daemon/cli tests green; scheduler + uds_roundtrip TSan-clean. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Upd9WhG9oppieig5nRDLig
91 lines
8.1 KiB
Markdown
91 lines
8.1 KiB
Markdown
# `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. This narrows the create-after-check race on the leaf; it is fully
|
||
closed by CORE opening the download target with `O_NOFOLLOW`. **Verified 2026-09-11:
|
||
`core/src/io/sparse_file.cpp` opens `O_WRONLY | O_CREAT | O_CLOEXEC | O_NOFOLLOW`** — a
|
||
symlink swapped in as the leaf after our check fails there with `ELOOP` ->
|
||
`Error::path_rejected`. (No `O_EXCL`: resume must be able to open an existing
|
||
`.veloxpart`.)
|
||
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 — one gap, narrowed
|
||
|
||
**The leaf-symlink TOCTOU is closed** (step 5, verified 2026-09-11: CORE opens the target
|
||
`O_NOFOLLOW`). What remains:
|
||
|
||
1. **An existing intermediate directory** swapped for an out-of-root symlink between our
|
||
`realpath` (step 3) and the write. Step 3 trusts `realpath` for the pre-existing
|
||
prefix; `O_NOFOLLOW` on the *file* open does not re-check the *directories* above it,
|
||
and a full `O_NOFOLLOW` directory chase would reject the legitimate symlinked
|
||
directories A16 requires us to allow.
|
||
|
||
What limits the exposure *today*: the download directory lives under `~/.local/share` /
|
||
`~/Downloads`, both `0700` — an attacker planting a symlink there already has write access
|
||
to the user's account. The unqualified "you are covered" version of this claim does not
|
||
hold until the CORE change lands; this is exactly the boundary where a reader stops
|
||
checking, so it is spelled out.
|
||
|
||
The post-M1 hardening for gap 1 is a per-step "resolve one component, re-validate the
|
||
running path against the roots" chase (systemd's `chase_symlinks` shape).
|