Conversation
getHashes() lists both and createHash/createHmac compute them, but every sign/verify entry point rejected them: crypto.sign with an RSA key threw ERR_OSSL_UNKNOWN_ALGORITHM_TYPE, EC keys threw ERR_OSSL_INVALID_DIGEST_TYPE, createSign().sign() threw a code-less TypeError, the RSA-SHA512/224 and RSA-SHA512/256 aliases threw ERR_CRYPTO_INVALID_DIGEST, and verify() with an RSA key returned false for every signature instead of throwing. BoringSSL exposes EVP_sha512_224/256 but omits NID_sha512_224/256 from kPKCS1SigPrefixes (so RSA_sign/RSA_verify raise RSA_R_UNKNOWN_ALGORITHM_TYPE) and from pkey_ec_ctrl's EVP_PKEY_CTRL_MD whitelist (EVP_R_INVALID_DIGEST_TYPE). Patch both tables with the id-sha512-224/256 DigestInfo prefixes from RFC 8017, and add the RSA-SHA512/224 and RSA-SHA512/256 aliases to getDigestByName so the OpenSSL long names Node advertises resolve. RSA-PSS already worked because RSA_sign_pss_mgf1 never consults the PKCS#1 prefix table.
|
Status: diff is green; CI is red on unrelated known-flaky tests. Ready for a maintainer to merge. Two consecutive runs (#76850, #76879) passed every crypto test on every lane, including the 14 new sha512-224/256 cases in Review threads all resolved through a2949b9:
Related to #20592 (makes the Repro before/after |
|
Updated 7:43 AM PT - Jul 21st, 2026
❌ @robobun, your commit 7444609 has some failures in 🧪 To try this PR locally: bunx bun-pr 34927That installs a local version of the PR into your bun-34927 --bun |
WalkthroughChangesSHA-512/224 and SHA-512/256 support was added to BoringSSL signing internals, ncrypto digest-name mapping, and crypto sign/verify tests covering RSA, RSA-PSS, and ECDSA. SHA-512 truncated signing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@scripts/build/deps/boringssl.ts`:
- Around line 42-46: Shorten the dependency note comment near the sha512t-sign
explanation to three content lines or fewer by combining its wording, while
preserving the explanation of the BoringSSL limitation and the condition for
removing the workaround.
In `@test/js/node/crypto/crypto-oneshot.test.ts`:
- Around line 159-176: Extend the crypto tests around the existing
crypto.verify() and createVerify() coverage to include RSA-PSS round trips for
both one-shot and streaming APIs. Generate or use randomized messages, sign with
RSA_PKCS1_PSS_PADDING, then verify through crypto.sign/crypto.verify and
createSign/createVerify, preserving the complete supported variant matrix and
asserting successful verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: abdf95a4-602f-45e8-bf62-35173897e09c
📒 Files selected for processing (4)
patches/boringssl/sha512t-sign.patchscripts/build/deps/boringssl.tssrc/jsc/bindings/ncrypto.cpptest/js/node/crypto/crypto-oneshot.test.ts
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…12/* out of node-crypto.test.js unsupported list sha2.h at the pinned fork commit already defines SHA512_224_DIGEST_LENGTH at line 201, so the patch's own define was a benign identical redefinition with a misleading comment. node-crypto.test.js still listed rsa-sha512/224 and rsa-sha512/256 in its unsupported array, asserting createHash() throws for them. getDigestByName now resolves both, so the test expectation was inverted.
There was a problem hiding this comment.
Both earlier findings (the redundant SHA512_224_DIGEST_LENGTH define and the stale unsupported entries in node-crypto.test.js) are addressed in a2949b9. I don't see anything else wrong — the RFC 8017 DigestInfo prefixes check out byte-for-byte and the tests pin against Node/OpenSSL fixtures — but this patches vendored BoringSSL crypto internals, so leaving the sign-off to a maintainer.
What was reviewed
- DigestInfo prefixes: verified the DER encoding for OIDs 2.16.840.1.101.3.4.2.{5,6} with NULL params — outer SEQUENCE lengths (0x2d/0x31), 19-byte prefix, and trailing OCTET STRING lengths (0x1c/0x20) all match RFC 8017 §9.2 note 1.
getDigestByNamealias placement sits inside the existingrsa-shaprefix branch and can't shadow the plain"512"case above it.- Test fixtures
rsa_{private,public}_2048.pem/ec_p256_{private,public}.pemexist; PKCS#1 fixtures are deterministic so they pin interop, not just self-round-trip.
Extended reasoning...
Overview
Enables crypto.sign/crypto.verify/createSign/createVerify for sha512-224 and sha512-256, which were already advertised by getHashes() and accepted by createHash but rejected at the signing layer. Three moving parts: a 41-line BoringSSL patch adding the two NIDs to kPKCS1SigPrefixes (rsa.cc.inc) and the EC EVP_PKEY_CTRL_MD whitelist (p_ec.cc); two new alias arms in ncrypto::getDigestByName for the RSA-SHA512/224 / RSA-SHA512/256 OpenSSL long names; and ~100 lines of tests covering one-shot + streaming across RSA PKCS#1 v1.5, RSA-PSS, and ECDSA with Node-generated known-answer fixtures. The two entries removed from node-crypto.test.js's unsupported list were the fix for my prior finding.
Security risks
This is signature crypto. The risk surface is a wrong DigestInfo prefix producing signatures that verify against Bun but not OpenSSL (or vice versa), or the EC whitelist relaxation letting through something it shouldn't. I hand-decoded both DER prefixes against RFC 8017 Appendix B.1 and they're correct. The EC change only adds two NIDs to an existing conjunctive whitelist; the comment in the patch is accurate that pkey_ec_sign hands the digest bytes straight to ECDSA_sign without consulting dctx->md. The tests cross-verify against OpenSSL-generated fixtures, so a prefix mismatch would fail loudly. No new attack surface — this makes previously-rejected inputs succeed with the standard encoding.
Level of scrutiny
High. This is a hand-written patch to a vendored TLS/crypto library. The change itself is small, mechanical, and mirrors two recently-merged siblings (#33518 for sha3-*, #34348 for ripemd160/md4 verify), and REVIEW.md-style test coverage is present including negative cases and known-answer vectors. But vendor crypto patches are exactly where a maintainer should be in the loop regardless of how clean the diff looks — both to sanity-check the DER bytes independently and to confirm the patch-file approach (vs. landing upstream in the fork) is what's wanted here.
Other factors
All prior review threads are resolved: the CodeRabbit comment-length nit and RSA-PSS coverage ask were handled in e46665a; my two findings (stale unsupported list, redundant macro define) were handled in a2949b9. The PR description flags a known trivial rebase conflict with #33518 in the shared p_ec.cc and kPKCS1SigPrefixes hunks depending on merge order — worth the merger being aware of.
There was a problem hiding this comment.
Both earlier findings are addressed in a2949b9 — the redundant SHA512_224_DIGEST_LENGTH define is gone from the patch and rsa-sha512/224/256 are removed from node-crypto.test.js's unsupported list. No new issues found this pass, but leaving the BoringSSL patch for a human sign-off since it touches signing internals.
What was reviewed:
- DigestInfo prefix bytes for both NIDs check out against RFC 8017 §9.2 (OIDs 2.16.840.1.101.3.4.2.{5,6}, NULL params, outer SEQUENCE lengths 0x2d/0x31, OCTET STRING lengths 0x1c/0x20, prefix len 19).
p_ec.ccwhitelist addition is gate-only;pkey_ec_signhands the digest straight toECDSA_sign, so no other EC-side change is needed.- New
rsa-shaalias arms ingetDigestByNamesit after the exact-"512"match, soRSA-SHA512still resolves toEVP_sha512(). - Fixture PEMs referenced by the new tests exist on disk.
Extended reasoning...
Overview
Enables sha512-224 / sha512-256 for crypto.sign/verify/createSign/createVerify by (1) a two-hunk BoringSSL patch adding both NIDs to kPKCS1SigPrefixes (RSA PKCS#1 v1.5) and pkey_ec_ctrl's EVP_PKEY_CTRL_MD whitelist, (2) two alias arms in ncrypto.cpp getDigestByName for RSA-SHA512/224 and RSA-SHA512/256, and (3) test coverage pinning byte-exact deterministic PKCS#1 signatures generated by Node/OpenSSL plus PSS/ECDSA round-trips and negative cases. The two entries removed from node-crypto.test.js's unsupported array are the follow-through I flagged last pass.
Security risks
The DigestInfo prefix bytes are the security-load-bearing part: a wrong encoding would produce signatures Node/OpenSSL reject (caught by the deterministic fixture test) or, worse, accept malformed structures on verify. I hand-decoded both 19-byte prefixes and they match RFC 8017 exactly — correct OIDs, NULL parameters, and length fields consistent with 28- and 32-byte digests. The EC change is a pure whitelist extension. The alias additions only widen name resolution to existing EVP_sha512_224/256 digests. No new parsing of untrusted input.
Level of scrutiny
High — this is a patch to vendored BoringSSL's RSA and EC signing paths. The change is small, mechanical, and mirrors the shape of the existing NID_sha224…NID_sha512 entries in the same tables, and it follows two prior merged PRs (#33518 sha3-*, #34348 ripemd160/md4) doing the same class of extension. The deterministic fixture test is strong evidence of interop correctness. Still, patching a crypto library's signing tables is exactly where a maintainer should look before merge.
Other factors
All prior review threads (CodeRabbit's comment-length and PSS-coverage nits, and my two findings) are resolved and reflected in the current diff. CI build #76879 is in flight. The PR description flags a trivial rebase interaction with #33518 on the shared p_ec.cc and kPKCS1SigPrefixes hunks — worth noting for whoever merges second.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-21, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What does this PR do?
crypto.getHashes()listssha512-224andsha512-256andcreateHash/createHmaccompute them correctly, but every signing entry point rejected them. Node.js signs and verifies both with RSA and EC keys.Before this change:
crypto.sign()threwERR_OSSL_UNKNOWN_ALGORITHM_TYPEcrypto.sign()threwERR_OSSL_INVALID_DIGEST_TYPEcreateSign(alg).sign()threw a code-lessTypeError: Failed to create signaturecrypto.verify()/createVerify().verify()returnedfalsefor every signature instead of throwingRSA-SHA512/224andRSA-SHA512/256aliases threwERR_CRYPTO_INVALID_DIGESTRoot cause
BoringSSL exposes
EVP_sha512_224/EVP_sha512_256and recognises the names inEVP_get_digestbyname, so Bun's digest lookup already succeeds. The rejection comes from inside BoringSSL:kPKCS1SigPrefixesincrypto/fipsmodule/rsa/rsa.cc.inchas no entries forNID_sha512_224/NID_sha512_256, soRSA_sign/RSA_verifyraiseRSA_R_UNKNOWN_ALGORITHM_TYPEfor PKCS#1 v1.5 padding.EVP_DigestVerifymaps that to a plain0, which Bun returns asfalse.pkey_ec_ctrl'sEVP_PKEY_CTRL_MDwhitelist incrypto/evp/p_ec.cconly admits sha1/224/256/384/512, soEVP_DigestSignInitwith an EC key raisesEVP_R_INVALID_DIGEST_TYPE.RSA-PSS already worked because
RSA_sign_pss_mgf1never consults the PKCS#1 prefix table.Fix
patches/boringssl/sha512t-sign.patchadds both NIDs to the two tables, with theid-sha512-224/id-sha512-256DigestInfo prefixes from RFC 8017 (NIST CSOR OIDs 2.16.840.1.101.3.4.2.5 and .6, NULL parameters). The prefixes were verified byte-exact against what Node/OpenSSL emit by raw-RSA-decrypting a Node signature.ncrypto.cppgetDigestByNamegains theRSA-SHA512/224andRSA-SHA512/256aliases so the OpenSSL long names Node advertises resolve (BoringSSL'snid_to_digest_mappingonly carriesRSA-SHA1throughRSA-SHA512).How did you verify your code works?
test/js/node/crypto/crypto-oneshot.test.tsadds coverage for both digests across one-shot/streaming sign and verify, with RSA PKCS#1 v1.5, RSA-PSS, and ECDSA. The PKCS#1 fixtures are deterministic signatures generated by Node, so the tests also pin byte-exact interop with OpenSSL rather than only proving self-consistency.This is the third member of the advertised-but-unsignable digest census after #33518 (
sha3-*) and #34348 (ripemd160/md4silent-false verify). Whichever of #33518 and this PR merges second will need a trivial rebase of the sharedp_ec.ccwhitelist andkPKCS1SigPrefixeshunks.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file