diff --git a/modules/decrypt-browser/src/decrypt.ts b/modules/decrypt-browser/src/decrypt.ts index 1e90ef44a..b322f629c 100644 --- a/modules/decrypt-browser/src/decrypt.ts +++ b/modules/decrypt-browser/src/decrypt.ts @@ -100,8 +100,21 @@ interface FramedDecryptOptions extends BodyDecryptOptions { async function bodyDecrypt ({ buffer, getSubtleDecrypt, headerInfo }: BodyDecryptOptions) { let readPos = headerInfo.headerIv.byteLength + headerInfo.rawHeader.byteLength + headerInfo.headerAuthTag.byteLength const clearBuffers: ArrayBuffer[] = [] + let sequenceNumber = 0 while (true) { + /* Keeping track of the sequence number myself. */ + sequenceNumber += 1 + const { clearBlob, frameInfo } = await framedDecrypt({ buffer, getSubtleDecrypt, headerInfo, readPos }) + + /* Precondition: The sequenceNumber is required to monotonically increase, starting from 1. + * This is to avoid a bad actor from abusing the sequence number on un-signed algorithm suites. + * If the frame size matched the data format (say NDJSON), + * then the data could be significantly altered just by rearranging the frames. + * Non-framed data returns a sequenceNumber of 1. + */ + needs(frameInfo.sequenceNumber === sequenceNumber, 'Encrypted body sequence out of order.') + clearBuffers.push(clearBlob) readPos = frameInfo.readPos if (frameInfo.isFinalFrame) { diff --git a/modules/decrypt-browser/test/decrypt.test.ts b/modules/decrypt-browser/test/decrypt.test.ts index 722f5f13a..4d90b8175 100644 --- a/modules/decrypt-browser/test/decrypt.test.ts +++ b/modules/decrypt-browser/test/decrypt.test.ts @@ -32,4 +32,15 @@ describe('decrypt', () => { expect(messageHeader.encryptionContext).to.deep.equal(fixtures.encryptionContext()) expect(test).to.deep.equal(fixtures.plaintext()) }) + + it('Precondition: The sequence number is required to monotonically increase, starting from 1.', async () => { + return decrypt( + fixtures.decryptKeyring(), + fixtures.frameSequenceOutOfOrder() + ).then(() => { + throw new Error('should not succeed') + }, err => { + expect(err).to.be.instanceOf(Error) + }) + }) }) diff --git a/modules/decrypt-browser/test/fixtures.ts b/modules/decrypt-browser/test/fixtures.ts index a755529ad..9d3dc5b52 100644 --- a/modules/decrypt-browser/test/fixtures.ts +++ b/modules/decrypt-browser/test/fixtures.ts @@ -38,6 +38,21 @@ export function encryptionContext () { } } +export function frameSequenceOutOfOrder () { + /* This is the string 'asdf' encrypted with ALG_AES128_GCM_IV12_TAG16. + * The data key is all zeros. + * The frames have been reordered in order to test decryption failure. + */ + return new Uint8Array([ + 1, 128, 0, 20, 111, 198, 208, 129, 64, 41, 115, 91, 247, 27, 148, 148, 102, 70, 149, 210, 0, 0, 0, 1, 0, 1, 107, 0, 1, 107, 0, 3, 0, 0, 0, 2, 0, 0, 0, 0, 12, 0, 0, 0, 1, // header + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 178, 85, 20, 93, 96, 34, 206, 116, 216, 115, 183, 217, 192, 84, 155, 15, // header auth + 0, 0, 0, 64, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 71, 217, 75, 144, 128, 252, 7, 157, 174, 2, 89, 248, 206, 24, 122, 69, 226, 95, // f + 0, 0, 0, 48, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 62, 114, 251, 4, 157, 182, 94, 8, 230, 234, 185, 222, 120, 209, 235, 186, 207, 112, // d + 0, 0, 0, 32, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 42, 44, 171, 202, 172, 191, 0, 232, 145, 31, 77, 90, 8, 233, 45, 106, 60, 176, // s + 0, 0, 0, 16, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 16, 248, 112, 28, 63, 229, 61, 238, 146, 255, 228, 38, 170, 100, 42, 251, 21, 208 // a + ]) +} + class TestKeyring extends KeyringWebCrypto { async _onEncrypt () : Promise { throw new Error('I should never see this error') diff --git a/modules/decrypt-node/src/decrypt_stream.ts b/modules/decrypt-node/src/decrypt_stream.ts index 0a64981b1..bc7647bfd 100644 --- a/modules/decrypt-node/src/decrypt_stream.ts +++ b/modules/decrypt-node/src/decrypt_stream.ts @@ -25,7 +25,7 @@ import Duplexify from 'duplexify' import { Duplex } from 'stream' // eslint-disable-line no-unused-vars // @ts-ignore -import { pipeline } from 'readable-stream' +import { pipeline, PassThrough } from 'readable-stream' export interface DecryptStreamOptions { maxBodySize?: number @@ -44,7 +44,13 @@ export function decryptStream ( const verifyStream = new VerifyStream({ maxBodySize }) const decipherStream = getDecipherStream() - pipeline(parseHeaderStream, verifyStream, decipherStream) + /* pipeline will _either_ stream.destroy or the callback. + * decipherStream uses destroy to dispose the material. + * So I tack a pass though stream onto the end. + */ + pipeline(parseHeaderStream, verifyStream, decipherStream, new PassThrough(), (err: Error) => { + if (err) stream.emit('error', err) + }) const stream = new Duplexify(parseHeaderStream, decipherStream) diff --git a/modules/decrypt-node/src/parse_header_stream.ts b/modules/decrypt-node/src/parse_header_stream.ts index 6087f13ab..e0e1f54e9 100644 --- a/modules/decrypt-node/src/parse_header_stream.ts +++ b/modules/decrypt-node/src/parse_header_stream.ts @@ -108,7 +108,17 @@ export class ParseHeaderStream extends PortableTransformWithType { // The header is parsed, pass control const readPos = rawHeader.byteLength + headerIv.byteLength + headerAuthTag.byteLength const tail = headerBuffer.slice(readPos) - this._transform = (chunk: any, _enc: string, cb: Function) => cb(null, chunk) + /* needs calls in downstream _transform streams will throw. + * But streams are async. + * So this error should be turned into an `.emit('error', ex)`. + */ + this._transform = (chunk: any, _enc: string, cb: Function) => { + try { + cb(null, chunk) + } catch (ex) { + this.emit('error', ex) + } + } // flush the tail. Stream control is now in the verify and decrypt streams return setImmediate(() => this._transform(tail, encoding, callback)) }) diff --git a/modules/decrypt-node/src/verify_stream.ts b/modules/decrypt-node/src/verify_stream.ts index c3d8dc7c1..87296ac0e 100644 --- a/modules/decrypt-node/src/verify_stream.ts +++ b/modules/decrypt-node/src/verify_stream.ts @@ -49,6 +49,7 @@ interface VerifyState { authTagBuffer: Buffer currentFrame?: BodyHeader signatureInfo: Buffer + sequenceNumber: number } export class VerifyStream extends PortableTransformWithType { @@ -56,7 +57,8 @@ export class VerifyStream extends PortableTransformWithType { private _verifyState: VerifyState = { buffer: Buffer.alloc(0), authTagBuffer: Buffer.alloc(0), - signatureInfo: Buffer.alloc(0) + signatureInfo: Buffer.alloc(0), + sequenceNumber: 0 } private _verify?: AWSVerify private _maxBodySize?: number @@ -118,6 +120,17 @@ export class VerifyStream extends PortableTransformWithType { */ needs(!this._maxBodySize || this._maxBodySize >= frameHeader.contentLength, 'maxBodySize exceeded.') + /* Keeping track of the sequence number myself. */ + state.sequenceNumber += 1 + + /* Precondition: The sequence number is required to monotonically increase, starting from 1. + * This is to avoid a bad actor from abusing the sequence number on un-signed algorithm suites. + * If the frame size matched the data format (say NDJSON), + * then the data could be significantly altered just by rearranging the frames. + * Non-framed data returns a sequenceNumber of 1. + */ + needs(frameHeader.sequenceNumber === state.sequenceNumber, 'Encrypted body sequence out of order.') + if (this._verify) { this._verify.update(frameBuffer.slice(0, frameHeader.readPos)) } diff --git a/modules/decrypt-node/test/decrypt.test.ts b/modules/decrypt-node/test/decrypt.test.ts index 41ccce584..ab0920d84 100644 --- a/modules/decrypt-node/test/decrypt.test.ts +++ b/modules/decrypt-node/test/decrypt.test.ts @@ -67,4 +67,12 @@ describe('decrypt', () => { expect(messageHeader.encryptionContext).to.deep.equal(fixtures.encryptionContext()) expect(test.toString('base64')).to.equal(fixtures.base64Plaintext()) }) + + it('Precondition: The sequence number is required to monotonically increase, starting from 1.', async () => { + return expect(decrypt( + fixtures.decryptKeyring(), + fixtures.frameSequenceOutOfOrder(), + { encoding: 'base64' } + )).to.rejectedWith(Error, 'Encrypted body sequence out of order.') + }) }) diff --git a/modules/decrypt-node/test/fixtures.ts b/modules/decrypt-node/test/fixtures.ts index 8829d6aa9..aa1682d76 100644 --- a/modules/decrypt-node/test/fixtures.ts +++ b/modules/decrypt-node/test/fixtures.ts @@ -37,6 +37,19 @@ export function encryptionContext () { } } +export function frameSequenceOutOfOrder () { + /* This is the string 'asdf' encrypted with ALG_AES128_GCM_IV12_TAG16. + * The data key is all zeros. + * The frames have been reordered in order to test decryption failure. + */ + return 'AYAAFG/G0IFAKXNb9xuUlGZGldIAAAABAAFrAAFrAAMAAAACAAAAAAwAAAAB' + // header + 'AAAAAAAAAAAAAAAAslUUXWAiznTYc7fZwFSbDw' + // header auth + 'AAAAQAAAAAAAAAAAAAAAR9lLkID8B52uAln4zhh6ReJf' + // f + 'AAAAMAAAAAAAAAAAAAAAPnL7BJ22Xgjm6rneeNHrus9w' + // d + 'AAAAIAAAAAAAAAAAAAAAKiyryqy/AOiRH01aCOktajyw' + // s + 'AAAAEAAAAAAAAAAAAAAAEPhwHD/lPe6S/+QmqmQq+xXQ' // a +} + export function decryptKeyring () { class TestKeyring extends KeyringNode { async _onEncrypt () : Promise {