fix(lemonade): validate downloaded archives

[rominf]

Summary

• Verify cached and newly downloaded Lemonade embeddable archives against pinned SHA-256 digests published for the official release artifacts.
• Download into an owner-private temporary file, verify it before publication, and serialize cache validation and publication across processes with standard file locks. The lock is held across extraction and copy-out, so the bytes that are verified are the bytes that are used.
• Replace the tar subprocess with in-process extraction (tar/flate2 for .tar.gz, zip for .zip), validating every entry name against the extraction root.
• Reject symlinks and other non-regular archive-tree entries, and require canonical source and destination paths to remain within their expected roots.

Root cause

Downloaded and cached archives were trusted without integrity verification. Extraction then shelled out to tar resolved from PATH, so the binary unpacking untrusted bytes was itself unpinned, and its traversal behaviour differed between GNU tar, the bsdtar shipped as Windows tar.exe, and busybox tar. Recursive embeddable-root discovery and copying also used path helpers that followed links, allowing archive-tree traversal outside the extracted tree.

Technical decisions

In-process extraction. Entry names are validated component-by-component against the extraction root, and only directories and regular files are written. A pax global header is skipped rather than rejected: it carries archive-wide metadata and describes no member, and git archive and tar --format=pax both emit one, so treating it as a member would refuse a valid archive. Sparse entries stay rejected — the crate would expand them correctly, but a few-byte header can declare a multi-gigabyte logical size, which would turn a digest-pinned archive into an unbounded write.

Link policy. Links are rejected rather than resolved, which is deliberately conservative and fails closed on an authentic archive. If a future Lemonade release ships one, the fix is to widen the policy (resolve the target, accept it only if it stays inside the extraction root), not to drop the check. This is documented on extract_archive.

Dependencies. Adds flate2, sha2, tar, tempfile and zip to this crate. All were already in the workspace lock, so the workspace dependency set — and therefore MANIFEST.md and THIRD_PARTY_NOTICES.txt — is unchanged. zip deliberately matches xtask's major and its pure-Rust deflate backend so the workspace carries one zip and one deflate implementation.

Scope. This hardens the archive acquisition and extraction boundary without redesigning runtime installation as a transaction. Runtime replacement already occurs after archive preparation, while the identified trust gap was accepting unverified archive bytes and following unsafe archive-tree entries; a broader transaction redesign would add unrelated scope and operational risk.

Risk

Medium. The install path is intentionally stricter and will discard a corrupt cached archive or reject an archive tree containing links or special entries. Valid official archives retain the existing install behavior.

Test plan

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml and found no matching stale xfail row to remove or narrow.
• Focused Lemonade archive-validation tests passed (61), covering digest validation, cache recovery, concurrent publication, cross-process locking, temporary-file cleanup, containment, symlink rejection on Unix and via a Windows junction, zip symlink rejection, and pax global headers.
• Workspace clippy passed with warnings denied.
• Formatting checks passed.
• cargo xtask manifest --check passed.
• Full workspace tests have two pre-existing proc_lifecycle failures in rocm-core, untouched by this change; everything else passes.
• Still needed before merge: a fresh clean-root install against the real v10.10.0 artifacts. CI runners reuse a pre-warmed runtime tree, so the logs show the digest check running (Using verified cached ...) but extraction being skipped — the in-process extractor has not yet been exercised end-to-end against the published archives.

[volen-silo]

Solid, well-tested hardening — the core acquisition path is genuinely good (see the end of this review). Three things I would like addressed before merge, all in the same theme: the trust boundary ends one step earlier than the PR describes.

Verified locally: cargo test -p rocm-engine-lemonade --lib (48 passed), cargo clippy -p rocm-engine-lemonade --all-targets -- -D warnings clean. Both pinned digests confirmed byte-correct against the live upstream release artifacts (downloaded and hashed lemonade-embeddable-10.10.0-windows-x64.zip and -ubuntu-x64.tar.gz from the v10.10.0 release).

