From 2574795d37a1745250bac43fa62aed91dd2c1f8d Mon Sep 17 00:00:00 2001 From: larabr Date: Fri, 22 Mar 2024 17:10:27 +0100 Subject: [PATCH] Fix wrong serialization of PKESK v6 for x25519/x448 (#1734) The cleartext session key symmetric algorithm was accidentally included in the packet. As a result, the generated messages may fail to parse and/or decrypt in other implementations. The messages would still decrypt successfully in OpenPGP.js, due to an overly permissive parsing procedure, which simply discarded the unused additional byte. We know also throw on unexpected cleartext symmetric algo in PKESK v6. --- src/crypto/crypto.js | 4 +- .../public_key_encrypted_session_key.js | 17 +++-- test/general/packet.js | 74 ++++++++++++++----- 3 files changed, 68 insertions(+), 27 deletions(-) diff --git a/src/crypto/crypto.js b/src/crypto/crypto.js index 04e2fe39..683f3e57 100644 --- a/src/crypto/crypto.js +++ b/src/crypto/crypto.js @@ -39,7 +39,7 @@ import ECDHXSymmetricKey from '../type/ecdh_x_symkey'; * Encrypts data using specified algorithm and public key parameters. * See {@link https://tools.ietf.org/html/rfc4880#section-9.1|RFC 4880 9.1} for public key algorithms. * @param {module:enums.publicKey} keyAlgo - Public key algorithm - * @param {module:enums.symmetric} symmetricAlgo - Cipher algorithm + * @param {module:enums.symmetric|null} symmetricAlgo - Cipher algorithm (v3 only) * @param {Object} publicParams - Algorithm-specific public key parameters * @param {Uint8Array} data - Session key data to be encrypted * @param {Uint8Array} fingerprint - Recipient fingerprint @@ -66,7 +66,7 @@ export async function publicKeyEncrypt(keyAlgo, symmetricAlgo, publicParams, dat } case enums.publicKey.x25519: case enums.publicKey.x448: { - if (!util.isAES(symmetricAlgo)) { + if (symmetricAlgo && !util.isAES(symmetricAlgo)) { // see https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/276 throw new Error('X25519 and X448 keys can only encrypt AES session keys'); } diff --git a/src/packet/public_key_encrypted_session_key.js b/src/packet/public_key_encrypted_session_key.js index 1f6c1d04..23a10563 100644 --- a/src/packet/public_key_encrypted_session_key.js +++ b/src/packet/public_key_encrypted_session_key.js @@ -128,9 +128,12 @@ class PublicKeyEncryptedSessionKeyPacket { } this.publicKeyAlgorithm = bytes[offset++]; this.encrypted = crypto.parseEncSessionKeyParams(this.publicKeyAlgorithm, bytes.subarray(offset)); - if (this.version === 3 && ( - this.publicKeyAlgorithm === enums.publicKey.x25519 || this.publicKeyAlgorithm === enums.publicKey.x448)) { - this.sessionKeyAlgorithm = enums.write(enums.symmetric, this.encrypted.C.algorithm); + if (this.publicKeyAlgorithm === enums.publicKey.x25519 || this.publicKeyAlgorithm === enums.publicKey.x448) { + if (this.version === 3) { + this.sessionKeyAlgorithm = enums.write(enums.symmetric, this.encrypted.C.algorithm); + } else if (this.encrypted.C.algorithm !== null) { + throw new Error('Unexpected cleartext symmetric algorithm'); + } } } @@ -174,9 +177,12 @@ class PublicKeyEncryptedSessionKeyPacket { */ async encrypt(key) { const algo = enums.write(enums.publicKey, this.publicKeyAlgorithm); - const encoded = encodeSessionKey(this.version, algo, this.sessionKeyAlgorithm, this.sessionKey); + // No symmetric encryption algorithm identifier is passed to the public-key algorithm for a + // v6 PKESK packet, as it is included in the v2 SEIPD packet. + const sessionKeyAlgorithm = this.version === 3 ? this.sessionKeyAlgorithm : null; + const encoded = encodeSessionKey(this.version, algo, sessionKeyAlgorithm, this.sessionKey); this.encrypted = await crypto.publicKeyEncrypt( - algo, this.sessionKeyAlgorithm, key.publicParams, encoded, key.getFingerprintBytes()); + algo, sessionKeyAlgorithm, key.publicParams, encoded, key.getFingerprintBytes()); } /** @@ -275,6 +281,7 @@ function decodeSessionKey(version, keyAlgo, decryptedData, randomSessionKey) { case enums.publicKey.x25519: case enums.publicKey.x448: return { + sessionKeyAlgorithm: null, sessionKey: decryptedData }; default: diff --git a/test/general/packet.js b/test/general/packet.js index 0cf26d7b..b40e2ba5 100644 --- a/test/general/packet.js +++ b/test/general/packet.js @@ -8,6 +8,8 @@ chaiUse(chaiAsPromised); import openpgp from '../initOpenpgp.js'; import crypto from '../../src/crypto'; import util from '../../src/util.js'; +import * as packet from '../../src/packet'; + import * as input from './testInputs.js'; @@ -469,32 +471,64 @@ export default () => describe('Packet', function() { }); }); - it('Public key encrypted symmetric key packet', function() { - const rsa = openpgp.enums.publicKey.rsaEncryptSign; - const keySize = 1024; + describe('Public key encrypted symmetric key packet - roundtrip', () => { + const testData = [{ + algoLabel: 'RSA', + publicKeyAlgorithm: openpgp.enums.publicKey.rsaEncryptSign, + paramsPromise: crypto.generateParams(openpgp.enums.publicKey.rsaEncryptSign, 1024, 65537) + }, + { + algoLabel: 'ECDH NIST P-256', + publicKeyAlgorithm: openpgp.enums.publicKey.ecdh, + paramsPromise: crypto.generateParams(openpgp.enums.publicKey.ecdh, null, openpgp.enums.curve.nistP256) + }, + { + algoLabel: 'ECDH x25519Legacy', + publicKeyAlgorithm: openpgp.enums.publicKey.ecdh, + paramsPromise: crypto.generateParams(openpgp.enums.publicKey.ecdh, null, openpgp.enums.curve.curve25519Legacy) + }, + { + algoLabel: 'x25519', + publicKeyAlgorithm: openpgp.enums.publicKey.x25519, + paramsPromise: crypto.generateParams(openpgp.enums.publicKey.x25519) + }]; - return crypto.generateParams(rsa, keySize, 65537).then(function({ publicParams, privateParams }) { - const enc = new openpgp.PublicKeyEncryptedSessionKeyPacket(); - enc.version = 3; - const msg = new openpgp.PacketList(); - const msg2 = new openpgp.PacketList(); + function testRoundtrip({ v6 }) { + testData.forEach(({ algoLabel, publicKeyAlgorithm, paramsPromise }) => { + it(`${algoLabel} (PKESK ${v6 ? 'v6' : 'v3'})`, async () => { + const { publicParams, privateParams } = await paramsPromise; + // cannot use the `openpgp` exported values, since the different context gives issues when internally + // evaluating the `OID` instanceof of `publicParams.oid`, as part of `pkesk.encrypt` and `decrypt` + const pkesk = new packet.PublicKeyEncryptedSessionKeyPacket(); + pkesk.version = v6 ? 6 : 3; + const msg = new packet.PacketList(); + const msg2 = new packet.PacketList(); - enc.sessionKey = new Uint8Array([1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,0,1,2]); - enc.publicKeyAlgorithm = openpgp.enums.publicKey.rsaEncryptSign; - enc.sessionKeyAlgorithm = openpgp.enums.symmetric.aes256; - enc.publicKeyID.bytes = '12345678'; - return enc.encrypt({ publicParams, getFingerprintBytes() {} }).then(async () => { + const privateKey = { + algorithm: publicKeyAlgorithm, + publicParams, + privateParams, + getFingerprintBytes: () => new Uint8Array(64) + }; + pkesk.sessionKey = new Uint8Array([1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,0,1,2,3,4,5,6,7,8,9,0,1,2]); + pkesk.publicKeyAlgorithm = publicKeyAlgorithm; + pkesk.sessionKeyAlgorithm = openpgp.enums.symmetric.aes256; + pkesk.publicKeyID.bytes = '12345678'; + await pkesk.encrypt({ publicParams: privateKey.publicParams, getFingerprintBytes: privateKey.getFingerprintBytes }); - msg.push(enc); - await msg2.read(msg.write(), allAllowedPackets); + msg.push(pkesk); + const allAllowedPackets = util.constructAllowedPackets([...Object.values(packet).filter(packetClass => !!packetClass.tag)]); + await msg2.read(msg.write(), allAllowedPackets); - const privateKey = { algorithm: openpgp.enums.publicKey.rsaEncryptSign, publicParams, privateParams, getFingerprintBytes() {} }; - return msg2[0].decrypt(privateKey).then(() => { - expect(stringify(msg2[0].sessionKey)).to.equal(stringify(enc.sessionKey)); - expect(msg2[0].sessionKeyAlgorithm).to.equal(enc.sessionKeyAlgorithm); + await msg2[0].decrypt(privateKey); + expect(msg2[0].sessionKey).to.deep.equal(pkesk.sessionKey); + expect(msg2[0].sessionKeyAlgorithm).to.equal(v6 ? null : pkesk.sessionKeyAlgorithm); }); }); - }); + } + + testRoundtrip({ v6: false }); + testRoundtrip({ v6: true }); }); it('Secret key packet (reading, unencrypted)', async function() {