Confine archive extraction to a Landlock child, and disclose it when the kernel cannot

[kridaydave]

Confine archive extraction to a Landlock child, and disclose it when the kernel cannot

Stacked on #71. Review/merge #71 first.

Why

Unpacking an unreviewed release ran inside the review process itself. A parser
bug in the extraction path therefore executed with the full privileges of
whatever invoked blueline, which for a CI review job is usually a token with
write access to the repository.

Extraction now happens in a child re-exec'd from current_exe() under a Landlock
ruleset scoped to the destination directory.

What changed

• src/sandbox.rs (new): Landlock-confined extraction child, SandboxLedger
  per review, and child_entrypoint wired ahead of the normal CLI entry so the
  child reaches the extractor instead of the command parser.
• review.rs: routes every extraction (target and baseline, tar and wheel)
  through the sandbox, and pushes the unavailability disclosure onto the verdict.
• error.rs: new BluelineError::Sandbox variant for a dead child.
• policy.rs: new [general] require_sandbox key.
• Cargo.toml: target-gated landlock = "0.4".

Design decisions worth arguing about

Unavailability is disclosed, never silently downgraded. On macOS, Windows, or
any kernel without Landlock, the extraction cannot be confined. Each affected
review carries P05_SANDBOX_UNAVAILABLE at LOW.

P05 does not touch the verdict band, by construction. It is pushed directly
onto verdict.findings rather than routed through apply_extra_findings, which
recomputes the band from the accumulated score. A disclosure that can move the
verdict is not a disclosure. ci.rs already pushes R10_LOCKFILE_HASH_MISMATCH
the same way.

A dead child is a hard error, not a fallback. If the child dies or breaks
protocol, extraction fails with extraction sandbox child failed. Retrying
in-process would read whatever the dead child managed to write, which is strictly
worse than the bug being fixed.

require_sandbox defaults to false. Unavailability is the normal case off
Linux, so refusing by default would make the tool unusable on four of six shipped
platforms for a check the operator never asked for. true means "refuse rather
than tell me".

Known limitation

This does not close drift entry 1 on its own. Confining what the child may
write says nothing about who else can read the destination directory afterwards.
That is the separate 0o700 defect #71 fixes, and this PR depends on it for the
extraction path to be sound.

Testing

Full gate green on the grafted branch: cargo fmt --all -- --check, cargo clippy --all-targets --locked -- -D warnings, cargo test --all-targets --locked
(896 tests), scripts/check-references.sh.

The fork-bomb guard is pinned by two tests:
the_test_harness_is_never_allowed_to_spawn_an_extraction_child and
extraction_in_a_test_never_re_execs_the_harness. Both were confirmed to fail
when may_spawn_child's cfg!(test) branch is removed, then pass again once
restored.

Reviewer warning

src/sandbox.rs is 971 new lines of unreviewed code and it has never been on a
PR. It warrants a real read, particularly the child protocol and the Landlock
ruleset construction.

Commit made by Claude Opus 4.5 in OpenCode on behalf of Kriday.

[kridaydave]

Code Review

Local gate re-run on the PR head (180b52c): cargo fmt --all -- --check, cargo clippy --all-targets --locked -- -D warnings, cargo test --all-targets --locked (896 passed, 0 failed), scripts/check-references.sh (36 path / 4 line citations, 28 lanes) — all green. CI's Rust jobs, mutation shards and both dogfood jobs also pass. The problems below are not caught by any of that.

Blockers

B1 — ConfineError::Failed is documented as "never a fallback" and is implemented as one.
src/sandbox.rs:407-413 splits confinement failure into Unsupported ("Nothing was restricted. Safe to fall back") and Failed ("The domain is partly applied, or the attempt failed after narrowing something. Never a fallback"). But run_child collapses both into Reply::Unavailable + exit::UNAVAILABLE (:349-354), and the parent maps UNAVAILABLE to ChildFailure::Skip (:677-694), which extract() answers with extract_local(kind, bytes, dest) — a completely unconfined extraction inside the review process (:534-544).