1. Verification and use are not atomic — the locks are released before extraction

engines/lemonade/src/lib.rs:986-1001

ensure_cached_archive acquires _process_guard and _archive_guard (lines 1410-1413), verifies the digest, and returns — dropping both guards. prepare_embeddable then calls extract_archive(&archive, &extract_root)? at line 1001 with no lock held, which re-opens the file from disk:

ensure_cached_archive(archive_url, &archive, embeddable_archive_sha256(), download_file)?;
...
extract_archive(&archive, &extract_root)?;

So the bytes that are verified and the bytes that are extracted are two separate reads with an unlocked window between them. A co-resident writer to <root>/downloads/ can swap the archive in that window and the install proceeds on unverified content — which is precisely the property the file lock was introduced to guarantee.

The check-then-act coverage inside ensure_cached_archive_after_invalid is correct; it is the boundary at the return that leaks. Suggestion: have ensure_cached_archive return the ArchiveCacheLock and hold it across extract_archive, or re-verify immediately before extraction.

2. extract_archive — the one step that writes untrusted bytes — is outside the fix

engines/lemonade/src/lib.rs:1456-1467

This function is unchanged by the PR (it appears in the diff only as context). It shells out to tar and lets it write directly into destination:

let mut command = ProcessCommand::new("tar");
command.arg(if runtime_is_windows() { "-xf" } else { "-xzf" });
command.arg(archive).arg("-C").arg(destination);

No entry-name validation, no --no-same-owner, and no post-extraction sweep. The new collect_embeddable_roots / copy_tree_entries checks only walk descendants of extract_root, so anything tar writes outside that root is never seen by them. Whatever traversal protection exists is whatever the locally installed tar happens to implement, which differs between GNU tar, the bsdtar that ships as Windows tar.exe, and busybox tar.

To be fair: this is only reachable if the digest pin is defeated, since ensure_cached_archive gates the call. So it is defense-in-depth, not a live vulnerability. But the root-cause section says the gap was "allowing archive-tree traversal outside the extracted tree", and the step that actually writes bytes to arbitrary paths is the one step left untouched. A containment sweep of extract_root after tar returns would close it cheaply.

Adjacent, pre-existing, worth a thought while you are here: ProcessCommand::new("tar") resolves the extractor from PATH. In a change about not trusting downloaded bytes, the binary that unpacks them is itself unpinned and unvalidated.

3. Symlink rejection is untested on Windows

engines/lemonade/src/lib.rs:3730 and :3753

embeddable_tree_rejects_external_file_symlink and embeddable_tree_rejects_external_directory_symlink are both #[cfg(unix)] and use std::os::unix::fs::symlink. Windows is this engine's primary platform — the Windows zip is the first-class artifact — so the entire is_symlink() rejection path added here (lines 1521, 1558, 1612) ships with no verification on the platform that matters most.

I checked the underlying semantics and the code is correct: Rust's Windows FileType::is_symlink() keys off the reparse point name-surrogate bit, so it returns true for junctions (IO_REPARSE_TAG_MOUNT_POINT) as well as symlinks. And a reparse point without that bit falls through to the !is_dir() && !is_file() arm and is rejected as a non-regular entry. Both good. But none of that is pinned down by a test.

Creating a symlink on Windows needs privilege, which I assume is why these are Unix-gated — but mklink /J creates a junction without elevation, so a junction-based test is feasible in CI where a symlink test is not, and the junction is exactly the case most likely to regress.

Non-blocking

