ffi, napi: purify NaNs before NaN-boxing doubles from native code - #32787
Conversation
JSC's JSVALUE64 encoding requires every boxed double to be either a non-NaN or the canonical quiet NaN (PureNaN.h). bun:ffi encoded native doubles with no purification, so a C function with a declared f64/f32 return, or a JSCallback taking an f64 argument, could hand JavaScript a NaN whose payload decodes as an arbitrary JSValue: true, undefined, an Int32, or a cell pointer whose address comes from native data. The same hole existed in read.f64/read.f32 and napi_create_double. - DOUBLE_TO_JSVALUE (FFI.h): canonicalize NaNs before adding DoubleEncodeOffset, mirroring JSC's purifyNaN(). Covers f64/f32 returns and JSCallback double arguments. - JSVALUE_TO_DOUBLE (FFI.h): decode int32-tagged JSValues. Integral JS numbers passed to f64/f32 arguments, and integral numbers returned from an f64 JSCallback, previously reached C as an impure NaN. The bug was invisible on JS -> C -> JS round trips because the missing purification re-encoded that NaN back into the original int32. - read.f64/read.f32: purify through the new JSValue::purify_nan. - napi_create_double: jsNumber(purifyNaN(value)), matching JSC's own JSValueMakeNumber. The ffi.test.fixture.*.c files are the committed viewSource dumps and are regenerated with the new header.
WalkthroughThe PR adds NaN purification to JSValue and FFI conversion paths, updates double encoding and decoding to use a canonical NaN pattern and correct int32 handling, and expands FFI tests and fixtures. ChangesNaN boxing across FFI boundaries
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review; the diff is green and the one red CI lane is unrelated (details below). Reproduced on 1.4.0-canary (942c222) with the cc() snippet in the description: an f64 return whose bits are 0xfffe000000000007 arrives in JS as CI on the latest push (build 65145): every lane is green except debian-13-x64-asan, where the only failing test is |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Checked #11844 and it is not fixed by this PR, so I am not adding the auto-close line. That issue's symbols only use |
![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 is a security fix touching JSValue NaN-boxing at the FFI/napi boundary plus a behavioral change to JSVALUE_TO_DOUBLE's int32 handling — worth a human pass to confirm the encoding invariants and that no other call sites depended on the old (cancelling) behavior.
Extended reasoning...
Overview
This PR fixes a JSValue forgery vulnerability where bun:ffi and napi_create_double NaN-box native doubles without purifying impure NaN payloads, allowing native code to forge arbitrary JSValues (immediates, Int32s, or cell pointers — i.e., type confusion / arbitrary read). It touches:
src/runtime/ffi/FFI.h: adds NaN purification toDOUBLE_TO_JSVALUEand int32-tag handling toJSVALUE_TO_DOUBLE(this header is compiled into every FFI call/callback trampoline)src/jsc/bindings/napi.cpp:napi_create_doublenow callspurifyNaN()src/runtime/ffi/FFIObject.rs:read.f64/read.f32purify via a newJSValue::purify_nanhelper insrc/jsc/JSValue.rs- Two regenerated
viewSourcefixture dumps and ~240 lines of new regression tests incc.test.ts
Security risks
The change closes a security hole rather than opening one. The fix follows JSC's own pattern (JSValueMakeNumber, typed-array adaptors). I don't see new attack surface introduced. However, because it sits exactly at the JSValue encoding boundary, any mistake here would itself be security-relevant — e.g., if the int32 decode path or the PureNaN constant were wrong, it could mis-encode values. The constants and logic look correct (0x7ff8000000000000 matches JSC's PNaNAsBits; the int32 branch mirrors JSVALUE_TO_INT64).
Level of scrutiny
High. This is low-level engine-boundary code where the JSVALUE64 tag encoding must be exactly right, and the PR itself documents that two pre-existing bugs were silently cancelling each other on round trips — which is precisely the kind of subtle invariant that benefits from a second pair of eyes. The JSVALUE_TO_DOUBLE change is also a real behavioral change for every FFI f64/f32 argument and JSCallback return; while clearly correct in isolation, a maintainer should confirm nothing else was relying on the old broken behavior.
Other factors
- Excellent test coverage added (forged-payload purification across f64/f32 returns, JSCallback args,
read.f64/f32, and napi; C-side observers for the int32→double decode fix). - The author explicitly flags adjacent unfixed instances (
v8::Number::New, Postgres/MySQL float8 decoders,UINT32_TO_JSVALUEoff-by-one) — a maintainer may want to weigh in on whether those should land together or be tracked separately. - CI was still building at the time of the last timeline entry.
- No CODEOWNERS match these paths, but FFI/napi/JSC bindings are core-runtime territory.
Same boundary as napi_create_double: the addon-supplied f64 is NaN-boxed into the Date constructor's argument JSValue before any timeClip runs, so an impure NaN payload decodes as a forged JSValue.
There was a problem hiding this comment.
Thanks for folding in the napi_create_date fix — that resolves my earlier comment. No further issues found, but since this changes JSValue encode/decode behavior on the FFI hot path and the napi boundary (security-relevant type-confusion surface), I'd like a human to sign off.
Extended reasoning...
Overview
This PR closes a JSValue-forgery primitive: bun:ffi and napi were NaN-boxing native f64 values without purifying impure NaNs, letting a well-typed C function (or napi addon) hand JS an arbitrary forged JSValue — including a cell pointer at an attacker-chosen address. The fix canonicalizes NaNs at every native→JS double boundary (DOUBLE_TO_JSVALUE in FFI.h, read.f64/f32, napi_create_double, napi_create_date), adds a JSValue::purify_nan helper, and fixes the inverse decode bug where int32-tagged JSValues passed to an f64 C argument arrived as NaN. ~260 lines of new regression tests in cc.test.ts cover forged payloads, the int32 decode, and the napi path; the two ffi.test.fixture.*.c files are regenerated viewSource dumps.
Security risks
This is itself a security hardening change. The risk profile is the inverse of a feature PR: the question is whether the new branches in DOUBLE_TO_JSVALUE / JSVALUE_TO_DOUBLE are exactly correct, since FFI.h is compiled into every FFI trampoline and any mistake here affects every f64/f32 argument and return. The val != val NaN check and the JSVALUE_IS_INT32 branch both mirror established JSC patterns (purifyNaN, asNumber), and the test matrix exercises ordinary doubles, the canonical NaN, and round-trips to confirm no regression. I see no new attack surface introduced.
Level of scrutiny
High. src/runtime/ffi/FFI.h is on the hot path of every bun:ffi call/callback, and napi.cpp / napi_body.rs sit at the native-addon boundary. The JSVALUE_TO_DOUBLE change is a behavior change (previously int32-tagged values became NaN; now they decode correctly), which the PR description argues is required to avoid breaking the round-trip once encode is purified — that reasoning is sound but worth a maintainer's confirmation. A human familiar with the FFI trampoline codegen should glance at this.
Other factors
My one prior inline comment (napi_create_date was missed) was addressed in f2a54fe with test coverage, and the author swept the rest of the napi surface. No CODEOWNERS apply to these paths. CI build #65145 was still in progress at last update. The PR explicitly defers v8::Number::New and the Postgres/MySQL float8 decoders to follow-ups, which is reasonable scoping.
…re (#33824) Fixes #33823 ### Problem `bun:sql` boxes the raw 8 bytes of a binary `float8`/`DOUBLE` column straight into a JSValue. A tagged-template or parameterized query automatically requests binary result format for numeric columns, so the bytes come from whatever sits on the other end of the socket (the database, a proxy, or a MITM on a plaintext connection). `jsNumber(double)` NaN-boxes by adding `DoubleEncodeOffset`. JSC's JSVALUE64 encoding is only sound when the boxed double is a non-NaN or the single canonical purified NaN. Any other NaN bit pattern collides with the tag ranges JSC uses for booleans, `undefined`, Int32, and cell pointers, so a server can make a `double precision` column come back as an arbitrary forged JSValue, or crash the process with a forged cell pointer. This is the same root cause already fixed for `bun:ffi`/N-API in #32787 and for `v8::Number::New` in #33072; that first PR explicitly called out the SQL decoders as a known, deferred gap. ``` panic(main thread): Segmentation fault at address 0x1234567D ``` ### Cause `src/jsc/bindings/SQLClient.cpp`, `DataCellTag::Double`: ```cpp return jsNumber(cell.value.number); ``` `cell.value.number` is produced by `f64::from_bits(...)` over the raw wire bytes (Postgres `parse_binary_float8`, MySQL `MYSQL_TYPE_DOUBLE`/`FLOAT`), with no validation or purification in between. Both producers feed this one sink. ### Fix Purify the NaN at the sink, mirroring `napi_create_double`: ```cpp return jsNumber(purifyNaN(cell.value.number)); ``` Purifying at the sink covers every `DataCellTag::Double` producer (Postgres and MySQL, `float8` and `float4`). Non-NaN doubles are untouched; any NaN bit pattern collapses to the canonical NaN. ### Verification New test `test/js/sql/postgres-binary-float-nan-box.test.ts` stands up a mock TCP server speaking the real Postgres v3 wire protocol and returns a binary `float8` column with forged bit patterns that previously decoded as `boolean true`, `undefined`, and an int32. With the fix each comes back as the number `NaN`; an ordinary double (`3.5`) round-trips unchanged. - Without the fix (`USE_SYSTEM_BUN=1`): the forged columns return `boolean`/`undefined`, assertions fail. - With the fix (`bun bd test`): 4 pass.
… forge a JSValue (#35435) ## What `File`/`Blob` store `lastModified` as a raw `f64`. The `lastModified` getter boxed that value with `JSValue::js_number(...)` without purifying NaN. The structured-clone deserialize path (`read_float`, a plain `f64::from_ne_bytes`) reads the value straight off the wire, so a crafted `v8.deserialize` / `bun:jsc.deserialize` buffer could plant an impure NaN. JSVALUE64 boxing is only sound for a non-NaN or the single canonical NaN. Any other NaN bit pattern collides with the tag ranges for `boolean`, `undefined`, `Int32`, and cell pointers, so `.lastModified` (a property that must always be a `Number`) decoded as a forged immediate or a forged cell pointer that segfaults when dereferenced. The constructor already mapped `NaN` to `0` (#33922), and the generic structured-clone number path already purifies (`jsNumber(purifyNaN(d))`), but the custom `File`/`Blob` tag getter skipped it. This is the same bug class and primitive as #33823/#33824 (`bun:sql` `float8`) in a sink those fixes did not cover. ## Repro (pre-fix) ```js import { serialize, deserialize } from "bun:jsc"; const sentinelBE = Buffer.from([0x3a, 0x1b, 0x2c, 0x3d, 0x4e, 0x5f, 0x60, 0x71]); const sentinel = sentinelBE.readDoubleBE(0); const sentinelLE = Buffer.alloc(8); sentinelLE.writeDoubleLE(sentinel, 0); const buf = Buffer.from(serialize(new File(["x"], "a.txt", { lastModified: sentinel }))); const at = buf.indexOf(sentinelLE); Buffer.from("0810000000feffff", "hex").copy(buf, at); // forged unmapped cell pointer const forged = deserialize(buf).lastModified; Object.prototype.toString.call(forged); // SEGV ``` ``` panic(main thread): Segmentation fault at address 0xFFFFFFFFFFFFFFFF ``` Under a debug + ASAN build the impure NaN trips `ASSERTION FAILED: !isImpureNaN(d)` at the moment `js_number` boxes it. ## Fix Purify every NaN at both `lastModified` boxing sites in `src/runtime/webcore/Blob.rs`, mirroring the sink-level fix used in #32787 and #33824. `JSValue::purify_nan` already exists. A deserialized `.lastModified` now always reads back as a `Number` (canonical NaN for a planted NaN), never a forged immediate or cell. ## Verification - New test in `test/js/web/structured-clone-blob-file.test.ts` forges `boolean`, `undefined`, `int32`, and cell-pointer payloads through both `bun:jsc.deserialize` and `v8.deserialize`, asserting each comes back as `number` NaN. It runs in a child process since the pre-fix build crashes. - Fails on the unfixed build (child aborts at the `!isImpureNaN` assert before producing output), passes with the fix. Closes #35434 <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/structured-clone-blob-file.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
… forge a JSValue (#35435) ## What `File`/`Blob` store `lastModified` as a raw `f64`. The `lastModified` getter boxed that value with `JSValue::js_number(...)` without purifying NaN. The structured-clone deserialize path (`read_float`, a plain `f64::from_ne_bytes`) reads the value straight off the wire, so a crafted `v8.deserialize` / `bun:jsc.deserialize` buffer could plant an impure NaN. JSVALUE64 boxing is only sound for a non-NaN or the single canonical NaN. Any other NaN bit pattern collides with the tag ranges for `boolean`, `undefined`, `Int32`, and cell pointers, so `.lastModified` (a property that must always be a `Number`) decoded as a forged immediate or a forged cell pointer that segfaults when dereferenced. The constructor already mapped `NaN` to `0` (#33922), and the generic structured-clone number path already purifies (`jsNumber(purifyNaN(d))`), but the custom `File`/`Blob` tag getter skipped it. This is the same bug class and primitive as #33823/#33824 (`bun:sql` `float8`) in a sink those fixes did not cover. ## Repro (pre-fix) ```js import { serialize, deserialize } from "bun:jsc"; const sentinelBE = Buffer.from([0x3a, 0x1b, 0x2c, 0x3d, 0x4e, 0x5f, 0x60, 0x71]); const sentinel = sentinelBE.readDoubleBE(0); const sentinelLE = Buffer.alloc(8); sentinelLE.writeDoubleLE(sentinel, 0); const buf = Buffer.from(serialize(new File(["x"], "a.txt", { lastModified: sentinel }))); const at = buf.indexOf(sentinelLE); Buffer.from("0810000000feffff", "hex").copy(buf, at); // forged unmapped cell pointer const forged = deserialize(buf).lastModified; Object.prototype.toString.call(forged); // SEGV ``` ``` panic(main thread): Segmentation fault at address 0xFFFFFFFFFFFFFFFF ``` Under a debug + ASAN build the impure NaN trips `ASSERTION FAILED: !isImpureNaN(d)` at the moment `js_number` boxes it. ## Fix Purify every NaN at both `lastModified` boxing sites in `src/runtime/webcore/Blob.rs`, mirroring the sink-level fix used in #32787 and #33824. `JSValue::purify_nan` already exists. A deserialized `.lastModified` now always reads back as a `Number` (canonical NaN for a planted NaN), never a forged immediate or cell. ## Verification - New test in `test/js/web/structured-clone-blob-file.test.ts` forges `boolean`, `undefined`, `int32`, and cell-pointer payloads through both `bun:jsc.deserialize` and `v8.deserialize`, asserting each comes back as `number` NaN. It runs in a child process since the pre-fix build crashes. - Fails on the unfixed build (child aborts at the `!isImpureNaN` assert before producing output), passes with the fix. Closes #35434 <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/structured-clone-blob-file.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
What
bun:ffiNaN-boxes native doubles without purifying them, so a well-typed C function returning anf64can hand JavaScript an arbitrary forged JSValue. The NaN payload picks what JS receives:true,undefined, an Int32, or a cell pointer whose address comes from native data (type confusion and an arbitrary read).On
1.4.0-canary (942c22237)this printsboolean true, then crashes dereferencing the forged cell:The relevant property is that the C code is memory-safe and in-contract: any native library that returns a double it read from attacker-controlled bytes (file, wire, database) becomes a JSValue forgery primitive.
Cause
DOUBLE_TO_JSVALUEinsrc/runtime/ffi/FFI.haddsDoubleEncodeOffsetto the raw bits with no NaN purification. JSC's JSVALUE64 encoding is only sound if every boxed double is a non-NaN or a purified NaN, which is why JSC purifies at every boundary where double bits come from outside the engine: Float64Array element loads (TypedArrayAdaptors.h), the structured-clone deserializer, andJSValueMakeNumberin its public C API. Bun skipped it on:f64/f32returns (the call trampoline)f64/f32arguments (the callback trampoline uses the same macro)read.f64/read.f32napi_create_doubleandnapi_create_date(an addon forwarding attacker bytes as a double has the same effect; V8 stores heap numbers so the same addon is harmless under Node)While writing the regression test, the decode direction turned out to be broken too:
JSVALUE_TO_DOUBLEnever handled int32-tagged JSValues, so an integral JS number passed to anf64/f32argument (or returned from anf64JSCallback) reached C as an impure NaN instead of the number:The two defects cancelled on JS -> C -> JS round trips: the impure NaN produced by the bad decode was re-encoded back into the original int32 by the unpurified encode, so
echo_f64(3)looked correct while the C code saw NaN. Purifying without fixing the decode would turn that accidental round trip into a visible NaN, so both are fixed together.Fix
DOUBLE_TO_JSVALUE: canonicalize NaNs toPureNaNbefore addingDoubleEncodeOffset, mirroringJSC::purifyNaN().JSVALUE_TO_DOUBLE: decode int32-tagged JSValues, like the neighboringJSVALUE_TO_INT64already does.reader::f64/reader::f32(FFIObject.rs): purify through a newJSValue::purify_nanhelper.napi_create_double(napi.cpp):jsNumber(purifyNaN(value)), matching JSC'sJSValueMakeNumber.napi_create_date(napi_body.rs): purify before the timestamp is boxed into the Date constructor's argument (flagged in review; with an impure NaN the unfixed constructor argument decodes asJSValue(true)and producesnew Date(1)).test/js/bun/ffi/ffi.test.fixture.*.care the committedviewSourcedumps and are regenerated with the new header.Same pattern, intentionally not changed here because each needs its own test harness:
v8::Number::Newin the V8 shim (V8Number.cpp) and the Postgres/MySQL binaryfloat8decoders also box unpurified doubles whose bits come from untrusted input (a hostile server rather than a hostile library). Also noticed while reading but untouched:UINT32_TO_JSVALUEaccepts2^31as an int32, so au32return of exactly2147483648comes back as-2147483648.Verification
test/js/bun/ffi/cc.test.ts,describe("double <-> JSValue conversions"):true,undefined, Int32, cell pointer, widened f32 NaN) comes back as a plainNaNfrom f64/f32 returns, the JSCallback f64 argument, andread.f64/read.f32; ordinary doubles and the canonical quiet NaN are unaffectednapi_create_doublecalled with an impure NaN producesNaN, andnapi_create_dateproduces an Invalid DateAll three fail on
1.4.0-canary (942c22237): the first crashes dereferencing the forged cell, the second reports that C saw NaN (int32_arg: 2), the third returnsboolean true. All pass with this change on a debug ASAN build. The existing JSCallback round-trip matrix inffi.test.js(48 cases) still passes.