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

@@ -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');
       }

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:

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() {