• No test for the PR's headline case: a download that succeeds but returns bytes failing the digest. poisoned_cached_archive_is_removed_and_redownloaded covers a poisoned cache; failed_download_removes_private_partial_file covers a download error. Nothing covers bad-content download. The code is correctly guarded (verify_sha256(&temporary_path, ...)? precedes publish_verified_archive), so this is a missing regression test rather than a bug — but a refactor that reordered those two lines would not be caught.
• "Owner-private temporary file" holds on Unix only. tempfile sets mode 0600 on Unix but sets no ACL or SECURITY_ATTRIBUTES on Windows, so the file inherits the parent directory's DACL — no better than plain File::create. The error context at line 1443 says "failed to create private download beside {}". Worth wording for what is actually guaranteed per platform.
• concurrent_valid_archive_publication_succeeds_for_both_installers (3451) and concurrent_invalid_archive_publication_fails_closed (3470) never exercise the locking path — they are a single-threaded for _ in 0..2 calling publish_verified_archive directly, never reaching lock_archive_cache or the mutex. They do genuinely test publish-conflict detection, so they are misnamed rather than useless; the real cross-process coverage is interprocess_lock_serializes_poisoned_validation_and_recovery.
• EMBEDDABLE_ARCHIVE_CACHE_LOCK is a single global static, so it serializes installs of every archive and env_root in-process even though the per-archive file lock already provides correctness. Combined with file.lock() having no timeout or try-lock, one install stuck inside its 15-minute download window blocks every other in-process install regardless of which archive it wants.
• copy_tree_entries has no depth bound while its sibling collect_embeddable_roots caps at depth > 4 (line 1517). Moot while the archive is digest-pinned; noting the asymmetry.

Worth calling out as good

• publish_verified_archive uses persist_noclobber and, on an AlreadyExists conflict, re-verifies the file the winner published rather than assuming it is fine — fails closed on the race.
• The temp file is created with tempfile_in(parent), i.e. the destination's own directory, so the persist is a same-filesystem atomic rename rather than a copy.
• The cache is re-verified on every use, not only on write.
• Mutex poisoning is recovered with PoisonError::into_inner instead of .unwrap(), and there is a test for it.
• interprocess_lock_serializes_poisoned_validation_and_recovery spawns real child processes via current_exe(), waits on stage markers, and asserts the second process did not acquire the lock while the first held it. That is a real concurrency test, and the #[cfg(test)] gating on the stage-marker and heartbeat instrumentation is clean — none of it compiles into the release binary.
• Containment uses component-wise Path::starts_with against roots that copy_tree canonicalizes on both the source and destination side — not the string-prefix bug this pattern usually has.

[rominf]

Thanks — the three blocking points were all correct, and one of them turned out to be sharper than the framing in the review. Addressed in 56e9ddf (plus 62334d3 recording the new dependencies). The reviewed commit a0b1f52 is untouched; everything below is a follow-up commit on top of it.

1. Verification and use are not atomic

Correct, and fixed at the boundary you identified. ensure_cached_archive now returns an ArchiveCacheGuard holding both the in-process exclusion and the per-archive file lock, and prepare_embeddable holds it across extract_archive, find_embeddable_root and copy_tree. The verified bytes and the extracted bytes are now the same locked read.

I confirmed the test is load-bearing rather than tautological: with the guard released before the return (the previous behaviour), archive_guard_outlives_verification_and_is_scoped_to_one_archive fails.

2. extract_archive is outside the fix

You were right that this was the untouched step, but the reproduction landed somewhere slightly different from the traversal framing, so recording both results:

• Traversal does not reproduce on GNU tar 1.35. A .tar.gz with a ../escaped.txt entry is refused by tar itself (Member name contains '..'), exit 2, nothing written. So on this host there was no escape — consistent with your own "defense-in-depth, not a live vulnerability" caveat.
• The PATH point you filed as "adjacent, worth a thought" does reproduce, trivially. Prepending a directory containing a shell script named tar to PATH and calling extract_archive executes the impostor. The extractor unpacking untrusted bytes was itself unpinned, and picking it from PATH is also what made the traversal behaviour depend on which tar happened to be installed.

