crypto: port upstream ncrypto correctness fixes - #33202
Conversation
Bun's ncrypto.{cpp,h} is a port of nodejs/ncrypto from February 2025.
Upstream has landed several small correctness fixes since then; this
brings over the ones that apply to code Bun already has.
- DataPointer::resize(): handle OPENSSL_realloc failure instead of
returning a DataPointer with a null data pointer and a nonzero
length (nodejs/ncrypto#37).
- CipherCtxPointer setIvLength / setAeadTag / setAeadTagLength /
getAeadTag: compare the EVP_CIPHER_CTX_ctrl return against > 0. The
raw int can be negative, which is truthy as a bool
(nodejs/ncrypto#36).
- BignumPointer::{GetWord,getWord}: return std::optional so a
BN_get_word overflow (reported as ULONG_MAX) is distinguishable from
a real value (nodejs/node#63895). Callers updated: the DiffieHellman
generator-below-2 check, and the two X509 legacy-object exponent
sites, which now report null for an exponent too wide for a word,
matching Node.
- BIOPointer::New(method): return an empty pointer for a null method
(nodejs/node#61788).
- EVPKeyCtxPointer::publicCheck(): remove an unconditional return that
made the EVP_PKEY_public_check_quick branch unreachable
(nodejs/node#59471).
- X509Name::Iterator::operator*(): free the buffer allocated by
ASN1_STRING_to_UTF8, and bail out on a negative size
(nodejs/node#60609).
Also fixes a typo from the original WTF::StringView port:
SSLCtxPointer::setCipherSuites passed ciphers.length() where
SSL_CTX_set_ciphersuites expects the string.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR changes BignumPointer::GetWord/getWord to return std::optional<BN_ULONG>, updates JSX509Certificate and JSDiffieHellmanConstructor callers to handle the optional, and applies several unrelated correctness fixes in ncrypto.cpp covering memory reallocation, null checks, cipher suite configuration, AEAD control return values, and X509 name UTF-8 buffer management, with new DiffieHellman generator tests. ChangesBignumPointer optional word API and callers
Unrelated ncrypto.cpp memory/return-value fixes
Sequence Diagram(s)sequenceDiagram
participant Test as node-crypto.test.js
participant Constructor as constructDiffieHellman
participant Bignum as BignumPointer
Test->>Constructor: createDiffieHellman(p, g)
Constructor->>Bignum: getWord() on generator
Bignum-->>Constructor: optional<BN_ULONG>
Constructor->>Constructor: reject if has_value() and value < 2
Constructor-->>Test: DH instance or bad generator error
Related Issues: None found in the provided context. Related PRs: None found in the provided context. Suggested labels: crypto, native, needs-review Suggested reviewers: None identified from the provided context. 🐰 A word optional, once assumed, now checked twice before consumed, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:25 PM PT - Jul 1st, 2026
@dylan-conway, your commit 72738d2 is building: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
BoringSSL's BN_ULONG is uint64_t on every 64-bit target, but unsigned long is 32 bits on LLP64 (Windows x64). The previous commit compared against the 64-bit overflow sentinel correctly, but still narrowed the result through std::optional<unsigned long>, so on Windows a BIGNUM in the 33-64 bit range truncated: a 5-byte DiffieHellman generator such as 0x0100000000 came back as 0 and was rejected as below 2, and a 33-bit X509 RSA public exponent (which BoringSSL permits) was misreported. Return std::optional<BN_ULONG> instead. No caller changes are needed: the DiffieHellman site compares against 2 and the X509 sites already cast to uint64_t. Adds a 33-bit generator case alongside the existing 72-bit one; it is the same on 64-bit-unsigned-long platforms but exercises the truncation on Windows. Upstream ncrypto keeps unsigned long because OpenSSL defines BN_ULONG as unsigned long on LP64; under BoringSSL the two types differ.
There was a problem hiding this comment.
Thanks for addressing the BN_ULONG truncation — the fix and the added 33-bit test look right. I didn't find anything else, but since this touches crypto primitives (DH generator validation, X509, AEAD tag handling) it's worth a human sign-off.
Extended reasoning...
Overview
This PR ports six upstream ncrypto correctness fixes from Node.js into Bun's src/jsc/bindings/ncrypto.{cpp,h}, updates the three call sites of BignumPointer::GetWord (JSDiffieHellmanConstructor.cpp and two spots in JSX509Certificate.cpp), fixes a Bun-local typo in SSLCtxPointer::setCipherSuites, and adds DiffieHellman generator boundary tests. On my previous pass I flagged that returning std::optional<unsigned long> would still truncate 33–64-bit values on LLP64 (Windows x64); the author changed the return type to std::optional<BN_ULONG> in fbdebd6 and added a 33-bit generator test case that pins the fix on Windows. That thread is resolved and I verified the current diff reflects it.
Security risks
The touched code is security-adjacent: DH generator validation, X509 RSA exponent reporting, AEAD tag get/set, and TLS 1.3 cipher-suite configuration. Each individual change tightens rather than loosens behavior (fail-on-realloc-null, > 0 instead of implicit-bool on EVP_CIPHER_CTX_ctrl, null-method guard, overflow signaling via std::nullopt), and the PR description accurately notes which branches are dead under BoringSSL. I don't see a way any of these introduce a bypass or downgrade — but crypto code is exactly where a subtle misreading of an OpenSSL/BoringSSL contract has outsized consequences, so a human familiar with Bun's BoringSSL fork should confirm.
Level of scrutiny
High. Even though most changes are OOM-only paths, non-BoringSSL #ifndef branches, or otherwise unreachable in Bun's build, the one live path — the DH buffer-generator < 2 check — sits directly on user-facing key-agreement setup. The author's analysis of std::optional < 2 semantics and the LLP64 narrowing is careful and correct, and the new tests cover the exact edge cases (0/1 rejected, 2 accepted, 33-bit and 72-bit accepted).
Other factors
The bug-hunting system found nothing on this revision. The PR description cites upstream references for every change and explicitly lists what was deliberately excluded and why. Test coverage was added in the right file. My only prior concern has been addressed with both a code fix and a targeted test. Deferring solely because auto-approval guidelines exclude security-sensitive crypto changes.
|
CI analysis for Buildkite build 67667 at fbdebd6 (the build was still running when this was written; these are the lanes that have finished or reported so far). The dominant failure is The remaining failures are single-lane flakes with no overlap with this diff (
One result from those lanes is directly relevant to this PR: |
There was a problem hiding this comment.
Thanks for addressing the BN_ULONG return type — the fix and the 33-bit Windows regression test look right. Deferring to a human for final sign-off since this touches crypto primitives (DH generator validation, AEAD tag handling) and intentionally diverges from upstream in DataPointer::resize.
Extended reasoning...
Overview
This PR ports six upstream ncrypto correctness fixes from Node.js into src/jsc/bindings/ncrypto.{cpp,h}, updates the three GetWord callers in JSX509Certificate.cpp and JSDiffieHellmanConstructor.cpp for the new std::optional<BN_ULONG> return type, fixes a Bun-local typo in dead non-BoringSSL code, and adds three DiffieHellman generator tests. Each change is individually small and cites the upstream PR.
Prior review
On the first revision I flagged that returning std::optional<unsigned long> still truncated 33–64-bit values on LLP64 (Windows x64). The author fixed this in fbdebd6 by switching to std::optional<BN_ULONG> and added a 33-bit generator test that pins the fix on Windows CI. That thread is resolved and I verified all four GetWord/getWord call sites are within the diff — no stale callers remain.
Security risks
The changes are defensive hardening (null checks, > 0 comparisons, realloc-failure handling, leak fix) plus one semantic change to BN_get_word overflow signalling. None weaken validation — the DH generator check is strictly more permissive only for values that were previously rejected due to truncation/sentinel confusion, which matches Node's intended behaviour. No new attack surface is introduced. However, DataPointer::resize deliberately diverges from upstream (upstream's version leaks the old block on realloc failure; this version frees it with OPENSSL_clear_free) — that divergence is well-reasoned in the description but is exactly the kind of judgment call a human reviewer should confirm.
Level of scrutiny
Crypto bindings warrant human review by policy even when the changes are mechanical upstream ports. Several hunks are in #ifndef OPENSSL_IS_BORINGSSL branches that never compile in Bun's build, and the PR description transparently documents which changes have observable effects vs. which are defensive/dead — but the live changes still touch DH parameter validation and AEAD tag control-return handling.
Other factors
No CODEOWNERS cover these files. The bug-hunter found nothing this run. The author's CI analysis confirms node-crypto.test.js (including the new tests) passed on all Windows lanes, and the remaining CI failures are unrelated repo-wide flakes. The PR description is unusually thorough about what was ported, what was deliberately excluded, and why each change is or isn't observable under BoringSSL.
2 participants
Bun's
src/jsc/bindings/ncrypto.{cpp,h}is a port of nodejs/ncrypto@134ac40 (February 2025), which maps to Node'sdeps/ncryptofrom late January 2025. This ports the upstream correctness fixes made since then that apply to code Bun already has, plus one Bun-local typo found while auditing the drift.Upstream fixes
DataPointer::resize(): handleOPENSSL_reallocfailure instead of returning aDataPointerwith a null pointer and a nonzero length. Also frees the old block on failure. Upstream's version calls afree()helper afterrelease()has already nulleddata_, which is a no-op, so the old block leaks; this keeps the intent without that.CipherCtxPointer::{setIvLength, setAeadTag, setAeadTagLength, getAeadTag}: compare theEVP_CIPHER_CTX_ctrlreturn against> 0. The rawintcan be negative, which converts totrue. BoringSSL normalizes-1to0, so this is masked in Bun's build today, but it is still the wrong check.BignumPointer::{GetWord, getWord}returnstd::optionalso aBN_get_wordoverflow (reported as the all-ones word) is distinguishable from the real all-ones value. The optional wrapsBN_ULONG, not upstream'sunsigned long: BoringSSL'sBN_ULONGisuint64_ton every 64-bit target, sounsigned longwould still silently truncate 33-to-64-bit values on LLP64 (Windows x64).BIOPointer::New(method): return an empty pointer for a null method.EVPKeyCtxPointer::publicCheck(): remove an unconditional return that made theEVP_PKEY_public_check_quickbranch unreachable. The whole block is non-BoringSSL, so it is dead in Bun's build.X509Name::Iterator::operator*(): free the buffer allocated byASN1_STRING_to_UTF8, and return early on a negative size (the out-pointer is never written on failure). Dead code today:JSX509Certificate.cpphas its own correct loop.GetWordcallers updatedJSDiffieHellmanConstructor.cpp: the generator-below-2 check becomeshas_value() && *word < 2. This is the one spot that needed care:std::optional<T> < 2compiles and evaluatesnullopt < 2astrue, so a naive translation would have started rejecting any generator wider than a word.JSX509Certificate.cpp(both legacy-object sites):exponentis reported asnullwhen the RSA public exponent does not fit in a word, matching Node. This branch is unreachable under BoringSSL, which rejects RSA public exponents wider than 33 bits at SPKI parse time.Bun-local typo
SSLCtxPointer::setCipherSuitespassedciphers.length()whereSSL_CTX_set_ciphersuitesexpects the string. It is in the non-BoringSSL#ifndefbranch so it never compiles in Bun's build; introduced by the originalWTF::StringViewport of the file.Testing
No test here can fail against the unmodified source on a Linux machine, because nothing in this diff changes observable behavior on Linux. I checked each one:
DataPointer::resizeonly diverges underOPENSSL_reallocfailure, theEVP_CIPHER_CTX_ctrlreturns are already normalized to0or1by BoringSSL,publicCheckandsetCipherSuitesare inside non-BoringSSL#ifndefbranches,BIOPointer::New(method)andX509Name::Iteratorhave no callers, andBN_ULONGandunsigned longare the same 64-bit type on LP64. The one real before-and-after is theBN_ULONGtruncation, and it only exists on Windows x64 (LLP64).So the new tests pin invariants rather than prove a failure.
node-crypto.test.jsgains threeDiffieHellmancases:optional < 2translation would have broken on every platform.unsigned long; on Windows x64 theunsigned longversion of this change truncates it to0and wrongly rejects it as below 2, so this is the real regression test for theBN_ULONGreturn type on that platform.Node v26 agrees on all three.
Local verification against the debug build
test/js/node/crypto/{crypto,node-crypto,crypto.key-objects,crypto-rsa,x509-subclass}.test.*: 677 pass, 0 failtest/js/node/test/parallel/test-crypto-{x509,dh,dh-errors,dh-constructor,dh-odd-key,dh-generate-keys,dh-shared,dh-padding,cipheriv-decipheriv,gcm-explicit-short-tag,gcm-implicit-short-tag,aes-wrap,padding-aes256,getcipherinfo,rsa-dsa}.js,test-webcrypto-encrypt-decrypt-aes.js,test-crypto-webcrypto-aes-decrypt-tag-too-small.js: all passDeliberately not included
Cipher::MAX_AUTH_TAG_LENGTH(nodejs/node#57803): of the three macros upstream'sstatic_assertuses, BoringSSL only definesEVP_GCM_TLS_TAG_LEN, so the assert degenerates to16 <= 16.ECKeyPointer::setPublicKeyRaw(nodejs/node#62396): the expensive check that rewrite avoids (theorder * Q == infinitystep ofEC_KEY_check_key) does not exist in BoringSSL'sEC_KEY_check_key, so there is no perf win for Bun.NCRYPTO_NO_DSA_KEYGEN,NCRYPTO_NO_EVP_DH, rejecting DSA inNewFromID, ...): those exist for vanilla BoringSSL CI, and Bun's DSA and DH paths work today against its BoringSSL.