Add UnparseablePacket to properly deal with key blocks that include malformed/unsupported packets (#1522)

When parsing errors are being ignored, packets that fail to parse are now
included in the resulting packet list as `UnparseablePacket`s . This way, when
parsing keys that contain unparsable (sub)key, we avoid associating the
following non-key packets to the wrong key entity.

On serialization, `UnparseablePacket`s are also included by writing their raw
packet body as it was read.

openpgp.d.ts

@@ -74,11 +74,11 @@ export abstract class Key {
 
 type AllowedKeyPackets = PublicKeyPacket | PublicSubkeyPacket | SecretKeyPacket | SecretSubkeyPacket | UserIDPacket | UserAttributePacket | SignaturePacket;
 export class PublicKey extends Key {
-  constructor(packetlist: PacketList<AnyKeyPacket>);
+  constructor(packetlist: PacketList<AnyPacket>);
 }
 
 export class PrivateKey extends PublicKey {
-  constructor(packetlist: PacketList<AnyKeyPacket>);
+  constructor(packetlist: PacketList<AnyPacket>);
   public revoke(reason?: ReasonForRevocation, date?: Date, config?: Config): Promise<PrivateKey>;
   public isDecrypted(): boolean;
   public addSubkey(options: SubkeyOptions): Promise<PrivateKey>;
@@ -527,7 +527,12 @@ export class TrustPacket extends BasePacket {
   static readonly tag: enums.packet.trust;
 }
 
-export type AnyPacket = BasePacket;
+export class UnparseablePacket {
+  tag: enums.packet;
+  write: () => Uint8Array;
+}
+
+export type AnyPacket = BasePacket | UnparseablePacket;
 export type AnySecretKeyPacket = SecretKeyPacket | SecretSubkeyPacket;
 export type AnyKeyPacket = BasePublicKeyPacket;
 

src/key/key.js

@@ -28,9 +28,15 @@ import Subkey from './subkey';
 import * as helper from './helper';
 import PrivateKey from './private_key';
 import PublicKey from './public_key';
+import { UnparseablePacket } from '../packet/packet';
 
 // A key revocation certificate can contain the following packets
 const allowedRevocationPackets = /*#__PURE__*/ util.constructAllowedPackets([SignaturePacket]);
+const mainKeyPacketTags = new Set([enums.packet.publicKey, enums.packet.privateKey]);
+const keyPacketTags = new Set([
+  enums.packet.publicKey, enums.packet.privateKey,
+  enums.packet.publicSubkey, enums.packet.privateSubkey
+]);
 
 /**
  * Abstract class that represents an OpenPGP key. Must contain a primary key.
@@ -51,8 +57,29 @@ class Key {
     let user;
     let primaryKeyID;
     let subkey;
+    let ignoreUntil;
+
     for (const packet of packetlist) {
+
+      if (packet instanceof UnparseablePacket) {
+        const isUnparseableKeyPacket = keyPacketTags.has(packet.tag);
+        if (isUnparseableKeyPacket && !ignoreUntil){
+          // Since non-key packets apply to the preceding key packet, if a (sub)key is Unparseable we must
+          // discard all non-key packets that follow, until another (sub)key packet is found.
+          if (mainKeyPacketTags.has(packet.tag)) {
+            ignoreUntil = mainKeyPacketTags;
+          } else {
+            ignoreUntil = keyPacketTags;
+          }
+        }
+        continue;
+      }
+
       const tag = packet.constructor.tag;
+      if (ignoreUntil) {
+        if (!ignoreUntil.has(tag)) continue;
+        ignoreUntil = null;
+      }
       if (disallowedPackets.has(tag)) {
         throw new Error(`Unexpected packet type: ${tag}`);
       }

src/packet/index.js

@@ -1,2 +1,3 @@
 export * from './all_packets';
 export { default as PacketList } from './packetlist';
+export { UnparseablePacket } from './packet';

src/packet/packet.js

@@ -309,3 +309,13 @@ export class UnsupportedError extends Error {
   }
 }
 
+export class UnparseablePacket {
+  constructor(tag, rawContent) {
+    this.tag = tag;
+    this.rawContent = rawContent;
+  }
+
+  write() {
+    return this.rawContent;
+  }
+}

src/packet/packetlist.js

@@ -3,6 +3,7 @@ import {
   readPackets, supportsStreaming,
   writeTag, writeHeader,
   writePartialLength, writeSimpleLength,
+  UnparseablePacket,
   UnsupportedError
 } from './packet';
 import util from '../util';
@@ -89,6 +90,9 @@ class PacketList extends Array {
                 // Those are also the ones we want to be more strict about and throw on parse errors
                 // (since we likely cannot process the message without these packets anyway).
                 await writer.abort(e);
+              } else {
+                const unparsedPacket = new UnparseablePacket(parsed.tag, parsed.packet);
+                await writer.write(unparsedPacket);
               }
               util.printDebugError(e);
             }
@@ -129,12 +133,13 @@ class PacketList extends Array {
     const arr = [];
 
     for (let i = 0; i < this.length; i++) {
+      const tag = this[i] instanceof UnparseablePacket ? this[i].tag : this[i].constructor.tag;
       const packetbytes = this[i].write();
       if (util.isStream(packetbytes) && supportsStreaming(this[i].constructor.tag)) {
         let buffer = [];
         let bufferLength = 0;
         const minLength = 512;
-        arr.push(writeTag(this[i].constructor.tag));
+        arr.push(writeTag(tag));
         arr.push(stream.transform(packetbytes, value => {
           buffer.push(value);
           bufferLength += value.length;
@@ -152,9 +157,9 @@ class PacketList extends Array {
           let length = 0;
           arr.push(stream.transform(stream.clone(packetbytes), value => {
             length += value.length;
-          }, () => writeHeader(this[i].constructor.tag, length)));
+          }, () => writeHeader(tag, length)));
         } else {
-          arr.push(writeHeader(this[i].constructor.tag, packetbytes.length));
+          arr.push(writeHeader(tag, packetbytes.length));
         }
         arr.push(packetbytes);
       }

test/general/config.js

@@ -10,12 +10,17 @@ module.exports = () => describe('Custom configuration', function() {
 
     const config = { ignoreUnsupportedPackets: true };
     const parsedMessage = await openpgp.readMessage({ armoredMessage: message.armor(), config });
-    expect(parsedMessage.packets.length).to.equal(1);
+    expect(parsedMessage.packets.length).to.equal(2);
+    expect(parsedMessage.packets[0].tag).to.equal(openpgp.enums.packet.symEncryptedSessionKey);
 
     config.ignoreUnsupportedPackets = false;
     await expect(
       openpgp.readMessage({ armoredMessage: message.armor(), config })
     ).to.be.rejectedWith(/Version 1 of the SKESK packet is unsupported/);
+    // writing of partially parsed message should succeed
+    await expect(
+      openpgp.readMessage({ armoredMessage: parsedMessage.armor(), config })
+    ).to.be.rejectedWith(/Version 1 of the SKESK packet is unsupported/);
   });
 
   it('openpgp.readSignature', async function() {
@@ -32,12 +37,17 @@ vAFM3jjrAQDgJPXsv8PqCrLGDuMa/2r6SgzYd03aw/xt1WM6hgUvhQD+J54Z
 
     const config = { ignoreUnsupportedPackets: true };
     const parsedSignature = await openpgp.readSignature({ armoredSignature: signature.armor(), config });
-    expect(parsedSignature.packets.length).to.equal(0);
+    expect(parsedSignature.packets.length).to.equal(1);
+    expect(parsedSignature.packets[0].tag).to.equal(openpgp.enums.packet.signature);
 
     config.ignoreUnsupportedPackets = false;
     await expect(
       openpgp.readSignature({ armoredSignature: signature.armor(), config })
     ).to.be.rejectedWith(/Version 1 of the signature packet is unsupported/);
+    // writing of partially parsed signature should succeed
+    await expect(
+      openpgp.readSignature({ armoredSignature: parsedSignature.armor(), config })
+    ).to.be.rejectedWith(/Version 1 of the signature packet is unsupported/);
   });
 
   it('openpgp.readKey', async function() {
@@ -50,6 +60,60 @@ vAFM3jjrAQDgJPXsv8PqCrLGDuMa/2r6SgzYd03aw/xt1WM6hgUvhQD+J54Z
     ).to.be.rejectedWith(/User ID string is too long/);
   });
 
+  it('openpgp.readKeys', async function() {
+    // Valid v4 key followed by modified key declared as v3 (unsupported) and another valid v4 key.
+    // When ignoring malfored/unsupported packets, we do not want the userID and subkey of the trailing key
+    // to be associated with the leading one
+    const partiallyUnsupportedKeyBlock = `-----BEGIN PGP PUBLIC KEY BLOCK-----
+
+xjMEYotwYxYJKwYBBAHaRw8BAQdAQrm/H1rTYvBLV2mP0+6u+jVa5iOgPIgA
+VkH1H7KipDrNDzx0ZXN0QHRlc3QuY29tPsKMBBAWCgAdBQJii3BjBAsJBwgD
+FQgKBBYAAgECGQECGwMCHgEAIQkQLavVE0KkGtwWIQQv90VxLmdeWJRzEWUt
+q9UTQqQa3L/3APwM4ypA9q/qml+ezCdVFilv9huZVSbPlQ06AN5E0ZclgwD9
+FeCHPwKqDkcKvqSQGdTv3QSefwjrt9oO8DI71vKjWQjOOARii3BjEgorBgEE
+AZdVAQUBAQdALl5wAhaoMgtlk7aV6v1DC3T+7kuNQVDZZPPPbxhaYwMDAQgH
+wngEGBYIAAkFAmKLcGMCGwwAIQkQLavVE0KkGtwWIQQv90VxLmdeWJRzEWUt
+q9UTQqQa3N16APwLtHt26M1o1yUtBfQ2yddFQb/Xi4Kq3PBG5ltUBj38EAD/
+aNfrR+NWb3LWRTe+LDuU7M+8ucdZ00TeAAOHGF11UAXGMwNii3B7FgkrBgEE
+AdpHDwEBB0CF7hJ4IhKdtYMa2hkA1ckjgBcZL5TaK/+A+laliBVh2s0WPGFu
+b3RoZXJ0ZXN0QHRlc3QuY29tPsKMBBAWCgAdBQJii3B7BAsJBwgDFQgKBBYA
+AgECGQECGwMCHgEAIQkQxKiJcMvjhmEWIQQgDYaTtkFIWF89hvXEqIlwy+OG
+YWnWAQDVjVaF4FpjV9rwhqqQ+pLQYWSjFGEQV9u05YPzOZWs0AEA4stxQp1H
+OtXx2S/tfY74d+I/QPTVHgB6TVcADtdKnQjOOARii3B7EgorBgEEAZdVAQUB
+AQdAsAnhg90WUEy1raZ/DrJ1MI9g8f2SBxUtvNfCikBwpWMDAQgHwngEGBYI
+AAkFAmKLcHsCGwwAIQkQxKiJcMvjhmEWIQQgDYaTtkFIWF89hvXEqIlwy+OG
+Ya2ZAQC5fDrNXuyqvjaJiVomAl7YnuFwR4tLlgJTVDDNkTOfvAD+IJo8ptfg
+/lzgTPMPLP8RgpGs8jU5cWhLlH6866UkAwXGMwRii3B/FgkrBgEEAdpHDwEB
+B0AU3y3+X4mAYxFDz54RroBsES1YTufnIndjbljQ4UCpcs0dPGFub3RoZXJh
+bm90aGVydGVzdEB0ZXN0LmNvbT7CjAQQFgoAHQUCYotwfwQLCQcIAxUICgQW
+AAIBAhkBAhsDAh4BACEJEDc5RdIx+aTBFiEE6N7yK4zw3IhhDLIwNzlF0jH5
+pMFQWwEAwUBNM2wHH3PexhLv4QpmteIg8I2wlYmuYk0w0GfAPywBAOuyKqxE
+g4vye4Mfs2Ns3FEUQP0y+YbAkZhxhjVX3gYJzjgEYotwfxIKKwYBBAGXVQEF
+AQEHQK1UDFW1ue61hhm1O57eSv29+A2gId5Zi9TEqP1mopgkAwEIB8J4BBgW
+CAAJBQJii3B/AhsMACEJEDc5RdIx+aTBFiEE6N7yK4zw3IhhDLIwNzlF0jH5
+pMH3oQEA/gjeM/XpBP/DIhqzQxAVtrDFlkKairQMRMVQfoU4vVcBAITA9cqc
+n9/quqtmyOtYOA6gXNCw0Fal3iANKBmsPmYI
+=O3ZV
+-----END PGP PUBLIC KEY BLOCK-----
+    `;
+    await expect(
+      openpgp.readKeys({ armoredKeys: partiallyUnsupportedKeyBlock, config: { ignoreUnsupportedPackets: false } })
+    ).to.be.rejectedWith(/key packet is unsupported/);
+
+    const parsedKeys = await openpgp.readKeys({ armoredKeys: partiallyUnsupportedKeyBlock, config: { ignoreUnsupportedPackets: true } });
+    expect(parsedKeys.length).to.equal(2);
+    expect(parsedKeys[0].subkeys.length).to.equal(1);
+    expect(parsedKeys[0].subkeys[0].getKeyID().toHex()).to.equal('0861c76681a34407');
+    expect(parsedKeys[0].users.length).to.equal(1);
+    expect(parsedKeys[0].users[0].userID.email).to.equal('test@test.com');
+    expect(await parsedKeys[0].getEncryptionKey().then(key => key.getKeyID().toHex())).to.equal('0861c76681a34407');
+
+    expect(parsedKeys[1].subkeys.length).to.equal(1);
+    expect(parsedKeys[1].subkeys[0].getKeyID().toHex()).to.equal('48050814f28f2263');
+    expect(parsedKeys[1].users.length).to.equal(1);
+    expect(parsedKeys[1].users[0].userID.email).to.equal('anotheranothertest@test.com');
+    expect(await parsedKeys[1].getEncryptionKey().then(key => key.getKeyID().toHex())).to.equal('48050814f28f2263');
+  });
 
   it('openpgp.generateKey', async function() {
     const v5KeysVal = openpgp.config.v5Keys;

test/general/packet.js

@@ -969,7 +969,8 @@ kePFjAnu9cpynKXu3usf8+FuBw2zLsg1Id1n7ttxoAte416KjBN9lFBt8mcu
       packets.push(signaturePacket);
       const bytes = packets.write();
       const parsed = await openpgp.PacketList.fromBinary(bytes, allAllowedPackets, { ...openpgp.config, ignoreUnsupportedPackets: true });
-      expect(parsed.length).to.equal(0);
+      expect(parsed.length).to.equal(1);
+      expect(parsed[0].tag).to.equal(openpgp.enums.packet.signature);
     });
 
     it('Throws on unknown packet version with `config.ignoreUnsupportedPackets` disabled', async function() {
@@ -1011,7 +1012,8 @@ kePFjAnu9cpynKXu3usf8+FuBw2zLsg1Id1n7ttxoAte416KjBN9lFBt8mcu
         openpgp.PacketList.fromBinary(bytes, allAllowedPackets, { ...openpgp.config, maxUserIDLength: 2, ignoreMalformedPackets: false })
       ).to.be.rejectedWith(/User ID string is too long/);
       const parsed = await openpgp.PacketList.fromBinary(bytes, allAllowedPackets, { ...openpgp.config, maxUserIDLength: 2, ignoreMalformedPackets: true });
-      expect(parsed.length).to.equal(0);
+      expect(parsed.length).to.equal(1);
+      expect(parsed[0].tag).to.equal(openpgp.enums.packet.userID);
     });
   });
 });