Two reachable paths get there:

• PathFd::new(dest) failing (:468, the only producer of RestrictError::Failed) — ENOENT if the temp dir went away, EMFILE/ENFILE under fd exhaustion, EACCES.
• try_set_no_new_privs failing under HardRequirement, which makes restrict_self() return Err; restrict_once maps that to RestrictError::Unsupported (:494), so after V1 also fails you get ConfineError::Unsupported("no Landlock ABI tier could be established") and an unconfined extraction with a disclosure blaming the ABI.

The second is the sharp one. restrict_self() is the only call in the chain that narrows anything — the code says so at :443-445 — yet its failure is diagnosed as "the kernel has no Landlock". A hardened container that blocks prctl(PR_SET_NO_NEW_PRIVS) gets a silent, fully-unconfined extraction. The structural root is that Reply has three outcome variants, so "confined and failed" and "never attempted" are indistinguishable by construction. Fix: carry Failed out of restrict_self/no_new_privs (not just PathFd) and give it its own exit code, mapped to ChildFailure::Unusable.

B2 — The tier ladder [V5, V1] skips V2/V3/V4, which is where Truncate and Refer live.
src/sandbox.rs:404-405. AccessFs::from_write adds Refer at V2 and Truncate at V3/V4. On a 5.13–6.6 kernel, ABI::V5 under HardRequirement errors, then ABI::V1 succeeds — so the child ends up with a V1 ruleset on a kernel that would have granted V4, silently, with no disclosure. The module's own doc (:389-403) calls Truncate at V3 "the one an extractor needs" and says that without it "open(2) with O_TRUNC outside the granted subtree is permitted, which is exactly the fs::File::create-on-a-mis-resolved-path bug class this layer exists to contain." On RHEL 8/9, Debian 11 and Ubuntu 20.04-era kernels the layer therefore does not contain the bug class it names. V4 adds no filesystem right over V3, so [V5, V3, V1] is a strict improvement and a one-line change.

Warnings

W1 — CI never executes the confined path. may_spawn_child() returns false under cfg!(test) (:577-582), so spawn_child always returns Skip(SelfExecUnavailable) in the suite and every review falls back in-process. Untested as a result: run_child's happy path; the actual Landlock ruleset (confine_reports_whether_the_kernel_actually_enforced, :942-959, accepts either outcome and never asserts the ruleset's effect); the framed reply from a real child; the exit-code → ChildFailure mapping for OK/REFUSED/UNAVAILABLE/UNUSABLE/unknown/signal; require_sandbox = true refusing a review (the key has zero tests anywhere); and the disclosure reaching verdict.findings from a real review. rebuild_error is only unit-tested directly.

the_child_refuses_when_it_was_given_arguments (:920-940) already proves assert_cmd::Command::cargo_bin works here, so this is cheap: run the built binary with BLUELINE_SANDBOX_CHILD=1, DEST_ENV, KIND_ENV=tar, feed a real gzipped tarball on stdin, assert exit OK plus the framed files/dirs/unpacked_bytes; a second case feeding a traversal tarball must exit REFUSED. On a Linux CI runner that also exercises the ruleset.

W2 — The docs go stale on merge. Commit 180b52c touches 11 files, none of them docs. After it lands, ARCHITECTURE.md:11 ("The temp dir is not an OS-level sandbox"), :120-125 ("No Landlock, seccomp, cap-std or capability-drop code exists in src/ today, and Cargo.lock carries none of those crates"), :141 (D4: "Landlock-sandbox the step (planned, not implemented — §1)"), the README's "no OS-level sandbox yet", and references/drift.md entry 1 are all false. drift.md's own doctrine is "fix the code, then the doc, in that order" — the code landed, the doc didn't. src/sandbox.rs also has no box in the §1 module diagram.

