diff --git a/src/message.js b/src/message.js index f21ac597..6bfe63de 100644 --- a/src/message.js +++ b/src/message.js @@ -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, getMessageGrammarValidator()); 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..52e2f993 100644 --- a/src/packet/aead_encrypted_data.js +++ b/src/packet/aead_encrypted_data.js @@ -106,7 +106,7 @@ class AEADEncryptedDataPacket { await runAEAD(this, 'decrypt', key, streamClone(this.encrypted)), allowedPackets, config, - getMessageGrammarValidator({ enforceDelay: false }) + getMessageGrammarValidator() ); } diff --git a/src/packet/compressed_data.js b/src/packet/compressed_data.js index e3dfa237..54b1c56f 100644 --- a/src/packet/compressed_data.js +++ b/src/packet/compressed_data.js @@ -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, getMessageGrammarValidator()); } /** diff --git a/src/packet/grammar.ts b/src/packet/grammar.ts index befd3074..e2991273 100644 --- a/src/packet/grammar.ts +++ b/src/packet/grammar.ts @@ -66,24 +66,13 @@ const isValidOpenPGPMessage = ( isValidSignedMessage(packetList, 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 }) => { +export const getMessageGrammarValidator = () => { 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). * @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) - + 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) { @@ -92,11 +81,7 @@ export const getMessageGrammarValidator = ({ delayReporting }: { delayReporting: } if (config.enforceGrammar) { throw error; - } else { - return true; } } - - return true; }; }; diff --git a/src/packet/packetlist.js b/src/packet/packetlist.js index b633f861..67205661 100644 --- a/src/packet/packetlist.js +++ b/src/packet/packetlist.js @@ -167,11 +167,9 @@ class PacketList extends Array { break; } 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); + if (!bytes.unauthenticated) { + grammarValidator?.(tagsRead, true, config); + } break; } } diff --git a/src/packet/sym_encrypted_integrity_protected_data.js b/src/packet/sym_encrypted_integrity_protected_data.js index 7941c7c5..9cc22bcf 100644 --- a/src/packet/sym_encrypted_integrity_protected_data.js +++ b/src/packet/sym_encrypted_integrity_protected_data.js @@ -184,23 +184,16 @@ class SymEncryptedIntegrityProtectedDataPacket { if (isArrayStream(encrypted)) encrypted = await streamReadToEnd(encrypted); let packetbytes; - let grammarValidator; 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 +212,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) { + packetbytes.unauthenticated = true; + } else { packetbytes = await streamReadToEnd(packetbytes); } } @@ -228,7 +223,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, getMessageGrammarValidator()); return true; } } diff --git a/test/general/packet.js b/test/general/packet.js index bf58466e..fab13aa8 100644 --- a/test/general/packet.js +++ b/test/general/packet.js @@ -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({ delayReporting: false }))).to.be.rejectedWith(/Data does not respect OpenPGP grammar/); + await expect(openpgp.PacketList.fromBinary(packets.write(), allAllowedPackets, openpgp.config, getMessageGrammarValidator())).to.be.rejectedWith(/Data does not respect OpenPGP grammar/); }); it('accepts padding and marker packets', async () => { @@ -1389,34 +1389,15 @@ 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, getMessageGrammarValidator()); 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 })); + const parsed = await openpgp.PacketList.fromBinary(unknownPacketTag63, allAllowedPackets, openpgp.config, getMessageGrammarValidator()); expect(parsed.length).to.equal(1); }); - - 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/); - }); }); });