Rather than add a containment sweep around an external binary of unknown behaviour, extraction now runs in-process (tar + flate2 for the Linux artifact, zip for the Windows one). Entry names are resolved component-by-component against the extraction root — absolute paths, .., root and prefix components, and separators smuggled inside a single component are rejected; symlinks, hardlinks, devices, fifos and sockets are rejected by entry type; files are written with create_new so a directory or a symlink planted between entries is not written through. Unix mode bits are masked to 0o755, dropping setuid/setgid/sticky.

Tests: extract_archive_does_not_execute_a_path_supplied_tar (red before this commit, green after), parent/nested-parent/absolute traversal entries, symlink entry, hostile zip entry, and a well-formed-tree case asserting the executable bit survives.

3. Symlink rejection untested on Windows

embeddable_tree_rejects_external_directory_junction added, using mklink /J exactly as you suggested. Your reading of the semantics matches what the test pins: the junction is reported as a symlink and hits the same rejection branch.

Verification gap, stated plainly: I cannot run this locally — no Windows host, and cross-compiling dies in ring's build script. I type-checked and linted both cfg(windows) arms by temporarily flipping the cfgs on Linux, which caught a real problem (the non-Unix set_entry_mode would have failed Windows clippy on missing_const_for_fn). Runtime behaviour is unverified until windows-build-and-test runs.

Non-blocking

• Bad-content download — download_with_wrong_content_is_rejected_and_leaves_no_archive added: a download that succeeds at the transport level but returns wrong bytes must not publish, and must leave no temporary behind.
• "Owner-private" wording — reworded to "temporary download", with a comment stating the 0600 guarantee is Unix-only and that Windows inherits the parent DACL.
• Misnamed concurrency tests — renamed to repeated_valid_archive_publication_is_idempotent and archive_publication_conflict_with_foreign_bytes_fails_closed, which is what they actually cover.
• Global lock — this one got worse before it got better: holding the guard across extraction widens the window, so the single static is gone. In-process exclusion is now a busy-set keyed by archive path with a condvar, and the test asserts both directions — the held archive blocks, an unrelated archive does not queue behind it.
• copy_tree_entries depth bound — added, sharing a MAX_ARCHIVE_TREE_DEPTH constant with collect_embeddable_roots. Note the two differ deliberately: the collector returns Ok(()) past the bound (it is searching), the copier bails (it would be silently dropping files).

Verification

Lemonade lib tests 58/58, workspace clippy clean, fmt clean, scripts/smoke_local.py clean, cargo xtask tpn and cargo xtask manifest regenerated for the new dependencies (zip/typed-path/zlib-rs — MIT/Zlib, already in the accepted list).

Two proc_lifecycle tests fail in my workspace run; they fail identically on unmodified origin/main in the same environment (WSL2), so they are pre-existing and unrelated. Windows behaviour is CI-only, as noted above.

[rominf]

CI is green on 62334d3, which closes the one verification gap I flagged above: embeddable_tree_rejects_external_directory_junction ran and passed in windows-build-and-test, alongside the traversal, symlink, hostile-zip, bad-content-download and archive-guard tests. So the reparse-point rejection path is now verified on Windows rather than only reasoned about.

Fixed

[juhovainio]

Review note: MAX_ARCHIVE_TREE_DEPTH reuse may reject legitimate archives

MAX_ARCHIVE_TREE_DEPTH (currently 4) was previously only a search limit inside collect_embeddable_roots — how deep to look for the directory containing lemond/lemonade before giving up, and on exceeding it the function just stops searching (return Ok(())), silently.

This PR reuses the same constant as a hard cap on the entire copied tree in copy_tree_entries. Once the embeddable root is found, copy_tree walks and copies everything under it, and this path is fatal on overflow: bail!("archive tree is deeper than {MAX_ARCHIVE_TREE_DEPTH} levels..."). That's a materially different guarantee than the original search-depth limit.

The new tests only exercise a tree 1-2 levels deep (package/lib/backend), so this behavior change isn't validated against the real "embeddable" archive's actual directory structure, which may plausibly nest deeper than 4 levels (shared libs, share/, include/license trees, etc.).

