fix(ext/node): fix multiple DiffieHellman crypto bugs#32531
Merged
bartlomieju merged 10 commits intodenoland:mainfrom Mar 9, 2026
Merged
fix(ext/node): fix multiple DiffieHellman crypto bugs#32531bartlomieju merged 10 commits intodenoland:mainfrom
bartlomieju merged 10 commits intodenoland:mainfrom
Conversation
Upgrades ed448-goldilocks from 0.8.3 to 0.14.0-pre.10 which adds signing support. Implements Ed448 and X448 key generation, import/export (PKCS#8, SPKI, JWK), and Ed448 sign/verify for Node.js crypto compat. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add missing modp1 (768-bit) and modp2 (1024-bit) DH groups - Fix prime conversion: Buffer.from(array) truncated 32-bit words to single bytes, now uses writeUInt32BE for correct serialization - Store generator with minimal byte representation (02 not 00000002) - Zero-pad computeSecret output to prime length per RFC 4346 - Fix DiffieHellmanGroup constructor identity - Handle ArrayBufferView prime argument in createDiffieHellman - Make generateKeys() idempotent when keys already exist, and only regenerate public key after setPrivateKey - Add input validation and callback support to diffieHellman() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… and panic - Fix BigUint::from_slice treating big-endian u32 MODULUS words as little-endian limbs in DH group key generation - Fix panic when deriving DH public key from private key by implementing modular exponentiation (g^x mod p) - Fix PKCS#8 DH private key parsing to decode DER INTEGER from OCTET STRING - Fix SPKI DH public key parsing to decode DER INTEGER from BIT STRING - Fix PKCS#8/SPKI export to DER-encode keys as ASN.1 INTEGERs - Fix ASN.1 prime encoding in dh_group_generate (was using native endianness) - Fix generator encoding to use minimal byte representation - Compare DH params by integer value instead of byte encoding - Remove dead u32_slice_to_u8_slice function Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kajukitli
reviewed
Mar 9, 2026
Contributor
kajukitli
left a comment
There was a problem hiding this comment.
Solid fix for the DH endianness and ASN.1 encoding issues. Looked through the changes:
Good stuff:
- The
BigUint::from_slice→from_bytes_befix indh.rsis correct. The MODULUS words were big-endian but being treated as little-endian limbs. - Zero-padding computeSecret to prime length per RFC 4346 - this was definitely causing interop issues
- Comparing DH params by integer value instead of byte encoding in
op_node_diffie_hellman- handles the leading 0x00 ASN.1 differences correctly - Proper DER INTEGER encoding/decoding for PKCS#8 and SPKI keys
One bug:
In keys.rs around line 1080 (ID_ED448 branch), there's:
.map_err(|_| X509PublicKeyError::InvalidEd25519Key)?;Should be InvalidEd448Key - copy/paste error.
Nit:
In diffiehellman.ts, using op_node_dh_compute_secret to derive the public key when private key is set externally works, but the naming is a bit confusing since "compute_secret" usually implies computing the shared secret. A comment would help clarify this is computing g^privateKey mod p.
Otherwise lgtm
kajukitli
reviewed
Mar 9, 2026
Contributor
kajukitli
left a comment
There was a problem hiding this comment.
Suggested fixes as inline diff:
diff --git a/ext/node_crypto/keys.rs b/ext/node_crypto/keys.rs
--- a/ext/node_crypto/keys.rs
+++ b/ext/node_crypto/keys.rs
@@ -1080,9 +1080,9 @@ impl KeyObjectHandle {
ID_ED448 => {
let data = spki.subject_public_key.as_ref();
let point_bytes: &[u8; 57] = data
- .try_into()
- .map_err(|_| X509PublicKeyError::InvalidEd25519Key)?;
+ .try_into()
+ .map_err(|_| X509PublicKeyError::InvalidEd448Key)?;
let vk = ed448_goldilocks::VerifyingKey::from_bytes(point_bytes)
- .map_err(|_| X509PublicKeyError::InvalidEd25519Key)?;
+ .map_err(|_| X509PublicKeyError::InvalidEd448Key)?;
AsymmetricPublicKey::Ed448(vk)
}
littledivy
reviewed
Mar 9, 2026
littledivy
approved these changes
Mar 9, 2026
Member
littledivy
left a comment
There was a problem hiding this comment.
LGTM, with the error variant fix
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…vements # Conflicts: # tests/node_compat/config.jsonc
…ation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BigUint::from_slicetreating big-endian u32 MODULUS words as little-endian limbs)g^x mod p)computeSecretzero-padding,generateKeysidempotency,ArrayBufferViewprime handling,diffieHellman()input validationTest plan
cargo test --test node_compat "parallel::test-crypto-dh-"— 10/13 pass (3 remaining needverifyErrorand stateless DH param normalization)cargo test --test node_compat "pummel::test-crypto-dh-"— 2/2 pass🤖 Generated with Claude Code