W3 — Feature map not updated. references/features/policy.md has no require_sandbox row and there is no sandbox lane or index entry, so the new policy key, the P05_SANDBOX_UNAVAILABLE rule id and the child protocol are unmapped. check-references.sh passes because it checks that cited paths resolve, not that every feature is cited.

W4 — Non-Linux pays the child round-trip. run_child reads the whole archive to EOF before attempting confinement (:335-339 then :343-354), so on macOS/Windows every extraction spawns a process, copies the entire tarball through a pipe, then discards it before falling back. Confine first, return UNAVAILABLE before reading stdin: free on Linux, removes the double copy on four of six shipped platforms.

W5 — MAX_REPLY_BYTES's doc claims a bound it doesn't provide. :46-49 says it stops "a child that streams nonsense" making "the parent allocate without bound". It doesn't — child.wait_with_output() (:656) collects the child's entire stdout, unbounded, before read_reply runs; the ceiling only caps the declared reply length. Not attacker-reachable today (the child is our binary), but the next reader will trust the guarantee.

Suggestions

• SandboxSkip advertises a taxonomy that doesn't exist. Production constructs only Unavailable and SelfExecUnavailable; UnsupportedPlatform, KernelWithoutLandlock, AbiUnavailable and ChildDied are test-only. confine() flattens every reason to one string, and the dead-child path returns BluelineError::Sandbox via died() (:666-672) rather than SandboxSkip::ChildDied. Collapsing to the two reachable variants is what surfaces B1.
• The ledger is per-review, not per-archive, so a confined target plus an unconfined baseline yields one disclosure reading "this release's archive was unpacked without OS-level Landlock confinement" — misstating which archive was unconfined.
• restrict_once's blanket Err(_) => Unsupported (:494) conflates over-ABI handle_access, no-Landlock create, and a real restrict_self failure. Splitting the last out is the minimal B1 fix.
• 300s child timeout (:640) is a five-minute stall for a wedged child in a CI review. Fine as a bound, just know the worst case.
• extract() now requires dest to exist (canonicalize(), :519-521) where safe_extract created it via unpack_in. Both callers pass a tempdir, so no change today, but the contract is stricter than the function it wraps.
• child_entrypoint runs before clap, so BLUELINE_SANDBOX_CHILD leaking into a CI environment changes CLI behaviour: ≥1 arg exits 4 with "refusing", 0 args without DEST_ENV exits 5. Guarded and documented, but it's a new env-var-shaped failure mode.

Looks good

• The direct verdict.findings.push instead of apply_extra_findings is correct, and doubly so: score_findings (heuristic.rs:636-663) scores Low at zero and never raises the band, so P05 cannot move the verdict even when apply_extra_findings runs later for R09/R24/rollups (review.rs:404-478). The stated invariant holds by construction, as claimed — I checked it rather than taking the comment's word.
• may_spawn_child is genuinely load-bearing and correctly placed, and the two pin tests assert on the guard itself rather than forking.
• The stdin writer thread is the right fix for the pipe deadlock, and wait_with_timeout → try_wait reaps, so the later wait_with_output cannot block. Verified against std's Child::wait status caching.
• read_reply's framing (length prefix + ceiling + truncation refusal) is the right shape for an untrusted peer, and rebuild_error refusing an unknown variant instead of defaulting to Extraction is the fail-closed direction.
• CompatLevel::HardRequirement plus the FullyEnforced check is the correct call and is what makes the rest safe; the tier ladder is the only thing wrong with the confinement itself.
• Reusing Stop unpacking unreviewed bytes into a world-readable directory, and refuse a signature flag that verifies nothing #71's private_temp_dir for both extractions is right — the 0o700 fix is what makes the child's PathFd grant meaningful.

Residual risk

The security claim rests entirely on spawn_child reaching a real confined child, which CI never exercises (W1). The fork-bomb revert-and-test described in the PR body was not re-run here, by design — reproducing it is the one thing that can cost a reboot.

Verdict: request changes. B1 is a fail-open in the exact scenario this PR exists to prevent, and B2 silently downgrades the control on a large slice of the kernel base; both are small fixes. The rest is solid engineering with a documentation and test-coverage tail.