Suggestion: decouple into two separate constants (a search-depth limit vs. a much larger — or unlimited — copy-depth limit), or verify the real official archive's actual nesting depth and add a test with realistic depth before merging.

[rominf]

Good catch — you were right that this was a materially different guarantee, and it's fixed in c64d184.

Verified against the real archive first. The official lemonade-embeddable-10.10.0-ubuntu-x64.tar.gz is flat: 11 entries total, the embeddable root is the top-level directory, and the deepest path below it is resources/<file> — one level. So the cap was not breaking today's Linux artifact. I could not verify the Windows zip's layout (not cached locally, no network in this session), so that side remains reasoned about rather than measured.

The structural objection stands regardless of the current archive, which is why I fixed it rather than just recording the measurement. The cap bought nothing: containment in copy_tree_entries is already enforced per entry — canonical-prefix checks on both source and destination, symlink and non-regular rejection — and unbounded depth is unreachable without symlinks, which are rejected. Its only live effect was to fail a legitimate deeper archive.

Now two constants:

• MAX_EMBEDDABLE_SEARCH_DEPTH (4) — unchanged semantics, still a non-fatal give-up in collect_embeddable_roots.
• MAX_COPY_RECURSION_DEPTH (64) — a stack-overflow backstop for copy_tree_entries only, far above any plausible layout, still fatal on overflow.

New test deeply_nested_embeddable_tree_is_copied copies a tree five levels below the embeddable root. I confirmed it is load-bearing rather than tautological: with the copy limit set back to 4 it fails with archive tree is deeper than 4 levels at .../lib/site-packages/backend/kernels/gfx.

Leaving this open for you to confirm — particularly the unmeasured Windows layout, if you have that artifact handy.

[rominf]

Force-pushed over the commit @volen-silo reviewed (a0b1f52 → c49e090). Summary of what changed since that review, since the old SHAs are no longer reachable.

Already addressed before this push (in 56e9ddf, which was pushed after the review):

• Verification and use are now atomic. ensure_cached_archive returns an ArchiveCacheGuard that prepare_embeddable holds across extraction and copy-out.
• extract_archive no longer shells out to tar. Extraction runs in-process, validating every entry name against the extraction root and rejecting links, devices and other non-regular entries. There is a test that a PATH-planted fake tar is no longer executed.
• Windows junction test for the reparse-point rejection path, plus a regression test for a download that succeeds with wrong bytes, and the two misnamed publish-conflict tests renamed and joined by real contention tests.

New in this push:

• Rebased onto current main, which resolves the Cargo.lock / MANIFEST.md / THIRD_PARTY_NOTICES.txt conflicts introduced by refactor(xtask): unify release packaging and install lifecycle E2E #139.
• zip now matches xtask's major and its pure-Rust deflate backend instead of zip 8 with deflate-flate2-zlib-rs. The workspace carries one zip and one deflate implementation, and zlib-rs and typed-path are no longer pulled in. As a result the workspace dependency set is unchanged from main, so MANIFEST.md and THIRD_PARTY_NOTICES.txt need no edits at all and are no longer touched by this PR.
• Bug fix: extraction rejected pax global headers. The tar crate consumes long-name, long-link and pax local headers itself but hands a global header to the caller, so any archive written by git archive or tar --format=pax failed with archive contains an unsupported entry (XGlobalHeader) at pax_global_header. Global headers are now skipped; a regression test covers it (verified failing before the fix). Sparse entries stay rejected, but on their own footing — the crate expands them correctly, but a few-byte header can declare a multi-gigabyte logical size.
• Both archive-cache locks announce a contended wait instead of blocking silently; the file lock is probed with try_lock first so the uncontended case stays quiet.
• Added a zip symlink-rejection test (that path had no coverage), and documented the link policy on extract_archive — why failing closed is deliberate, and that the fix if upstream ever ships a symlink is to resolve-and-contain rather than drop the check.
• Dropped the zip path's !is_file() arm as unreachable: zip defines is_file as the complement of the directory and symlink cases handled above it.

