diff --git a/src/config/config.d.ts b/src/config/config.d.ts index 240f99ba..e063fb4d 100644 --- a/src/config/config.d.ts +++ b/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; diff --git a/src/config/config.js b/src/config/config.js index dc53a12f..135a02ec 100644 --- a/src/config/config.js +++ b/src/config/config.js @@ -228,7 +228,6 @@ export default { * Parsing of packets is normally restricted to a predefined set of packets. For example a Sym. Encrypted Integrity Protected Data Packet can only * contain a certain set of packets including LiteralDataPacket. With this setting we can allow additional packets, which is probably not advisable * as a global config setting, but can be used for specific function calls (e.g. decrypt method of Message). - * NB: `config.enforceGrammar` may need to be disabled as well. * @memberof module:config * @property {Array} additionalAllowedPackets Allow additional packets on parsing. Defined as array of packet classes, e.g. [PublicKeyPacket] */ diff --git a/src/message.js b/src/message.js index f21ac597..68dda0a3 100644 --- a/src/message.js +++ b/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({ delayReporting: false })); + const packetlist = await PacketList.fromBinary(input, allowedMessagePackets, config, new MessageGrammarValidator()); const message = new Message(packetlist); message.fromStream = streamType; return message; diff --git a/src/packet/aead_encrypted_data.js b/src/packet/aead_encrypted_data.js index e2129fdb..d184c64c 100644 --- a/src/packet/aead_encrypted_data.js +++ b/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({ enforceDelay: false }) + new MessageGrammarValidator() ); } diff --git a/src/packet/compressed_data.js b/src/packet/compressed_data.js index e3dfa237..cdeddea0 100644 --- a/src/packet/compressed_data.js +++ b/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({ enforceDelay: false })); + this.packets = await PacketList.fromBinary(await decompressionFn(this.compressed), allowedPackets, config, new MessageGrammarValidator()); } /** diff --git a/src/packet/grammar.ts b/src/packet/grammar.ts index bc578343..67cfe3f7 100644 --- a/src/packet/grammar.ts +++ b/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,109 +12,131 @@ 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) => { - // Encrypted Message: Encrypted Data | ESK Sequence, Encrypted Data. - const isValidESKSequence = (tagList: enums.packet[], _acceptPartial: boolean) => ( - 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 (encryptedDataPacketIndex === tagList.length - 1) && - isValidESKSequence(tagList.slice(0, encryptedDataPacketIndex), acceptPartial); -}; - -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; -}; - -const isUnknownPacketTag = (tag: number): tag is enums.packet => { - try { - enums.read(enums.packet, tag); - return false; - } catch (e) { - return true; - } -}; +enum MessageType { + EmptyMessage, // incl. empty signed message + PlaintextOrEncryptedData, + EncryptedSessionKeys, + StandaloneAdditionalAllowedData +} /** - * 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 acceptPartial - whether the list of tags corresponds to a partially-parsed message - * @returns whether the list of tags is valid + * 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. */ -const isValidOpenPGPMessage = ( - notNormalizedList: number[] /** might have unknown tags */, - 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); -}; - -/** - * If `delayReporting === false`, the grammar validator throws as soon as an invalid packet sequence is detected during parsing. - * This setting MUST NOT be used when parsing unauthenticated decrypted data, to avoid instantiating decryption oracles. - * Passing `delayReporting === true` allows checking the grammar validity in an async manner, by - * only reporting the validity status after parsing is done (i.e. and authentication is expected to - * have been enstablished) - */ -export const getMessageGrammarValidator = ({ delayReporting }: { delayReporting: boolean }) => { - let logged = false; - +export class MessageGrammarValidator { + // 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; /** - * @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). - * @throws on grammar error, provided `config.enforceGrammar` is enabled. - */ - return (list: number[], isPartial: boolean, config: Config): true | null => { - if (delayReporting && isPartial) return null; // delay until the full message has been parsed (i.e. authenticated) - - 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) { - throw error; - } else { - return 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 additionalAllowedPackets - object containing packets which are allowed anywhere in the sequence, except they cannot precede a OPS packet + * @throws {GrammarError} on invalid `packet` input + */ + recordPacket(packet: enums.packet, additionalAllowedPackets?: { [key in enums.packet]: any }) { + 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 | and + // OPS | Signature | | Signature and + // OPS | | 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 (!additionalAllowedPackets?.[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 (!additionalAllowedPackets?.[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 (!additionalAllowedPackets?.[packet]) { + throw new GrammarError(`Unexpected packet ${packet} in state ${this.state}`); + } + this.state = MessageType.EncryptedSessionKeys; + } } + } - return true; - }; -}; + /** + * Signal end of the packet sequence for final validity check + * @throws {GrammarError} on invalid sequence + */ + recordEnd() { + 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'); + } + } + } +} diff --git a/src/packet/packet.js b/src/packet/packet.js index e703fa54..4dfcfa9f 100644 --- a/src/packet/packet.js +++ b/src/packet/packet.js @@ -20,7 +20,7 @@ * @module packet/packet */ -import { ArrayStream, getWriter as streamGetWriter, getReader as streamGetReader } from '@openpgp/web-stream-tools'; +import { ArrayStream, getWriter as streamGetWriter } from '@openpgp/web-stream-tools'; import enums from '../enums'; import util from '../util'; @@ -114,8 +114,7 @@ export function supportsStreaming(tag) { * @param {Function} callback - Function to call with the parsed packet * @returns {Boolean} Returns false if the stream was empty and parsing is done, and true otherwise. */ -export async function readPackets(input, callback) { - const reader = streamGetReader(input); +export async function readPacket(reader, useStreamType, callback) { let writer; let callbackReturned; try { @@ -146,8 +145,8 @@ export async function readPackets(input, callback) { const packetSupportsStreaming = supportsStreaming(tag); let packet = null; - if (packetSupportsStreaming) { - if (util.isStream(input) === 'array') { + if (useStreamType && packetSupportsStreaming) { + if (useStreamType === 'array') { const arrayStream = new ArrayStream(); writer = streamGetWriter(arrayStream); packet = arrayStream; @@ -240,38 +239,6 @@ export async function readPackets(input, callback) { } } while (wasPartialLength); - // If this was not a packet that "supports streaming", we peek to check - // whether it is the last packet in the message. We peek 2 bytes instead - // of 1 because the beginning of this function also peeks 2 bytes, and we - // want to cut a `subarray` of the correct length into `web-stream-tools`' - // `externalBuffer` as a tiny optimization here. - // - // If it *was* a streaming packet (i.e. the data packets), we peek at the - // entire remainder of the stream, in order to forward errors in the - // remainder of the stream to the packet data. (Note that this means we - // read/peek at all signature packets before closing the literal data - // packet, for example.) This forwards MDC errors to the literal data - // stream, for example, so that they don't get lost / forgotten on - // decryptedMessage.packets.stream, which we never look at. - // - // An example of what we do when stream-parsing a message containing - // [ one-pass signature packet, literal data packet, signature packet ]: - // 1. Read the one-pass signature packet - // 2. Peek 2 bytes of the literal data packet - // 3. Parse the one-pass signature packet - // - // 4. Read the literal data packet, simultaneously stream-parsing it - // 5. Peek until the end of the message - // 6. Finish parsing the literal data packet - // - // 7. Read the signature packet again (we already peeked at it in step 5) - // 8. Peek at the end of the stream again (`peekBytes` returns undefined) - // 9. Parse the signature packet - // - // Note that this means that if there's an error in the very end of the - // stream, such as an MDC error, we throw in step 5 instead of in step 8 - // (or never), which is the point of this exercise. - const nextPacket = await reader.peekBytes(packetSupportsStreaming ? Infinity : 2); if (writer) { await writer.ready; await writer.close(); @@ -280,7 +247,6 @@ export async function readPackets(input, callback) { // eslint-disable-next-line callback-return await callback({ tag, packet }); } - return !nextPacket || !nextPacket.length; } catch (e) { if (writer) { await writer.abort(e); @@ -292,7 +258,6 @@ export async function readPackets(input, callback) { if (writer) { await callbackReturned; } - reader.releaseLock(); } } @@ -321,6 +286,18 @@ export class UnknownPacketError extends UnsupportedError { } } +export class MalformedPacketError extends UnsupportedError { + constructor(...params) { + super(...params); + + if (Error.captureStackTrace) { + Error.captureStackTrace(this, UnsupportedError); + } + + this.name = 'MalformedPacketError'; + } +} + export class UnparseablePacket { constructor(tag, rawContent) { this.tag = tag; diff --git a/src/packet/packetlist.js b/src/packet/packetlist.js index cf034861..41be1c16 100644 --- a/src/packet/packetlist.js +++ b/src/packet/packetlist.js @@ -1,16 +1,16 @@ import { transformPair as streamTransformPair, transform as streamTransform, getWriter as streamGetWriter, getReader as streamGetReader, clone as streamClone } from '@openpgp/web-stream-tools'; import { - readPackets, supportsStreaming, + readPacket, supportsStreaming, writeTag, writeHeader, writePartialLength, writeSimpleLength, UnparseablePacket, UnsupportedError, - UnknownPacketError + UnknownPacketError, + MalformedPacketError } from './packet'; import util from '../util'; import enums from '../enums'; import defaultConfig from '../config'; -import { GrammarError } from './grammar'; /** * Instantiate a new packet given its tag @@ -47,13 +47,15 @@ class PacketList extends Array { * @param {Uint8Array | ReadableStream} bytes - binary data to parse * @param {Object} allowedPackets - mapping where keys are allowed packet tags, pointing to their Packet class * @param {Object} [config] - full configuration, defaults to openpgp.config + * @param {function(enums.packet[], boolean, Object): void} [grammarValidator] + * @param {Boolean} [delayErrors] - delay errors until the input stream has been read completely * @returns {PacketList} parsed list of packets * @throws on parsing errors * @async */ - static async fromBinary(bytes, allowedPackets, config = defaultConfig, grammarValidator = null) { + static async fromBinary(bytes, allowedPackets, config = defaultConfig, grammarValidator = null, delayErrors = false) { const packets = new PacketList(); - await packets.read(bytes, allowedPackets, config, grammarValidator); + await packets.read(bytes, allowedPackets, config, grammarValidator, delayErrors); return packets; } @@ -63,72 +65,138 @@ class PacketList extends Array { * @param {Object} allowedPackets - mapping where keys are allowed packet tags, pointing to their Packet class * @param {Object} [config] - full configuration, defaults to openpgp.config * @param {function(enums.packet[], boolean, Object): void} [grammarValidator] + * @param {Boolean} [delayErrors] - delay errors until the input stream has been read completely * @throws on parsing errors * @async */ - async read(bytes, allowedPackets, config = defaultConfig, grammarValidator = null) { + async read(bytes, allowedPackets, config = defaultConfig, grammarValidator = null, delayErrors = false) { + let additionalAllowedPackets; if (config.additionalAllowedPackets.length) { - allowedPackets = { ...allowedPackets, ...util.constructAllowedPackets(config.additionalAllowedPackets) }; + additionalAllowedPackets = util.constructAllowedPackets(config.additionalAllowedPackets); + allowedPackets = { ...allowedPackets, ...additionalAllowedPackets }; } this.stream = streamTransformPair(bytes, async (readable, writable) => { + const reader = streamGetReader(readable); const writer = streamGetWriter(writable); - const writtenTags = []; try { + let useStreamType = util.isStream(readable); while (true) { await writer.ready; - const done = await readPackets(readable, async parsed => { + let unauthenticatedError; + let wasStream; + await readPacket(reader, useStreamType, async parsed => { try { if (parsed.tag === enums.packet.marker || parsed.tag === enums.packet.trust || parsed.tag === enums.packet.padding) { - // According to the spec, these packet types should be ignored and not cause parsing errors, even if not esplicitly allowed: + // According to the spec, these packet types should be ignored and not cause parsing errors, even if not explicitly allowed: // - Marker packets MUST be ignored when received: https://github.com/openpgpjs/openpgpjs/issues/1145 // - Trust packets SHOULD be ignored outside of keyrings (unsupported): https://datatracker.ietf.org/doc/html/rfc4880#section-5.10 // - [Padding Packets] MUST be ignored when received: https://datatracker.ietf.org/doc/html/draft-ietf-openpgp-crypto-refresh#name-padding-packet-tag-21 return; } const packet = newPacketFromTag(parsed.tag, allowedPackets); + // Unknown packets throw in the call above, we ignore them + // in the grammar checker. + try { + grammarValidator?.recordPacket(parsed.tag, additionalAllowedPackets); + } catch (e) { + if (config.enforceGrammar) { + throw e; + } else { + util.printDebugError(e); + } + } packet.packets = new PacketList(); packet.fromStream = util.isStream(parsed.packet); - await packet.read(parsed.packet, config); + wasStream = packet.fromStream; + try { + await packet.read(parsed.packet, config); + } catch (e) { + if (!(e instanceof UnsupportedError)) { + throw util.wrapError(new MalformedPacketError(`Parsing ${packet.constructor.name} failed`), e); + } + 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. // Packet Tags from 0 to 39 are critical. Packet Tags from 40 to 63 are non-critical. - if (e instanceof UnknownPacketError) { - if (parsed.tag <= 39) { - await writer.abort(e); + const throwUnknownPacketError = + e instanceof UnknownPacketError && + parsed.tag <= 39; + // In case of unsupported packet versions/algorithms/etc, we ignore the error by default + // (unless the packet is a data packet, see below). + const throwUnsupportedError = + e instanceof UnsupportedError && + !(e instanceof UnknownPacketError) && + !config.ignoreUnsupportedPackets; + // In case of packet parsing errors, e.name was set to 'MalformedPacketError' above. + // By default, we throw for these errors. + const throwMalformedPacketError = + e instanceof MalformedPacketError && + !config.ignoreMalformedPackets; + // The packets that support streaming are the ones that contain message data. + // Those are also the ones we want to be more strict about and throw on all errors + // (since we likely cannot process the message without these packets anyway). + const throwDataPacketError = supportsStreaming(parsed.tag); + // Throw all other errors, including `GrammarError`s, disallowed packet errors, and unexpected errors. + const throwOtherError = !( + e instanceof UnknownPacketError || + e instanceof UnsupportedError || + e instanceof MalformedPacketError + ); + if ( + throwUnknownPacketError || + throwUnsupportedError || + throwMalformedPacketError || + throwDataPacketError || + throwOtherError + ) { + if (delayErrors) { + unauthenticatedError = e; } else { - return; + await writer.abort(e); } - } - - const throwUnsupportedError = !config.ignoreUnsupportedPackets && e instanceof UnsupportedError; - const throwMalformedError = !config.ignoreMalformedPackets && !(e instanceof UnsupportedError); - const throwGrammarError = e instanceof GrammarError; - if (throwUnsupportedError || throwMalformedError || throwGrammarError || supportsStreaming(parsed.tag)) { - // The packets that support streaming are the ones that contain message data. - // 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); - writtenTags.push(parsed.tag); - grammarValidator?.(writtenTags, true, config); } util.printDebugError(e); } }); + if (wasStream) { + // Don't allow more than one streaming packet, as read errors + // may get lost in the second packet's data stream. + useStreamType = null; + } + + // If there was a parse error, read the entire input first + // in case there's an MDC error, which should take precedence. + if (unauthenticatedError) { + await reader.readToEnd(); + // eslint-disable-next-line @typescript-eslint/no-throw-literal + throw unauthenticatedError; + } + + // We peek to check whether this was the last packet. + // We peek 2 bytes instead of 1 because `readPacket` also + // peeks 2 bytes, and we want to cut a `subarray` of the + // correct length into `web-stream-tools`' `externalBuffer` + // as a tiny optimization here. + const nextPacket = await reader.peekBytes(2); + const done = !nextPacket || !nextPacket.length; 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); + try { + grammarValidator?.recordEnd(); + } catch (e) { + if (config.enforceGrammar) { + throw e; + } else { + util.printDebugError(e); + } + } await writer.ready; await writer.close(); return; diff --git a/src/packet/sym_encrypted_integrity_protected_data.js b/src/packet/sym_encrypted_integrity_protected_data.js index 7941c7c5..9fbfdb49 100644 --- a/src/packet/sym_encrypted_integrity_protected_data.js +++ b/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([ @@ -184,23 +184,17 @@ class SymEncryptedIntegrityProtectedDataPacket { if (isArrayStream(encrypted)) encrypted = await streamReadToEnd(encrypted); let packetbytes; - let grammarValidator; + let delayErrors = false; if (this.version === 2) { if (this.cipherAlgorithm !== sessionKeyAlgorithm) { // sanity check throw new Error('Unexpected session key algorithm'); } packetbytes = await runAEAD(this, 'decrypt', key, encrypted); - grammarValidator = getMessageGrammarValidator({ delayReporting: false }); } else { const { blockSize } = getCipherParams(sessionKeyAlgorithm); const decrypted = await cipherMode.cfb.decrypt(sessionKeyAlgorithm, key, encrypted, new Uint8Array(blockSize)); - // Grammar validation cannot be run before message integrity has been enstablished, - // to avoid leaking info about the unauthenticated message structure. - const releaseUnauthenticatedStream = util.isStream(encrypted) && config.allowUnauthenticatedStream; - grammarValidator = getMessageGrammarValidator({ delayReporting: releaseUnauthenticatedStream }); - // there must be a modification detection code packet as the // last packet and everything gets hashed except the hash itself const realHash = streamSlice(streamPassiveClone(decrypted), -20); @@ -219,7 +213,9 @@ class SymEncryptedIntegrityProtectedDataPacket { const bytes = streamSlice(tohash, blockSize + 2); // Remove random prefix packetbytes = streamSlice(bytes, 0, -2); // Remove MDC packet packetbytes = streamConcat([packetbytes, streamFromAsync(() => verifyHash)]); - if (!releaseUnauthenticatedStream) { + if (util.isStream(encrypted) && config.allowUnauthenticatedStream) { + delayErrors = true; + } else { packetbytes = await streamReadToEnd(packetbytes); } } @@ -228,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, grammarValidator); + this.packets = await PacketList.fromBinary(packetbytes, allowedPackets, config, new MessageGrammarValidator(), delayErrors); return true; } } diff --git a/src/util.js b/src/util.js index c57ea431..3c6bf707 100644 --- a/src/util.js +++ b/src/util.js @@ -593,17 +593,23 @@ const util = { }).join('\n'); }, - wrapError: function(message, error) { - if (!error) { - return new Error(message); + wrapError: function(error, cause) { + if (!cause) { + if (error instanceof Error) { + return error; + } + return new Error(error); } - // update error message - try { - error.message = message + ': ' + error.message; - } catch (e) {} - - return error; + if (error instanceof Error) { + // update error message + try { + error.message += ': ' + cause.message; + error.cause = cause; + } catch (e) {} + return error; + } + return new Error(error + ': ' + cause.message, { cause }); }, /** diff --git a/test/general/config.js b/test/general/config.js index 98e22af5..89c6a969 100644 --- a/test/general/config.js +++ b/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(/Unexpected packet/); }); it('openpgp.readSignature', async function() { diff --git a/test/general/packet.js b/test/general/packet.js index 76982462..c998ef4a 100644 --- a/test/general/packet.js +++ b/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)) { @@ -1335,8 +1335,8 @@ kePFjAnu9cpynKXu3usf8+FuBw2zLsg1Id1n7ttxoAte416KjBN9lFBt8mcu it('Ignores non-critical packet even with tolerant mode disabled', async function() { const unknownPacketTag63 = util.hexToUint8Array('ff0a750064bf943d6e756c6c'); // non-critical tag - await expect(openpgp.PacketList.fromBinary(unknownPacketTag63, allAllowedPackets, { ...openpgp.config, ignoreUnsupportedPackets: false, ignoreMalformedPackets: false })).to.eventually.have.length(0); - await expect(openpgp.PacketList.fromBinary(unknownPacketTag63, allAllowedPackets, { ...openpgp.config, ignoreUnsupportedPackets: true, ignoreMalformedPackets: true })).to.eventually.have.length(0); + await expect(openpgp.PacketList.fromBinary(unknownPacketTag63, allAllowedPackets, { ...openpgp.config, ignoreUnsupportedPackets: false, ignoreMalformedPackets: false })).to.eventually.have.length(1); + await expect(openpgp.PacketList.fromBinary(unknownPacketTag63, allAllowedPackets, { ...openpgp.config, ignoreUnsupportedPackets: true, ignoreMalformedPackets: true })).to.eventually.have.length(1); }); it('Throws on disallowed packet even with tolerant mode enabled', async function() { @@ -1375,11 +1375,237 @@ 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); + m1.recordPacket(openpgp.enums.packet.onePassSignature); + m1.recordPacket(openpgp.enums.packet.literalData); + m1.recordPacket(openpgp.enums.packet.signature); + expect(() => m1.recordEnd()).to.not.throw(); + + // OPS | Sig | Literal | Sig + const m2 = new MessageGrammarValidator(); + m2.recordPacket(openpgp.enums.packet.onePassSignature); + m2.recordPacket(openpgp.enums.packet.signature); + m2.recordPacket(openpgp.enums.packet.literalData); + m2.recordPacket(openpgp.enums.packet.signature); + 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); + m3.recordPacket(openpgp.enums.packet.signature); + m3.recordPacket(openpgp.enums.packet.literalData); + expect(() => m3.recordEnd()).to.throw(); + + // Sig - should throw due to standalone signature packet + const m4 = new MessageGrammarValidator(); + m4.recordPacket(openpgp.enums.packet.signature); + expect(() => m3.recordEnd()).to.throw(); + + // ESK | Sig | SEIPD - should throw + const m5 = new MessageGrammarValidator(); + m5.recordPacket(openpgp.enums.packet.publicKeyEncryptedSessionKey); + expect(() => m5.recordPacket(openpgp.enums.packet.signature)).to.throw(); + }); + + it('standalone additional allowed packets should be valid', () => { + const additionalAllowedPackets = { [openpgp.PublicKeyPacket.tag]: 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)).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, getMessageGrammarValidator({ delayReporting: false }))).to.be.rejectedWith(/Data does not respect OpenPGP grammar/); + 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 () => { + const literalPackets = new openpgp.PacketList(); + literalPackets.push(new openpgp.LiteralDataPacket()); + literalPackets.push(new openpgp.LiteralDataPacket()); + const encrypted = new openpgp.SymEncryptedIntegrityProtectedDataPacket(); + encrypted.version = 1; + encrypted.packets = literalPackets; + const packets = new openpgp.PacketList(); + packets.push(encrypted); + await encrypted.encrypt(openpgp.enums.symmetric.aes128, new Uint8Array(16)); + await expect(openpgp.decrypt({ + message: await openpgp.readMessage({ + binaryMessage: packets.write() + }), + sessionKeys: [{ algorithm: 'aes128', data: new Uint8Array(16) }] + })).to.be.rejectedWith(/Unexpected packet/); + }); + + it('reject duplicate literal packet inside encrypted data (streaming)', async () => { + const literalPackets = new openpgp.PacketList(); + literalPackets.push(new openpgp.LiteralDataPacket()); + literalPackets.push(new openpgp.LiteralDataPacket()); + const encrypted = new openpgp.SymEncryptedIntegrityProtectedDataPacket(); + encrypted.version = 1; + encrypted.packets = literalPackets; + const packets = new openpgp.PacketList(); + packets.push(encrypted); + await encrypted.encrypt(openpgp.enums.symmetric.aes128, new Uint8Array(16)); + const decrypted = await openpgp.decrypt({ + message: await openpgp.readMessage({ + binaryMessage: new ReadableStream({ + start(controller) { + controller.enqueue(packets.write()); + controller.close(); + } + }) + }), + sessionKeys: [{ algorithm: 'aes128', data: new Uint8Array(16) }], + config: { + allowUnauthenticatedStream: true + } + }); + await expect(stream.readToEnd(decrypted.data)).to.be.rejectedWith(/Unexpected packet/); + }); + + it('reject duplicate literal packet inside encrypted data (MDC error gets precedence)', async () => { + const literalPackets = new openpgp.PacketList(); + literalPackets.push(new openpgp.LiteralDataPacket()); + const literal = new openpgp.LiteralDataPacket(); + literal.data = new Uint8Array(1000); + literalPackets.push(literal); + const encrypted = new openpgp.SymEncryptedIntegrityProtectedDataPacket(); + encrypted.version = 1; + encrypted.packets = literalPackets; + const packets = new openpgp.PacketList(); + packets.push(encrypted); + await encrypted.encrypt(openpgp.enums.symmetric.aes128, new Uint8Array(16)); + const encryptedData = packets.write(); + encryptedData[encryptedData.length - 5] ^= 1; + const decrypted = await openpgp.decrypt({ + message: await openpgp.readMessage({ + binaryMessage: new ReadableStream({ + start(controller) { + controller.enqueue(encryptedData.subarray(0, 500)); + controller.enqueue(encryptedData.subarray(500)); + controller.close(); + } + }) + }), + sessionKeys: [{ algorithm: 'aes128', data: new Uint8Array(16) }], + config: { + allowUnauthenticatedStream: true + } + }); + await expect(stream.readToEnd(decrypted.data)).to.be.rejectedWith(/Modification detected/); + }); + + it('reject malformed packet inside encrypted data', async () => { + const literalPackets = new openpgp.PacketList(); + const signature = new openpgp.SignaturePacket(); + signature.signatureData = signature.signedHashValue = new Uint8Array([4, 4]); + signature.params = {}; + literalPackets.push(signature); + literalPackets.push(new openpgp.LiteralDataPacket()); + const encrypted = new openpgp.SymEncryptedIntegrityProtectedDataPacket(); + encrypted.version = 1; + encrypted.packets = literalPackets; + const packets = new openpgp.PacketList(); + packets.push(encrypted); + await encrypted.encrypt(openpgp.enums.symmetric.aes128, new Uint8Array(16)); + const encryptedData = packets.write(); + await expect(openpgp.decrypt({ + message: await openpgp.readMessage({ + binaryMessage: new ReadableStream({ + start(controller) { + controller.enqueue(encryptedData.subarray(0, 500)); + controller.enqueue(encryptedData.subarray(500)); + controller.close(); + } + }) + }), + sessionKeys: [{ algorithm: 'aes128', data: new Uint8Array(16) }], + config: { + allowUnauthenticatedStream: true + } + })).to.be.rejectedWith(/Missing signature creation time subpacket/); + }); + + it('reject malformed packet inside encrypted data (MDC error gets precedence)', async () => { + const literalPackets = new openpgp.PacketList(); + const signature = new openpgp.SignaturePacket(); + signature.signatureData = signature.signedHashValue = new Uint8Array([4, 4]); + signature.params = {}; + literalPackets.push(signature); + literalPackets.push(new openpgp.LiteralDataPacket()); + const encrypted = new openpgp.SymEncryptedIntegrityProtectedDataPacket(); + encrypted.version = 1; + encrypted.packets = literalPackets; + const packets = new openpgp.PacketList(); + packets.push(encrypted); + await encrypted.encrypt(openpgp.enums.symmetric.aes128, new Uint8Array(16)); + const encryptedData = packets.write(); + encryptedData[encryptedData.length - 5] ^= 1; + await expect(openpgp.decrypt({ + message: await openpgp.readMessage({ + binaryMessage: new ReadableStream({ + start(controller) { + controller.enqueue(encryptedData.subarray(0, 500)); + controller.enqueue(encryptedData.subarray(500)); + controller.close(); + } + }) + }), + sessionKeys: [{ algorithm: 'aes128', data: new Uint8Array(16) }], + config: { + allowUnauthenticatedStream: true + } + })).to.be.rejectedWith(/Modification detected/); }); it('accepts padding and marker packets', async () => { @@ -1389,33 +1615,14 @@ 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({ delayReporting: false })); + 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 - - const messageGrammarValidatorWithLatentReporting = getMessageGrammarValidator({ delayReporting: true }); - const parsed2 = await openpgp.PacketList.fromBinary(packets.write(), allAllowedPackets, openpgp.config, messageGrammarValidatorWithLatentReporting); - expect(parsed2.length).to.equal(1); - const sampleGrammarValidatorReturnValue = isPartial => messageGrammarValidatorWithLatentReporting([] /* valid */, isPartial, openpgp.config); - expect(sampleGrammarValidatorReturnValue(true)).to.be.null; - expect(sampleGrammarValidatorReturnValue(false)).to.be.true; }); it('accepts unknown packets', async () => { const unknownPacketTag63 = util.hexToUint8Array('ff0a750064bf943d6e756c6c'); // non-critical tag - const parsed = await openpgp.PacketList.fromBinary(unknownPacketTag63, allAllowedPackets, openpgp.config, getMessageGrammarValidator({ delayReporting: false })); - expect(parsed.length).to.equal(0); - }); - - it('delay reporting', () => { - const messageGrammarValidatorWithLatentReporting = getMessageGrammarValidator({ delayReporting: true }); - - const sampleGrammarValidatorReturnValueValid = isPartial => messageGrammarValidatorWithLatentReporting([] /* valid */, isPartial, openpgp.config); - expect(sampleGrammarValidatorReturnValueValid(true)).to.be.null; - expect(sampleGrammarValidatorReturnValueValid(false)).to.be.true; - - const sampleGrammarValidatorReturnValueInvalid = isPartial => messageGrammarValidatorWithLatentReporting([openpgp.enums.packet.literalData, openpgp.enums.packet.literalData] /* invalid */, isPartial, openpgp.config); - expect(sampleGrammarValidatorReturnValueInvalid(true)).to.be.null; - expect(() => sampleGrammarValidatorReturnValueInvalid(false)).to.throw(/Data does not respect OpenPGP grammar/); + const parsed = await openpgp.PacketList.fromBinary(unknownPacketTag63, allAllowedPackets, openpgp.config, new MessageGrammarValidator()); + expect(parsed.length).to.equal(1); }); }); }); diff --git a/test/general/signature.js b/test/general/signature.js index 05360536..78c23f00 100644 --- a/test/general/signature.js +++ b/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('Missing trailing 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].signature).to.be.rejectedWith('Data does not respect OpenPGP grammar'); + 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);