---

Reviewed by Hermes Agent

[kridaydave]

Review of bf67fb8 (Stop calling a half-applied Landlock domain an unavailable kernel)

Reviewed against f739d4a. Parse/format verified locally; the full gate was not re-run.

Verdict

Both blockers from my first review are properly closed, and this is the strongest commit in the stack. The ConfineError::Unsupported/Failed split now runs end to end through a new Reply::Unusable + exit::UNUSABLE, and a_child_that_cannot_confine_never_reports_unavailable pins it by driving the real binary against a destination that does not exist. The restrict_once staging rewrite is the right shape — only restrict_self narrows, so only its failure is Failed. V3 is in the ladder. W4 from my review is closed: confine before reading stdin, and no spawn at all off Linux.

Two finds worth calling out on their own:

• The read_reply off-by-one. split_at(nl) + rest.get(1..count + 1) versus stdout[nl + 1..] — the old form parsed only because serde_json skips leading whitespace, so a framing error that leaked the header newline was invisible to every test. Good catch, and the right fix.
• map_child_result as a pure function. Given the fork-bomb guard blocks spawning from the harness, making the exit-code mapping unit-testable is what makes the fail-closed core of this module actually pinned. Every arm has a test.

Remaining issues

1. framed_reply_bytes still converts an oversized refusal into an unavailability — the in-process fallback over a partial tree is still reachable. (src/sandbox.rs:304-313)

if body.len() > MAX_REPLY_BYTES {
    let short = Reply::Unavailable { reason: ... };
    return serde_json::to_vec(&short).unwrap_or_default();
}

The child writes a Reply::Refused, exits REFUSED, and the parent's map_child_result falls through to read_reply, which now returns Reply::Unavailable → ChildFailure::Skip. extract() then records a LOW disclosure and calls extract_local — re-running the extraction in-process, unconfined, over a dest the child has already written a partial tree into. That is precisely the outcome the module's own doc comment calls "strictly worse than the bug being fixed."

This survived verbatim from f739d4a; this commit didn't touch framed_reply_bytes. Refusal messages embed archive entry paths, so the trigger is attacker-controlled (a GNU long name, or a duplicate-entry message that repeats the path twice).

Suggested fix: keep the outcome, lose only the free text. bounded_reply truncates the message with a str::get-based cut (a byte offset can land mid-character, and a panic there reads to the parent as a dead child) and appends [message truncated to fit the reply frame]. Then the parent rebuilds the extractor's own error and stops.

2. The write-side bound doesn't count the length prefix, so a narrow deadlock window remains. (src/sandbox.rs:322-325)

write_reply emits writeln!(out, "{}", body.len()) then the body. A body at exactly MAX_REPLY_BYTES is 65542 bytes on the wire against a 64 KiB pipe, and the parent doesn't drain stdout until wait_with_timeout returns — so the child blocks in write_all and the parent waits out CHILD_TIMEOUT. Same bug class as the one f739d4a fixed, narrowed from unbounded to a 6-byte window rather than closed.

Worth noting: a_reply_at_exactly_the_ceiling_passes_through now enshrines a 65536-byte body as the intended maximum, so a test actively asserts the deadlock-prone size is legal.

Suggested fix: const MAX_REPLY_BODY_BYTES: usize = MAX_REPLY_BYTES - 32; and bound the body against that. Keep the reader's ceiling at MAX_REPLY_BYTES — the reader's job is refusing a hostile peer, the writer's is fitting a pipe, and conflating the two is what produced this. Pin both separately, plus a the_body_ceiling_leaves_room_for_the_length_prefix test so the reserve can't be removed by accident.

3. tests/sandbox_spawns.rs uses unsafe { std::env::set_var } to reach the marker branch. (tests/sandbox_spawns.rs:42-51)

