fix: encrypt/decrypt node (#133)

Small frames exacerbate a race between VerifyStream and DecipherStream.
Once an `AuthTag` event is sent to DecipherStream
the current frame is decrypted and verified before being flushed.
More frame data will not be processed by DecipherStream
because DecipherStream will not execute the _transform callback
until the frame is flushed.
However nothing stops VerifyStream from continuing to process data.
VerifyStream will push the next frame's data,
which will be cached by DecipherStream.
But once VerifyStream reaches the end of the frame it will emit `AuthTag`.
This means the DecipherStream will receive the AuthTag before the frame data
and it will correctly fail on precondition checks.

To resolve this a simple callback is passed from VerifyStream to DecipherStream
in the `AuthTag` event.

Tests for both encrypt and decrypt are included here,
because that is how this issue was found.

Author: seebees

modules/decrypt-node/package.json

@@ -24,16 +24,17 @@
     "tslib": "^1.9.3"
   },
   "devDependencies": {
-    "@aws-crypto/encrypt-node": "^0.1.0-preview.1",
     "@types/chai": "^4.1.4",
     "@types/mocha": "^5.2.5",
     "@types/node": "^11.11.4",
     "@typescript-eslint/eslint-plugin": "^1.9.0",
     "@typescript-eslint/parser": "^1.9.0",
+    "@types/from2": "^2.3.0",
     "chai": "^4.1.2",
     "mocha": "^5.2.0",
     "nyc": "^14.0.0",
     "standard": "^12.0.1",
+    "from2": "^2.3.0",
     "ts-node": "^7.0.1",
     "typescript": "^3.5.0"
   },

modules/decrypt-node/src/decipher_stream.ts

@@ -68,9 +68,9 @@ export function getDecipherStream () {
             decipherInfo = info
           })
           .on('BodyInfo', this._onBodyHeader)
