fix(aes128): validate PKCS#7 padding in decryptBufferCBC

yarn-project/foundation/src/crypto/aes128/index.test.ts

@@ -18,47 +18,78 @@ describe('aes128', () => {
     return Buffer.concat([data, paddingBuffer]);
   };
 
-  // PKCS#7 padding removal
-  const removePadding = (paddedBuffer: Buffer): Buffer => {
-    // We get padding length from the last byte - in PKCS#7 all the padded bytes contain padding length
-    // and there is always some padding.
-    const paddingToRemove = paddedBuffer[paddedBuffer.length - 1];
-    return paddedBuffer.subarray(0, paddedBuffer.length - paddingToRemove);
+  // Encrypt data with Node's built-in AES-128-CBC
+  const encrypt = (data: Buffer, key: Buffer, iv: Buffer): Buffer => {
+    const cipher = createCipheriv('aes-128-cbc', key, iv);
+    cipher.setAutoPadding(false);
+    return Buffer.concat([cipher.update(pad(data)), cipher.final()]);
   };
 
   it('should correctly encrypt input', async () => {
     const data = randomBytes(32);
     const key = randomBytes(16);
     const iv = randomBytes(16);
 
-    const paddedData = pad(data);
-
-    const cipher = createCipheriv('aes-128-cbc', key, iv);
-    cipher.setAutoPadding(false);
-    const expected = Buffer.concat([cipher.update(paddedData), cipher.final()]);
-
     const result: Buffer = await aes128.encryptBufferCBC(data, iv, key);
 
-    expect(result).toEqual(expected);
+    expect(result).toEqual(encrypt(data, key, iv));
   });
 
   it('should correctly decrypt input', async () => {
     const data = randomBytes(32);
     const key = randomBytes(16);
     const iv = randomBytes(16);
 
-    const paddedData = pad(data);
-
-    const cipher = createCipheriv('aes-128-cbc', key, iv);
-    cipher.setAutoPadding(false);
-    const ciphertext = Buffer.concat([cipher.update(paddedData), cipher.final()]);
+    const ciphertext = encrypt(data, key, iv);
 
     const decipher = createDecipheriv('aes-128-cbc', key, iv);
     decipher.setAutoPadding(false);
-    const expected = removePadding(Buffer.concat([decipher.update(ciphertext), decipher.final()]));
+    const decrypted = Buffer.concat([decipher.update(ciphertext), decipher.final()]);
+    const expected = decrypted.subarray(0, decrypted.length - decrypted[decrypted.length - 1]);
 
     const result: Buffer = await aes128.decryptBufferCBC(ciphertext, iv, key);
 
     expect(result).toEqual(expected);
   });
+
+  it('should throw on invalid PKCS#7 padding length (0)', async () => {
+    const key = randomBytes(16);
+    const iv = randomBytes(16);
+    // Craft a ciphertext whose last plaintext byte decrypts to 0x00 (invalid padding length).
+    // We encrypt a block where the last byte is 0x00 using raw (no-padding) encryption.
+    const plaintext = Buffer.alloc(16, 0x00);
+    const cipher = createCipheriv('aes-128-cbc', key, iv);
+    cipher.setAutoPadding(false);
+    const ciphertext = Buffer.concat([cipher.update(plaintext), cipher.final()]);
+
+    await expect(aes128.decryptBufferCBC(ciphertext, iv, key)).rejects.toThrow('Invalid PKCS#7 padding length: 0');
+  });
+
+  it('should throw on invalid PKCS#7 padding length (> 16)', async () => {
+    const key = randomBytes(16);
+    const iv = randomBytes(16);
+    // Craft a ciphertext whose last plaintext byte is 0x11 = 17, which exceeds the block size.
+    const plaintext = Buffer.alloc(16, 0x11);
+    const cipher = createCipheriv('aes-128-cbc', key, iv);
+    cipher.setAutoPadding(false);
+    const ciphertext = Buffer.concat([cipher.update(plaintext), cipher.final()]);
+
+    await expect(aes128.decryptBufferCBC(ciphertext, iv, key)).rejects.toThrow('Invalid PKCS#7 padding length: 17');
+  });
+
+  it('should throw when padding bytes are inconsistent', async () => {
+    const key = randomBytes(16);
+    const iv = randomBytes(16);
+    // Last byte says padding length is 4, but the 3 bytes before it are not 0x04.
+    const plaintext = Buffer.alloc(16, 0x00);
+    plaintext[15] = 0x04;
+    plaintext[14] = 0x04;
+    plaintext[13] = 0x04;
+    plaintext[12] = 0x01; // should be 0x04 — inconsistent
+    const cipher = createCipheriv('aes-128-cbc', key, iv);
+    cipher.setAutoPadding(false);
+    const ciphertext = Buffer.concat([cipher.update(plaintext), cipher.final()]);
+
+    await expect(aes128.decryptBufferCBC(ciphertext, iv, key)).rejects.toThrow('Invalid PKCS#7 padding');
+  });
 });

yarn-project/foundation/src/crypto/aes128/index.ts

@@ -59,10 +59,19 @@ export class Aes128 {
    * @param iv - AES initialization vector.
    * @param key - Key to decrypt with.
    * @returns Decrypted data.
+   * @throws If the decrypted buffer has invalid PKCS#7 padding.
    */
   public async decryptBufferCBC(data: Uint8Array, iv: Uint8Array, key: Uint8Array) {
     const paddedBuffer = await this.decryptBufferCBCKeepPadding(data, iv, key);
-    const paddingToRemove = paddedBuffer[paddedBuffer.length - 1];
-    return paddedBuffer.subarray(0, paddedBuffer.length - paddingToRemove);
+    const paddingLen = paddedBuffer[paddedBuffer.length - 1];
+    if (paddingLen === 0 || paddingLen > 16) {
+      throw new Error(`Invalid PKCS#7 padding length: ${paddingLen}`);
+    }
+    for (let i = paddedBuffer.length - paddingLen; i < paddedBuffer.length; i++) {
+      if (paddedBuffer[i] !== paddingLen) {
+        throw new Error('Invalid PKCS#7 padding');
+      }
+    }
+    return paddedBuffer.subarray(0, paddedBuffer.length - paddingLen);
   }
 }