Repository navigation
fix(node_crypto): apply RFC 7748 scalar decoding for X448 - #36466
Conversation
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Correct fix and a good consolidation. The node_crypto paths were doing
EdwardsScalar::from_bytes_mod_order(&scalar_bytes.into())which reduces the scalar modulo the Ed448 group order — but RFC 7748's decodeScalar448 clamps instead (clear the low two bits, set bit 447), and a clamped scalar deliberately exceeds the group order. Reducing it produces a different point, so an imported X448 key derived a different public key and a different shared secret in Deno than in Node or OpenSSL. That's a silent interop failure, and the worst kind: key agreement just doesn't agree.
The right call was noticing WebCrypto already had this correct (from #35155) and that the two implementations had diverged, then hoisting the correct one rather than copying the fix. libs/crypto is a slightly odd home for a pure-Rust curve implementation given the crate is otherwise the aws-lc wrapper, but it's the natural shared point between ext/crypto and ext/node_crypto and it pulls in no deno_core dependency, so no layering problem.
Checked the mechanical parts: BASE_POINT as [5, 0, ...] is u = 5 and is equivalent to the MontgomeryPoint::GENERATOR it replaces — the moved public_key_from_scalar test is what pins that. subtle becoming a direct dependency of ext/crypto is just replacing the re-export that came via ed448-goldilocks. Low-order public key rejection is preserved on both paths.
The unit_node test is the one that matters most: hand-building the PKCS#8/SPKI wrappers means it exercises the imported-key path, which is exactly where the bug lived, rather than only generated keys where both sides would have been consistently wrong.
CI has lint title and pre-build failing, but both at ~15m0s, which reads as job timeouts rather than real failures — worth a re-run to confirm before merging. LGTM.
## Summary - share the RFC 7748 X448 operation between WebCrypto and Node crypto - apply X448 scalar decoding when deriving Node-compatible public keys and shared secrets - retain rejection of low-order public keys ## Problem The Node crypto paths converted 56-byte X448 private keys by reducing them modulo the Ed448 group order. RFC 7748 instead requires clearing the scalar's two least-significant bits and setting its most-significant bit before scalar multiplication. As a result, imported X448 private keys could derive different public keys and shared secrets in Deno than in Node/OpenSSL. Both Node crypto paths now use the same RFC 7748 implementation as WebCrypto. ## Tests - RFC 7748 public-key and shared-secret vectors - imported PKCS#8 key compatibility with Node/OpenSSL - generated peer agreement - low-order public-key rejection - focused Rust unit tests, Node crypto unit test, clippy, formatting, and TypeScript lint
2 participants
Summary
Problem
The Node crypto paths converted 56-byte X448 private keys by reducing them modulo the Ed448 group order. RFC 7748 instead requires clearing the scalar's two least-significant bits and setting its most-significant bit before scalar multiplication.
As a result, imported X448 private keys could derive different public keys and shared secrets in Deno than in Node/OpenSSL. Both Node crypto paths now use the same RFC 7748 implementation as WebCrypto.
Tests