Change MessageGrammarValidator into a state machine

Instead of checking the list of packets for every new packet, check the
new packet given the state we've recorded from the previous packets.

src/message.js

@@ -36,7 +36,7 @@ import {
   OnePassSignaturePacket,
   SignaturePacket
 } from './packet';
-import { getMessageGrammarValidator } from './packet/grammar';
+import { MessageGrammarValidator } from './packet/grammar';
 
 // A Message can contain the following packets
 const allowedMessagePackets = /*#__PURE__*/ util.constructAllowedPackets([
@@ -879,7 +879,7 @@ export async function readMessage({ armoredMessage, binaryMessage, config, ...re
     }
     input = data;
   }
-  const packetlist = await PacketList.fromBinary(input, allowedMessagePackets, config, getMessageGrammarValidator());
+  const packetlist = await PacketList.fromBinary(input, allowedMessagePackets, config, new MessageGrammarValidator());
   const message = new Message(packetlist);
   message.fromStream = streamType;
   return message;

src/packet/aead_encrypted_data.js

@@ -28,7 +28,7 @@ import CompressedDataPacket from './compressed_data';
 import OnePassSignaturePacket from './one_pass_signature';
 import SignaturePacket from './signature';
 import PacketList from './packetlist';
-import { getMessageGrammarValidator } from './grammar';
+import { MessageGrammarValidator } from './grammar';
 
 // An AEAD-encrypted Data packet can contain the following packet types
 const allowedPackets = /*#__PURE__*/ util.constructAllowedPackets([
@@ -106,7 +106,7 @@ class AEADEncryptedDataPacket {
       await runAEAD(this, 'decrypt', key, streamClone(this.encrypted)),
       allowedPackets,
       config,
-      getMessageGrammarValidator()
+      new MessageGrammarValidator()
     );
   }
 

src/packet/compressed_data.js

@@ -25,7 +25,7 @@ import LiteralDataPacket from './literal_data';
 import OnePassSignaturePacket from './one_pass_signature';
 import SignaturePacket from './signature';
 import PacketList from './packetlist';
-import { getMessageGrammarValidator } from './grammar';
+import { MessageGrammarValidator } from './grammar';
 
 // A Compressed Data packet can contain the following packet types
 const allowedPackets = /*#__PURE__*/ util.constructAllowedPackets([
@@ -114,7 +114,7 @@ class CompressedDataPacket {
     }
 
     // Decompressing a Compressed Data packet MUST also yield a valid OpenPGP Message
-    this.packets = await PacketList.fromBinary(await decompressionFn(this.compressed), allowedPackets, config, getMessageGrammarValidator());
+    this.packets = await PacketList.fromBinary(await decompressionFn(this.compressed), allowedPackets, config, new MessageGrammarValidator());
   }
 
   /**

src/packet/grammar.ts

@@ -1,6 +1,4 @@
-import { type Config } from '../config';
 import enums from '../enums';
-import util from '../util';
 
 export class GrammarError extends Error {
   constructor(...params: any[]) {
@@ -14,74 +12,60 @@ 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
+]);
 
-const isValidLiteralMessage = (tagList: enums.packet[]) => tagList.length === 1 && tagList[0] === enums.packet.literalData;
+export class MessageGrammarValidator {
-const isValidCompressedMessage = (tagList: enums.packet[]) => tagList.length === 1 && tagList[0] === enums.packet.compressedData;
+  sawDataPacket: boolean = false;
-const isValidEncryptedMessage = (tagList: enums.packet[]) => {
+  sawESKs: number = 0;
-  // Encrypted Message: Encrypted Data | ESK Sequence, Encrypted Data.
+  sawOPSs: number = 0;
-  const isValidESKSequence = (tagList: enums.packet[]) => (
+  sawTrailingSigs: number = 0;
-    tagList.every(packetTag => new Set([enums.packet.publicKeyEncryptedSessionKey, enums.packet.symEncryptedSessionKey]).has(packetTag))
-  );
-  const encryptedDataPacketIndex = tagList.findIndex(tag => new Set([enums.packet.aeadEncryptedData, enums.packet.symmetricallyEncryptedData, enums.packet.symEncryptedIntegrityProtectedData]).has(tag));
-  if (encryptedDataPacketIndex < 0) {
-    return isValidESKSequence(tagList);
-  }
 
-  return (encryptedDataPacketIndex === tagList.length - 1) &&
+  recordPacket(packet: enums.packet) {
-    isValidESKSequence(tagList.slice(0, encryptedDataPacketIndex));
+    if (packet === enums.packet.publicKeyEncryptedSessionKey || packet === enums.packet.symEncryptedSessionKey) {
-};
+      if (this.sawDataPacket) {
-
+        throw new GrammarError('Encrypted session key packet following data packet');
-const isValidSignedMessage = (tagList: enums.packet[], acceptPartial: boolean) => {
-  // Signature Packet, OpenPGP Message | One-Pass Signed Message.
-  if (tagList.findIndex(tag => tag === enums.packet.signature) === 0) {
-    return isValidOpenPGPMessage(tagList.slice(1), acceptPartial);
-  }
-
-  // One-Pass Signed Message:
-  //    One-Pass Signature Packet, OpenPGP Message, Corresponding Signature Packet.
-  if (tagList.findIndex(tag => tag === enums.packet.onePassSignature) === 0) {
-    const correspondingSigPacketIndex = util.findLastIndex(tagList, tag => tag === enums.packet.signature);
-    if (correspondingSigPacketIndex !== tagList.length - 1 && !acceptPartial) {
-      return false;
-    }
-    return isValidOpenPGPMessage(tagList.slice(1, correspondingSigPacketIndex < 0 ? undefined : correspondingSigPacketIndex), acceptPartial);
-  }
-
-  return false;
-};
-
-/**
- * Implements grammar checks based on https://www.rfc-editor.org/rfc/rfc9580.html#section-10.3 .
- * @param packetList - list of packet tags to validate; marker/padding/unknown packet tags are expected to have been already filtered out.
- * @param acceptPartial - whether the list of tags corresponds to a partially-parsed message
- * @returns whether the list of tags is valid
- */
-const isValidOpenPGPMessage = (
-  packetList: enums.packet[],
-  acceptPartial: boolean
-): boolean => {
-  return isValidLiteralMessage(packetList) ||
-    isValidCompressedMessage(packetList) ||
-    isValidEncryptedMessage(packetList) ||
-    isValidSignedMessage(packetList, acceptPartial);
-};
-
-export const getMessageGrammarValidator = () => {
-  let logged = false;
-
-  /**
-  * @throws on grammar error, provided `config.enforceGrammar` is enabled.
-  */
-  return (list: number[], isPartial: boolean, config: Config): undefined => {
-    if (!isValidOpenPGPMessage(list, isPartial)) {
-      const error = new GrammarError(`Data does not respect OpenPGP grammar [${list}]`);
-      if (!logged) {
-        util.printDebugError(error);
-        logged = true;
       }
-      if (config.enforceGrammar) {
+      this.sawESKs++;
-        throw error;
+    } 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;
+    } else {
+      throw new GrammarError(`Unexpected packet: ${packet}`);
     }
-  };
+  }
-};
+
+  recordEnd() {
+    if (this.sawOPSs !== this.sawTrailingSigs) {
+      throw new GrammarError('Mismatched one-pass signature and signature packets');
+    }
+  }
+}

src/packet/packetlist.js

@@ -73,7 +73,6 @@ class PacketList extends Array {
     if (config.additionalAllowedPackets.length) {
       allowedPackets = { ...allowedPackets, ...util.constructAllowedPackets(config.additionalAllowedPackets) };
     }
-    const tagsRead = [];
     this.stream = streamTransformPair(bytes, async (readable, writable) => {
       const reader = streamGetReader(readable);
       const writer = streamGetWriter(writable);
@@ -95,7 +94,15 @@ class PacketList extends Array {
               const packet = newPacketFromTag(parsed.tag, allowedPackets);
               // Unknown packets throw in the call above, we ignore them
               // in the grammar checker.
-              tagsRead.push(parsed.tag);
+              try {
+                grammarValidator?.recordPacket(parsed.tag);
+              } catch (e) {
+                if (config.enforceGrammar) {
+                  throw e;
+                } else {
+                  util.printDebugError(e);
+                }
+              }
               packet.packets = new PacketList();
               packet.fromStream = util.isStream(parsed.packet);
               wasStream = packet.fromStream;
@@ -179,7 +186,15 @@ class PacketList extends Array {
           if (done) {
             // Here we are past the MDC check for SEIPDv1 data, hence
             // the data is always authenticated at this point.
-            grammarValidator?.(tagsRead, false, config);
+            try {
+              grammarValidator?.recordEnd();
+            } catch (e) {
+              if (config.enforceGrammar) {
+                throw e;
+              } else {
+                util.printDebugError(e);
+              }
+            }
             await writer.ready;
             await writer.close();
             return;
@@ -198,12 +213,8 @@ class PacketList extends Array {
         this.push(value);
       } else {
         this.stream = null;
-        break;
       }
-      if (supportsStreaming(value.constructor.tag)) {
+      if (done || supportsStreaming(value.constructor.tag)) {
-        if (!delayErrors) {
-          grammarValidator?.(tagsRead, true, config);
-        }
         break;
       }
     }

src/packet/sym_encrypted_integrity_protected_data.js

@@ -28,7 +28,7 @@ import OnePassSignaturePacket from './one_pass_signature';
 import SignaturePacket from './signature';
 import PacketList from './packetlist';
 import { UnsupportedError } from './packet';
-import { getMessageGrammarValidator } from './grammar';
+import { MessageGrammarValidator } from './grammar';
 
 // A SEIP packet can contain the following packet types
 const allowedPackets = /*#__PURE__*/ util.constructAllowedPackets([
@@ -224,7 +224,7 @@ class SymEncryptedIntegrityProtectedDataPacket {
     // MUST yield a valid OpenPGP Message.
     // - Decrypting a version 2 Symmetrically Encrypted and Integrity Protected Data packet
     // MUST yield a valid Optionally Padded Message.
-    this.packets = await PacketList.fromBinary(packetbytes, allowedPackets, config, getMessageGrammarValidator(), delayErrors);
+    this.packets = await PacketList.fromBinary(packetbytes, allowedPackets, config, new MessageGrammarValidator(), delayErrors);
     return true;
   }
 }

test/general/config.js

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

test/general/packet.js

@@ -13,7 +13,7 @@ import * as random from '../../src/crypto/random';
 
 import * as input from './testInputs.js';
 import { mockCryptoRandomGenerator, restoreCryptoRandomGenerator } from '../mockRandom.ts';
-import { getMessageGrammarValidator } from '../../src/packet/grammar.js';
+import { MessageGrammarValidator } from '../../src/packet/grammar.js';
 
 function stringify(array) {
   if (stream.isStream(array)) {
@@ -1379,7 +1379,7 @@ kePFjAnu9cpynKXu3usf8+FuBw2zLsg1Id1n7ttxoAte416KjBN9lFBt8mcu
         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, getMessageGrammarValidator())).to.be.rejectedWith(/Data does not respect OpenPGP grammar/);
+        await expect(openpgp.PacketList.fromBinary(packets.write(), allAllowedPackets, openpgp.config, new MessageGrammarValidator())).to.be.rejectedWith(/Multiple data packets in message/);
       });
 
       it('reject duplicate literal packet inside encrypted data', async () => {
@@ -1397,7 +1397,7 @@ kePFjAnu9cpynKXu3usf8+FuBw2zLsg1Id1n7ttxoAte416KjBN9lFBt8mcu
             binaryMessage: packets.write()
           }),
           sessionKeys: [{ algorithm: 'aes128', data: new Uint8Array(16) }]
-        })).to.be.rejectedWith(/Data does not respect OpenPGP grammar/);
+        })).to.be.rejectedWith(/Multiple data packets in message/);
       });
 
       it('reject duplicate literal packet inside encrypted data (streaming)', async () => {
@@ -1424,7 +1424,7 @@ kePFjAnu9cpynKXu3usf8+FuBw2zLsg1Id1n7ttxoAte416KjBN9lFBt8mcu
             allowUnauthenticatedStream: true
           }
         });
