fix: sequence number order (#158)

The sequence number **must** be honored.
This is especially important for non-signed algorithm suites.

The test vector added to this PR illustrates this.
It is the encrypted string `’asdf’` with a frame length of 1.
The frames have been rearranged to `’fdsa’`.
Without this change the ESDK would decrypt this blob.

Author: seebees

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) {

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)
+    })
+  })
 })

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<WebCryptoEncryptionMaterial> {
     throw new Error('I should never see this error')

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)
 

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))
       })

modules/decrypt-node/src/verify_stream.ts

@@ -49,14 +49,16 @@ interface VerifyState {
   authTagBuffer: Buffer
   currentFrame?: BodyHeader
   signatureInfo: Buffer
+  sequenceNumber: number
 }
 
 export class VerifyStream extends PortableTransformWithType {
   private _headerInfo!: HeaderInfo
   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))
       }

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.')
+  })
 })

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<NodeEncryptionMaterial> {