It compiles — forbid(unsafe_code) is in src/, and integration tests are separate crate roots — and the safety argument is carefully written. But the argument is "this is the only thread in this binary that touches the environment," which holds only because there is exactly one test in the file. Nothing enforces that; the moment someone adds a second test it's a data race, and set_var is unsafe since edition 2024 precisely because a concurrent reader is UB.

Suggested fix: split the guard into may_spawn_child_given(marker_set: bool) — a pure function — with may_spawn_child() reading the env and delegating. Both halves get pinned as ordinary parallel-safe tests, the unsafe disappears, and CHILD_ENV can go back to pub(crate) since nothing outside the crate needs it.

Suggestions

• SandboxSkip::KernelWithoutLandlock and AbiUnavailable are dead. confine() returns Unsupported(String) and folds the ladder's "no ABI tier" message into that string, so neither variant is ever constructed in production — only in tests. Six variants advertising a taxonomy that doesn't exist.
• Two of the three real-child tests aren't #[cfg(target_os = "linux")]-gated. a_real_child_extracts_and_reports_its_stats and a_real_child_refuses_a_traversal_archive assert exit::OK / exit::REFUSED, which requires Landlock on the runner. ubuntu-latest has it, so CI passes, but a Landlock-less Linux box would fail. The third (a_child_that_cannot_confine_never_reports_unavailable) is correctly gated and correctly Landlock-independent — PathFd::new fails before anything else.
• child_traversal_tarball's hand-rolled checksum isn't verified by its test. The arithmetic is right (sum over the header with the checksum field as spaces, {:06o}\0 format), but if it were wrong the test would still pass: a tar-rs read error and validate_entry_path's rejection are both BluelineError::Extraction, both exit REFUSED. The test doesn't discriminate about why it refused.
• The ledger is per-review, not per-archive. A confined target plus an unconfined baseline produces one disclosure reading "this release's archive was unpacked without OS-level Landlock confinement" — misstating which archive was unconfined.

Looks good

• The refusal_variant extraction — the mapping now lives in one function instead of inline in run_child, so a new extractor variant can't silently fall through to the wrong name.
• Confine-before-read, and no spawn off Linux. On Linux the reorder is free (a pipe needs no filesystem grant); off Linux it removes a spawn plus a full copy of the tarball per extraction.
• The raw-stdout bound in read_reply with an honest doc comment that says what it does not do ("It does not bound what the parent allocates overall... That is accepted because the child is our own binary"). That is the correct way to write a security comment.
• a_stdout_at_exactly_the_margin_parses and an_oversized_stdout_is_refused_without_parsing together — one pins that the guard allows legal framing overhead, the other pins that a megabyte stdout hits the ceiling guard by name rather than accidentally refusing via "no length prefix". That second assertion is what stops a mutant that never fires from reading as covered.
• Splitting spawn_child so the non-Linux arm is a plain expression, with the note that an early return would leave the rest as unreachable code that -D warnings rejects on macOS and Windows. That's a portability detail that's easy to get wrong.

Residual risk

The security claim rests on spawn_child reaching a real confined child, which the harness still cannot exercise — may_spawn_child returns false under cfg(test). The three run_child_binary tests close most of that gap from outside, which is the right approach and a real improvement over where this was two commits ago. Issue 1 above is the one place the fail-closed model still leaks in the direction of "extract anyway".

---

Reviewed by Hermes Agent

[kridaydave]

Adversarial review

Recommendation: request changes. CI is red and several findings are defects in the fail-closed discipline this repo is built around. Read from the diff and from CI logs, not from the PR description.

Gate state

The body says "full gate green on the grafted branch" — true before 178c1cf, false at HEAD. Mutation shards 9, 11, 12 and 16 all fail (3 missed, 1 timeout), all in src/sandbox.rs.

Blockers

1. read_reply's boundary test sits 32 bytes off the boundary — src/sandbox.rs:418, mutant > → >= survives.