-        await expect(stream.readToEnd(decrypted.data)).to.be.rejectedWith(/Data does not respect OpenPGP grammar/);
+        await expect(stream.readToEnd(decrypted.data)).to.be.rejectedWith(/Multiple data packets in message/);
       });
 
       it('reject duplicate literal packet inside encrypted data (MDC error gets precedence)', async () => {
@@ -1529,13 +1529,13 @@ kePFjAnu9cpynKXu3usf8+FuBw2zLsg1Id1n7ttxoAte416KjBN9lFBt8mcu
         packets.push(padding);
         packets.push(new openpgp.MarkerPacket());
         packets.push(new openpgp.LiteralDataPacket());
-        const parsed = await openpgp.PacketList.fromBinary(packets.write(), allAllowedPackets, openpgp.config, getMessageGrammarValidator());
+        const parsed = await openpgp.PacketList.fromBinary(packets.write(), allAllowedPackets, openpgp.config, new MessageGrammarValidator());
         expect(parsed.length).to.equal(1); // marker and padding packets are always dropped on parsing
       });
 
       it('accepts unknown packets', async () => {
         const unknownPacketTag63 = util.hexToUint8Array('ff0a750064bf943d6e756c6c'); // non-critical tag
-        const parsed = await openpgp.PacketList.fromBinary(unknownPacketTag63, allAllowedPackets, openpgp.config, getMessageGrammarValidator());
+        const parsed = await openpgp.PacketList.fromBinary(unknownPacketTag63, allAllowedPackets, openpgp.config, new MessageGrammarValidator());
         expect(parsed.length).to.equal(1);
       });
     });

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('Data does not respect OpenPGP grammar');
+        await expect(stream.readToEnd(data)).to.be.rejectedWith('Mismatched one-pass signature and signature packets');
         expect(signatures).to.have.length(1);
-        await expect(signatures[0].verified).to.be.rejectedWith('Data does not respect OpenPGP grammar');
+        await expect(signatures[0].verified).to.be.rejectedWith('Mismatched one-pass signature and signature packets');
-        await expect(signatures[0].signature).to.be.rejectedWith('Data does not respect OpenPGP grammar');
+        await expect(signatures[0].signature).to.be.rejectedWith('Mismatched one-pass signature and signature packets');
       });
       await openpgp.verify({ verificationKeys: [pubKey], message: messageWithoutGrammar, config: { minRSABits: 1024 } }).then(async ({ data, signatures }) => {
         expect(await stream.readToEnd(data)).to.equal(plaintext);