transpiler cache: bind section hashes to input and verify unconditionally - #35101
transpiler cache: bind section hashes to input and verify unconditionally#35101robobun wants to merge 3 commits into
Conversation
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 22 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Related to #32741 but does not close it. That issue tracks several fixed-seed wyhash uses (trustedDependencies, scoped-registry auth, patch-hash, MySQL statement cache, and the transpiler cache collision in PoC6). This PR only hardens the transpiler cache against the tampered-entry / zeroed-hash path; a true wyhash collision between two distinct sources (PoC6) still maps to the same cache filename and is not addressed here. |
|
CI on build 77666: The two hard failures are node-parallel GC timing tests that never enter the changed code path (both are under the transpiler cache's 4 KB source floor):
The remaining failures are retry-pass flakes ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this hardens a security-adjacent path (the on-disk transpiler cache that decides which JS bytes get executed), so a maintainer should sign off on the threat-model framing — particularly the acknowledged limitation that input_hash is derivable from the cache filename, and the relationship to the remaining items in #32741.
What was reviewed:
- Save/load symmetry: all three encoding arms (UTF8/LATIN1/UTF16), sourcemap, and esm_record now hash with the same
input_hashseed on both sides; version bump to 24 invalidates old entries. - The reordering in the LATIN1 arm now checks
read_bytesbefore hashing (previously hashed before the short-read check). - Confirmed
Bun.hash.wyhashin the tests routes to the samebun_wyhash::Wyhashthe loader uses, so the updated out-of-range-index test still reaches the deserializer. SEED/hash()remain live (used forinput_hashandfeatures_hash), not dead code.
Extended reasoning...
Overview
This PR changes how the runtime transpiler cache (~/.bun/install/cache/@t@/*.pile) validates its payload sections on load. Previously, output_hash / esm_record_hash were computed with a fixed wyhash seed of 42 and verification was skipped whenever the stored hash field was zero; the sourcemap section was never verified at all. The change (a) seeds each section hash with the entry's input_hash so the stored hash binds the payload to the source that keyed the entry, (b) removes the != 0 bypasses so verification is unconditional, (c) adds the missing sourcemap check, and (d) bumps EXPECTED_VERSION to 24 so pre-existing entries (whose hashes were computed under seed 42) are discarded rather than mis-rejected.
Two files touched: src/jsc/RuntimeTranspilerCache.rs (save + load paths) and test/cli/run/transpiler-cache.test.ts (two new tamper-rejection tests, plus the existing out-of-range-string-index test updated to recompute esm_record_hash under the new seed so its corrupted record still reaches the deserializer under test).
Security risks
The transpiler cache is what determines which transpiled JS bytes are handed to JSC for execution, so a bypass in its integrity check is effectively arbitrary code execution against any process that reads a poisoned entry. This PR strictly tightens verification (never loosens it), so the change itself does not introduce new attack surface. The author is explicit that this is corruption detection rather than a security boundary — wyhash is not a MAC, and input_hash (the new seed) is derivable from the cache filename, so an attacker with write access to the cache directory can still forge a passing hash. That framing looks correct to me, but it's exactly the kind of threat-model call a maintainer should ratify, especially given the robobun note that this addresses only one facet of #32741 (the zeroed-hash bypass) and not the wyhash-collision filename case.
Level of scrutiny
Medium-high. The mechanical change is small and easy to verify — swap the seed argument at each Wyhash::hash call site, drop three != 0 guards, add one sourcemap check, bump a version constant. I traced all three output-encoding arms plus the sourcemap and esm_record sections and confirmed save/load are symmetric. The LATIN1 arm's reordering (length check now precedes the hash) is a small correctness improvement. SEED and the free hash() function are still used for input_hash and features_hash, so no dead code was introduced. But because this sits on the boundary between "cached bytes on disk" and "bytes we execute", it warrants a human look rather than a bot approval.
Other factors
Tests are solid: the two new cases exercise both the zeroed-hash and forged-fixed-seed-hash paths and assert the original output is printed (i.e., the entry was rejected and re-transpiled), and the first case additionally asserts a fresh entry with a nonzero hash replaced the rejected one. I confirmed Bun.hash.wyhash (used in the updated existing test to forge a passing esm_record_hash) is backed by the same bun_wyhash::Wyhash implementation the loader uses (src/runtime/api/HashObject.rs:49), so that test will still reach the module-record deserializer it was written to cover. The tamperOutput helper's returned subarray view is used only to compute a hash over already-mutated in-memory bytes before a fresh re-read from disk — no aliasing hazard. CI is still building; no results yet.
…oot into bun_sys Section hashes (output / sourcemap / esm_record) are now seeded with `input_hash` instead of the fixed SEED, so a forged payload whose hash was recomputed with the constant seed is still rejected. Adopts the approach from #35101 and carries its test over. `is_trusted_cache_root` is moved into bun_sys as the single implementation for both this cache and bunx, and the lstat error arm is tightened so only ENOENT is trusted (the caller will create the directory); every other errno fails closed.
|
A superset of the verification part of this change is in #39717 (header hash, exact layout check against the file size, all section hashes checked, regular file gate). This branch conflicts with main now. |
The on-disk runtime transpiler cache verified each payload section (output, sourcemap, esm_record) only against its own self-declared hash in the entry header, and skipped the check entirely when that hash was zero. The sourcemap section was never verified at all. Because the cache is content-addressed by wyhash of the source bytes alone, any process that can arrange for the same source bytes to be transpiled (and can write the cache directory) can overwrite the entry's output and zero the hash field; the next reader of that source then executes the replaced output. Seed output_hash / sourcemap_hash / esm_record_hash with the entry's input_hash instead of the fixed seed, verify every section unconditionally on load (including the previously unchecked sourcemap), and bump the cache version so older entries are discarded. This does not make the cache a security boundary (wyhash is not a MAC); callers that must not trust a shared cache should set BUN_RUNTIME_TRANSPILER_CACHE_PATH=0.
1db27e2 to
fc4e436
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for adding the sourcemap tamper case in 2084d7b — that closes out my earlier coverage note, and no new issues turned up on this pass. I'm leaving this for a human because the author's own note above says this branch now conflicts with main and #39717 carries a superset of the verification logic; someone needs to decide whether to land this, rebase, or fold it into #39717.
Checked: write side (Entry::save) and all three read-side encoding arms use the same Wyhash::hash(input_hash, …) seed; the Latin-1 arm's reordered length-check-before-hash still runs under the errdefer scopeguard so the WTF string is deref'd on InvalidHash; the version bump to 26 discards pre-change entries so no stale-format entry reaches the new checks.
Extended reasoning...
Overview
Two files: src/jsc/RuntimeTranspilerCache.rs (seed section hashes with input_hash instead of the fixed SEED = 42, remove the != 0 skip on all three section-hash verifications, add the previously-missing sourcemap check, bump EXPECTED_VERSION 25 → 26) and test/cli/run/transpiler-cache.test.ts (three new tamper tests plus an update to the existing esm-record corruption test to recompute the hash under the new seed).
Security risks
The transpiler cache controls what bytes get handed to JSC as executable source, so this is security-adjacent by nature. The change strictly tightens verification (unconditional checks, input-bound seed) and the PR is explicit that wyhash is not a MAC and input_hash is derivable from the filename — this is corruption detection, not a trust boundary. I don't see a way for this diff to weaken the existing posture: the version bump ensures no old-seed entry survives, and the only removed conditional was !esm_record.is_empty() on the write side, which is now covered by hashing the empty slice (whose read side is gated on esm_record_byte_length > 0 anyway).
Level of scrutiny
Medium-high. The Rust diff is small and mechanical (same three-line pattern repeated across the UTF-8/Latin-1/UTF-16 output arms plus sourcemap and esm_record), and the scopeguard error-path cleanup is preserved in every arm — I traced the reordered Latin-1 arm specifically and errdefer still fires on the new InvalidHash return. But because a wrong hash comparison here silently discards every cache entry on every run (perf regression) or, worse, accepts a tampered one, and because the author flagged that main has diverged and #39717 already ships a superset (header hash, exact layout check, regular-file gate), a maintainer should decide the merge strategy rather than an automated approval.
Other factors
- My prior review's only finding (sourcemap check untested) was addressed in 2084d7b; the new test asserts the entry is rewritten with a nonzero
sourcemap_hash, which the author confirmed fails when the check is deleted. - The
hash()free function andSEEDconstant are still used byget()forinput_hashandfeatures_hash, so they're not dead. - CI on the earlier build was green for the touched test file across all lanes; the latest commit's build (#101539) status wasn't visible in the timeline.
- The open question of #39717 vs. this PR is a project-management call, not a code-correctness one.
|
Closing in favor of #39717. #39717 checks the same three section hashes unconditionally and also adds a header hash, a layout check against the file size, and a regular-file gate. It catches the zeroed-length and flipped-type shapes this PR does not. The one thing this PR does that #39717 does not is seed the section hashes with #35747 covers a different layer (per-uid cache root and ownership check) and is not replaced by either. |
2 participants
What
The runtime transpiler disk cache (
~/.bun/install/cache/@t@/*.pile) verified each payload section only against its own self-declared hash in the entry header, and skipped the check entirely when that field was zero. The sourcemap section was never verified at all.Why
The cache is content-addressed by
wyhash(source bytes)alone. Any process that can cause the same source bytes to be transpiled, and that can write the cache directory, can overwrite the entry's output section and zerooutput_hash; the next reader of that source then executes the replaced output verbatim.Repro shape
agent/copy.tsandgrader/grader.tscontain byte-identical source (≥ 4 KiB).bun run agent/copy.tswrites a legitimate cache entry keyed bywyhash(source)..pilefile's output section and zeroesoutput_hashat byte 46.bun run grader/grader.tsreads the same cache entry, seesoutput_hash == 0, skips verification, and executes the attacker's JS.Fix
output_hash/sourcemap_hash/esm_record_hashwith the entry'sinput_hashinstead of the fixed seed 42, so the stored hash binds each payload to the source bytes that keyed the entry.!= 0skips; added the missing sourcemap check).EXPECTED_VERSIONto 26 so existing entries are discarded.This is corruption detection, not a security boundary: wyhash is not a MAC and
input_hashis derivable from the cache filename. Callers that must not trust a shared cache directory should setBUN_RUNTIME_TRANSPILER_CACHE_PATH=0.Tests
test/cli/run/transpiler-cache.test.tsgains three cases underrejects tampered entries. Two tamper the output section: one zeroesoutput_hash, one writeswyhash(42, tampered_output). Both printTAMPERED_OUTPUT_*on 1.4.0 andORIGINAL_OUTPUT_*with this change. The third overwrites the sourcemap payload and zeroessourcemap_hash. On 1.4.0 the entry is accepted and the stored hash stays 0. With this change the entry is rejected and rewritten with a real hash.The existing out-of-range-string-index test is updated to recompute
esm_record_hashunder the new seed so it still reaches the deserializer it covers.[review] gate passed · iteration 4 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 4
evidence per changed file