transpiler cache: always verify output/sourcemap/esm_record hashes on load - #35102
transpiler cache: always verify output/sourcemap/esm_record hashes on load#35102robobun wants to merge 1 commit into
Conversation
WalkthroughChangesThe transpiler cache now always validates hashes for cached output code, sourcemaps, and ES module records. Tests cover zeroed output hashes and corrupted module-record indices. Transpiler cache integrity
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
… load Entry::load() skipped the output/esm_record integrity check when the stored hash field was zero, and never verified the sourcemap section at all. An entry's input_hash, input_byte_length and features_hash are all derivable from the public source bytes, so whoever can write a .pile at the expected cache path can prepend arbitrary code to the cached output and zero output_hash to have it accepted unchecked on the next run. Entry::save() has always written all three hashes, so existing on-disk entries already carry the values the loader now compares against.
816e836 to
66c2907
Compare
|
Linked #32741 in the PR body as related but not auto-closing: that issue covers several distinct wyhash-collision vectors (trustedDependencies, scoped-registry, patch cache, MySQL prepared statements, and PoC6's transpiler-cache |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing in favor of #35101, which was opened first and is a superset of this change: it removes the same |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/transpiler-cache.test.ts`:
- Around line 364-367: Shorten the comment above the esm_record hash derivation
to three lines or fewer while preserving that it recomputes the hash so the
corrupted record passes integrity validation and reaches deserialization, using
the same wyhash variant and seed as Bun.hash.wyhash.
- Around line 283-291: Shorten the explanatory comment above the transpiler
cache test to three lines or fewer while preserving the essential security
rationale: a zero stored output hash must still trigger verification of cached
output. Keep the issue reference and remove the extended attack-scenario
details.
- Around line 282-330: Update the regression test around the first and second
bunRun calls in rejects a cache entry whose output_hash is zeroed to assert that
both successful runs have exactly empty stderr, matching this file’s bunRun test
convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ea6c2a3-11b4-4bdc-a6d4-166eeb5fe73b
📒 Files selected for processing (2)
src/jsc/RuntimeTranspilerCache.rstest/cli/run/transpiler-cache.test.ts
| test("rejects a cache entry whose output_hash is zeroed", () => { | ||
| // https://github.com/oven-sh/bun/issues/2829 | ||
| // | ||
| // The on-disk transpiler cache entry stores a wyhash of the transpiled | ||
| // output in the header. On load the hash of the output bytes is compared | ||
| // against that stored value. A stored hash of zero must not skip that | ||
| // check: anyone who can write to the cache directory can compute the | ||
| // cache filename for a known source file, prepend arbitrary code to the | ||
| // cached output, and zero the stored output_hash so the altered output | ||
| // is accepted without being re-verified. | ||
|
|
||
| // A >=4 KiB source file so it is eligible for the cache. | ||
| writeFileSync(join(temp_dir, "a.js"), dummyFile(50 * 1024, "output-hash-zero", "clean")); | ||
|
|
||
| // First run transpiles the file and writes the cache entry. | ||
| const first = bunRun(join(temp_dir, "a.js"), env); | ||
| expect(first.stdout).toBe("clean"); | ||
| expect(newCacheCount()).toBe(1); | ||
| const pile = join(cache_dir, readdirSync(cache_dir)[0]); | ||
|
|
||
| // Rewrite the cached output: prepend a payload, then zero output_hash so the | ||
| // only integrity check on the output bytes is bypassed. | ||
| const data = readFileSync(pile); | ||
| const outOff = Number(data.readBigUInt64LE(OUTPUT_BYTE_OFFSET_AT)); | ||
| const outLen = Number(data.readBigUInt64LE(OUTPUT_BYTE_LENGTH_AT)); | ||
| const smOff = Number(data.readBigUInt64LE(SOURCEMAP_BYTE_OFFSET_AT)); | ||
| const smLen = Number(data.readBigUInt64LE(SOURCEMAP_BYTE_LENGTH_AT)); | ||
| const esmOff = Number(data.readBigUInt64LE(ESM_RECORD_BYTE_OFFSET_AT)); | ||
| const esmLen = Number(data.readBigUInt64LE(ESM_RECORD_BYTE_LENGTH_AT)); | ||
|
|
||
| const payload = Buffer.from('console.log("PLANTED");\n'); | ||
| const newOut = Buffer.concat([payload, data.subarray(outOff, outOff + outLen)]); | ||
| const sm = data.subarray(smOff, smOff + smLen); | ||
| const esm = data.subarray(esmOff, esmOff + esmLen); | ||
|
|
||
| const header = Buffer.from(data.subarray(0, METADATA_SIZE)); | ||
| header.writeBigUInt64LE(BigInt(METADATA_SIZE), OUTPUT_BYTE_OFFSET_AT); | ||
| header.writeBigUInt64LE(BigInt(newOut.length), OUTPUT_BYTE_LENGTH_AT); | ||
| header.writeBigUInt64LE(0n, OUTPUT_HASH_AT); | ||
| header.writeBigUInt64LE(BigInt(METADATA_SIZE + newOut.length), SOURCEMAP_BYTE_OFFSET_AT); | ||
| header.writeBigUInt64LE(BigInt(METADATA_SIZE + newOut.length + sm.length), ESM_RECORD_BYTE_OFFSET_AT); | ||
| writeFileSync(pile, Buffer.concat([header, newOut, sm, esm])); | ||
|
|
||
| // Second run must not execute the planted payload. The entry's integrity | ||
| // check fails, so the cache entry is discarded and the source file is | ||
| // re-transpiled from scratch. | ||
| const second = bunRun(join(temp_dir, "a.js"), env); | ||
| expect(second.stdout).toBe("clean"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Correct regression coverage; add empty-stderr assertions per this file's convention.
Logic correctly reproduces the CVE scenario and verifies rejection. However, first.stderr/second.stderr aren't asserted, unlike the established convention in this file.
Based on learnings from this exact file (PR 34721): "prefer asserting exact empty stderr for successful subprocess runs" for bunRun-based tests.
♻️ Proposed addition
const first = bunRun(join(temp_dir, "a.js"), env);
expect(first.stdout).toBe("clean");
+ expect(first.stderr).toBe("");
expect(newCacheCount()).toBe(1); const second = bunRun(join(temp_dir, "a.js"), env);
expect(second.stdout).toBe("clean");
+ expect(second.stderr).toBe("");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("rejects a cache entry whose output_hash is zeroed", () => { | |
| // https://github.com/oven-sh/bun/issues/2829 | |
| // | |
| // The on-disk transpiler cache entry stores a wyhash of the transpiled | |
| // output in the header. On load the hash of the output bytes is compared | |
| // against that stored value. A stored hash of zero must not skip that | |
| // check: anyone who can write to the cache directory can compute the | |
| // cache filename for a known source file, prepend arbitrary code to the | |
| // cached output, and zero the stored output_hash so the altered output | |
| // is accepted without being re-verified. | |
| // A >=4 KiB source file so it is eligible for the cache. | |
| writeFileSync(join(temp_dir, "a.js"), dummyFile(50 * 1024, "output-hash-zero", "clean")); | |
| // First run transpiles the file and writes the cache entry. | |
| const first = bunRun(join(temp_dir, "a.js"), env); | |
| expect(first.stdout).toBe("clean"); | |
| expect(newCacheCount()).toBe(1); | |
| const pile = join(cache_dir, readdirSync(cache_dir)[0]); | |
| // Rewrite the cached output: prepend a payload, then zero output_hash so the | |
| // only integrity check on the output bytes is bypassed. | |
| const data = readFileSync(pile); | |
| const outOff = Number(data.readBigUInt64LE(OUTPUT_BYTE_OFFSET_AT)); | |
| const outLen = Number(data.readBigUInt64LE(OUTPUT_BYTE_LENGTH_AT)); | |
| const smOff = Number(data.readBigUInt64LE(SOURCEMAP_BYTE_OFFSET_AT)); | |
| const smLen = Number(data.readBigUInt64LE(SOURCEMAP_BYTE_LENGTH_AT)); | |
| const esmOff = Number(data.readBigUInt64LE(ESM_RECORD_BYTE_OFFSET_AT)); | |
| const esmLen = Number(data.readBigUInt64LE(ESM_RECORD_BYTE_LENGTH_AT)); | |
| const payload = Buffer.from('console.log("PLANTED");\n'); | |
| const newOut = Buffer.concat([payload, data.subarray(outOff, outOff + outLen)]); | |
| const sm = data.subarray(smOff, smOff + smLen); | |
| const esm = data.subarray(esmOff, esmOff + esmLen); | |
| const header = Buffer.from(data.subarray(0, METADATA_SIZE)); | |
| header.writeBigUInt64LE(BigInt(METADATA_SIZE), OUTPUT_BYTE_OFFSET_AT); | |
| header.writeBigUInt64LE(BigInt(newOut.length), OUTPUT_BYTE_LENGTH_AT); | |
| header.writeBigUInt64LE(0n, OUTPUT_HASH_AT); | |
| header.writeBigUInt64LE(BigInt(METADATA_SIZE + newOut.length), SOURCEMAP_BYTE_OFFSET_AT); | |
| header.writeBigUInt64LE(BigInt(METADATA_SIZE + newOut.length + sm.length), ESM_RECORD_BYTE_OFFSET_AT); | |
| writeFileSync(pile, Buffer.concat([header, newOut, sm, esm])); | |
| // Second run must not execute the planted payload. The entry's integrity | |
| // check fails, so the cache entry is discarded and the source file is | |
| // re-transpiled from scratch. | |
| const second = bunRun(join(temp_dir, "a.js"), env); | |
| expect(second.stdout).toBe("clean"); | |
| }); | |
| test("rejects a cache entry whose output_hash is zeroed", () => { | |
| // https://github.com/oven-sh/bun/issues/2829 | |
| // | |
| // The on-disk transpiler cache entry stores a wyhash of the transpiled | |
| // output in the header. On load the hash of the output bytes is compared | |
| // against that stored value. A stored hash of zero must not skip that | |
| // check: anyone who can write to the cache directory can compute the | |
| // cache filename for a known source file, prepend arbitrary code to the | |
| // cached output, and zero the stored output_hash so the altered output | |
| // is accepted without being re-verified. | |
| // A >=4 KiB source file so it is eligible for the cache. | |
| writeFileSync(join(temp_dir, "a.js"), dummyFile(50 * 1024, "output-hash-zero", "clean")); | |
| // First run transpiles the file and writes the cache entry. | |
| const first = bunRun(join(temp_dir, "a.js"), env); | |
| expect(first.stdout).toBe("clean"); | |
| expect(first.stderr).toBe(""); | |
| expect(newCacheCount()).toBe(1); | |
| const pile = join(cache_dir, readdirSync(cache_dir)[0]); | |
| // Rewrite the cached output: prepend a payload, then zero output_hash so the | |
| // only integrity check on the output bytes is bypassed. | |
| const data = readFileSync(pile); | |
| const outOff = Number(data.readBigUInt64LE(OUTPUT_BYTE_OFFSET_AT)); | |
| const outLen = Number(data.readBigUInt64LE(OUTPUT_BYTE_LENGTH_AT)); | |
| const smOff = Number(data.readBigUInt64LE(SOURCEMAP_BYTE_OFFSET_AT)); | |
| const smLen = Number(data.readBigUInt64LE(SOURCEMAP_BYTE_LENGTH_AT)); | |
| const esmOff = Number(data.readBigUInt64LE(ESM_RECORD_BYTE_OFFSET_AT)); | |
| const esmLen = Number(data.readBigUInt64LE(ESM_RECORD_BYTE_LENGTH_AT)); | |
| const payload = Buffer.from('console.log("PLANTED");\n'); | |
| const newOut = Buffer.concat([payload, data.subarray(outOff, outOff + outLen)]); | |
| const sm = data.subarray(smOff, smOff + smLen); | |
| const esm = data.subarray(esmOff, esmOff + esmLen); | |
| const header = Buffer.from(data.subarray(0, METADATA_SIZE)); | |
| header.writeBigUInt64LE(BigInt(METADATA_SIZE), OUTPUT_BYTE_OFFSET_AT); | |
| header.writeBigUInt64LE(BigInt(newOut.length), OUTPUT_BYTE_LENGTH_AT); | |
| header.writeBigUInt64LE(0n, OUTPUT_HASH_AT); | |
| header.writeBigUInt64LE(BigInt(METADATA_SIZE + newOut.length), SOURCEMAP_BYTE_OFFSET_AT); | |
| header.writeBigUInt64LE(BigInt(METADATA_SIZE + newOut.length + sm.length), ESM_RECORD_BYTE_OFFSET_AT); | |
| writeFileSync(pile, Buffer.concat([header, newOut, sm, esm])); | |
| // Second run must not execute the planted payload. The entry's integrity | |
| // check fails, so the cache entry is discarded and the source file is | |
| // re-transpiled from scratch. | |
| const second = bunRun(join(temp_dir, "a.js"), env); | |
| expect(second.stdout).toBe("clean"); | |
| expect(second.stderr).toBe(""); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/cli/run/transpiler-cache.test.ts` around lines 282 - 330, Update the
regression test around the first and second bunRun calls in rejects a cache
entry whose output_hash is zeroed to assert that both successful runs have
exactly empty stderr, matching this file’s bunRun test convention.
Source: Learnings
What
Entry::load()compared the cached output againstmetadata.output_hashonly when the stored value was non-zero:The same
!= 0guard applied toesm_record_hash, andsourcemapwas never verified at all.Why it matters
A
.pileentry is addressed byhex(wyhash(source)), and its binding to the input isinput_hash+input_byte_length+features_hash, all of which are derivable from the public source bytes and a copy of Bun. The only check on the transpiled output itself isoutput_hash, and a stored zero disabled it.Anyone who can write to the transpiler cache directory can therefore take any known source file (for example a widely-used
node_modulesmodule), compute its cache path, prepend a payload to the cached output, zerooutput_hash, and have that payload execute the next time the genuine source is run.Reproduction (lands in
test/cli/run/transpiler-cache.test.ts):Before: stdout is
PLANTED\nclean.After: the entry fails
InvalidHash, is unlinked, the source is re-transpiled, stdout isclean.Fix
Entry::save()has always writtenoutput_hash,sourcemap_hash, and (for non-empty records)esm_record_hash, so the loader can compare unconditionally:!= 0guard onoutput_hashin all three encoding branches!= 0guard onesm_record_hashsourcemap_hashwhensourcemap_byte_length > 0No format change and no version bump: every entry a released Bun has written already carries the values the loader now compares against.
The existing out-of-range-esm-record-index test previously relied on zeroing
esm_record_hashto reach the deserializer; it now recomputes the hash withBun.hash.wyhash(bytes, 42n)so the corrupted record still gets that far.Scope
This removes the zero-hash bypass. A writer who controls the cache directory can still forge a matching wyhash, so a per-user keyed hash and an ownership/mode check on the
@t@directory (asbunxdoes for its temp cache) remain as follow-up hardening. Related: #32741 (this addresses the zero-hash shortcut; theinput_hashcollision in PoC6 and the keyed-hash hardening are separate).