a_declared_count_at_exactly_the_ceiling_parses builds its fixture via reply_at_exactly_the_ceiling(), which pads to MAX_REPLY_BODY_BYTES (65504), not MAX_REPLY_BYTES (65536). Neither > nor >= distinguishes those two numbers, so the mutant is unobservable and the fixture never reaches the boundary its comment claims to pin. The comment at 1468 — "so the boundary tests below prove something about > rather than >=" — is the tell: it pins the body ceiling, not the count ceiling at 418.

This is the same defect class AGENTS.md documents as having shipped a 256 MiB hole, in the file this PR's own body flags as "warrants a real read."

2. Two tests call restrict_self() on the libtest process itself — src/sandbox.rs:1939, :1959.

Landlock is irreversible and thread-inheriting; the module doc says so at 16-19. I verified rather than assumed — on this kernel (ABI 7), a thread spawned after landlock_restrict_self() on the same process:

restrict_self rc = 0
thread_after_restrict -> DENIED: [Errno 2] No usable temporary directory found

So every thread libtest spawns after those two tests is permanently narrowed to a temp dir that is then deleted. It hasn't bitten in six runs, which is luck rather than safety — one --test-threads scheduling change, or one added temp-dir test, poisons unrelated tests with EPERM. run_child_binary exists precisely because observing confinement from the harness is unsafe; these two tests bypass that discipline.

3. cut_to's end -= 1 → end /= 1 hangs the full 300s — src/sandbox.rs:376.

a_cut_never_splits_a_multi_byte_character checks correctness across the range but nothing checks termination.

4. [general] require_sandbox is a fail-open, patched with a test instead of a fix — src/policy.rs:727-748.

Policy has no deny_unknown_fields, so an operator who writes [general] require_sandbox = true gets unconfined extraction plus a LOW disclosure. The new test pins the wrong-table behaviour rather than fixing it. The code comment calls this a fail-open; for a tool whose stated purpose is failing closed, the fix is rejecting unknown keys.

5. The AUR path never reaches the sandbox.

review.rs confines the archive, but registry/aur.rs builds that archive by running git archive over a git clone of an attacker-chosen repo (aur.rs:696, 604-606) as a subprocess of the unconfined parent. git's archive code is a materially larger attack surface than tar-rs and sits entirely outside the Landlock domain. The threat model in the body — "a parser bug in the extraction path executes with the full privileges of whatever invoked blueline" — is closed for tar-rs and wheel parsing only. The "Known limitation" section names the 0o700 read-back issue but not this, which is larger.

Concern

MAX_REPLY_BYTES is not a bound on parent allocation (src/sandbox.rs:51-58).

The comment admits this but the framing reads otherwise. wait_with_output collects the child's entire stdout before read_reply runs, so a hostile stdout past the framing is already in memory when the ceiling is checked. "The child is our own binary, not an attacker-controlled peer" is doing heavy lifting: the reply content is derived from attacker-controlled entry paths, which is the entire reason fit_to_frame exists.

What's genuinely good

The exit-code mapping is the strongest part of the change. Unusable vs Unavailable is the right split; every unknown exit code and every signal maps to refuse-not-fallback; trusting a confession (Reply::Unusable on a clean exit) over the exit code is correct. may_spawn_child's two-arm pinning via may_spawn_child_given is a well-built response to a real incident. Keeping confine as one cfg-gated body rather than two functions — specifically so mutation testing can reach the non-Linux arm — is exactly the structural fix AGENTS.md asks for.

Independently confirmed working: a real confined child, driven as the built binary, extracts both tar and wheel with correct stats reported. The happy path is sound.

Suggested order of work

1. Fix the two boundary fixtures so they sit on the boundary they name (blockers 1, 3) and get the shards green.
2. Move the two in-harness confine() tests behind run_child_binary, or drop them — the V5/V3/V1 ladder is already pinned by the child-driven tests (blocker 2).
3. deny_unknown_fields on Policy, or reject unknown [general] keys (blocker 4).
4. Decide what to do about git on the AUR path and say so in "Known limitation" either way (blocker 5).