MessageGrammarValidator: implement more exhaustive checks

Some invalid sequences were not detected after refactor

src/config/config.d.ts

@@ -18,7 +18,7 @@ export interface Config {
   ignoreUnsupportedPackets: boolean;
   ignoreMalformedPackets: boolean;
   enforceGrammar: boolean;
-  additionalAllowedPackets: Array<{ new(): any }>;
+  additionalAllowedPackets: Array<{ new(): any, tag: enums.packet }>;
   versionString: string;
   commentString: string;
   allowInsecureDecryptionWithSigningKeys: boolean;

src/packet/grammar.ts

@@ -1,4 +1,5 @@
 import enums from '../enums';
+import type { Config } from '../config';
 
 export class GrammarError extends Error {
   constructor(...params: any[]) {
@@ -12,58 +13,132 @@ export class GrammarError extends Error {
   }
 }
 
-const encryptedDataPackets = new Set([
-  enums.packet.aeadEncryptedData,
-  enums.packet.symmetricallyEncryptedData,
-  enums.packet.symEncryptedIntegrityProtectedData
-]);
-const dataPackets = new Set([
-  enums.packet.literalData,
-  enums.packet.compressedData,
-  ...encryptedDataPackets
-]);
+enum MessageType {
+  EmptyMessage, // incl. empty signed message
+  PlaintextOrEncryptedData,
+  EncryptedSessionKeys,
+  StandaloneAdditionalAllowedData
+}
 
+/**
+ * Implement OpenPGP message grammar based on: https://www.rfc-editor.org/rfc/rfc9580.html#section-10.3 .
+ * It is slightly more lenient as it also allows standalone ESK sequences, as well as empty (signed) messages.
+ * This latter case is needed to allow unknown packets.
+ * A new `MessageGrammarValidator` instance must be created for each packet sequence, as the instance is stateful:
+ * - `recordPacket` must be called for each packet in the sequence; the function will throw as soon as
+ *  an invalid packet is detected.
+ * - `recordEnd` must be called at the end of the packet sequence to confirm its validity.
+ */
 export class MessageGrammarValidator {
-  sawDataPacket: boolean = false;
-  sawESKs: number = 0;
-  sawOPSs: number = 0;
-  sawTrailingSigs: number = 0;
+  // PDA validator inspired by https://blog.jabberhead.tk/2022/10/26/implementing-packet-sequence-validation-using-pushdown-automata/ .
+  private state: MessageType = MessageType.EmptyMessage;
+  private leadingOnePassSignatureCounter: number = 0;
 
-  recordPacket(packet: enums.packet) {
-    if (packet === enums.packet.publicKeyEncryptedSessionKey || packet === enums.packet.symEncryptedSessionKey) {
-      if (this.sawDataPacket) {
-        throw new GrammarError('Encrypted session key packet following data packet');
-      }
-      this.sawESKs++;
-    } else if (packet === enums.packet.onePassSignature) {
-      if (this.sawDataPacket) {
-        throw new GrammarError('One-pass signature packet following data packet');
-      }
-      if (this.sawESKs) {
-        throw new GrammarError('One-pass signature packet following encrypted session key packet');
-      }
-      this.sawOPSs++;
-    } else if (packet === enums.packet.signature) {
-      if (this.sawESKs) {
-        throw new GrammarError('Signature packet following encrypted session key packet');
-      }
-      if (this.sawDataPacket) {
-        this.sawTrailingSigs++;
-      }
-    } else if (dataPackets.has(packet)) {
-      if (this.sawDataPacket) {
-        throw new GrammarError('Multiple data packets in message');
-      }
-      if (this.sawESKs && !encryptedDataPackets.has(packet)) {
-        throw new GrammarError('Non-encrypted data packet following ESK packet');
-      }
-      this.sawDataPacket = true;
+  /**
+   * Determine validity of the next packet in the sequence.
+   * NB: padding, marker and unknown packets are expected to already be filtered out on parsing,
+   * and are not accepted by `recordPacket`.
+   * @param packet - packet to validate
+   * @param config - needed to determine the `additionalAllowedPackets`: these are allowed anywhere in the sequence, except they cannot precede a OPS packet
+   * @throws {GrammarError} on invalid `packet` input
+   */
+  recordPacket(packet: enums.packet, config: Config) {
+    const additionalAllowedPacketsTags = new Set(config.additionalAllowedPackets.map(c => c.tag));
+    switch (this.state) {
+      case MessageType.EmptyMessage:
+      case MessageType.StandaloneAdditionalAllowedData:
+        switch (packet) {
+          case enums.packet.literalData:
+          case enums.packet.compressedData:
+          case enums.packet.aeadEncryptedData:
+          case enums.packet.symEncryptedIntegrityProtectedData:
+          case enums.packet.symmetricallyEncryptedData:
+            this.state = MessageType.PlaintextOrEncryptedData;
+            return;
+          case enums.packet.signature:
+            // Signature | <AdditionalAllowedPacketsOnly> and
+            // OPS | Signature | <AdditionalAllowedPacketsOnly> | Signature and
+            // OPS | <AdditionalAllowedPacketsOnly> | Signature are allowed
+            if (this.state === MessageType.StandaloneAdditionalAllowedData) {
+              if (--this.leadingOnePassSignatureCounter < 0) {
+                throw new GrammarError('Trailing signature packet without OPS');
+              }
+            }
+            // this.state remains EmptyMessage or StandaloneAdditionalAllowedData
+            return;
+          case enums.packet.onePassSignature:
+            if (this.state === MessageType.StandaloneAdditionalAllowedData) {
+              // we do not allow this case, for simplicity
+              throw new GrammarError('OPS following StandaloneAdditionalAllowedData');
+            }
+            this.leadingOnePassSignatureCounter++;
+            // this.state remains EmptyMessage
+            return;
+          case enums.packet.publicKeyEncryptedSessionKey:
+          case enums.packet.symEncryptedSessionKey:
+            this.state = MessageType.EncryptedSessionKeys;
+            return;
+          default:
+            if (!additionalAllowedPacketsTags.has(packet)) {
+              throw new GrammarError(`Unexpected packet ${packet} in state ${this.state}`);
+            }
+            this.state = MessageType.StandaloneAdditionalAllowedData;
+            return;
+        }
+      case MessageType.PlaintextOrEncryptedData:
+        switch (packet) {
+          case enums.packet.signature:
+            if (--this.leadingOnePassSignatureCounter < 0) {
+              throw new GrammarError('Trailing signature packet without OPS');
+            }
+            this.state = MessageType.PlaintextOrEncryptedData;
+            return;
+          default:
+            if (!additionalAllowedPacketsTags.has(packet)) {
+              throw new GrammarError(`Unexpected packet ${packet} in state ${this.state}`);
+            }
+            this.state = MessageType.PlaintextOrEncryptedData;
+            return;
+        }
+      case MessageType.EncryptedSessionKeys:
+        switch (packet) {
+          case enums.packet.publicKeyEncryptedSessionKey:
+          case enums.packet.symEncryptedSessionKey:
+            this.state = MessageType.EncryptedSessionKeys;
+            return;
+          case enums.packet.symEncryptedIntegrityProtectedData:
+          case enums.packet.aeadEncryptedData:
+          case enums.packet.symmetricallyEncryptedData:
+            this.state = MessageType.PlaintextOrEncryptedData;
+            return;
+          case enums.packet.signature:
+            if (--this.leadingOnePassSignatureCounter < 0) {
+              throw new GrammarError('Trailing signature packet without OPS');
+            }
+            this.state = MessageType.PlaintextOrEncryptedData;
+            return;
+          default:
+            if (!additionalAllowedPacketsTags.has(packet)) {
+              throw new GrammarError(`Unexpected packet ${packet} in state ${this.state}`);
+            }
+            this.state = MessageType.EncryptedSessionKeys;
+        }
     }
   }
 
+  /**
+   * Signal end of the packet sequence for final validity check
+   * @throws {GrammarError} on invalid sequence
+   */
   recordEnd() {
-    if (this.sawOPSs !== this.sawTrailingSigs) {
-      throw new GrammarError('Mismatched one-pass signature and signature packets');
+    switch (this.state) {
+      case MessageType.EmptyMessage: // needs to be allowed for PacketLists that only include unknown packets
+      case MessageType.PlaintextOrEncryptedData:
+      case MessageType.EncryptedSessionKeys:
+      case MessageType.StandaloneAdditionalAllowedData:
+        if (this.leadingOnePassSignatureCounter > 0) {
+          throw new GrammarError('Missing trailing signature packets');
+        }
     }
   }
 }

src/packet/packetlist.js

@@ -95,7 +95,7 @@ class PacketList extends Array {
               // Unknown packets throw in the call above, we ignore them
               // in the grammar checker.
               try {
-                grammarValidator?.recordPacket(parsed.tag);
+                grammarValidator?.recordPacket(parsed.tag, config);
               } catch (e) {
                 if (config.enforceGrammar) {
                   throw e;

test/general/config.js

@@ -35,7 +35,7 @@ habAyxd1AGKaNp1wbGFpbnRleHQgbWVzc2FnZQ==
 
     await expect(
       openpgp.readMessage({ armoredMessage: skeskPlusLiteralData, config: { enforceGrammar: true } })
-    ).to.be.rejectedWith(/Non-encrypted data packet following ESK packet/);
+    ).to.be.rejectedWith(/Unexpected packet/);
   });
 
   it('openpgp.readSignature', async function() {

test/general/packet.js

@@ -1375,11 +1375,97 @@ kePFjAnu9cpynKXu3usf8+FuBw2zLsg1Id1n7ttxoAte416KjBN9lFBt8mcu
     });
 
     describe('Grammar validation', async function () {
+      describe('MessageGrammarValidator - unit tests', () => {
+        it('valid nested signed messages should be valid', () => {
+          // Sig | OPS | Literal | Sig
+          const m1 = new MessageGrammarValidator();
+          m1.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets: [] });
+          m1.recordPacket(openpgp.enums.packet.onePassSignature, { additionalAllowedPackets: [] });
+          m1.recordPacket(openpgp.enums.packet.literalData, { additionalAllowedPackets: [] });
+          m1.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets: [] });
+          expect(() => m1.recordEnd()).to.not.throw();
+
+          // OPS | Sig | Literal | Sig
+          const m2 = new MessageGrammarValidator();
+          m2.recordPacket(openpgp.enums.packet.onePassSignature, { additionalAllowedPackets: [] });
+          m2.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets: [] });
+          m2.recordPacket(openpgp.enums.packet.literalData, { additionalAllowedPackets: [] });
+          m2.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets: [] });
+          expect(() => m2.recordEnd()).to.not.throw();
+
+          // OPS | Sig | Literal -  should throw due to missing trailing signature
+          const m3 = new MessageGrammarValidator();
+          m3.recordPacket(openpgp.enums.packet.onePassSignature, { additionalAllowedPackets: [] });
+          m3.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets: [] });
+          m3.recordPacket(openpgp.enums.packet.literalData, { additionalAllowedPackets: [] });
+          expect(() => m3.recordEnd()).to.throw();
+
+          // Sig -  should throw due to standalone signature packet
+          const m4 = new MessageGrammarValidator();
+          m4.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets: [] });
+          expect(() => m3.recordEnd()).to.throw();
+
+          // ESK | Sig | SEIPD -  should throw
+          const m5 = new MessageGrammarValidator();
+          m5.recordPacket(openpgp.enums.packet.publicKeyEncryptedSessionKey, { additionalAllowedPackets: [] });
+          expect(() => m5.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets: [] })).to.throw();
+        });
+
+        it('standalone additional allowed packets should be valid', () => {
+          const additionalAllowedPackets = [openpgp.PublicKeyPacket];
+          // Sig | OPS | PublicKeyPacket | Sig
+          const m1 = new MessageGrammarValidator();
+          m1.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets });
+          m1.recordPacket(openpgp.enums.packet.onePassSignature, { additionalAllowedPackets });
+          m1.recordPacket(openpgp.enums.packet.publicKey, { additionalAllowedPackets });
+          m1.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets });
+          expect(() => m1.recordEnd()).to.not.throw();
+
+          // OPS | Sig | PublicKeyPacket | Sig
+          const m2 = new MessageGrammarValidator();
+          m2.recordPacket(openpgp.enums.packet.onePassSignature, { additionalAllowedPackets });
+          m2.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets });
+          m2.recordPacket(openpgp.enums.packet.publicKey, { additionalAllowedPackets });
+          m2.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets });
+          expect(() => m2.recordEnd()).to.not.throw();
+
+          // standalone PublicKeyPacket
+          const m3 = new MessageGrammarValidator();
+          m3.recordPacket(openpgp.enums.packet.publicKey, { additionalAllowedPackets });
+          expect(() => m3.recordEnd()).to.not.throw();
+
+          // OPS | Sig | PublicKey -  should throw due to missing trailing signature
+          const m4 = new MessageGrammarValidator();
+          m4.recordPacket(openpgp.enums.packet.onePassSignature, { additionalAllowedPackets });
+          m4.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets });
+          m4.recordPacket(openpgp.enums.packet.publicKey, { additionalAllowedPackets });
+          expect(() => m4.recordEnd()).to.throw();
+
+          // Sig | PublicKey | Sig | PublicKey -  should throw
+          const m5 = new MessageGrammarValidator();
+          m5.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets });
+          m5.recordPacket(openpgp.enums.packet.publicKey, { additionalAllowedPackets });
+          expect(() => m5.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets })).to.throw();
+
+          // Sig | PublicKey | OPS | PublicKey | Sig -  should throw
+          const m6 = new MessageGrammarValidator();
+          m6.recordPacket(openpgp.enums.packet.signature, { additionalAllowedPackets });
+          m6.recordPacket(openpgp.enums.packet.publicKey, { additionalAllowedPackets });
+          expect(() => m6.recordPacket(openpgp.enums.packet.onePassSignature, { additionalAllowedPackets })).to.throw();
+        });
+
+        it('standalone disallowed packets should not be valid', () => {
+          // standalone PublicKeyPacket
+          const m1 = new MessageGrammarValidator();
+          expect(() => m1.recordPacket(openpgp.enums.packet.publicKey, { additionalAllowedPackets: [] })).to.throw();
+        });
+      });
+
       it('reject duplicate literal packet', async () => {
         const packets = new openpgp.PacketList();
         packets.push(new openpgp.LiteralDataPacket());
         packets.push(new openpgp.LiteralDataPacket());
-        await expect(openpgp.PacketList.fromBinary(packets.write(), allAllowedPackets, openpgp.config, new MessageGrammarValidator())).to.be.rejectedWith(/Multiple data packets in message/);
+        await expect(openpgp.PacketList.fromBinary(packets.write(), allAllowedPackets, openpgp.config, new MessageGrammarValidator())).to.be.rejectedWith(/Unexpected packet/);
       });
 
       it('reject duplicate literal packet inside encrypted data', async () => {
@@ -1397,7 +1483,7 @@ kePFjAnu9cpynKXu3usf8+FuBw2zLsg1Id1n7ttxoAte416KjBN9lFBt8mcu
             binaryMessage: packets.write()
           }),
           sessionKeys: [{ algorithm: 'aes128', data: new Uint8Array(16) }]
