Conversation
Bun.CryptoHasher.hash, Bun.SHA*.hash, Bun.sha, Bun.randomUUIDv5 and Bun.indexOfLine all captured an ArrayBuffer-backed input slice and then coerced a later argument that can run JS (a boxed String's toString or an object's valueOf). That JS can transfer(0) the input's backing ArrayBuffer and spray a same-size allocation, leaving the captured ptr/len pointing at a recycled foreign block. The digest/scan then runs over freed memory. For the hash entry points, add StringOrBuffer::refresh_buffer and re-snapshot the input's ArrayBuffer from its JSValue after the output argument has been coerced, so a detached input is observed as length 0. For randomUUIDv5 and indexOfLine, decode the later argument (namespace / offset) before snapshotting the buffer; the namespace result is copied into a local [u8; 16] and offset is a plain integer, so the reverse ordering is not exposed to the same hazard.
WalkthroughChangesThe PR updates argument coercion order and boxed-string handling to prevent user-code-driven Buffer-detachment safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:11 PM PT - Jul 21st, 2026
✅ @autofix-ci[bot], your commit 3557f2a216c532d33461da839298d882a4ae8ce5 passed in 🧪 To try this PR locally: bunx bun-pr 34964That installs a local version of the PR into your bun-34964 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Same pattern as the hash entry points: password's ArrayBuffer is snapshotted via StringOrBuffer::from_js, then the hash argument is coerced (a boxed String's toString can detach the password buffer), then password.slice() is read. Re-snapshot password after the hash argument has been decoded. Also tighten the new tests: trim comments to the invariant being asserted and check the specific error message in the reverse-direction case.
There was a problem hiding this comment.
Thanks — all three items from my earlier pass are addressed in 8dda462. I didn't find anything further, but since this is a native memory-safety fix touching crypto argument marshaling across five files, I'll leave the final sign-off to a human.
What was reviewed:
refresh_buffercorrectly re-readsArrayBufferfields viaas_array_bufferon the storedJSValue; the else-branch zeroes ptr/len/byte_len for the None case, andMarkedArrayBuffer::{owns_buffer,pinned}are preserved.randomUUIDv5reorder: confirmedJSValue::is_string()isis_string_like()(includesStringObject), soto_bun_stringon a boxed namespace does invoke usertoString; namespace copied to local[u8;16]makes the reverse direction safe. Error-message ordering changes only when both name and namespace are invalid — no existing test asserts that.indexOfLinereorder: existing "handles non-number offset" tests still hold ({}/"2"coerce without side effects before the buffer is read).
Extended reasoning...
Overview
This PR closes a use-after-free bug class where several Bun.* entry points snapshot an ArrayBuffer-backed input as a raw (ptr, len) and only afterwards coerce a later argument that can run user JS (a boxed String's toString, or valueOf via coerce_to_int64). That JS can transfer(0) the input's backing store and spray a same-size allocation, leaving the captured slice pointing at foreign memory. The fix adds StringOrBuffer::refresh_buffer / BlobOrStringOrBuffer::refresh_buffer (re-snapshot the Buffer variant from its stored JSValue) and applies it after the later-arg coercion in CryptoHasher::hash, StaticCryptoHasher::hash, the Bun.sha adapter, and Bun.password.verifySync; Bun.randomUUIDv5 and Bun.indexOfLine are fixed by reordering the coercion before the buffer snapshot.
Security risks
The PR removes a UAF; the concern is whether the fix itself introduces anything. refresh_buffer calls JSValue::as_array_buffer, which is a pure C++ field read (no JS re-entry), and overwrites only the ArrayBuffer sub-struct of MarkedArrayBuffer (which is Copy; .value is preserved because as_array_buffer writes out.value = self). The reorders in randomUUIDv5/indexOfLine change only error-path observable ordering for Bun-native APIs. No new attack surface is added.
Level of scrutiny
High — native memory safety in crypto-adjacent code. I traced each call site's coercion order (including the reverse direction: input's toString detaching the output buffer, which was already safe and is now pinned by a test), verified is_string() ⇒ is_string_like() so the boxed-String hazard is real for randomUUIDv5, checked that js_password_object_hash_sync and CryptoHasher::update decode their later arg before snapshotting the input (already safe, as the PR description states), and confirmed the existing indexOfLine non-number-offset tests still hold under the new ordering.
Other factors
All three findings from my earlier review were addressed in 8dda462: the missed verifySync sibling now calls refresh_buffer with a matching test, the 4-line before/after comments were trimmed to the invariant, and the reverse-direction test's bare .toThrow() now asserts /TypedArray must be at least 32 bytes/. Tests are added in existing files per CLAUDE.md conventions and assert against the empty-input digest (the correct post-detach observable). Given the guideline to defer on security-sensitive/crypto paths even when the change is mechanical, I'm leaving final approval to a maintainer rather than shadow-approving.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This approach is incorrect. We should do the arg coercion first.
|
Reordering so the later argument is coerced before the input buffer is captured fixes the reported direction cleanly. The wrinkle is the reverse: with output decoded first and input decoded second, a boxed I can see two ways to keep both directions safe without a post-coercion re-snapshot:
Happy to go with either; I'll default to (1) since it's the smaller change and aligns with Node, unless you'd prefer (2) or are fine leaving the reverse case as-is. |
Replace the post-coercion re-snapshot with a decode order that runs all user JS before any ArrayBuffer slice is captured: * Bun.CryptoHasher.hash / Bun.SHA*.hash / Bun.sha: decode the output argument first, then the input via BlobOrStringOrBuffer::from_js_no_string_object so the input decode never calls user toString and the already-captured output buffer stays valid. * Bun.password.verifySync: decode the hash argument first, then the password with allow_string_object = false. * Bun.randomUUIDv5 / Bun.indexOfLine were already reordered in the previous commits. BlobOrStringOrBuffer::from_js_maybe_file_maybe_async now takes allow_string_object and from_js_no_string_object is added as a convenience wrapper. Boxed String inputs to the hash/verifySync entry points are now rejected (matching Node's ERR_INVALID_ARG_TYPE for crypto.Hash#update).
|
Reworked in 03d272c: the output/hash/namespace/offset argument is coerced first, then the input buffer is captured last, and the post-coercion re-snapshot helper is gone. To keep the reverse direction safe (a boxed |
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/crypto/CryptoHasher.rs (1)
1219-1258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect ordering, but duplicated across 3 call sites.
Same "coerce output first, then decode input via
from_js_no_string_object" pattern is repeated verbatim here, inCryptoHasher::hash(lines 264-291), and inBunObject.rs'ssha(). Extracting a shared helper (e.g.decode_hash_args(global, input_value, output_value) -> JsResult<(BlobOrStringOrBuffer, Option<StringOrBuffer>)>) would reduce the risk that a future hasher API forgets the ordering and reintroduces this UAF class.♻️ Sketch of a shared helper
fn decode_hash_input_output( global: &JSGlobalObject, input_value: JSValue, output_value: JSValue, ) -> JsResult<(BlobOrStringOrBuffer, Option<StringOrBuffer>)> { let output = if output_value.is_undefined_or_null() { None } else { StringOrBuffer::from_js(global, output_value)? }; let Some(input) = BlobOrStringOrBuffer::from_js_no_string_object(global, input_value)? else { return Err(global.throw_invalid_arguments(format_args!("expected blob, string or buffer"))); }; Ok((input, output)) }🤖 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 `@src/runtime/crypto/CryptoHasher.rs` around lines 1219 - 1258, Extract the repeated output-first/input-second decoding logic into a shared helper such as decode_hash_input_output, reusing the existing StringOrBuffer and BlobOrStringOrBuffer conversions and invalid-argument handling. Update CryptoHasher::hash and the other CryptoHasher hash call site plus BunObject::sha to call this helper, preserving the ordering that keeps output coercion ahead of input decoding.
🤖 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.
Outside diff comments:
In `@src/runtime/crypto/CryptoHasher.rs`:
- Around line 1219-1258: Extract the repeated output-first/input-second decoding
logic into a shared helper such as decode_hash_input_output, reusing the
existing StringOrBuffer and BlobOrStringOrBuffer conversions and
invalid-argument handling. Update CryptoHasher::hash and the other CryptoHasher
hash call site plus BunObject::sha to call this helper, preserving the ordering
that keeps output coercion ahead of input decoding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1ffcdb4-2dd7-48a3-ae6c-23c9ff6e09aa
📒 Files selected for processing (9)
src/runtime/api/BunObject.rssrc/runtime/crypto/CryptoHasher.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/node/types.rssrc/runtime/webcore/Crypto.rstest/js/bun/util/bun-cryptohasher.test.tstest/js/bun/util/index-of-line.test.tstest/js/bun/util/password.test.tstest/js/bun/util/randomUUIDv5.test.ts
There was a problem hiding this comment.
The rework in 03d272c looks correct — I traced both directions (later-arg toString detaching the input, and boxed-String input detaching the output) through all five entry points and each buffer is now captured after every user-JS coercion. Deferring only because the boxed-String-input rejection is a user-visible API change that the author explicitly flagged for maintainer preference (option 1 vs 2) and proceeded on the default; a quick sign-off on that trade-off would be good.
What was reviewed:
- Reordered decode in
CryptoHasher::hash/StaticCryptoHasher::hash/Bun.sha/verifySync/randomUUIDv5/indexOfLine: input buffer is captured last and its decode (from_js_no_string_object/allow_string_object=false) never runs JS, so the already-captured output/hash buffer stays valid. randomUUIDv5reverse direction: namespace is copied into a local[u8;16]before name decode, so a boxed-String name detaching the namespace buffer is harmless.arguments_old→arguments_undefrefactor preserves the "Missing argument" vs "expected blob, string or buffer" error ordering;arguments.lengates everyptr[i]access.verifySyncnow validateshashbeforepassword; checked existingpassword.test.tscoercion tests — none assert which arg name appears first, so they still pass.
Extended reasoning...
Overview
Memory-safety fix across five Rust host functions (CryptoHasher::hash, StaticCryptoHasher::hash, Bun.sha, Bun.password.verifySync, Bun.randomUUIDv5) plus Bun.indexOfLine: each previously snapshotted an ArrayBuffer-backed input as a raw (ptr, len) and then coerced a later argument that can run user JS (StringOrBuffer::from_js on a boxed String, coerce_to_int64's valueOf), letting that JS detach the input's backing store. The fix reorders decoding so the JS-running coercion happens first and the input buffer is captured last, and — to keep the reverse direction safe — the input decode now uses allow_string_object=false so it never runs user JS. BlobOrStringOrBuffer::from_js_maybe_file_maybe_async gains an allow_string_object parameter with a from_js_no_string_object convenience wrapper. Four new regression tests exercise the detach-during-coercion path.
This is the third revision; my three earlier findings (missing verifySync fix, 4-line comments, bare .toThrow()) were addressed in 8dda462, then the approach was reworked in 03d272c from "re-snapshot after coercion" to "coerce first, capture last, reject boxed-String inputs". I re-reviewed the rework end-to-end.
Security risks
The bug being fixed is a use-after-free reading recycled bmalloc memory into a hash digest / password comparison / UUID / line search — reachable from any JS that can pass a boxed String with an overridden toString. The fix closes it by ensuring no raw buffer pointer is held across a call that can enter JS. I verified from_js_no_string_object → from_js_maybe_async_into(.., allow_string_object=false) returns Ok(false) for StringObject/DerivedStringObject before ever calling String::from_js, so the input decode genuinely never runs user JS. Primitive-string inputs still go through String::from_js, which on JSType::String reads the WTF string directly without invoking JS.
Level of scrutiny
High — memory safety in crypto/native bindings. The reorder itself is mechanical, but it composes with a next_eat-closure → arguments_undef refactor in CryptoHasher.rs and swaps validation order in verifySync, so I traced argument-count edge cases (len==0, len==1) and confirmed is_string() = is_string_like() (includes StringObject) so the randomUUIDv5 test's boxed-String namespace does reach to_bun_string.
Other factors
The one thing I'd want a maintainer to glance at is the API-surface trade-off: boxed String inputs to Bun.CryptoHasher.hash/Bun.SHA*.hash/Bun.sha/Bun.password.verifySync are now rejected with "expected string or buffer" (matching Node's ERR_INVALID_ARG_TYPE for Hash#update). The author explicitly surfaced two options and asked for a preference before defaulting to this one; nobody weighed in. It's the smaller and Node-aligned choice, but it's still a behavior change on Bun-native APIs that deserves a human ack rather than a bot approval.
|
Superseded by #35757, which consolidates all four sites into one change: |
This branch has not been deployed
2 participants
Problem
Bun.CryptoHasher.hash,Bun.SHA*.hash/Bun.MD5.hash,Bun.sha,Bun.password.verifySync,Bun.randomUUIDv5andBun.indexOfLineall snapshot an ArrayBuffer-backed input as a raw(ptr, len)and only afterwards coerce a later argument that can run JS:StringOrBuffer::from_js(allow_string_object=true)calls a boxedString'stoString, andcoerce_to_int64callsvalueOf. That JS caninput.buffer.transfer(0)and spray a same-size allocation, after which the captured slice points at a recycled foreign heap block. ASan does not flag this because the backing store lives in bmalloc.Fix
Coerce the later argument before capturing the input buffer's slice:
Bun.CryptoHasher.hash/Bun.SHA*.hash/Bun.sha: decode the output argument first, then the input viaBlobOrStringOrBuffer::from_js_no_string_objectso the input decode never calls usertoStringand the already-captured output buffer stays valid.Bun.password.verifySync: decode the hash argument first, then the password withallow_string_object = false.Bun.randomUUIDv5: decode the namespace before the name; the namespace is copied into a local[u8; 16].Bun.indexOfLine: coerceoffsetbefore callingas_array_bufferon the first argument.BlobOrStringOrBuffer::from_js_maybe_file_maybe_asyncnow takesallow_string_objectandfrom_js_no_string_objectis added as a convenience wrapper. BoxedStringinputs to the hash/verifySync entry points are now rejected with "expected string or buffer" (matching Node'sERR_INVALID_ARG_TYPEforcrypto.Hash#update); primitive string inputs are unchanged.CryptoHasher#update(input, encoding)andBun.password.hashSync(password, algorithm)were audited and already decode the later argument before capturing the input buffer.Tests
New tests in
test/js/bun/util/bun-cryptohasher.test.ts,test/js/bun/util/password.test.ts,test/js/bun/util/randomUUIDv5.test.tsandtest/js/bun/util/index-of-line.test.tsdetach the input during the later argument's coercion and assert the result matches the detached (empty) input rather than the recycled spray. Each test fails on current main and passes with this change.[review] gate passed · iteration 1 · 9 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file