Repository navigation
Conversation
A new runtime-dispatched kernel in highway_strings.cpp XORs every byte pair into four accumulators and tests them once, after the last load. x64 hosts with AVX2 or AVX-512 get wider vectors than the Nehalem baseline allows. Shorter inputs stay on BoringSSL CRYPTO_memcmp, where a dispatch call cannot pay off. The highwayStringsForTesting shim gains a constantTimeEq op so that tests reach the kernel at every length.
|
Updated 8:34 AM PT - Sep 17th, 2026
✅ @robobun, your commit 67a99966465f5df94f39a974277c8b1b8f369b41 passed in 🧪 To try this PR locally: bunx bun-pr 43095That installs a local version of the PR into your bun-43095 --bun |
|
Status Reproduced with the script from the report (
Related: oven-sh/boringssl#13 repairs the |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesConstant-time equality
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The new SIMD comparison path retains length validation and has coverage for dispatch-relevant sizes and mismatch positions. No merge-blocking issue is identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs, but since it replaces the compare behind crypto.timingSafeEqual with a new hand-written SIMD kernel, a human look at the constant-time argument is still worthwhile.
What was reviewed:
- Kernel bounds in
ConstantTimeEqualImpl(len < Npartial load, 4N block loop, single-vector loop, overlapping tail atlen - N) — every load stays inside[0, len), and thelen < Npath only reaches production through the test shim sincetimingSafeEqualroutes lengths under 64 toCRYPTO_memcmp. - Both entry points (
node:cryptoandglobalThis.crypto) sharenode_crypto_binding.rs::timing_safe_equal, so the threshold applies to both; the Rust wrapper rejects unequal lengths before the unsafe call. - Allowlist entries follow the existing
BUN_HWY_DISPATCHgroup format; the aarch64 SVE2_128 entry is hand-derived per the description and was not scanned locally, so the first arm64 release scan is the real check. - The kernel has no explicit value barrier on the OR-accumulator; the reduction shape gives the compiler no reason to introduce an early exit, but that is the property a crypto maintainer should sign off on.
Extended reasoning...
Overview
The PR adds ConstantTimeEqualImpl to src/jsc/bindings/highway_strings.cpp (Highway, runtime-dispatched), a C-ABI wrapper highway_constant_time_eq, a safe Rust wrapper bun_highway::constant_time_eq plus CONSTANT_TIME_EQ_MIN_LEN = 64 in src/highway/lib.rs, and routes timing_safe_equal in src/runtime/node/node_crypto_binding.rs through it for inputs of 64 bytes or more (BoringSSL CRYPTO_memcmp below that). The bun:internal-for-testing shim gains a constantTimeEq op, three baseline-static allowlists get the new dispatched symbols, and test/js/node/crypto/crypto.test.ts gains bit-flip sweeps through both public entry points and the raw kernel. A mitata bench is added.
Security risks
timingSafeEqual exists to prevent timing side channels in MAC/token comparison, so the risk is a kernel that leaks the mismatch position through timing. The kernel's control flow depends only on len: no data-dependent branch, the accumulators are reduced once via AllBits0 after the last load. There is no value_barrier-style construct, so the constant-time property rests on the compiler not transforming the OR-reduction into an early exit — very unlikely for this shape, but it is a guarantee the code cannot enforce and a human crypto reviewer should accept explicitly. Memory safety: all loads are within [0, len) for len >= N; the len < N path uses LoadN (masked/partial), and the len == 0 case only arrives via the test shim (production sends < 64 to BoringSSL). The Rust wrapper checks equal lengths before the unsafe call, and the JS binding already validated both are ArrayBufferViews of equal byte length.
Level of scrutiny
High. This is a user-facing crypto primitive with a security contract, and the change replaces a vetted library implementation with new native code. The bug hunt exhausted its candidates (dry_streak) without findings, and the ruled-out list covered the debug-lane test runtime, the LoadN with len 0 path, the allowlist feature ceilings, and the missing value barrier. None of those turned into a defect, but per the approval guidelines crypto paths should not be auto-approved.
Other factors
Test coverage is strong for correctness: every bit position for lengths 0..137 through both APIs, stride-61 plus last-64-byte coverage for larger sizes that exercise the block loop and overlapping tail on 16/32/64-byte vectors, and the raw kernel for 0..330 via the shim (the only way to hit the partial-load path). The tests cannot verify the timing property, which the description acknowledges. The aarch64 allowlist entry was not scanned locally per the author, so the first arm64 release CI run is the real validation of that line. The PR description's perf numbers are claims with a benchmark file attached; I did not reproduce them.
The kernel reduced its accumulators with AllBits0, a boolean the compiler can reason about. The per-lane mask now passes through an empty asm statement first, the same idiom as BoringSSL's value_barrier_w, so the compiler cannot tell that only "any difference" matters.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
The kernel now has an explicit value barrier (82ee85d). It no longer returns Timing does not change (64 B 21.8 ns, 1 KB 26.6 ns, 64 KB 850 ns per call). The sweeps pass, and the baseline scanner reports the same feature sets on the linux x64 release build. The arm64 allowlist entry still waits for the first arm64 release scan in CI. |
There was a problem hiding this comment.
I re-reviewed the revised kernel and didn't find any bugs; since this changes the compare behind crypto.timingSafeEqual and the constant-time property can't be asserted by the test suite, a human look at the crypto path is still worthwhile.
What was reviewed:
ConstantTimeEqualImpl(src/jsc/bindings/highway_strings.cpp): checked thelen < Npartial-load path (includinglen == 0), thelen - Noverlapping tail (only reached whenlen >= N, so no underflow), and that the fold is tested once throughValueBarrieron the mask word rather thanAllTrue.bun_highway::constant_time_eq/node_crypto_binding.rs: the length-mismatch error still fires before dispatch, so the kernel only sees equal lengths; the 64-byte threshold keeps short inputs onCRYPTO_memcmp.- Allowlist entries: symbol counts (168->169, 280->286) match the added lines; feature ceilings are consistent with
BitsFromMaskon the AVX-512 variants. - Testing shim:
constantTimeEqrejects a numericargand unequal lengths before touching the kernel.
Extended reasoning...
Overview
The PR adds a Highway-dispatched constant-time byte comparison (ConstantTimeEqualImpl) in src/jsc/bindings/highway_strings.cpp, exposes it through src/highway/lib.rs as bun_highway::constant_time_eq, and routes crypto.timingSafeEqual (src/runtime/node/node_crypto_binding.rs) to it for inputs of 64 bytes or more, keeping BoringSSL's CRYPTO_memcmp below that. Supporting changes: baseline-static allowlist entries for the new per-target symbols, a constantTimeEq op on the existing highwayStringsForTesting shim, a mitata benchmark, and a test sweep in test/js/node/crypto/crypto.test.ts. Since the earlier review on this PR, two commits landed: one that routes the kernel's result through an inline-asm value barrier, and one that shortens comments.
Security risks
This is a timing-side-channel primitive. Functional correctness (returns equal/unequal correctly) is well covered by the bit-flip sweep, but the security property — that execution time does not depend on where the inputs differ — is not testable in CI and rests on code structure: every load depends only on len, XOR results are OR-folded into accumulators, and the single test happens after the last load on a mask word passed through __asm__("" : "+r"(v)). I read the revised kernel and could not find a path where the compiler has license to short-circuit, but this is exactly the category where a human with crypto familiarity should confirm the reasoning (and ideally inspect the emitted assembly for the shipped targets, as the PR description claims was done for AVX2). No memory-safety issues were found: the len - N tail load is only reached after the len < N early return, LoadN handles 0..N-1 bytes, and the Rust wrapper only calls the kernel with equal-length slices.
Level of scrutiny
High. The change is small in line count but sits under crypto.timingSafeEqual, a security API whose whole contract is the timing property. Approval by an automated review is not appropriate here regardless of how clean the code looks; the right outcome is a human sign-off on the crypto path, which is why this is a defer rather than an approve. The bug hunt exited on dry_streak with no findings, which supports the "no functional bugs found" statement but does not settle the timing claim.
Other factors
The allowlist edits are mechanical and internally consistent (counts updated, entries in the existing gated section, feature ceilings plausible given BitsFromMask compiles to vpmovmskb/kmov+BMI2 on AVX-512). The testing shim validates argument types and lengths before calling the kernel, so the test-only surface cannot produce out-of-bounds reads. The new tests exercise lengths 0..330 through the raw kernel (covering the partial-load path and both loop shapes on every target) and every bit position for 0..137 bytes through the public API, which is a solid functional net. A prior review from this app's identity exists on the first push; this body is limited to what changed since then and to what was re-verified, rather than restating earlier points.
|
#43885 is the preview pin of oven-sh/boringssl#13 (the one-line Measured overlap with this PR,
With the repaired loop, 64 to 256 byte compares are within 3 to 5 ns of the kernel. The gain of this PR starts at about 1 KB and grows with the length. #43885 also covers |
This branch has not been deployed
2 participants
Problem
crypto.timingSafeEqualon 1 KB buffers costs 140 ns per call on main, 40 ns on Bun 1.4.2.CRYPTO_memcmp, a byte loop. Since LLVM 23 (Upgrade LLVM 21.1.8 → 23.1.1 and Rust nightly to 2026-09-15 #42851), clang puts 4 bytes in a 128-bit vector instead of 16 ([LoopVectorize] i8 OR-reduction vectorized with i32 lanes since InstSimplify zext(trunc nuw) fold (#204089) llvm/llvm-project#222142). Return CRYPTO_memcmp's accumulator through a value barrier boringssl#13 repairs that loop for every caller. This PR is thetimingSafeEqualpart.Fix
timingSafeEqualcalls a Highway kernel (ConstantTimeEqualImpl,highway_strings.cpp). AVX2 and AVX-512 hosts get wider vectors than the baseline allows, as in buffer: restore swap16/32/64 and multi-byte indexOf throughput #39616. Shorter inputs stay onCRYPTO_memcmp. 1 KB costs 26 ns.test/js/node/crypto/crypto.test.ts(the new kernel sweep fails on main), node'stest-crypto-timing-safe-equal.js,domjit.test.ts, the baseline scanner.CRYPTO_memcmpand left it slow. This PR is the rework. Open: overrideCRYPTO_memcmpin the fork.Background
memcmpstops at the first mismatch, so timed guesses recover a MAC bytewise.scripts/verify-baseline-staticscans release binaries for instructions above the baseline. Dispatched kernels need allowlist entries.Notes
Numbers. ns per
timingSafeEqualcall, best of 25 batches, Xeon Platinum 8375C (AVX-512), linux x64 release builds. "main" is a local release build at c6b7fcb, the LLVM 23 merge commit. The official canary at that commit shows the same slowdown.bench/crypto/timing-safe-equal.mjs(new, mitata) gives the same picture. Node 26.3 needs 699 ns at 1 KB.Other hosts at 1 KB, measured on an earlier revision of this branch with the same kernel: windows x64 (Xeon 8573C) 109-114 ns on the canary and 26 ns with the kernel. Windows arm64 (Cobalt 100, SVE2_128) 106-107 ns and 46 ns. macOS and linux arm64 are not measured. On arm64 and on x64 hosts without AVX2 the kernel has the same vector width as the repaired BoringSSL loop, so it only ties that loop.
What this PR does not fix.
KeyObject.equalsalso callsCRYPTO_memcmpand has the same slowdown (1 KB keys: 31 ns on 1.4.2, 130 ns on main). So do the BoringSSL-internal callers, which compare 8 to 64 bytes, except ML-KEM decapsulation (1088 or 1568 bytes). oven-sh/boringssl#13 returns the accumulator throughvalue_barrier_w, which brings the old loop back under clang 23 on x64 and arm64. In a local release build with that patch,KeyObject.equalson 1 KB keys costs 37 ns. #43885 pins that commit as a preview, so CI covers it, and its numbers show the overlap: with the repaired loop, 64 to 256 byte compares are within 3 to 5 ns of this kernel, and the gain of this PR starts at about 1 KB.WTF::constantTimeMemcmphas the same loop shape. Its three WebCrypto callers compare 32 to 64 bytes, and this PR leaves them alone.Why clang 23 is slower. In C,
x |= a[i] ^ b[i]computes inintand truncates back touint8_t, andreturn xextends again. An InstSimplify fold new in LLVM 23 (zext(trunc nuw X) -> X) makes the reduction phii32. The loop vectorizer then picks 4 x i32: two 4-byte scalar XORs,movd,pmovzxbd,por, 8 bytes per iteration. Clang 17, and the 1.4.2 binary, keep ani8phi: 2 xmovdqu/pxor/poron 16 x i8, 32 bytes per iteration. On arm64, clang 23 emits 16tbl+ 16orrper 64 bytes.Kernel choice. Microbenchmark of the compare alone at 1 KB: C loop with clang 23 120 ns, the old loop 23-27 ns, Highway kernel 22-32 ns on SSE4, 14-21 ns on AVX2, 12-23 ns on AVX-512 (aligned to unaligned buffers). Four accumulators keep the 128-bit targets level with the old loop. One or two accumulators lose 30 to 80 percent there. The threshold is 64 bytes: from there the kernel ties or beats the repaired loop on every target, and below it a dispatch call cannot pay off.
Value barrier. The kernel does not return
AllBits0(diff). It takes one bit per byte lane that differs (BitsFromMask(Ne(diff, 0))), passes that word through an empty asm statement (the idiom of BoringSSL'svalue_barrier_w), and compares it with zero. The compiler cannot tell that only "any difference" matters, so it has no license to stop at the first one. The AVX2 variant ends invpcmpeqb,vpmovmskb,not,test,sete. Timing does not change.Checks. The kernel text was run standalone against every length from 0 to 1099, every byte position, bits 0 and 7, on the SSE4, AVX2 and AVX3_DL targets. The baseline scanner passes on local linux x64 and windows x64 release builds with the new entries (6 each, plus 1 for arm64 that follows the existing
N_SVE2_128entries and is not scanned locally). It reports[AVX, AVX2]for the AVX2 symbol and[AVX, AVX512BW, AVX512F, BMI2]for the five AVX-512 symbols on both.Tests. The sweeps through
node:cryptoandglobalThis.cryptopass on main too: the public behavior does not change, and a timing assertion is not stable under the debug and ASAN builds. The kernel sweep drives the kernel through the existinghighwayStringsForTestingshim (new opconstantTimeEq). It fails on main withTypeError: unknown op, and it is the only way to reach the kernel below 64 bytes, which includes the partial-load path for inputs shorter than one vector.Self-review. The first shape replaced
bun_boringssl_sys::constant_time_eqwith an inline Rustu64fold plus this kernel, and moved the CSRF, bcrypt and SCRAM checks to it. The review rejected that shape. Those compares are 23 to 64 bytes and gain nothing. The fold had only a best-effortblack_boxbarrier.CRYPTO_memcmpitself stayed slow, soKeyObject.equalskept the regression. The rework keeps those callers onCRYPTO_memcmp, repairs the loop in the fork, and scopes the speed claim beyond 1.4.2 to x64 hosts with AVX2 or AVX-512. With the fork override,CRYPTO_memcmpwould become a Bun-defined symbol backed by this kernel, and every TLS tag check would run Bun's code.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/crypto/crypto.test.ts