From e2efffa380ea8f4c9dda1c5d3a0f3e38fe0dea16 Mon Sep 17 00:00:00 2001 From: spypsy Date: Tue, 31 Mar 2026 10:01:22 +0000 Subject: [PATCH] fix(aes128): validate PKCS#7 padding in decryptBufferCBC --- .../src/crypto/aes128/index.test.ts | 69 ++++++++++++++----- .../foundation/src/crypto/aes128/index.ts | 13 +++- 2 files changed, 61 insertions(+), 21 deletions(-) diff --git a/yarn-project/foundation/src/crypto/aes128/index.test.ts b/yarn-project/foundation/src/crypto/aes128/index.test.ts index 496c1b0a93c6..211162228763 100644 --- a/yarn-project/foundation/src/crypto/aes128/index.test.ts +++ b/yarn-project/foundation/src/crypto/aes128/index.test.ts @@ -18,12 +18,11 @@ 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 () => { @@ -31,15 +30,9 @@ describe('aes128', () => { 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 () => { @@ -47,18 +40,56 @@ describe('aes128', () => { 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'); + }); }); diff --git a/yarn-project/foundation/src/crypto/aes128/index.ts b/yarn-project/foundation/src/crypto/aes128/index.ts index e45d58dea764..73a4a5d50a25 100644 --- a/yarn-project/foundation/src/crypto/aes128/index.ts +++ b/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); } }