fix: sequence number order

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/fixtures.ts

@@ -38,6 +38,20 @@ 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 [
+    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/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 () => {
+    expect(decrypt(
+      fixtures.decryptKeyring(),
+      fixtures.frameSequenceOutOfOrder(),
+      { encoding: 'base64' }
+    )).to.rejectedWith(Error)
+  })
 })

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