From e010101c42ed12dc90915b69f1f3b6d0dace8925 Mon Sep 17 00:00:00 2001 From: Daniel Huigens Date: Mon, 19 May 2025 20:56:31 +0200 Subject: [PATCH] Simplify grammar checker --- src/packet/grammar.ts | 44 +++++++++++----------------------------- src/packet/packetlist.js | 23 +++++++++++---------- 2 files changed, 24 insertions(+), 43 deletions(-) diff --git a/src/packet/grammar.ts b/src/packet/grammar.ts index bc578343..befd3074 100644 --- a/src/packet/grammar.ts +++ b/src/packet/grammar.ts @@ -15,20 +15,20 @@ export class GrammarError extends Error { } -const isValidLiteralMessage = (tagList: enums.packet[], _acceptPartial: boolean) => tagList.length === 1 && tagList[0] === enums.packet.literalData; -const isValidCompressedMessage = (tagList: enums.packet[], _acceptPartial: boolean) => tagList.length === 1 && tagList[0] === enums.packet.compressedData; -const isValidEncryptedMessage = (tagList: enums.packet[], acceptPartial: boolean) => { +const isValidLiteralMessage = (tagList: enums.packet[]) => tagList.length === 1 && tagList[0] === enums.packet.literalData; +const isValidCompressedMessage = (tagList: enums.packet[]) => tagList.length === 1 && tagList[0] === enums.packet.compressedData; +const isValidEncryptedMessage = (tagList: enums.packet[]) => { // Encrypted Message: Encrypted Data | ESK Sequence, Encrypted Data. - const isValidESKSequence = (tagList: enums.packet[], _acceptPartial: boolean) => ( + const isValidESKSequence = (tagList: enums.packet[]) => ( 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, acceptPartial); + return isValidESKSequence(tagList); } return (encryptedDataPacketIndex === tagList.length - 1) && - isValidESKSequence(tagList.slice(0, encryptedDataPacketIndex), acceptPartial); + isValidESKSequence(tagList.slice(0, encryptedDataPacketIndex)); }; const isValidSignedMessage = (tagList: enums.packet[], acceptPartial: boolean) => { @@ -50,39 +50,20 @@ const isValidSignedMessage = (tagList: enums.packet[], acceptPartial: boolean) = return false; }; -const isUnknownPacketTag = (tag: number): tag is enums.packet => { - try { - enums.read(enums.packet, tag); - return false; - } catch (e) { - return true; - } -}; - /** * Implements grammar checks based on https://www.rfc-editor.org/rfc/rfc9580.html#section-10.3 . - * @param notNormalizedList - list of packet tags to validate + * @param packetList - list of packet tags to validate * @param acceptPartial - whether the list of tags corresponds to a partially-parsed message * @returns whether the list of tags is valid */ const isValidOpenPGPMessage = ( - notNormalizedList: number[] /** might have unknown tags */, + packetList: number[], acceptPartial: boolean ): boolean => { - // Take care of packet tags that can appear anywhere in the sequence: - // 1. A Marker packet (Section 5.8) can appear anywhere in the sequence. - // 2. An implementation MUST be able to process Padding packets anywhere else in an OpenPGP stream so that future revisions of this document may specify further locations for padding. - // 3. An unknown non-critical packet MUST be ignored (criticality is enforced on parsing). - const normalizedList: enums.packet[] = notNormalizedList.filter(tag => ( - tag !== enums.packet.marker && - tag !== enums.packet.padding && - !isUnknownPacketTag(tag) - )); - - return isValidLiteralMessage(normalizedList, acceptPartial) || - isValidCompressedMessage(normalizedList, acceptPartial) || - isValidEncryptedMessage(normalizedList, acceptPartial) || - isValidSignedMessage(normalizedList, acceptPartial); + return isValidLiteralMessage(packetList) || + isValidCompressedMessage(packetList) || + isValidEncryptedMessage(packetList) || + isValidSignedMessage(packetList, acceptPartial); }; /** @@ -95,7 +76,6 @@ const isValidOpenPGPMessage = ( export const getMessageGrammarValidator = ({ delayReporting }: { delayReporting: boolean }) => { let logged = false; - /** * @returns `true` on successful grammar validation; if `delayReporting` is set, `null` is returned * if validation is still pending (partial parsing, waiting for authentication to be confirmed). diff --git a/src/packet/packetlist.js b/src/packet/packetlist.js index e0eeb57f..b633f861 100644 --- a/src/packet/packetlist.js +++ b/src/packet/packetlist.js @@ -69,9 +69,9 @@ class PacketList extends Array { if (config.additionalAllowedPackets.length) { allowedPackets = { ...allowedPackets, ...util.constructAllowedPackets(config.additionalAllowedPackets) }; } + const tagsRead = []; this.stream = streamTransformPair(bytes, async (readable, writable) => { const writer = streamGetWriter(writable); - const writtenTags = []; try { while (true) { await writer.ready; @@ -85,6 +85,9 @@ class PacketList extends Array { return; } const packet = newPacketFromTag(parsed.tag, allowedPackets); + // Unknown packets throw in the call above, we ignore them + // in the grammar checker. + tagsRead.push(parsed.tag); packet.packets = new PacketList(); packet.fromStream = util.isStream(parsed.packet); try { @@ -96,12 +99,6 @@ class PacketList extends Array { throw e; } await writer.write(packet); - writtenTags.push(parsed.tag); - // The `writtenTags` are only sensitive if we are parsing an _unauthenticated_ decrypted stream, - // since they can enable an decryption oracle. - // It's responsibility of the caller to pass a `grammarValidator` that takes care of - // postponing error reporting until the data has been authenticated. - grammarValidator?.(writtenTags, true, config); } catch (e) { // If an implementation encounters a critical packet where the packet type is unknown in a packet sequence, // it MUST reject the whole packet sequence. On the other hand, an unknown non-critical packet MUST be ignored. @@ -141,8 +138,6 @@ class PacketList extends Array { } else { const unparsedPacket = new UnparseablePacket(parsed.tag, parsed.packet); await writer.write(unparsedPacket); - writtenTags.push(parsed.tag); - grammarValidator?.(writtenTags, true, config); } util.printDebugError(e); } @@ -150,7 +145,7 @@ 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?.(writtenTags, false, config); + grammarValidator?.(tagsRead, false, config); await writer.ready; await writer.close(); return; @@ -169,8 +164,14 @@ class PacketList extends Array { this.push(value); } else { this.stream = null; + break; } - if (done || supportsStreaming(value.constructor.tag)) { + if (supportsStreaming(value.constructor.tag)) { + // The `tagsRead` are only sensitive if we are parsing an _unauthenticated_ decrypted stream, + // since they can enable an decryption oracle. + // It's responsibility of the caller to pass a `grammarValidator` that takes care of + // postponing error reporting until the data has been authenticated. + grammarValidator?.(tagsRead, true, config); break; } }