-        })).to.be.rejectedWith(/Multiple data packets in message/);
+        })).to.be.rejectedWith(/Unexpected packet/);
       });
 
       it('reject duplicate literal packet inside encrypted data (streaming)', async () => {
@@ -1424,7 +1510,7 @@ kePFjAnu9cpynKXu3usf8+FuBw2zLsg1Id1n7ttxoAte416KjBN9lFBt8mcu
             allowUnauthenticatedStream: true
           }
         });
-        await expect(stream.readToEnd(decrypted.data)).to.be.rejectedWith(/Multiple data packets in message/);
+        await expect(stream.readToEnd(decrypted.data)).to.be.rejectedWith(/Unexpected packet/);
       });
 
       it('reject duplicate literal packet inside encrypted data (MDC error gets precedence)', async () => {

test/general/signature.js

@@ -1881,10 +1881,10 @@ hkJiXopCSWKSlQInL1devkJJUWJmTmZeugJYlpdLAagQJM0JpsCqIQZwKgAA
       expect(pubKey.getKeys(keyIDs[0])).to.not.be.empty;
 
       await openpgp.verify({ verificationKeys: [pubKey], message, config: { minRSABits: 1024 } }).then(async ({ data, signatures }) => {
-        await expect(stream.readToEnd(data)).to.be.rejectedWith('Mismatched one-pass signature and signature packets');
+        await expect(stream.readToEnd(data)).to.be.rejectedWith('Missing trailing signature packets');
         expect(signatures).to.have.length(1);
-        await expect(signatures[0].verified).to.be.rejectedWith('Mismatched one-pass signature and signature packets');
-        await expect(signatures[0].signature).to.be.rejectedWith('Mismatched one-pass signature and signature packets');
+        await expect(signatures[0].verified).to.be.rejectedWith('Missing trailing signature packets');
+        await expect(signatures[0].signature).to.be.rejectedWith('Missing trailing signature packets');
       });
       await openpgp.verify({ verificationKeys: [pubKey], message: messageWithoutGrammar, config: { minRSABits: 1024 } }).then(async ({ data, signatures }) => {
         expect(await stream.readToEnd(data)).to.equal(plaintext);