Repository navigation
node:crypto, webcrypto: reject small-order Ed25519 points in verify #46509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,13 @@ import { bunEnv, bunExe, isASAN, isDebug, isWindows, tempDir } from "harness"; | |
| import { totalmem } from "node:os"; | ||
| import { createContext, runInContext, runInThisContext, Script } from "node:vm"; | ||
| import path from "path"; | ||
| import { | ||
| ED25519_IDENTITY, | ||
| ED25519_SMALL_ORDER_POINTS, | ||
| hexBytes, | ||
| identityRSignature, | ||
| zeroSignatureWithR, | ||
| } from "../../web/crypto/ed25519-small-order"; | ||
|
|
||
| function readFile(...args: [path: string, encoding: BufferEncoding]) { | ||
| const result = fs.readFileSync(...args); | ||
|
|
@@ -1834,6 +1841,76 @@ test("Ed25519 should work", async () => { | |
| } | ||
| }); | ||
|
|
||
| // https://github.com/oven-sh/bun/issues/46252 | ||
| describe("Ed25519 verify rejects small-order points", () => { | ||
| const jwkKey = (hex: string) => | ||
| createPublicKey({ | ||
| key: { kty: "OKP", crv: "Ed25519", x: Buffer.from(hexBytes(hex)).toString("base64url") }, | ||
| format: "jwk", | ||
| }); | ||
|
|
||
| test("a small-order public key imports but verifies nothing", () => { | ||
| const results: Record<string, boolean> = {}; | ||
| for (const { name, hex, message } of ED25519_SMALL_ORDER_POINTS) { | ||
| const key = jwkKey(hex); | ||
| expect(key.asymmetricKeyType).toBe("ed25519"); | ||
| results[name] = verify(null, Buffer.from(message), key, zeroSignatureWithR(ED25519_IDENTITY)); | ||
| } | ||
| expect(results).toEqual(Object.fromEntries(ED25519_SMALL_ORDER_POINTS.map(p => [p.name, false]))); | ||
| }); | ||
|
|
||
| test("a small-order R is rejected", () => { | ||
| const results: Record<string, boolean> = {}; | ||
| for (const { name, hex, messageAsR } of ED25519_SMALL_ORDER_POINTS) { | ||
| if (messageAsR === undefined) continue; | ||
| results[name] = verify(null, Buffer.from(messageAsR), jwkKey(hex), zeroSignatureWithR(hex)); | ||
| } | ||
| expect(Object.keys(results)).toHaveLength(8); | ||
| expect(results).toEqual(Object.fromEntries(Object.keys(results).map(name => [name, false]))); | ||
| }); | ||
|
|
||
| test("an identity R under an honest key is rejected", () => { | ||
| const { publicKey, privateKey } = generateKeyPairSync("ed25519"); | ||
| const message = Buffer.from("hello"); | ||
| const signature = identityRSignature(privateKey.export({ format: "jwk" }), message); | ||
| expect(verify(null, message, publicKey, signature)).toBeFalse(); | ||
| }); | ||
|
|
||
| test("every key input form and the callback form reject the identity key", async () => { | ||
| const keyObject = jwkKey(ED25519_IDENTITY); | ||
| const message = Buffer.from("any message at all"); | ||
| const signature = zeroSignatureWithR(ED25519_IDENTITY); | ||
| const pem = keyObject.export({ format: "pem", type: "spki" }) as string; | ||
| const der = keyObject.export({ format: "der", type: "spki" }) as Buffer; | ||
| expect(createPublicKey(pem).equals(keyObject)).toBeTrue(); | ||
|
|
||
| const { promise, resolve, reject } = Promise.withResolvers<boolean>(); | ||
| verify(null, message, keyObject, signature, (err, result) => (err ? reject(err) : resolve(result))); | ||
|
|
||
| expect({ | ||
| keyObject: verify(null, message, keyObject, signature), | ||
| pem: verify(null, message, pem, signature), | ||
| der: verify(null, message, { key: der, format: "der", type: "spki" }, signature), | ||
| cryptoKey: verify(null, message, keyObject.toCryptoKey("Ed25519", false, ["verify"]), signature), | ||
| callback: await promise, | ||
| }).toEqual({ keyObject: false, pem: false, der: false, cryptoKey: false, callback: false }); | ||
| }); | ||
|
|
||
| test("a real signature still verifies and a digest is still rejected", async () => { | ||
| const { publicKey, privateKey } = generateKeyPairSync("ed25519"); | ||
| const message = Buffer.from("hello"); | ||
| const signature = sign(null, message, privateKey); | ||
| expect(verify(null, message, publicKey, signature)).toBeTrue(); | ||
| const { promise, resolve, reject } = Promise.withResolvers<boolean>(); | ||
| verify(null, message, publicKey, signature, (err, result) => (err ? reject(err) : resolve(result))); | ||
| expect(await promise).toBeTrue(); | ||
| const tampered = Buffer.from(signature); | ||
| tampered[0] ^= 1; | ||
| expect(verify(null, message, publicKey, tampered)).toBeFalse(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): the "digest is still rejected" assertion is a bare Why this was flaggedThe test at test/js/node/crypto/crypto.key-objects.test.ts:1909 calls Verification: nit. Whenever the digest-path error for an Ed25519 |
||
| expect(() => verify("sha256", message, publicKey, signature)).toThrow(); | ||
| }); | ||
| }); | ||
|
|
||
| test("ECDSA should work", async () => { | ||
| const { publicKey, privateKey } = generateKeyPairSync("ec", { namedCurve: "prime256v1" }); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,44 @@ const vectors = require('../fixtures/crypto/eddsa')(); | |
|
|
||
| const supportsContext = hasOpenSSL(3, 2); | ||
|
|
||
| const smallOrderVerifyVectors = [ | ||
| { | ||
| name: 'Ed25519', | ||
| publicKey: Buffer.from( | ||
| 'c7176a703d4dd84fba3c0b760d10670f2a2053fa2c39ccc64ec7fd7792ac03fa', | ||
| 'hex'), | ||
| signature: Buffer.from( | ||
| 'c7176a703d4dd84fba3c0b760d10670f2a2053fa2c39ccc64ec7fd7792ac037a' + | ||
| '0000000000000000000000000000000000000000000000000000000000000000', | ||
| 'hex'), | ||
| data: Buffer.from( | ||
| '8c93255d71dcab10e8f379c26200f3c7bd5f09d9bc3068d3ef4edeb4853022b6', | ||
| 'hex'), | ||
| }, | ||
| ]; | ||
|
|
||
| if (!process.features.openssl_is_boringssl) { | ||
| smallOrderVerifyVectors.push({ | ||
| name: 'Ed448', | ||
| publicKey: Buffer.concat([Buffer.from([1]), Buffer.alloc(56)]), | ||
| signature: Buffer.concat([Buffer.from([1]), Buffer.alloc(113)]), | ||
| data: Buffer.from([1, 2, 3]), | ||
| }); | ||
| } | ||
|
|
||
| async function testSmallOrderVerify({ name, publicKey, signature, data }) { | ||
| const key = await subtle.importKey( | ||
| 'raw', | ||
| publicKey, | ||
| { name }, | ||
| false, | ||
| ['verify']); | ||
|
|
||
| assert.strictEqual( | ||
| await subtle.verify({ name }, key, signature, data), | ||
| false); | ||
| } | ||
|
|
||
|
Comment on lines
+18
to
+55
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
# Check whether Bun defines process.features.openssl_is_boringssl
rg -n 'openssl_is_boringssl' --glob '!test/js/node/test/**' | head -20Repository: oven-sh/bun Length of output: 1244 🏁 Script executed: set -eu
base='7a748413f944621d0d8cdb0708ec807a9dfc31fa'
head='c2207b316b3ada3daa7b37ff2cb2f6527568c005'
printf '%s\n' '--- changed paths ---'
git diff --name-status "$base" "$head" -- test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.js test/js/web/crypto/web-crypto.test.ts
printf '%s\n' '--- exact diff for reviewed file ---'
git diff --find-renames "$base" "$head" -- test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.js
printf '%s\n' '--- file header and relevant test structure ---'
nl -ba test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.js | sed -n '1,90p'
rg -n -F -- 'verify rejects small-order' test/js/web/crypto/web-crypto.test.ts || true
rg -n -F -- 'c7176a703d4dd84fba3c0b760d10670f2a2053fa2c39ccc64ec7fd7792ac03fa' test/js/web/crypto/web-crypto.test.ts || true
printf '%s\n' '--- process feature definition context ---'
nl -ba src/jsc/bindings/BunProcess.cpp | sed -n '4675,4715p'
printf '%s\n' '--- repository guidance mentioning vendored Node tests ---'
rg -n -i -F -- 'vendored' -- test src .github AGENTS.md CLAUDE.md 2>/dev/null || true
rg -n -i -F -- 'test/js/node/test/parallel' -- . 2>/dev/null | head -40 || trueRepository: oven-sh/bun Length of output: 39370 🏁 Script executed: set -eu
printf '%s\n' '--- test tree guidance ---'
nl -ba test/tsconfig.json | sed -n '35,55p'
find test/js/node/test -maxdepth 2 -iname 'README*' -o -iname '*upstream*' -o -iname '*vendor*' | sort | head -40
printf '%s\n' '--- Bun-owned small-order coverage ---'
nl -ba test/js/web/crypto/web-crypto.test.ts | sed -n '540,635p'
printf '%s\n' '--- provenance comments in the reviewed Node test ---'
rg -n -i -e 'node\.js' -e 'upstream' -e 'ported' -e 'adapt' -e 'bun' test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.js test/js/node/test/common/index.js test/js/node/test/README* 2>/dev/null || true
printf '%s\n' '--- recent history for the reviewed test path ---'
git log -8 --format='%h %s' -- test/js/node/test/parallel/test-webcrypto-sign-verify-eddsa.jsRepository: oven-sh/bun Length of output: 13047 Keep the vendored Node test unchanged.
Suggested removal-const smallOrderVerifyVectors = [
- {
- name: 'Ed25519',
- publicKey: Buffer.from(
- 'c7176a703d4dd84fba3c0b760d10670f2a2053fa2c39ccc64ec7fd7792ac03fa',
- 'hex'),
- signature: Buffer.from(
- 'c7176a703d4dd84fba3c0b760d10670f2a2053fa2c39ccc64ec7fd7792ac037a' +
- '0000000000000000000000000000000000000000000000000000000000000000',
- 'hex'),
- data: Buffer.from(
- '8c93255d71dcab10e8f379c26200f3c7bd5f09d9bc3068d3ef4edeb4853022b6',
- 'hex'),
- },
-];
-
-if (!process.features.openssl_is_boringssl) {
- smallOrderVerifyVectors.push({
- name: 'Ed448',
- publicKey: Buffer.concat([Buffer.from([1]), Buffer.alloc(56)]),
- signature: Buffer.concat([Buffer.from([1]), Buffer.alloc(113)]),
- data: Buffer.from([1, 2, 3]),
- });
-}
-
-async function testSmallOrderVerify({ name, publicKey, signature, data }) {
- const key = await subtle.importKey(
- 'raw',
- publicKey,
- { name },
- false,
- ['verify']);
-
- assert.strictEqual(
- await subtle.verify({ name }, key, signature, data),
- false);
-}
-
- smallOrderVerifyVectors.forEach((vector) => {
- variations.push(testSmallOrderVerify(vector));
- });The separate BoringSSL concern does not apply to Bun. 🤖 Prompt for AI AgentsSource: Learnings |
||
| async function testVerify({ name, | ||
| context, | ||
| publicKeyBuffer, | ||
|
|
@@ -260,6 +298,9 @@ async function testSign({ name, | |
| variations.push(testVerify(vector)); | ||
| variations.push(testSign(vector)); | ||
| }); | ||
| smallOrderVerifyVectors.forEach((vector) => { | ||
| variations.push(testSmallOrderVerify(vector)); | ||
| }); | ||
|
|
||
| await Promise.all(variations); | ||
| })().then(common.mustCall()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Maintainers get no test that catches the public-key half of the new filter being removed: every new case still passes, so a regression would re-admit the forgery from #46252. Each small-order-key case pairs the key with a small-order R (identity or the same point), so the R clause at ncrypto.cpp:3717 alone rejects it. Fix: add a case under a small-order key with an ordinary R that only the publicKey clause rejects, e.g. key 01 00..00 with signature R = 5866666666666666666666666666666666666666666666666666666666666666, S = 1, any message, expecting false; cover the one-shot, callback and subtle.verify paths. Same pattern at web-crypto.test.ts:595, web-crypto.test.ts:625, crypto.key-objects.test.ts:1882. [also at: test/js/web/crypto/web-crypto.test.ts:595 - Maintainers get no test that pins the public-key half of the new check: a build that drops the key clause still passes every new test.]
Why this was flagged
The filter in src/jsc/bindings/ncrypto.cpp:3717 has two load-bearing clauses: reject a small-order R and reject a small-order public key. Every new test uses a small-order R: crypto.key-objects.test.ts:1857 and web-crypto.test.ts:595 sign with zeroSignatureWithR(ED25519_IDENTITY) (R = identity), the 'small-order R' cases use R = the key point, and the ported test-webcrypto-sign-verify-eddsa.js:25 vector uses an order-8 R. If the
!isSmallOrderEd25519Point(publicKey.data)clause is deleted, the R clause still returns false for all of them and the suite stays green. But with that clause gone a verifier accepts a forgery: under public key A = identity, signature R = encoding of the base point 58 66..66, S = 1 satisfies [1]B = R + [h]A for every message, and ED25519_verify at ncrypto.cpp:3710 returns 1. R = B is not in kEd25519SmallOrderPoints, so only the public-key clause rejects it. On the base branch this forgery is accepted; after merging it is rejected, but no test proves the clause that rejects it, contrary to the repository's own rule that deleting each load-bearing clause of a fix must break at least one test.Verification: src/jsc/bindings/ncrypto.cpp:3717
return !isSmallOrderEd25519Point(signature.data) && !isSmallOrderEd25519Point(publicKey.data);has two independent clauses, and the publicKey clause is independently load-bearing. Every new test pairs a small-order key with a small-order R, so the R clause alone turns each one false. Deleting the publicKey clause therefore leaves every assertion green.