From 9494268459f453ded91082645b175368dc467d56 Mon Sep 17 00:00:00 2001 From: seebees Date: Thu, 7 Jan 2021 21:04:06 -0800 Subject: [PATCH 1/2] fix: Boundary condition error in VerifyStream Fixes: #507 Tests exist that push a single byte at a time. This will hit every boundary condition. However this is not complete enough. State can be reset as the code transitions from one boundary condition to another. The prime example of this problem the `VerifyStream`. It has 4 boundary in the `_transform` function. The body header, the body, the auth tag, and signature. By sending 1 byte as the code transitions from the body header to the body the code handling the exiting of the body header has no way to interact with the body, because the 1 byte of data completely isolates these code branches. In this case the code was buffering the body header in `frameBuffer` but when exiting this section and passing control to the actual content section, it used the `chunk` to form the tail. When passing a single byte, this tail would _always_ be empty, because it would have been incorporated into the frame header. Adding tests to span the boundary conditions to verify both this fix and that there are not other lurking. --- modules/decrypt-node/src/verify_stream.ts | 4 +- modules/decrypt-node/test/decrypt.test.ts | 130 ++++++++++++++++++---- 2 files changed, 108 insertions(+), 26 deletions(-) diff --git a/modules/decrypt-node/src/verify_stream.ts b/modules/decrypt-node/src/verify_stream.ts index d3fcb6aa9..707ee077b 100644 --- a/modules/decrypt-node/src/verify_stream.ts +++ b/modules/decrypt-node/src/verify_stream.ts @@ -163,7 +163,7 @@ export class VerifyStream extends PortableTransformWithType { if (this._verify) { this._verify.update(frameBuffer.slice(0, frameHeader.readPos)) } - const tail = chunk.slice(frameHeader.readPos) + const tail = frameBuffer.slice(frameHeader.readPos) this.emit('BodyInfo', frameHeader) state.currentFrame = frameHeader return setImmediate(() => this._transform(tail, enc, callback)) @@ -219,7 +219,7 @@ export class VerifyStream extends PortableTransformWithType { */ this._verifyState.finalAuthTagReceived = true /* Overwriting the _transform function. - * Data flow control is not handled here. + * Data flow control is now handled here. */ this._transform = ( chunk: Buffer, diff --git a/modules/decrypt-node/test/decrypt.test.ts b/modules/decrypt-node/test/decrypt.test.ts index 2674fc19e..81525d668 100644 --- a/modules/decrypt-node/test/decrypt.test.ts +++ b/modules/decrypt-node/test/decrypt.test.ts @@ -59,33 +59,86 @@ describe('decrypt', () => { fixtures.base64CiphertextAlgAes256GcmIv12Tag16HkdfSha384EcdsaP384(), 'base64' ) - const i = ciphertext.values() - const ciphertextStream = from( - ( - _: number, - next: (err: Error | null, chunk: Uint8Array | null) => void - ) => { - /* Pushing 1 byte at time is the most annoying thing. - * This is done intentionally to hit _every_ boundary condition. - */ - const { value, done } = i.next() - if (done) return next(null, null) - next(null, new Uint8Array([value])) - } - ) - const { plaintext: test, messageHeader } = await decrypt( - fixtures.decryptKeyring(), - ciphertextStream + /* Pushing 1 byte at time is annoying. + * But there can be state that can get reset + * from a 1 byte push when the buffering + * is internal to the stream. + * So while 1 byte will hit the beginning + * boundary conditions, + * and the exiting boundary conditions, + * but it will never span any boundary conditions. + * The prime example of this is + * the `VerifyStream`. + * It has 4 boundary in the `_transform` function. + * The body header, the body, the auth tag, and signature. + * By sending 1 byte + * as the code transitions from the body header + * to the body + * the code handling the exiting + * of the body header has no way to interact + * with the body, + * because the 1 byte of data + * completely isolates these code branches. + * So I push at different sizes to hit + * as many boundary conditions as possible. + * Why 20 as a max? + * Permuting every possible combination + * would take too long + * and so anything short of _everything_ + * is a compromise. + * 20 seems large enough. + * I did not do an initial offset + * because that just moves me closer + * to permuting every option. + * An alternative would be a variable chunk size. + * Doing this randomly is a bad idea. + * Tests that only fail _sometimes_ + * are bad tests. + * It is too easy to try again, + * and when everything passes just move on. + * And again, doing every permutation + * is too expensive at this time. + */ + const results = await Promise.all( + [ + { size: 1 }, + { size: 2 }, + { size: 3 }, + { size: 4 }, + { size: 5 }, + { size: 6 }, + { size: 7 }, + { size: 8 }, + { size: 9 }, + { size: 10 }, + { size: 11 }, + { size: 12 }, + { size: 13 }, + { size: 14 }, + { size: 15 }, + { size: 16 }, + { size: 17 }, + { size: 18 }, + { size: 19 }, + { size: 20 }, + ].map(async (op) => + decrypt( + fixtures.decryptKeyring(), + chunkCipherTextStream(ciphertext, op) + ) + ) ) - expect(messageHeader.suiteId).to.equal( - AlgorithmSuiteIdentifier.ALG_AES256_GCM_IV12_TAG16_HKDF_SHA384_ECDSA_P384 - ) - expect(messageHeader.encryptionContext).to.deep.equal( - fixtures.encryptionContext() - ) - expect(test.toString('base64')).to.equal(fixtures.base64Plaintext()) + results.map(({ plaintext: test, messageHeader }) => { + expect(messageHeader.suiteId).to.equal( + AlgorithmSuiteIdentifier.ALG_AES256_GCM_IV12_TAG16_HKDF_SHA384_ECDSA_P384 + ) + 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 () => { @@ -125,3 +178,32 @@ describe('decrypt', () => { ).to.rejectedWith(Error, 'maxBodySize exceeded.') }) }) + +function chunkCipherTextStream(ciphertext: Buffer, { size }: { size: number }) { + const i = ciphertext.values() + + return from( + ( + _: number, + next: (err: Error | null, chunk: Uint8Array | null) => void + ) => { + const { value, done } = eat(i, size) + if (done) return next(null, null) + next(null, new Uint8Array(value)) + } + ) + + function eat(i: IterableIterator, size: number) { + return Array(size) + .fill(1) + .reduce>( + ({ value }) => { + const { value: item, done } = i.next() + if (done && value.length) return { value, done: false } + if (!done) value.push(item) + return { value, done } + }, + { value: [], done: false } + ) + } +} From 688d7074c4476ad68b82f15cd53503c75e8af82a Mon Sep 17 00:00:00 2001 From: seebees Date: Fri, 8 Jan 2021 14:17:44 -0800 Subject: [PATCH 2/2] Update modules/decrypt-node/test/decrypt.test.ts Co-authored-by: Robin Salkeld --- modules/decrypt-node/test/decrypt.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/decrypt-node/test/decrypt.test.ts b/modules/decrypt-node/test/decrypt.test.ts index 81525d668..7153d41fc 100644 --- a/modules/decrypt-node/test/decrypt.test.ts +++ b/modules/decrypt-node/test/decrypt.test.ts @@ -70,7 +70,7 @@ describe('decrypt', () => { * but it will never span any boundary conditions. * The prime example of this is * the `VerifyStream`. - * It has 4 boundary in the `_transform` function. + * It has 4 byte boundary in the `_transform` function. * The body header, the body, the auth tag, and signature. * By sending 1 byte * as the code transitions from the body header