Repository navigation
Conversation
crypto.verify() and crypto.subtle.verify() return false when the Ed25519 public key or the R of the signature is a small-order point (89fbb78). No test covered that. Add the cases and the fixture of the superseded pull request 46509: five cases for node:crypto, five for WebCrypto, over fourteen encodings that each have a witness message. Update test-webcrypto-sign-verify-eddsa.js to the Node v26.12.1 copy, which has Node's small-order vector.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Small-order vectors and signature helpers test/js/web/crypto/ed25519-small-order.ts |
Adds Ed25519 small-order point fixtures and helpers for constructing signatures used in verification tests. |
Node crypto verification coverage test/js/node/crypto/crypto.key-objects.test.ts |
Adds verification tests for small-order public keys and signature points, identity cases, and multiple key input forms. Tests also check valid and tampered signatures and digest-based verification errors. |
Web Crypto verification and EdDSA assertions test/js/web/crypto/web-crypto.test.ts, test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.js |
Adds Web Crypto verification tests for small-order vectors. Updates FIPS RSA-PSS key sizes and Ed448 oversized-context error assertions. |
Possibly related issues
- oven-sh/bun#46252 reports forged Ed25519 signatures accepted by
crypto.subtle.verifyandnode:crypto.verifyunder small-order public keys. This PR adds regression tests for rejecting small-order public keys and signatureRpoints.
Possibly related PRs
- oven-sh/bun#47567 adds production checks for small-order Ed25519 and Ed448 points during signature verification. This PR adds tests for related Ed25519 verification cases.
Suggested reviewers: dylan-conway
Merge Risk: 
004d2
-
The changes add verification regression tests without an established runtime behavior change. No actionable merge-blocking issue remains; merge after normal checks pass.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: tests for rejecting small-order Ed25519 points during verification. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description explains what the PR changes and how the author verified it. It does not use the template’s exact headings, but it provides the required information in the Problem, Fix, and Verified s… Full details: Description check
-
Explanation
The description explains what the PR changes and how the author verified it. It does not use the template’s exact headings, but it provides the required information in the Problem, Fix, and Verified sections.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
|
Status: ready for review. This PR adds tests only. How the tests were checked
bun bd test test/js/web/crypto/web-crypto.test.ts -t "small-order"
bun bd test test/js/node/crypto/crypto.key-objects.test.ts -t "small-order"
bun bd test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.jsCI: build 136523 finished red on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the edits to the ported Node file test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.js: the rewritten Ed448 context-length assertions and the Ed448 small-order vector are unreachable on Bun (test/js/node/test/fixtures/crypto/eddsa.js only pushes Ed448 when !process.features.openssl_is_boringssl, and isBoringSSL is exported from common/crypto.js), so they cannot fail here; whether the file now matches upstream v26.12.1 byte-for-byte is a claim I could not verify offline and is worth a human check given the directory's verbatim-upstream rule. The 14 encodings in ed25519-small-order.ts match the kEd25519SmallOrderPoints table in src/jsc/bindings/ncrypto.cpp one-for-one, and the helper module has no .test. suffix so it is not collected as a test on its own.
Extended reasoning...
Test-only change (+352/-16) adding Ed25519 small-order verify coverage to web-crypto.test.ts and crypto.key-objects.test.ts via a new shared fixture, plus edits to a ported Node test; it exercises the crypto verify path but touches no source. Two inline findings were posted (public-key branch of hasSmallOrderEdDsaPoint never reached because every key case also uses a small-order R; bare toThrow). The ported-file edits are dead on BoringSSL builds and the upstream-match claim is unverifiable offline, so a human look is still worthwhile.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Every case for a small-order public key used a signature whose R is the identity point, so the check on R rejected it before the key was read. Add a forged signature with an honest R for each of the 14 encodings, in both suites: R is the public key of the RFC 8032 TEST 1 seed, S is the scalar of that seed, and the message makes h a multiple of 8. Before 89fbb78 all 14 verify as true. Assert the error code of a digest on an Ed25519 key in place of a bare toThrow().
|
On the vendored file: curl -sL https://raw.githubusercontent.com/nodejs/node/v26.12.1/test/parallel/test-webcrypto-sign-verify-eddsa.js | sha256sum
sha256sum test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.jsThe Ed448 parts of that file do not run on BoringSSL, as the review says. The two inline findings are addressed in 3e1469b. |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Both points from my earlier review are addressed by the second commit (forgery cases with an honest R now reach the public-key branch in both suites; the digest assertion checks a specific error code), and only a nit remains inline.
Beyond that, I also checked: the forgeryWithHonestR construction (S = a, R = [a]B, message chosen so h is a multiple of 8) correctly makes the unfiltered equation hold for every point of order dividing 8; isBoringSSL is exported from test/js/node/test/common/crypto.js; and the rewritten Ed448 context assertions in the ported file are unreachable under Bun because the eddsa fixture only adds Ed448 when !process.features.openssl_is_boringssl. I could not verify offline that test-webcrypto-sign-verify-eddsa.js is a verbatim Node v26.12.1 copy, so a human diff against upstream is worth doing before merge.
Extended reasoning...
Test-only change (+421/-16, no src/ edits) adding Ed25519 small-order rejection coverage to node:crypto and WebCrypto suites via a new shared fixture with bigint scalar arithmetic, plus an update of a ported Node test file. It exercises crypto verification paths but changes no runtime code. The second commit addressed both prior inline findings; the one remaining finding is a tautological assertion nit. Not approved outright because the ported-file "verbatim upstream copy" claim cannot be verified offline and the hand-rolled fixture math warrants a human glance.
The assertion on the first 32 bytes of the forged signature compared a constant with itself. Compare the R of every forged signature with the public key that node:crypto derives from the seed.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I checked the current diff against the earlier threads: the honest-R forgery now reaches the public-key branch for all 14 encodings, the digest assertion pins ERR_OSSL_EVP_COMMAND_NOT_SUPPORTED, and the R check in crypto.key-objects.test.ts now compares against the key derived from the seed rather than the fixture constant. I also confirmed isBoringSSL is exported by test/js/node/test/common/crypto.js, so the Ed448 vector gate in the updated Node file resolves; I could not verify the claim that the Node file is byte-identical to upstream v26.12.1, which a human may want to spot-check.
Extended reasoning...
Test-only change across four files: a new Ed25519 small-order fixture module, parallel suites in web-crypto.test.ts and crypto.key-objects.test.ts, and an update to the vendored Node test test-webcrypto-sign-verify-eddsa.js. It touches no production code and only exercises the existing verify rejection path. Not approving because a verified finding is being withheld from the posted set and the vendored-file upstream-identity claim cannot be checked offline.
The fixture listed a message for each point as a constant that nothing checked. An entry with a wrong message would verify as false on every build, and the suites could not tell. Every forgery now searches for its own message, as the one with an honest R does. The constants were correct: before 89fbb78 all 14 entries verify as true as a key, and all 8 canonical ones as R, with the old messages and with the derived ones.
There was a problem hiding this comment.
Thanks for the follow-up commits — every point from the earlier rounds is now addressed in the code, and this automated review found no bugs in the latest version. A human look is still worthwhile for one thing: the edit to test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.js is described as a byte-identical sync to Node v26.12.1, which cannot be verified from this checkout, and that directory is meant to hold unmodified upstream tests.
What was reviewed:
- The fixture math in
test/js/web/crypto/ed25519-small-order.ts: each forgery satisfies[S]B = R + [h]Awith h reduced mod L before the mod-8 test, matching BoringSSL's verifier; the 14-entry table matcheskEd25519SmallOrderPointsinsrc/jsc/bindings/ncrypto.cpprow for row. forgeryWithHonestRcases now reach the public-key branch atncrypto.cpp:1996-2004with a prime-order R, and the node:crypto suite checks that R against the seed's exported public key.- The ported Node file:
isBoringSSLis exported bytest/js/node/test/common/crypto.js,crypto.getFipsexists in Bun, and the Ed448 branches are unreachable under BoringSSL becausefixtures/crypto/eddsa.jsskips Ed448 — so the changed context-message assertions cannot affect Bun's run.
Extended reasoning...
Test-only change (+405/-16) adding Ed25519 small-order rejection coverage to web-crypto.test.ts and crypto.key-objects.test.ts via a new shared helper, plus an edit to a ported Node test in the parallel directory; no src/ is touched and the filter it exercises (hasSmallOrderEdDsaPoint) already exists on the base. It touches no security-sensitive runtime surface itself, only tests for a crypto verification path. All four inline findings from prior runs are addressed in the current code and no new issues were found. Defer rather than approve because the PR's claims that the parallel-directory file is byte-identical to upstream Node and that the cases fail on the pre-fix build cannot be verified here (no network or build tools), and that directory's convention is to not carry local edits.
Related to #46252
Problem
crypto.verify()andcrypto.subtle.verify()returnfalsefor an Ed25519 public key or signatureRthat is a small-order point. No test covers that.Fix
test/js/web/crypto/ed25519-small-order.ts: 14 encodings of small-order points. Each forgery derives a message that an unfiltered verifier accepts.test/js/web/crypto/web-crypto.test.tsand six totest/js/node/crypto/crypto.key-objects.test.ts. One case in each suite forges a signature with an honestR, so only the check on the public key can reject it.test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.jsto the Node v26.12.1 copy (byte-identical). It has Node's small-order vector.Background
[S]B = R + [h]A.Ais the public key andRis one half of the signature.Ais such a point andhis a multiple of 8,[h]Avanishes. ThenR = [S]Bverifies with no private key.Behaviour change: none
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/crypto/crypto.key-objects.test.ts