From f8d0e6052fedd0064010ebff2a437e198c381cc1 Mon Sep 17 00:00:00 2001 From: larabr <7375870+larabr@users.noreply.github.com> Date: Thu, 16 May 2024 10:01:07 +0200 Subject: [PATCH] Detect invalid ECDSA, EdDSA and ECDH public key point encodings on usage We now throw on unexpected leading byte. This change is primarily intended to help with debugging, in case of malformed params. In fact, in case of wrong point size, the operations would already fail anyway, just in lower-level functions. --- src/crypto/public_key/elliptic/ecdh.js | 2 + src/crypto/public_key/elliptic/ecdsa.js | 4 +- .../public_key/elliptic/eddsa_legacy.js | 3 ++ test/crypto/elliptic.js | 51 ++++++++++--------- 4 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/crypto/public_key/elliptic/ecdh.js b/src/crypto/public_key/elliptic/ecdh.js index cf6cbbed..dcb8d6f2 100644 --- a/src/crypto/public_key/elliptic/ecdh.js +++ b/src/crypto/public_key/elliptic/ecdh.js @@ -131,6 +131,7 @@ export async function encrypt(oid, kdfParams, data, Q, fingerprint) { const m = pkcs5.encode(data); const curve = new CurveWithOID(oid); + checkPublicPointEnconding(oid, Q); const { publicKey, sharedKey } = await genPublicEphemeralKey(curve, Q); const param = buildEcdhParam(enums.publicKey.ecdh, oid, kdfParams, fingerprint); const { keySize } = getCipherParams(kdfParams.cipher); @@ -193,6 +194,7 @@ async function genPrivateEphemeralKey(curve, V, Q, d) { */ export async function decrypt(oid, kdfParams, V, C, Q, d, fingerprint) { const curve = new CurveWithOID(oid); + checkPublicPointEnconding(oid, Q); checkPublicPointEnconding(oid, V); const { sharedKey } = await genPrivateEphemeralKey(curve, V, Q, d); const param = buildEcdhParam(enums.publicKey.ecdh, oid, kdfParams, fingerprint); diff --git a/src/crypto/public_key/elliptic/ecdsa.js b/src/crypto/public_key/elliptic/ecdsa.js index 76fe73d1..0527c13a 100644 --- a/src/crypto/public_key/elliptic/ecdsa.js +++ b/src/crypto/public_key/elliptic/ecdsa.js @@ -24,7 +24,7 @@ import enums from '../../../enums'; import util from '../../../util'; import { getRandomBytes } from '../../random'; import hash from '../../hash'; -import { CurveWithOID, webCurves, privateToJWK, rawPublicToJWK, validateStandardParams, nodeCurves } from './oid_curves'; +import { CurveWithOID, webCurves, privateToJWK, rawPublicToJWK, validateStandardParams, nodeCurves, checkPublicPointEnconding } from './oid_curves'; import { bigIntToUint8Array } from '../../biginteger'; const webCrypto = util.getWebCrypto(); @@ -46,6 +46,7 @@ const nodeCrypto = util.getNodeCrypto(); */ export async function sign(oid, hashAlgo, message, publicKey, privateKey, hashed) { const curve = new CurveWithOID(oid); + checkPublicPointEnconding(oid, publicKey); if (message && !util.isStream(message)) { const keyPair = { publicKey, privateKey }; switch (curve.type) { @@ -92,6 +93,7 @@ export async function sign(oid, hashAlgo, message, publicKey, privateKey, hashed */ export async function verify(oid, hashAlgo, signature, message, publicKey, hashed) { const curve = new CurveWithOID(oid); + checkPublicPointEnconding(oid, publicKey); // See https://github.com/openpgpjs/openpgpjs/pull/948. // NB: the impact was more likely limited to Brainpool curves, since thanks // to WebCrypto availability, NIST curve should not have been affected. diff --git a/src/crypto/public_key/elliptic/eddsa_legacy.js b/src/crypto/public_key/elliptic/eddsa_legacy.js index 966f8dbe..b24ab161 100644 --- a/src/crypto/public_key/elliptic/eddsa_legacy.js +++ b/src/crypto/public_key/elliptic/eddsa_legacy.js @@ -25,6 +25,7 @@ import nacl from '@openpgp/tweetnacl'; import util from '../../../util'; import enums from '../../../enums'; import hash from '../../hash'; +import { checkPublicPointEnconding } from './oid_curves'; /** * Sign a message using the provided legacy EdDSA key @@ -41,6 +42,7 @@ import hash from '../../hash'; * @async */ export async function sign(oid, hashAlgo, message, publicKey, privateKey, hashed) { + checkPublicPointEnconding(oid, publicKey); if (hash.getHashByteLength(hashAlgo) < hash.getHashByteLength(enums.hash.sha256)) { // see https://tools.ietf.org/id/draft-ietf-openpgp-rfc4880bis-10.html#section-15-7.2 throw new Error('Hash algorithm too weak for EdDSA.'); @@ -67,6 +69,7 @@ export async function sign(oid, hashAlgo, message, publicKey, privateKey, hashed * @async */ export async function verify(oid, hashAlgo, { r, s }, m, publicKey, hashed) { + checkPublicPointEnconding(oid, publicKey); if (hash.getHashByteLength(hashAlgo) < hash.getHashByteLength(enums.hash.sha256)) { throw new Error('Hash algorithm too weak for EdDSA.'); } diff --git a/test/crypto/elliptic.js b/test/crypto/elliptic.js index 3f0a6383..80059cea 100644 --- a/test/crypto/elliptic.js +++ b/test/crypto/elliptic.js @@ -10,6 +10,7 @@ import config from '../../src/config'; import util from '../../src/util.js'; import elliptic_data from './elliptic_data'; +import OID from '../../src/type/oid.js'; const key_data = elliptic_data.key_data; /* eslint-disable no-invalid-this */ @@ -80,22 +81,25 @@ export default () => describe('Elliptic Curve Cryptography @lightweight', functi }); })); }); - it('Signature verification', function (done) { - expect( - elliptic_curves.ecdsa.verify('nistP256', 8, signature_data.signature, signature_data.message, signature_data.pub, signature_data.hashed) - ).to.eventually.be.true.notify(done); + it('Signature verification', async function () { + const curve = new elliptic_curves.CurveWithOID('nistP256'); + await expect( + elliptic_curves.ecdsa.verify(new OID(curve.oid), 8, signature_data.signature, signature_data.message, signature_data.pub, signature_data.hashed) + ).to.eventually.be.true; }); - it('Invalid signature', function (done) { - expect( - elliptic_curves.ecdsa.verify('nistP256', 8, signature_data.signature, signature_data.message, key_data.nistP256.pub, signature_data.hashed) - ).to.eventually.be.false.notify(done); + it('Invalid signature', async function () { + const curve = new elliptic_curves.CurveWithOID('nistP256'); + await expect( + elliptic_curves.ecdsa.verify(new OID(curve.oid), 8, signature_data.signature, signature_data.message, key_data.nistP256.pub, signature_data.hashed) + ).to.eventually.be.false; }); - it('Signature generation', function () { - return elliptic_curves.ecdsa.sign('nistP256', 8, signature_data.message, key_data.nistP256.pub, key_data.nistP256.priv, signature_data.hashed).then(async signature => { - await expect( - elliptic_curves.ecdsa.verify('nistP256', 8, signature, signature_data.message, key_data.nistP256.pub, signature_data.hashed) - ).to.eventually.be.true; - }); + it('Signature generation', async function () { + const curve = new elliptic_curves.CurveWithOID('nistP256'); + const oid = new OID(curve.oid); + const signature = await elliptic_curves.ecdsa.sign(oid, 8, signature_data.message, key_data.nistP256.pub, key_data.nistP256.priv, signature_data.hashed); + await expect( + elliptic_curves.ecdsa.verify(oid, 8, signature, signature_data.message, key_data.nistP256.pub, signature_data.hashed) + ).to.eventually.be.true; }); }); describe('ECDSA signature', function () { @@ -137,13 +141,15 @@ export default () => describe('Elliptic Curve Cryptography @lightweight', functi enableNative(); }; - const verify_signature = async function (oid, hash, r, s, message, pub) { + const verify_signature = async function (curveName, hash, r, s, message, pub) { if (util.isString(message)) { message = util.stringToUint8Array(message); } else if (!util.isUint8Array(message)) { message = new Uint8Array(message); } const ecdsa = elliptic_curves.ecdsa; + const curve = new elliptic_curves.CurveWithOID(curveName); + const oid = new OID(curve.oid); return ecdsa.verify( oid, hash, { r: new Uint8Array(r), s: new Uint8Array(s) }, message, new Uint8Array(pub), await hashMod.digest(hash, message) ); @@ -191,15 +197,10 @@ export default () => describe('Elliptic Curve Cryptography @lightweight', functi } await expect(verify_signature( 'secp256k1', 8, [], [], [], [] - )).to.eventually.be.false; + )).to.be.rejectedWith(/Invalid point encoding/); await expect(verify_signature( 'secp256k1', 8, [], [], [], secp256k1_invalid_point_format - )).to.eventually.be.false; - }); - it('secp256k1 - Invalid point', async function () { - if (!config.useEllipticFallback && !util.getNodeCrypto()) { - this.skip(); // webcrypto does not implement secp256k1: JS fallback tested instead - } + )).to.be.rejectedWith(/Invalid point encoding/); await expect(verify_signature( 'secp256k1', 8, [], [], [], secp256k1_invalid_point )).to.eventually.be.false; @@ -241,6 +242,8 @@ export default () => describe('Elliptic Curve Cryptography @lightweight', functi }); const curves = ['secp256k1' , 'nistP256', 'nistP384', 'nistP521', 'brainpoolP256r1', 'brainpoolP384r1', 'brainpoolP512r1']; curves.forEach(curveName => it(`${curveName} - Sign and verify message`, async function () { + const curve = new elliptic_curves.CurveWithOID(curveName); + const oid = new OID(curve.oid); const { Q: keyPublic, secret: keyPrivate } = await elliptic_curves.generate(curveName); const message = new Uint8Array([ 0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, @@ -248,8 +251,8 @@ export default () => describe('Elliptic Curve Cryptography @lightweight', functi ]); const messageDigest = await hashMod.digest(openpgp.enums.hash.sha512, message); await testNativeAndFallback(async () => { - const signature = await elliptic_curves.ecdsa.sign(curveName, openpgp.enums.hash.sha512, message, keyPublic, keyPrivate, messageDigest); - await expect(elliptic_curves.ecdsa.verify(curveName, openpgp.enums.hash.sha512, signature, message, keyPublic, messageDigest)).to.eventually.be.true; + const signature = await elliptic_curves.ecdsa.sign(oid, openpgp.enums.hash.sha512, message, keyPublic, keyPrivate, messageDigest); + await expect(elliptic_curves.ecdsa.verify(oid, openpgp.enums.hash.sha512, signature, message, keyPublic, messageDigest)).to.eventually.be.true; }); })); });