-          .on('AuthTag', async (authTag: Buffer) => {
+          .on('AuthTag', async (authTag: Buffer, next: Function) => {
             try {
-              await this._onAuthTag(authTag)
+              await this._onAuthTag(authTag, next)
             } catch (e) {
               this.emit('error', e)
             }
@@ -134,7 +134,7 @@ export function getDecipherStream () {
       super._read(size)
     }
 
-    _onAuthTag = async (authTag: Buffer) => {
+    _onAuthTag = async (authTag: Buffer, next:Function) => {
       const { decipher, content, contentLength } = decipherState
       /* Precondition: _onAuthTag must be called only after a frame has been accumulated.
        * However there is an edge case.  The final frame _can_ be zero length.
@@ -169,6 +169,17 @@ export function getDecipherStream () {
         }
       }
 
+      /* This frame is complete.
+       * Need to notify the VerifyStream continue.
+       * See the note in `AuthTag` for details.
+       * The short answer is that for small frame sizes,
+       * the "next" frame associated auth tag may be
+       * parsed and send before the "current" is processed.
+       * This will cause the auth tag event to fire before
+       * any _transform events fire and a 'Lengths do not match' precondition to fail.
+       */
+      next()
+
       // This frame is complete. Notify _transform to continue, see needs above for more details
       if (frameComplete) frameComplete()
       // reset for next frame.

modules/decrypt-node/src/decrypt.ts

@@ -36,7 +36,7 @@ export interface DecryptOptions {
 
 export async function decrypt (
   cmm: NodeMaterialsManager|KeyringNode,
-  ciphertext: Buffer|Uint8Array|Readable|string,
+  ciphertext: Buffer|Uint8Array|Readable|string|NodeJS.ReadableStream,
   { encoding, maxBodySize } : DecryptOptions = {}
 ): Promise<DecryptOutput> {
   const stream = decryptStream(cmm, { maxBodySize })

modules/decrypt-node/src/verify_stream.ts

@@ -121,8 +121,6 @@ export class VerifyStream extends PortableTransformWithType {
       if (this._verify) {
         this._verify.update(frameBuffer.slice(0, frameHeader.readPos))
       }
-      // clear the buffer.  It _could_ have cipher text...
-      state.buffer = Buffer.alloc(0)
       const tail = chunk.slice(frameHeader.readPos)
       this.emit('BodyInfo', frameHeader)
       state.currentFrame = frameHeader
@@ -155,23 +153,49 @@ export class VerifyStream extends PortableTransformWithType {
         state.authTagBuffer = Buffer.concat([authTagBuffer, chunk])
         return callback()
       } else {
-        state.authTagBuffer = Buffer.concat([authTagBuffer, chunk], tagLengthBytes)
+        const finalAuthTagBuffer = Buffer.concat([authTagBuffer, chunk], tagLengthBytes)
         if (this._verify) {
-          this._verify.update(state.authTagBuffer)
+          this._verify.update(finalAuthTagBuffer)
         }
-        this.emit('AuthTag', state.authTagBuffer)
-        const tail = chunk.slice(left)
-        if (!currentFrame.isFinalFrame) {
-          state.buffer = Buffer.alloc(0)
-          state.currentFrame = undefined
-          state.authTagBuffer = Buffer.alloc(0)
+        /* Reset state.
+         * Ciphertext buffers and authTag buffers need to be cleared.
+         */
+        state.buffer = Buffer.alloc(0)
+        state.currentFrame = undefined
+        state.authTagBuffer = Buffer.alloc(0)
+        /* After the final frame the file format is _much_ simpler.
+         * Making sure the cascading if blocks fall to the signature can be tricky and brittle.
+         * After the final frame, just moving on to concatenate the signature is much simpler.
+         */
+        if (currentFrame.isFinalFrame) {
+          /* Overwriting the _transform function.
+           * Data flow control is not handled here.
+           */
+          this._transform = (chunk: Buffer, _enc: string, callback: Function) => {
+            if (chunk.length) {
+              state.signatureInfo = Buffer.concat([state.signatureInfo, chunk])
+            }
+
+            callback()
+          }
         }
-        return setImmediate(() => this._transform(tail, enc, callback))
-      }
-    }
 
-    if (chunk.length) {
-      state.signatureInfo = Buffer.concat([state.signatureInfo, chunk])
+        const tail = chunk.slice(left)
+        /* The decipher_stream uses the `AuthTag` event to flush the accumulated frame.
+         * This is because ciphertext should never be returned until it is verified.
+         * i.e. the auth tag checked.
+         * This can create an issue if the chucks and frame size are small.
+         * If the verify stream continues processing and sends the next auth tag,
+         * before the current auth tag has been completed.
+         * This is basically a back pressure issue.
+         * Since the frame size, and consequently the high water mark,
+         * can not be know when the stream is created,
+         * the internal stream state would need to be modified.
+         * I assert that a simple callback is a simpler way to handle this.
+         */
+        const next = () => this._transform(tail, enc, callback)
+        return this.emit('AuthTag', finalAuthTagBuffer, next)
+      }
     }
 
     callback()

modules/decrypt-node/test/decrypt.test.ts

@@ -17,45 +17,54 @@
 
 import { expect } from 'chai'
 import 'mocha'
-import {
-  NodeDecryptionMaterial, // eslint-disable-line no-unused-vars
-  NodeEncryptionMaterial, // eslint-disable-line no-unused-vars
-  KeyringNode, EncryptedDataKey,
-  KeyringTraceFlag, AlgorithmSuiteIdentifier
-} from '@aws-crypto/material-management-node'
-
-// import * as fs from 'fs'
-
-import { encrypt } from '@aws-crypto/encrypt-node'
-import { decrypt } from '../src/decrypt'
-
-describe('simple', () => {
-  it('decrypt what I encrypt', async () => {
-    class TestKeyring extends KeyringNode {
-      async _onEncrypt (material: NodeEncryptionMaterial) {
-        const unencryptedDataKey = new Uint8Array(material.suite.keyLengthBytes).fill(1)
-        const trace = { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_GENERATED_DATA_KEY }
-        const edk = new EncryptedDataKey({ providerId: 'k', providerInfo: 'k', encryptedDataKey: new Uint8Array(3) })
-        return material
-          .setUnencryptedDataKey(unencryptedDataKey, trace)
-          .addEncryptedDataKey(edk, KeyringTraceFlag.WRAPPING_KEY_ENCRYPTED_DATA_KEY)
-      }
-      async _onDecrypt (material: NodeDecryptionMaterial) {
-        const unencryptedDataKey = new Uint8Array(material.suite.keyLengthBytes).fill(1)
-        const trace = { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_DECRYPTED_DATA_KEY }
-        return material.setUnencryptedDataKey(unencryptedDataKey, trace)
-      }
-    }
-
-    const keyRing = new TestKeyring()
-    const suiteId = AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16
-
-    const plaintext = 'asdf'
-    const { ciphertext } = await encrypt(keyRing, plaintext, { suiteId })
-
-    const { plaintext: test, messageHeader } = await decrypt(keyRing, ciphertext)
-
-    expect(messageHeader.suiteId).to.equal(suiteId)
-    expect(test.toString()).to.equal(plaintext)
+import { AlgorithmSuiteIdentifier } from '@aws-crypto/material-management-node'
+import { decrypt } from '../src/index'
+import * as fixtures from './fixtures'
+import from from 'from2'
+
+describe('decrypt', () => {
+  it('string with encoding', async () => {
+    const { plaintext: test, messageHeader } = await decrypt(
+      fixtures.decryptKeyring(),
+      fixtures.base64CiphertextAlgAes256GcmIv12Tag16HkdfSha384EcdsaP384(),
+      { encoding: 'base64' }
+    )
+
+    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('buffer', async () => {
+    const { plaintext: test, messageHeader } = await decrypt(
+      fixtures.decryptKeyring(),
+      Buffer.from(fixtures.base64CiphertextAlgAes256GcmIv12Tag16HkdfSha384EcdsaP384(), 'base64')
+    )
+
+    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('stream', async () => {
+    const ciphertext = Buffer.from(fixtures.base64CiphertextAlgAes256GcmIv12Tag16HkdfSha384EcdsaP384(), 'base64')
+    const i = ciphertext.values()
+    const ciphertextStream = from((_: number, next: Function) => {
+      /* 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
+    )
+
+    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())
   })
 })

modules/decrypt-node/test/fixtures.ts

@@ -0,0 +1,53 @@
+/*
+ * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"). You may not use
+ * this file except in compliance with the License. A copy of the License is
+ * located at
+ *
+ *     http://aws.amazon.com/apache2.0/
+ *
+ * or in the "license" file accompanying this file. This file is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/* eslint-env mocha */
+
+import {
+  NodeDecryptionMaterial, // eslint-disable-line no-unused-vars
+  NodeEncryptionMaterial, // eslint-disable-line no-unused-vars
+  KeyringNode,
+  KeyringTraceFlag
+} from '@aws-crypto/material-management-node'
+
+export function base64CiphertextAlgAes256GcmIv12Tag16HkdfSha384EcdsaP384 () {
+  return 'AYADeJgnuW8vpQmi5QoqHIZWhjkAcAACABVhd3MtY3J5cHRvLXB1YmxpYy1rZXkAREFuWXRGRWV3Wm0rMjhLaElHcHg4UmhrYVVhTGNjSnB5ZjFud0lWUUZHbXlwZ3poSDJYZFNJQko0c0tpU0gzY2t6dz09AAZzaW1wbGUAB2NvbnRleHQAAQABawABawADAAAAAgAAAAAMAAAABQAAAAAAAAAAAAAAABqRZqpijpYGNM6P1L/78AUAAAABAAAAAAAAAAAAAAABIg1k1IeKV+CPUVBnpUkgyVUUZl7wAAAAAgAAAAAAAAAAAAAAAjl6P288VtjjKYeZA7mSeeJgjIUHbAAAAAMAAAAAAAAAAAAAAAO7OY+25yJkVcFvMMXn7VztyOhuIQoAAAAEAAAAAAAAAAAAAAAEG6jOHAz3NwyxgUjm5XFNMBx+2CCvAAAABQAAAAAAAAAAAAAABYRtGxVPUKbha73ay/kYrpl8Drik2gAAAAYAAAAAAAAAAAAAAAbosyHzP31p9EdOf3+dSa5gGfRW9e0AAAAHAAAAAAAAAAAAAAAHsulmBR4FQMbTk+00j5Fa/jD73/UJAAAACAAAAAAAAAAAAAAACMKgPZWTdDKzdPhXQDenInSRW/eOLgAAAAkAAAAAAAAAAAAAAAkdfSyNpBYk9XbFhf6DUnr2acw5lC4AAAAKAAAAAAAAAAAAAAAKnJpofr1UwwPy/+aqviMTrHXgOhM8AAAACwAAAAAAAAAAAAAAC9lvtW1lzA9RGUjnIGadlEhLxRC/FAAAAAwAAAAAAAAAAAAAAAyqJBaQEdmkOUX7uCki3Gh17YlQU3MAAAANAAAAAAAAAAAAAAANEK36ZE9VLiIj2X50N73UHEUtm0BbAAAADgAAAAAAAAAAAAAADkkr1fxL3qLbbC7OSDHqDnrBonOwxQAAAA8AAAAAAAAAAAAAAA8qcNFG+ofU3sOEZd8OXB/rkz0vDa8AAAAQAAAAAAAAAAAAAAAQ3KdsWJ/P8hF8aOhQdQP3v1KBDpB5AAAAEQAAAAAAAAAAAAAAEWyQGXefoGv9ZDfXUi93q+wUQGPzVwAAABIAAAAAAAAAAAAAABIDL/v5IY/z+s28FWzVo46vKNjOEeoAAAATAAAAAAAAAAAAAAATy1uc+McQfMJD8GrAJUaKlyTbXgFgAAAAFAAAAAAAAAAAAAAAFB6Sh2Po4oetBUwm1ABP9F9e1T70GAAAABUAAAAAAAAAAAAAABWm2oOg6agE6jzm3iDZ1brMSTHCOG8AAAAWAAAAAAAAAAAAAAAWsdIbfir5Dame3Uxkri54N2P7rqn6AAAAFwAAAAAAAAAAAAAAF6iPI1YW4fZzyL/355ZHBOLG3VPf1AAAABgAAAAAAAAAAAAAABj5Kjd5Twiu6bpb4o+jas0LRRJFH64AAAAZAAAAAAAAAAAAAAAZTf4xiUOtHeZmi+80M3Oay452R/rJAAAAGgAAAAAAAAAAAAAAGp+ET0LYxOX4JEL8gJudVVPW6qIv3AAAABsAAAAAAAAAAAAAABuTreBPGwJ2bftxQ6Kjwekfth4vWtsAAAAcAAAAAAAAAAAAAAAcdLoFVjR+yx4NVo1BSxv8Llya90EFAAAAHQAAAAAAAAAAAAAAHcFqEIL2wsYK36KQHyJqvJTiF/6nlQAAAB4AAAAAAAAAAAAAAB57QTT/UVRxBucxfhQRYeEU0mUeFxcAAAAfAAAAAAAAAAAAAAAfJyKwIcAURvMfN/Gd5MchygA20EYHAAAAIAAAAAAAAAAAAAAAILXRfQjIux8TeED/TdHHdLuaUEWWZgAAACEAAAAAAAAAAAAAACEi1SsfUozCXF0mCT/tHN8zVvSyWF4AAAAiAAAAAAAAAAAAAAAiFPt44yxRbwruA1F5YkYNokeDLmdiAAAAIwAAAAAAAAAAAAAAIwqdX86PI6IZgTs2SMHo4tLExClkIgAAACQAAAAAAAAAAAAAACQJGEuD6oBPBXU8iupaaNJFzEH/zKcAAAAlAAAAAAAAAAAAAAAlyQiA+1xRREA/qe5Djux6WaPEyUzhAAAAJgAAAAAAAAAAAAAAJqsZT21o1ikdiLkExG949WuTdw1mQQAAACcAAAAAAAAAAAAAACekCgcIX2x9/3zx982dDXfKUQSqARQAAAAoAAAAAAAAAAAAAAAocSNt9kEXLUF0Mydaj4MiBo1WrmGGAAAAKQAAAAAAAAAAAAAAKRHbcJJmpG367RxDInqlcBefk34RbgAAACoAAAAAAAAAAAAAACqmDdWYD/QVD9isxpCTm4KE+j6HKdMAAAArAAAAAAAAAAAAAAAreua98WTPIWH6dSAdzfYWPM9q9hoGAAAALAAAAAAAAAAAAAAALA+DQHkvoxKqVP3dmTQoM17QR4hz1gAAAC0AAAAAAAAAAAAAAC3TCjJBU0hDgBiC/bAHZe5T9CoMfTQAAAAuAAAAAAAAAAAAAAAujkLmjR2G1at5H5QHzKg/B2zNIH+mAAAALwAAAAAAAAAAAAAAL6+0F5aK0j3xqvgrsjmkzt7rZYUQQAAAADAAAAAAAAAAAAAAADDZMoeMElExOKgTTa0/gKqBPiRAqF4AAAAxAAAAAAAAAAAAAAAxbk1Qj+CqjC+gruT6bljBsQD5YTBVAAAAMgAAAAAAAAAAAAAAMhjQQjFR5A9Kn5ot/h4nqKrDTZJsNgAAADMAAAAAAAAAAAAAADO2SB3R/RrukhQx7/jxmjWiLknnnj0AAAA0AAAAAAAAAAAAAAA0wXykERn6CEIMhDCuLhUBmVn6fCu7AAAANQAAAAAAAAAAAAAANf7M3//4JJPLi+mmkKec2QrmuprdigAAADYAAAAAAAAAAAAAADadAVLY8PSrHytIi05tgse0HdyYVikAAAA3AAAAAAAAAAAAAAA3dj606o4y/YZw7gGHrD6JrGWQULV2AAAAOAAAAAAAAAAAAAAAOPgZF/TYVQogBfVMR6P4q5YWnSozUwAAADkAAAAAAAAAAAAAADl41/2WlW/Aq+EVJSHVH8eolMg7stIAAAA6AAAAAAAAAAAAAAA6IdfaZedkARnjm0CYxQhB28ljrigJAAAAOwAAAAAAAAAAAAAAO5PRn7sBV99dQJosnpj8Dy61bUW//QAAADwAAAAAAAAAAAAAADwkmUiXJJBJ4KvATXEeY1b2cOVPDOr/////AAAAPQAAAAAAAAAAAAAAPQAAAABAZDjPrFjtf/NJrKKMK2W9AGgwZgIxAN4h4KUn2VHZhxd/PQlZSmawzL1txgo79vsZjVhV15xqyMZLLcpNuNmK3hNHA83v+AIxAP0Sga/B1gZuyGmQK2cSnDdRIL6bmAzzeTiMcjRoJ6KrYRbLwg8mzmdQLgdvSoPtFg=='
+}
+
+export function base64Plaintext () {
+  return '3Ye0RVTIjYp9Yvi+81Dzq9h9gAUF6akM1mqTbPKMhmwgxTWuj6Wlf8UFUMG7zALPDpN77EleMS3dXUOGlr/nalmwXkBseEo+QxCJgeo6WMuB2xQHZqJT+0glM3mcl2FWwiQDZ9G84dYOW1KSDfiyISe9rTqARl0fmEnD1oB6zlP4cYg7+DDTxOOvw5RndoiOBZ+mLbZT9vHTsJkWB3HgFO06dFAtwSgjUAEaNWMjk04vIT+9SBvql6cOk+GLfUdkH33chNk22yKPF2UQ6+lvW0YqGODIfTBQXypPuuKYXJ3T583YxeiKoxuxZFpVNkg30r5cYPOYulINy+YrWQIbNFRP9Zk0CNkAJ7zsIMhQ8IXH+zG1bQmwh1RDGSAfZhmsR147Jsi6qty9Fe9O'
+}
+
+export function encryptionContext () {
+  return {
+    'aws-crypto-public-key': 'AnYtFEewZm+28KhIGpx8RhkaUaLccJpyf1nwIVQFGmypgzhH2XdSIBJ4sKiSH3ckzw==',
+    simple: 'context'
+  }
+}
+
+export function decryptKeyring () {
+  class TestKeyring extends KeyringNode {
+    async _onEncrypt () : Promise<NodeEncryptionMaterial> {
+      throw new Error('I should never see this error')
+    }
+    async _onDecrypt (material: NodeDecryptionMaterial) {
+      const unencryptedDataKey = new Uint8Array(material.suite.keyLengthBytes).fill(0)
+      const trace = { keyNamespace: 'k', keyName: 'k', flags: KeyringTraceFlag.WRAPPING_KEY_DECRYPTED_DATA_KEY }
+      return material.setUnencryptedDataKey(unencryptedDataKey, trace)
+    }
+  }
+
+  return new TestKeyring()
+}

modules/encrypt-node/package.json

@@ -24,11 +24,13 @@
   },
   "devDependencies": {
     "@types/chai": "^4.1.4",
+    "@types/from2": "^2.3.0",
     "@types/mocha": "^5.2.5",
     "@types/node": "^11.11.4",
     "@typescript-eslint/eslint-plugin": "^1.9.0",
     "@typescript-eslint/parser": "^1.9.0",
     "chai": "^4.1.2",
+    "from2": "^2.3.0",
     "mocha": "^5.2.0",
     "nyc": "^14.0.0",
     "standard": "^12.0.1",

modules/encrypt-node/src/encrypt.ts

@@ -23,7 +23,7 @@ export interface EncryptOutput {
 
 export async function encrypt (
   cmm: KeyringNode|NodeMaterialsManager,
-  plaintext: Buffer|Uint8Array|Readable|string,
+  plaintext: Buffer|Uint8Array|Readable|string|NodeJS.ReadableStream,
   op: EncryptInput = {}
 ): Promise<EncryptOutput> {
   const stream = encryptStream(cmm, op)