node:crypto: default to SHA256 for EC keys in one-shot sign/verify with null algorithm#33753
node:crypto: default to SHA256 for EC keys in one-shot sign/verify with null algorithm#33753robobun wants to merge 1 commit into
Conversation
…y with null algorithm When crypto.sign(null, data, ecKey) or crypto.verify(null, ...) is called with an EC or DSA key, OpenSSL v3 (used by Node.js) queries the key's default digest via OSSL_PKEY_PARAM_DEFAULT_DIGEST and receives SHA256. BoringSSL has no such fallback and fails EVP_DigestSignInit with EVP_R_NO_DEFAULT_DIGEST. Bun already worked around this for RSA keys by explicitly selecting SHA256 when the algorithm argument is null or undefined. Extend the same treatment to EC and DSA keys (isSigVariant) so the one-shot sign and verify paths match Node.js.
|
Updated 6:05 AM PT - Jul 8th, 2026
❌ @robobun, your commit 8d442cc has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33753That installs a local version of the PR into your bun-33753 --bun |
|
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 (2)
WalkthroughModifies the default digest selection logic in CryptoSignJob.cpp so SHA256 is applied when no algorithm is specified for both RSA and other signature variants (e.g., EC/DSA), not just RSA. Updates the regression test to replace RSA/Ed25519 assertions with parametrized EC sign/verify test coverage. ChangesEC/DSA default digest fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
I didn't find any issues — the change is a one-line extension of the existing RSA SHA256-default to EC/DSA and is well-tested — but since it's in the node:crypto sign/verify path I'm leaving it for a human to sign off.
Extended reasoning...
Overview
This PR extends an existing BoringSSL/OpenSSL compatibility shim in SignJobCtx::fromJS (src/jsc/bindings/node/crypto/CryptoSignJob.cpp). Previously, when crypto.sign()/crypto.verify() were called with a null/undefined algorithm, Bun explicitly defaulted to SHA256 only for RSA keys (to match OpenSSL v3's OSSL_PKEY_PARAM_DEFAULT_DIGEST behavior that BoringSSL lacks). The fix adds || isSigVariant() to that condition, which covers EVP_PKEY_EC and EVP_PKEY_DSA — verified in ncrypto.cpp:2781-2786. The rest of the diff is comment updates and 18 new test cases in the existing regression test file covering P-256/384/521, sync/async, DER/ieee-p1363 encoding, and cross-verification against explicit "sha256".
Security risks
None identified. The change does not weaken any verification path — it makes a previously-throwing call succeed with a secure, well-established default (SHA256), exactly mirroring Node.js/OpenSSL v3 behavior. The cross-verification tests confirm that sign(null, ...) output verifies against verify("sha256", ...) and vice versa, so the digest selection is correct rather than merely self-consistent. Ed25519/Ed448 (one-shot variants) remain unaffected since isSigVariant() is false for them.
Level of scrutiny
The native change is mechanically trivial (one added disjunct on an existing condition) and follows the exact pattern already in place for RSA. However, it lives in the node:crypto sign/verify implementation, which is security-sensitive by policy. A quick human confirmation that SHA256 is the right default for EC/DSA under OpenSSL v3 (the PR cites ec_kmgmt.c and dsa_kmgmt.c) is warranted.
Other factors
The PR description is thorough with upstream source references, the tests include negative cases (bad signature → false, wrong data → false), and the author confirmed the new tests fail on system Bun and pass on the debug build. No prior reviews or outstanding comments on the PR.
|
CI failures on build 70479 are unrelated to this change:
This diff touches only |
What does this PR do?
Fixes
crypto.sign(null, data, ecKey)andcrypto.verify(null, data, ecKey, sig)throwingERR_OSSL_NO_DEFAULT_DIGESTinstead of defaulting to SHA256 like Node.js.Reproduction
Node v26.3.0 prints
true. Bun throws:The same happens for
undefined, for all EC curves (P-256/384/521), for the async callback variants, and forcrypto.verify(null, ..., <bad sig>)which should returnfalsebut throws.Cause
When
algorithmisnull/undefined, Node passes a NULL digest toEVP_DigestSignInit. OpenSSL v3 then queries the key'sOSSL_PKEY_PARAM_DEFAULT_DIGESTand receives"SHA256"from the EC keymgmt provider (providers/implementations/keymgmt/ec_kmgmt.c), same as it does for RSA and DSA.BoringSSL has no default-digest mechanism:
do_sigver_initincrypto/fipsmodule/digestsign/digestsign.cc.incfails withEVP_R_NO_DEFAULT_DIGESTwhenevertype == NULLand the key uses a prehash method (EC, RSA, DSA).Bun already compensates for RSA by explicitly selecting SHA256 when the algorithm argument is nullish. EC and DSA were missing from that branch.
Fix
Extend the existing SHA256 default in
SignJobCtx::fromJSto coverisSigVariant()(EC and DSA) in addition toisRsaVariant(). This matches OpenSSL v3 behaviour exactly: verified that in Node,crypto.sign(null, data, ecKey)cross-verifies withcrypto.verify("sha256", data, ecKey, sig)and vice versa.Verification
Added EC coverage (P-256/384/521, sync + async, DER + ieee-p1363, bad-signature-returns-false) to
test/regression/issue/11029-crypto-verify-null-algorithm.test.tsalongside the existing RSA/Ed25519 cases.USE_SYSTEM_BUN=1 bun test: 18 new EC tests fail withERR_OSSL_NO_DEFAULT_DIGEST, 5 existing tests passbun bd test: 23/23 passtest/js/node/test/parallel/test-crypto-sign-verify.jsand related crypto sign tests still pass