fix: bounded WP-uffd accept (#261) + harden tag/id path joins (#259, #260)#262
Merged
Conversation
…260) Triage of three automated-audit reports from @asmit25805. Verified each against the code — one real bug, two low-severity hardening (the "High security" ratings were overstated, but the fixes are cheap and correct). #261 (real, robustness): request_wp_uffd spawns an accept() thread with the UnixListener MOVED into it, then issues a PUT (10s timeout) that makes FC dial back. If FC accepts the PUT but never connects — or the PUT itself fails — the blocking accept() never returns, so accept_thread.join() wedges forever. The old "the listener drops at end of scope" comment was wrong: the listener is owned by the thread, not the outer scope. Extracted a deadline-bounded accept_with_deadline() (non-blocking poll, deadline = API timeout + 2s) so a non-connecting FC surfaces as a clean error instead of a hang. Linux-only regression test. #259 (defense in depth): pack_chain joined a chain's parent_tag onto snap_root without validation. The value comes from a local snapshot.json the packer owns (the untrusted direction — unpack — already validates via is_safe_pack_tag), but a corrupted/hand-edited edge like `../../etc` would escape snap_root. Reject non-tag names with the same rule unpack uses. #260 (defense in depth): delete_sandbox spliced a sandbox id into the request path unvalidated. Ids from --all/--tag come from the daemon's own list response (trusted); an explicit CLI id is user-controlled, and `../snapshots` would traverse the URL to another endpoint. No privilege boundary is crossed (the CLI talks only to the operator's own daemon), but reject `[^A-Za-z0-9_-]` up front for a clear error instead of a confusing 404. New sdk-side unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 24, 2026
|
Thanks for the quick triage and the fixes, Wayland! Glad the bounded accept and the defense-in-depth path validation are in main now. Appreciate the fast turnaround! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Triage of three automated-audit reports from @asmit25805. I verified each against the actual code before acting — one real bug + two low-severity hardenings. The "🟠 High" ratings were overstated, but all three fixes are cheap and correct.
#261 — real (robustness)
request_wp_uffdrunsaccept()in a thread with theUnixListenermoved into it, then issues a PUT (10 s timeout) that makes Firecracker dial back. If FC accepts the PUT but never connects (or the PUT fails), the blockingaccept()never returns →accept_thread.join()hangs forever. The old comment ("listener drops at end of scope → accept unblocks") was wrong — the listener is owned by the thread. Fixed with a deadline-boundedaccept_with_deadline()(deadline = API timeout + 2 s) so a non-connecting FC is a clean error, not a hang. Linux-only regression test.#259 — defense in depth
pack_chainjoined a chainparent_tagontosnap_rootunvalidated. The value is from a local snapshot.json the packer owns (the untrusted direction —unpack— already validates viais_safe_pack_tag), but a corrupted edge like../../etcwould escapesnap_root. Now rejected with the same rule.#260 — defense in depth
delete_sandboxspliced a sandbox id into the request path unvalidated. Ids from--all/--tagcome from the daemon's own list (trusted); an explicit CLI id is user-controlled and../snapshotswould traverse the URL. No privilege boundary is crossed (CLI → operator's own daemon), but reject[^A-Za-z0-9_-]for a clear error instead of a confusing 404. New unit tests.Tests
accept_with_deadlinetimes-out test (Linux),is_valid_sandbox_id+delete_sandbox-rejects-bad-id tests.cargo build/clippy -D warnings/fmt --checkclean;cargo test -p forkd-vmm -p forkd-cligreen on the dev box.🤖 Generated with Claude Code