diff --git a/src/crypto/public_key/elliptic/ecdh.js b/src/crypto/public_key/elliptic/ecdh.js index 8bd03f82..cf6cbbed 100644 --- a/src/crypto/public_key/elliptic/ecdh.js +++ b/src/crypto/public_key/elliptic/ecdh.js @@ -21,7 +21,7 @@ */ import nacl from '@openpgp/tweetnacl'; -import { CurveWithOID, jwkToRawPublic, rawPublicToJWK, privateToJWK, validateStandardParams } from './oid_curves'; +import { CurveWithOID, jwkToRawPublic, rawPublicToJWK, privateToJWK, validateStandardParams, checkPublicPointEnconding } from './oid_curves'; import * as aesKW from '../../aes_kw'; import { getRandomBytes } from '../../random'; import hash from '../../hash'; @@ -193,6 +193,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, V); const { sharedKey } = await genPrivateEphemeralKey(curve, V, Q, d); const param = buildEcdhParam(enums.publicKey.ecdh, oid, kdfParams, fingerprint); const { keySize } = getCipherParams(kdfParams.cipher); diff --git a/src/crypto/public_key/elliptic/oid_curves.js b/src/crypto/public_key/elliptic/oid_curves.js index 5aeddfb6..b7c151dd 100644 --- a/src/crypto/public_key/elliptic/oid_curves.js +++ b/src/crypto/public_key/elliptic/oid_curves.js @@ -57,7 +57,8 @@ const curves = { node: nodeCurves[enums.curve.nistP256], web: webCurves[enums.curve.nistP256], payloadSize: 32, - sharedSize: 256 + sharedSize: 256, + wireFormatLeadingByte: 0x04 }, [enums.curve.nistP384]: { oid: [0x06, 0x05, 0x2B, 0x81, 0x04, 0x00, 0x22], @@ -67,7 +68,8 @@ const curves = { node: nodeCurves[enums.curve.nistP384], web: webCurves[enums.curve.nistP384], payloadSize: 48, - sharedSize: 384 + sharedSize: 384, + wireFormatLeadingByte: 0x04 }, [enums.curve.nistP521]: { oid: [0x06, 0x05, 0x2B, 0x81, 0x04, 0x00, 0x23], @@ -77,7 +79,8 @@ const curves = { node: nodeCurves[enums.curve.nistP521], web: webCurves[enums.curve.nistP521], payloadSize: 66, - sharedSize: 528 + sharedSize: 528, + wireFormatLeadingByte: 0x04 }, [enums.curve.secp256k1]: { oid: [0x06, 0x05, 0x2B, 0x81, 0x04, 0x00, 0x0A], @@ -85,14 +88,16 @@ const curves = { hash: enums.hash.sha256, cipher: enums.symmetric.aes128, node: nodeCurves[enums.curve.secp256k1], - payloadSize: 32 + payloadSize: 32, + wireFormatLeadingByte: 0x04 }, [enums.curve.ed25519Legacy]: { oid: [0x06, 0x09, 0x2B, 0x06, 0x01, 0x04, 0x01, 0xDA, 0x47, 0x0F, 0x01], keyType: enums.publicKey.eddsaLegacy, hash: enums.hash.sha512, node: false, // nodeCurves.ed25519 TODO - payloadSize: 32 + payloadSize: 32, + wireFormatLeadingByte: 0x40 }, [enums.curve.curve25519Legacy]: { oid: [0x06, 0x0A, 0x2B, 0x06, 0x01, 0x04, 0x01, 0x97, 0x55, 0x01, 0x05, 0x01], @@ -100,7 +105,8 @@ const curves = { hash: enums.hash.sha256, cipher: enums.symmetric.aes128, node: false, // nodeCurves.curve25519 TODO - payloadSize: 32 + payloadSize: 32, + wireFormatLeadingByte: 0x40 }, [enums.curve.brainpoolP256r1]: { oid: [0x06, 0x09, 0x2B, 0x24, 0x03, 0x03, 0x02, 0x08, 0x01, 0x01, 0x07], @@ -108,7 +114,8 @@ const curves = { hash: enums.hash.sha256, cipher: enums.symmetric.aes128, node: nodeCurves[enums.curve.brainpoolP256r1], - payloadSize: 32 + payloadSize: 32, + wireFormatLeadingByte: 0x04 }, [enums.curve.brainpoolP384r1]: { oid: [0x06, 0x09, 0x2B, 0x24, 0x03, 0x03, 0x02, 0x08, 0x01, 0x01, 0x0B], @@ -116,7 +123,8 @@ const curves = { hash: enums.hash.sha384, cipher: enums.symmetric.aes192, node: nodeCurves[enums.curve.brainpoolP384r1], - payloadSize: 48 + payloadSize: 48, + wireFormatLeadingByte: 0x04 }, [enums.curve.brainpoolP512r1]: { oid: [0x06, 0x09, 0x2B, 0x24, 0x03, 0x03, 0x02, 0x08, 0x01, 0x01, 0x0D], @@ -124,7 +132,8 @@ const curves = { hash: enums.hash.sha512, cipher: enums.symmetric.aes256, node: nodeCurves[enums.curve.brainpoolP512r1], - payloadSize: 64 + payloadSize: 64, + wireFormatLeadingByte: 0x04 } }; @@ -268,8 +277,23 @@ async function validateStandardParams(algo, oid, Q, d) { return true; } +/** + * Check whether the public point has a valid encoding. + * NB: this function does not check e.g. whether the point belongs to the curve. + */ +function checkPublicPointEnconding(oid, V) { + const curveName = oid.getName(); + const { payloadSize, wireFormatLeadingByte } = curves[curveName]; + + const pointSize = (curveName === enums.curve.curve25519Legacy || curveName === enums.curve.ed25519Legacy) ? payloadSize : payloadSize * 2; + + if (V[0] !== wireFormatLeadingByte || V.length !== pointSize + 1) { + throw new Error('Invalid point encoding'); + } +} + export { - CurveWithOID, curves, webCurves, nodeCurves, generate, getPreferredHashAlgo, jwkToRawPublic, rawPublicToJWK, privateToJWK, validateStandardParams + CurveWithOID, curves, webCurves, nodeCurves, generate, getPreferredHashAlgo, jwkToRawPublic, rawPublicToJWK, privateToJWK, validateStandardParams, checkPublicPointEnconding }; ////////////////////////// diff --git a/test/crypto/ecdh.js b/test/crypto/ecdh.js index f4dede03..54408418 100644 --- a/test/crypto/ecdh.js +++ b/test/crypto/ecdh.js @@ -72,14 +72,6 @@ export default () => describe('ECDH key exchange @lightweight', function () { '', 2, 7, [], [], [], [], [] )).to.be.rejectedWith(Error, /Unknown curve/).notify(done); }); - it('Invalid ephemeral key', function (done) { - if (!openpgp.config.useEllipticFallback && !util.getNodeCrypto()) { - this.skip(); - } - expect(decrypt_message( - 'secp256k1', 2, 7, [], [], [], [], [] - )).to.be.rejectedWith(Error, /Private key is not valid for specified curve|second arg must be public key/).notify(done); - }); it('Invalid elliptic public key', function (done) { if (!openpgp.config.useEllipticFallback && !util.getNodeCrypto()) { this.skip(); @@ -145,8 +137,8 @@ export default () => describe('ECDH key exchange @lightweight', function () { const kdfParams = new KDFParams({ hash: curve.hash, cipher: curve.cipher }); const data = random.getRandomBytes(16); await expect( - ecdh.encrypt(oid, kdfParams, data, Q1.subarray(1), fingerprint1) - ).to.be.rejectedWith(/Public key is not valid for specified curve|Failed to translate Buffer to a EC_POINT|second arg must be public key/); + ecdh.encrypt(oid, kdfParams, data, Q1, fingerprint1) + ).to.be.rejectedWith(/Invalid point encoding/); }); it('Different keys', async function () { @@ -212,6 +204,26 @@ export default () => describe('ECDH key exchange @lightweight', function () { const { publicKey: V, wrappedKey: C } = await ecdh.encrypt(oid, kdfParams, data, Q, fingerprint1); expect(await ecdh.decrypt(oid, kdfParams, V, C, Q, d, fingerprint1)).to.deep.equal(data); }); + + it(`${curveName} - Detect invalid PKESK public point encoding on decryption`, async function () { + const curve = new elliptic_curves.CurveWithOID(curveName); + const oid = new OID(curve.oid); + const kdfParams = new KDFParams({ hash: curve.hash, cipher: curve.cipher }); + const data = random.getRandomBytes(16); + const Q = key_data[curveName].pub; + const d = key_data[curveName].priv; + const { publicKey: V, wrappedKey: C } = await ecdh.encrypt(oid, kdfParams, data, Q, fingerprint1); + + const publicPointWithoutPrefixByte = V.subarray(1); + const publicPointWithUnexpectedPrefixByte = new Uint8Array([0x1, ...publicPointWithoutPrefixByte]); + const publicPointWithUnexpectedSize = V.subarray(0, V.length - 1); + + const expectedError = /Invalid point encoding/; + await expect(ecdh.decrypt(oid, kdfParams, publicPointWithoutPrefixByte, C, Q, d, fingerprint1)).to.be.rejectedWith(expectedError); + await expect(ecdh.decrypt(oid, kdfParams, publicPointWithUnexpectedPrefixByte, C, Q, d, fingerprint1)).to.be.rejectedWith(expectedError); + await expect(ecdh.decrypt(oid, kdfParams, publicPointWithUnexpectedSize, C, Q, d, fingerprint1)).to.be.rejectedWith(expectedError); + + }); }); describe('Comparing decrypting with and without native crypto', () => {