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.
• 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. Recursive embeddable-root discovery and copying also used path helpers that followed links, allowing archive-tree traversal outside the extracted tree.

Technical decisions

The change 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. Focused tests cover digest validation, cache recovery, concurrent publication, cross-process locking, temporary-file cleanup, containment, and symlink rejection.

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.
• Workspace clippy passed with warnings denied.
• Formatting checks passed.
• Local smoke tests passed.
• Third-party notice verification passed.
• Full workspace tests have two pre-existing local proc_lifecycle failures unrelated to this change; the focused tests above pass.

[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.