From 721b918296d2b7653149a566fe79b91c343d43c6 Mon Sep 17 00:00:00 2001 From: larabr <7375870+larabr@users.noreply.github.com> Date: Fri, 25 Jul 2025 18:54:25 +0200 Subject: [PATCH] Key validation: use WebCrypto API when available for curve25519 For Ed25519/Ed25519Legacy native validation code does a sign-verify check over random data. This is faster than re-deriving the public point using tweetnacl. If the native implementation is not available, we fall back to re-deriving the public point only. For X25519/Curve25519Legacy, both the native and fallback flows do an ecdh exchange; in the fallback case, this results in slower performance compared to the existing check, but encryption subkeys are hardly ever validated directly (only in case of gnu-dummy keys), and this solution keeps the code simpler. Separately, all validation tests have been updated to use valid params from a different key, rather than corrupted parameters. --- src/crypto/public_key/elliptic/ecdh_x.js | 21 +++++---- src/crypto/public_key/elliptic/eddsa.js | 39 +++++++++++----- .../public_key/elliptic/eddsa_legacy.js | 16 +++---- src/crypto/public_key/elliptic/oid_curves.js | 15 +++---- test/crypto/validate.js | 44 +++++++++++-------- 5 files changed, 79 insertions(+), 56 deletions(-) diff --git a/src/crypto/public_key/elliptic/ecdh_x.js b/src/crypto/public_key/elliptic/ecdh_x.js index 43ce91b2..1dac58b9 100644 --- a/src/crypto/public_key/elliptic/ecdh_x.js +++ b/src/crypto/public_key/elliptic/ecdh_x.js @@ -82,14 +82,19 @@ export async function generate(algo) { */ export async function validateParams(algo, A, k) { switch (algo) { - case enums.publicKey.x25519: { - /** - * Derive public point A' from private key - * and expect A == A' - */ - const { publicKey } = x25519.box.keyPair.fromSecretKey(k); - return util.equalsUint8Array(A, publicKey); - } + case enums.publicKey.x25519: + // Validation is typically not run for ECDH, since encryption subkeys are only validated + // for gnu-dummy keys. + // So, for simplicity, we do an encrypt-decrypt round even if WebCrypto support is not available + try { + const { ephemeralPublicKey, sharedSecret } = await generateEphemeralEncryptionMaterial(algo, A); + const recomputedSharedSecret = await recomputeSharedSecret(algo, ephemeralPublicKey, A, k); + + return util.equalsUint8Array(sharedSecret, recomputedSharedSecret); + } catch (_) { + return false; + } + case enums.publicKey.x448: { const x448 = await util.getNobleCurve(enums.publicKey.x448); /** diff --git a/src/crypto/public_key/elliptic/eddsa.js b/src/crypto/public_key/elliptic/eddsa.js index 86d9f17e..7afdde85 100644 --- a/src/crypto/public_key/elliptic/eddsa.js +++ b/src/crypto/public_key/elliptic/eddsa.js @@ -23,7 +23,7 @@ import ed25519 from '@openpgp/tweetnacl'; import util from '../../../util'; import enums from '../../../enums'; -import { getHashByteLength } from '../../hash'; +import { computeDigest, getHashByteLength } from '../../hash'; import { getRandomBytes } from '../../random'; import { b64ToUint8Array, uint8ArrayToB64 } from '../../../encoding/base64'; @@ -179,15 +179,34 @@ export async function verify(algo, hashAlgo, { RS }, m, publicKey, hashed) { */ export async function validateParams(algo, A, seed) { switch (algo) { - case enums.publicKey.ed25519: { - /** - * Derive public point A' from private key - * and expect A == A' - * TODO: move to sign-verify using WebCrypto (same as ECDSA) when curve is more widely implemented - */ - const { publicKey } = ed25519.sign.keyPair.fromSeed(seed); - return util.equalsUint8Array(A, publicKey); - } + case enums.publicKey.ed25519: + // If webcrypto support is available, we sign-verify random data, as the import-export + // functions might not implement validity checks. + // If we need to fallback to JS, we instead only re-derive the public key, + // as this is much faster than sign-verify. + try { + const webCrypto = util.getWebCrypto(); + const jwkPrivate = privateKeyToJWK(algo, A, seed); + const jwkPublic = publicKeyToJWK(algo, A); + + const privateCryptoKey = await webCrypto.importKey('jwk', jwkPrivate, 'Ed25519', false, ['sign']); + const publicCryptoKey = await webCrypto.importKey('jwk', jwkPublic, 'Ed25519', false, ['verify']); + + const randomData = getRandomBytes(8); + const signature = new Uint8Array( + await webCrypto.sign('Ed25519', privateCryptoKey, randomData) + ); + + const verified = await webCrypto.verify('Ed25519', publicCryptoKey, signature, randomData); + return verified; + } catch (err) { + if (err.name !== 'NotSupportedError') { + return false; + } + + const { publicKey } = ed25519.sign.keyPair.fromSeed(seed); + return util.equalsUint8Array(A, publicKey); + } case enums.publicKey.ed448: { const ed448 = await util.getNobleCurve(enums.publicKey.ed448); diff --git a/src/crypto/public_key/elliptic/eddsa_legacy.js b/src/crypto/public_key/elliptic/eddsa_legacy.js index 682f53bb..9ab45da8 100644 --- a/src/crypto/public_key/elliptic/eddsa_legacy.js +++ b/src/crypto/public_key/elliptic/eddsa_legacy.js @@ -21,12 +21,11 @@ * @module crypto/public_key/elliptic/eddsa_legacy */ -import nacl from '@openpgp/tweetnacl'; import util from '../../../util'; import enums from '../../../enums'; import { getHashByteLength } from '../../hash'; import { CurveWithOID, checkPublicPointEnconding } from './oid_curves'; -import { sign as eddsaSign, verify as eddsaVerify } from './eddsa'; +import { sign as eddsaSign, verify as eddsaVerify, validateParams as eddsaValidateParams } from './eddsa'; /** * Sign a message using the provided legacy EdDSA key @@ -97,12 +96,9 @@ export async function validateParams(oid, Q, k) { return false; } - /** - * Derive public point Q' = dG from private key - * and expect Q == Q' - */ - const { publicKey } = nacl.sign.keyPair.fromSeed(k); - const dG = new Uint8Array([0x40, ...publicKey]); // Add public key prefix - return util.equalsUint8Array(Q, dG); - + // First byte is relevant for encoding purposes only + if (Q.length < 1 || Q[0] !== 0x40) { + return false; + } + return eddsaValidateParams(enums.publicKey.ed25519, Q.subarray(1), k); } diff --git a/src/crypto/public_key/elliptic/oid_curves.js b/src/crypto/public_key/elliptic/oid_curves.js index 7404cbe6..975dcd8e 100644 --- a/src/crypto/public_key/elliptic/oid_curves.js +++ b/src/crypto/public_key/elliptic/oid_curves.js @@ -26,7 +26,7 @@ import { uint8ArrayToB64, b64ToUint8Array } from '../../../encoding/base64'; import OID from '../../../type/oid'; import { UnsupportedError } from '../../../packet/packet'; import { generate as eddsaGenerate } from './eddsa'; -import { generate as ecdhXGenerate } from './ecdh_x'; +import { generate as ecdhXGenerate, validateParams as ecdhXValidateParams } from './ecdh_x'; const webCrypto = util.getWebCrypto(); const nodeCrypto = util.getNodeCrypto(); @@ -252,17 +252,12 @@ async function validateStandardParams(algo, oid, Q, d) { } if (curveName === enums.curve.curve25519Legacy) { - d = d.slice().reverse(); - // Re-derive public point Q' - const { publicKey } = nacl.box.keyPair.fromSecretKey(d); - - Q = new Uint8Array(Q); - const dG = new Uint8Array([0x40, ...publicKey]); // Add public key prefix - if (!util.equalsUint8Array(dG, Q)) { + const dLittleEndian = d.slice().reverse(); + // First byte is relevant for encoding purposes only + if (Q.length < 1 || Q[0] !== 0x40) { return false; } - - return true; + return ecdhXValidateParams(enums.publicKey.x25519, Q.subarray(1), dLittleEndian); } const nobleCurve = await util.getNobleCurve(enums.publicKey.ecdsa, curveName); // excluding curve25519Legacy, ecdh and ecdsa use the same curves diff --git a/test/crypto/validate.js b/test/crypto/validate.js index 8ed63089..7fd74124 100644 --- a/test/crypto/validate.js +++ b/test/crypto/validate.js @@ -90,8 +90,10 @@ async function generatePrivateKeyObject(options) { export default () => { describe('EdDSA parameter validation (legacy format)', function() { let eddsaKey; + let anotherEddsaKey; before(async () => { eddsaKey = await generatePrivateKeyObject({ curve: 'ed25519Legacy' }); + anotherEddsaKey = await generatePrivateKeyObject({ curve: 'ed25519Legacy' }); }); it('EdDSA params should be valid', async function() { @@ -100,11 +102,10 @@ export default () => { it('detect invalid edDSA Q', async function() { const eddsaKeyPacket = await cloneKeyPacket(eddsaKey); - const Q = eddsaKeyPacket.publicParams.Q; - Q[0]++; + eddsaKeyPacket.publicParams.Q = anotherEddsaKey.keyPacket.publicParams.Q; await expect(eddsaKeyPacket.validate()).to.be.rejectedWith('Key is invalid'); - const infQ = new Uint8Array(Q.length); + const infQ = new Uint8Array(eddsaKeyPacket.publicParams.Q.length); eddsaKeyPacket.publicParams.Q = infQ; await expect(eddsaKeyPacket.validate()).to.be.rejectedWith('Key is invalid'); }); @@ -198,13 +199,19 @@ export default () => { describe(`ECC ${curve} parameter validation`, () => { let ecdsaKey; let ecdhKey; + let anotherEcdsaKey; + let anotherEcdhKey; before(async () => { if (curve !== 'curve25519Legacy') { ecdsaKey = await generatePrivateKeyObject({ curve }); ecdhKey = ecdsaKey.subkeys[0]; + anotherEcdsaKey = await generatePrivateKeyObject({ curve }); + anotherEcdhKey = anotherEcdsaKey.subkeys[0]; } else { const eddsaKey = await generatePrivateKeyObject({ curve: 'ed25519Legacy' }); ecdhKey = eddsaKey.subkeys[0]; + const anotherEddsaKey = await generatePrivateKeyObject({ curve: 'ed25519Legacy' }); + anotherEcdhKey = anotherEddsaKey.subkeys[0]; } }); @@ -220,10 +227,9 @@ export default () => { this.skip(); } const keyPacket = await cloneKeyPacket(ecdsaKey); - const Q = keyPacket.publicParams.Q; - Q[16]++; + keyPacket.publicParams.Q = anotherEcdsaKey.keyPacket.publicParams.Q; await expect(keyPacket.validate()).to.be.rejectedWith('Key is invalid'); - const infQ = new Uint8Array(Q.length); + const infQ = new Uint8Array(anotherEcdsaKey.keyPacket.publicParams.Q.length); infQ[0] = 4; keyPacket.publicParams.Q = infQ; await expect(keyPacket.validate()).to.be.rejectedWith('Key is invalid'); @@ -235,11 +241,10 @@ export default () => { it(`ECDH ${curve} - detect invalid Q`, async function() { const keyPacket = await cloneKeyPacket(ecdhKey); - const Q = keyPacket.publicParams.Q; - Q[16]++; + keyPacket.publicParams.Q = anotherEcdhKey.keyPacket.publicParams.Q; await expect(keyPacket.validate()).to.be.rejectedWith('Key is invalid'); - const infQ = new Uint8Array(Q.length); + const infQ = new Uint8Array(keyPacket.publicParams.Q.length); keyPacket.publicParams.Q = infQ; infQ[0] = 4; await expect(keyPacket.validate()).to.be.rejectedWith('Key is invalid'); @@ -252,9 +257,13 @@ export default () => { describe(`Ed${curveID}/X${curveID} parameter validation`, function() { let eddsaKey; let ecdhXKey; + let anotherEddsaKey; + let anotherEcdhXKey; before(async () => { eddsaKey = await generatePrivateKeyObject({ type: `curve${curveID}` }); ecdhXKey = eddsaKey.subkeys[0]; + anotherEddsaKey = await generatePrivateKeyObject({ type: `curve${curveID}` }); + anotherEcdhXKey = anotherEddsaKey.subkeys[0]; }); it(`Ed${curveID} params should be valid`, async function() { @@ -263,11 +272,10 @@ export default () => { it(`detect invalid Ed${curveID} public point`, async function() { const eddsaKeyPacket = await cloneKeyPacket(eddsaKey); - const A = eddsaKeyPacket.publicParams.A; - A[0]++; + eddsaKeyPacket.publicParams.A = anotherEddsaKey.keyPacket.publicParams.A; await expect(eddsaKeyPacket.validate()).to.be.rejectedWith('Key is invalid'); - const infA = new Uint8Array(A.length); + const infA = new Uint8Array(eddsaKeyPacket.publicParams.A.length); eddsaKeyPacket.publicParams.A = infA; await expect(eddsaKeyPacket.validate()).to.be.rejectedWith('Key is invalid'); }); @@ -278,11 +286,10 @@ export default () => { it(`detect invalid X${curveID} public point`, async function() { const ecdhXKeyPacket = await cloneKeyPacket(ecdhXKey); - const A = ecdhXKeyPacket.publicParams.A; - A[0]++; + ecdhXKeyPacket.publicParams.A = anotherEcdhXKey.keyPacket.publicParams.A; await expect(ecdhXKeyPacket.validate()).to.be.rejectedWith('Key is invalid'); - const infA = new Uint8Array(A.length); + const infA = new Uint8Array(ecdhXKeyPacket.publicParams.A.length); ecdhXKeyPacket.publicParams.A = infA; await expect(ecdhXKeyPacket.validate()).to.be.rejectedWith('Key is invalid'); }); @@ -291,8 +298,10 @@ export default () => { describe('RSA parameter validation', function() { let rsaKey; + let anotherRsaKey; before(async () => { rsaKey = await generatePrivateKeyObject({ type: 'rsa', rsaBits: 2048 }); + anotherRsaKey = await generatePrivateKeyObject({ type: 'rsa', rsaBits: 2048 }); }); it('generated RSA params are valid', async function() { @@ -301,15 +310,14 @@ export default () => { it('detect invalid RSA n', async function() { const keyPacket = await cloneKeyPacket(rsaKey); - const n = keyPacket.publicParams.n; - n[0]++; + keyPacket.publicParams.n = anotherRsaKey.keyPacket.publicParams.n; await expect(keyPacket.validate()).to.be.rejectedWith('Key is invalid'); }); it('detect invalid RSA e', async function() { const keyPacket = await cloneKeyPacket(rsaKey); const e = keyPacket.publicParams.e; - e[0]++; + e[0]++; // e is hard-coded so we don't take it from `anotherRsaKey` await expect(keyPacket.validate()).to.be.rejectedWith('Key is invalid'); }); });