Verification: 61 lemonade tests pass, workspace clippy clean with -D warnings, cargo fmt --check clean, cargo xtask manifest --check clean. The two proc_lifecycle failures in the full workspace run reproduce identically on unmodified main and are unrelated.

One gap called out in the description: CI runners reuse a pre-warmed runtime tree, so their logs show the digest check running but extraction being skipped. A fresh clean-root install against the real v10.10.0 artifacts is still needed before merge.

The CodeQL threads are left open for a maintainer to dismiss.

[r0x0r]

Post-merge review notes (informational — PR is already merged and approved).

Strengths

• Sound threat model. The three trust boundaries — unverified bytes, the unpinned PATH-resolved tar, and link-following traversal — are each closed. write_entry_file using create_new(true) correctly defends against a symlink planted between entries (extraction-time TOCTOU).
• Fail-closed link policy is consistent and well-documented on extract_archive, with the widen-don't-drop guidance repeated at each enforcement point (extraction, root discovery, copy-out).
• Excellent test coverage. Hostile tar names stamped into raw headers, absolute/../nested traversal, symlink entries (tar + zip), pax-global-header acceptance, the PATH-supplied-tar subprocess test, cross-process lock serialization, and publish-conflict-fails-closed. Unusually thorough.
• Correct concurrency reasoning: the guard is held across extraction and copy-out, so the verified bytes are the used bytes; poisoned-mutex recovery via into_inner; per-archive keying avoids serializing unrelated installs.

Observations / residual risks

None are correctness bugs given the pinned-digest invariant; item 1 is the only one worth an active follow-up.

1. Windows end-to-end path is still unverified (as the PR description notes). download_file_to_path calls File::create(destination) while tempfile::NamedTempFile holds an open handle to the same path. Rust std opens with FILE_SHARE_READ|WRITE|DELETE, so the second handle should succeed and persist_noclobber (a rename) works — but this was never exercised against the real published zip. A clean-root install on Windows against the v10.10.0 artifact would confirm rather than infer.

2. No decompressed-size cap in extract_zip (zip-bomb). Acceptable because the archive is digest-pinned, but the safety here rests implicitly on the digest check, unlike the tar path where the sparse-entry rejection is called out explicitly.

3. Residual TOCTOU in copy_tree_entries between canonicalize(&source_path) and fs::copy(&canonical_source, …). Low risk since the extraction root is a process-private temp dir under an exclusive lock, but containment leans on that locking rather than being intrinsic.

4. copy_tree switched create_dir_all → create_dir for the destination root, tightening the precondition to require a pre-existing parent. Fine for the current prepare_embeddable caller (parent root exists); just worth noting for future callers.

[rominf]

Thanks for the post-merge look.

1. Windows end-to-end path — agreed, still unverified in the sense you mean. CI's windows-build-and-test did exercise the junction-rejection path and the rest of the suite on Windows, but a real clean-root install against the published -windows-x64.zip (the gap called out in the PR description) hasn't happened yet. Noting it rather than closing it out here.
2. No decompressed-size cap in extract_zip — correct, and accepted as-is: the safety net is the digest pin, same as the tar path's sparse-entry rejection, just not spelled out as explicitly in the zip branch. Not planning a separate cap unless the trust model around the digest changes.
3. TOCTOU in copy_tree_entries — agreed on the mechanism; it's caught by the archive-cache lock scoping the whole extraction root as process-private rather than being intrinsic to the copy step itself. Fair to flag as "leans on locking," not "structurally impossible."
4. create_dir_all → create_dir — right, and deliberate for the current caller (prepare_embeddable, where the parent always exists); flagging it in review for whoever adds the next caller is exactly the right call.

No action planned against this PR (already merged); logging these as known, accepted trade-offs for now.