From 9683779dbf6f3ffde6860286f96c92abe63b9bd2 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Tue, 11 Mar 2025 14:20:02 +0100 Subject: [PATCH] [PM-17984] Remove AES128CBC-HMAC encryption (#13304) * Remove AES128CBC-HMAC encryption * Increase test coverage --- .../crypto/abstractions/encrypt.service.ts | 1 - .../encrypt.service.implementation.ts | 19 ------ .../crypto/services/encrypt.service.spec.ts | 62 +++++++++---------- .../platform/enums/encryption-type.enum.ts | 7 +-- .../models/domain/enc-array-buffer.spec.ts | 39 ++++++------ .../models/domain/enc-array-buffer.ts | 1 - .../platform/models/domain/enc-string.spec.ts | 2 - .../src/platform/models/domain/enc-string.ts | 6 +- .../domain/symmetric-crypto-key.spec.ts | 15 ----- .../models/domain/symmetric-crypto-key.ts | 3 - 10 files changed, 50 insertions(+), 105 deletions(-) diff --git a/libs/common/src/key-management/crypto/abstractions/encrypt.service.ts b/libs/common/src/key-management/crypto/abstractions/encrypt.service.ts index 484327bcd27..f7f064f5251 100644 --- a/libs/common/src/key-management/crypto/abstractions/encrypt.service.ts +++ b/libs/common/src/key-management/crypto/abstractions/encrypt.service.ts @@ -36,7 +36,6 @@ export abstract class EncryptService { ): Promise; abstract rsaEncrypt(data: Uint8Array, publicKey: Uint8Array): Promise; abstract rsaDecrypt(data: EncString, privateKey: Uint8Array): Promise; - abstract resolveLegacyKey(key: SymmetricCryptoKey, encThing: Encrypted): SymmetricCryptoKey; /** * @deprecated Replaced by BulkEncryptService, remove once the feature is tested and the featureflag PM-4154-multi-worker-encryption-service is removed * @param items The items to decrypt diff --git a/libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts b/libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts index 8a001886837..d426340c277 100644 --- a/libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts +++ b/libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts @@ -78,8 +78,6 @@ export class EncryptServiceImplementation implements EncryptService { throw new Error("No key provided for decryption."); } - key = this.resolveLegacyKey(key, encString); - // DO NOT REMOVE OR MOVE. This prevents downgrade to mac-less CBC, which would compromise integrity and confidentiality. if (key.macKey != null && encString?.mac == null) { this.logService.error( @@ -145,8 +143,6 @@ export class EncryptServiceImplementation implements EncryptService { throw new Error("Nothing provided for decryption."); } - key = this.resolveLegacyKey(key, encThing); - // DO NOT REMOVE OR MOVE. This prevents downgrade to mac-less CBC, which would compromise integrity and confidentiality. if (key.macKey != null && encThing.macBytes == null) { this.logService.error( @@ -298,19 +294,4 @@ export class EncryptServiceImplementation implements EncryptService { this.logService.error(msg); } } - - /** - * Transform into new key for the old encrypt-then-mac scheme if required, otherwise return the current key unchanged - * @param encThing The encrypted object (e.g. encString or encArrayBuffer) that you want to decrypt - */ - resolveLegacyKey(key: SymmetricCryptoKey, encThing: Encrypted): SymmetricCryptoKey { - if ( - encThing.encryptionType === EncryptionType.AesCbc128_HmacSha256_B64 && - key.encType === EncryptionType.AesCbc256_B64 - ) { - return new SymmetricCryptoKey(key.key, EncryptionType.AesCbc128_HmacSha256_B64); - } - - return key; - } } diff --git a/libs/common/src/key-management/crypto/services/encrypt.service.spec.ts b/libs/common/src/key-management/crypto/services/encrypt.service.spec.ts index cff695f4829..3ce0d5883d2 100644 --- a/libs/common/src/key-management/crypto/services/encrypt.service.spec.ts +++ b/libs/common/src/key-management/crypto/services/encrypt.service.spec.ts @@ -325,6 +325,25 @@ describe("EncryptService", () => { }); }); + describe("decryptToUtf8", () => { + it("throws if no key is provided", () => { + return expect(encryptService.decryptToUtf8(null, null)).rejects.toThrow( + "No key provided for decryption.", + ); + }); + it("returns null if key is mac key but encstring has no mac", async () => { + const key = new SymmetricCryptoKey( + makeStaticByteArray(64, 0), + EncryptionType.AesCbc256_HmacSha256_B64, + ); + const encString = new EncString(EncryptionType.AesCbc256_B64, "data"); + + const actual = await encryptService.decryptToUtf8(encString, key); + expect(actual).toBeNull(); + expect(logService.error).toHaveBeenCalled(); + }); + }); + describe("rsa", () => { const data = makeStaticByteArray(10, 100); const encryptedData = makeStaticByteArray(10, 150); @@ -370,17 +389,16 @@ describe("EncryptService", () => { return expect(encryptService.rsaDecrypt(encString, null)).rejects.toThrow("No private key"); }); - it.each([ - EncryptionType.AesCbc256_B64, - EncryptionType.AesCbc128_HmacSha256_B64, - EncryptionType.AesCbc256_HmacSha256_B64, - ])("throws if encryption type is %s", async (encType) => { - encString.encryptionType = encType; + it.each([EncryptionType.AesCbc256_B64, EncryptionType.AesCbc256_HmacSha256_B64])( + "throws if encryption type is %s", + async (encType) => { + encString.encryptionType = encType; - await expect(encryptService.rsaDecrypt(encString, privateKey)).rejects.toThrow( - "Invalid encryption type", - ); - }); + await expect(encryptService.rsaDecrypt(encString, privateKey)).rejects.toThrow( + "Invalid encryption type", + ); + }, + ); it("decrypts data with provided key", async () => { cryptoFunctionService.rsaDecrypt.mockResolvedValue(data); @@ -398,30 +416,6 @@ describe("EncryptService", () => { }); }); - describe("resolveLegacyKey", () => { - it("creates a legacy key if required", async () => { - const key = new SymmetricCryptoKey(makeStaticByteArray(32), EncryptionType.AesCbc256_B64); - const encString = mock(); - encString.encryptionType = EncryptionType.AesCbc128_HmacSha256_B64; - - const actual = encryptService.resolveLegacyKey(key, encString); - - const expected = new SymmetricCryptoKey(key.key, EncryptionType.AesCbc128_HmacSha256_B64); - expect(actual).toEqual(expected); - }); - - it("does not create a legacy key if not required", async () => { - const encType = EncryptionType.AesCbc256_HmacSha256_B64; - const key = new SymmetricCryptoKey(makeStaticByteArray(64), encType); - const encString = mock(); - encString.encryptionType = encType; - - const actual = encryptService.resolveLegacyKey(key, encString); - - expect(actual).toEqual(key); - }); - }); - describe("hash", () => { it("hashes a string and returns b64", async () => { cryptoFunctionService.hash.mockResolvedValue(Uint8Array.from([1, 2, 3])); diff --git a/libs/common/src/platform/enums/encryption-type.enum.ts b/libs/common/src/platform/enums/encryption-type.enum.ts index a0ffe679279..fd484dc2fdf 100644 --- a/libs/common/src/platform/enums/encryption-type.enum.ts +++ b/libs/common/src/platform/enums/encryption-type.enum.ts @@ -1,6 +1,6 @@ export enum EncryptionType { AesCbc256_B64 = 0, - AesCbc128_HmacSha256_B64 = 1, + // Type 1 was the unused and removed AesCbc128_HmacSha256_B64 AesCbc256_HmacSha256_B64 = 2, Rsa2048_OaepSha256_B64 = 3, Rsa2048_OaepSha1_B64 = 4, @@ -17,12 +17,10 @@ export function encryptionTypeToString(encryptionType: EncryptionType): string { } /** The expected number of parts to a serialized EncString of the given encryption type. - * For example, an EncString of type AesCbc256_B64 will have 2 parts, and an EncString of type - * AesCbc128_HmacSha256_B64 will have 3 parts. + * For example, an EncString of type AesCbc256_B64 will have 2 parts * * Example of annotated serialized EncStrings: * 0.iv|data - * 1.iv|data|mac * 2.iv|data|mac * 3.data * 4.data @@ -33,7 +31,6 @@ export function encryptionTypeToString(encryptionType: EncryptionType): string { */ export const EXPECTED_NUM_PARTS_BY_ENCRYPTION_TYPE = { [EncryptionType.AesCbc256_B64]: 2, - [EncryptionType.AesCbc128_HmacSha256_B64]: 3, [EncryptionType.AesCbc256_HmacSha256_B64]: 3, [EncryptionType.Rsa2048_OaepSha256_B64]: 1, [EncryptionType.Rsa2048_OaepSha1_B64]: 1, diff --git a/libs/common/src/platform/models/domain/enc-array-buffer.spec.ts b/libs/common/src/platform/models/domain/enc-array-buffer.spec.ts index 45a45ffe087..de0fea58e36 100644 --- a/libs/common/src/platform/models/domain/enc-array-buffer.spec.ts +++ b/libs/common/src/platform/models/domain/enc-array-buffer.spec.ts @@ -5,28 +5,28 @@ import { EncArrayBuffer } from "./enc-array-buffer"; describe("encArrayBuffer", () => { describe("parses the buffer", () => { - test.each([ - [EncryptionType.AesCbc128_HmacSha256_B64, "AesCbc128_HmacSha256_B64"], - [EncryptionType.AesCbc256_HmacSha256_B64, "AesCbc256_HmacSha256_B64"], - ])("with %c%s", (encType: EncryptionType) => { - const iv = makeStaticByteArray(16, 10); - const mac = makeStaticByteArray(32, 20); - // We use the minimum data length of 1 to test the boundary of valid lengths - const data = makeStaticByteArray(1, 100); + test.each([[EncryptionType.AesCbc256_HmacSha256_B64, "AesCbc256_HmacSha256_B64"]])( + "with %c%s", + (encType: EncryptionType) => { + const iv = makeStaticByteArray(16, 10); + const mac = makeStaticByteArray(32, 20); + // We use the minimum data length of 1 to test the boundary of valid lengths + const data = makeStaticByteArray(1, 100); - const array = new Uint8Array(1 + iv.byteLength + mac.byteLength + data.byteLength); - array.set([encType]); - array.set(iv, 1); - array.set(mac, 1 + iv.byteLength); - array.set(data, 1 + iv.byteLength + mac.byteLength); + const array = new Uint8Array(1 + iv.byteLength + mac.byteLength + data.byteLength); + array.set([encType]); + array.set(iv, 1); + array.set(mac, 1 + iv.byteLength); + array.set(data, 1 + iv.byteLength + mac.byteLength); - const actual = new EncArrayBuffer(array); + const actual = new EncArrayBuffer(array); - expect(actual.encryptionType).toEqual(encType); - expect(actual.ivBytes).toEqualBuffer(iv); - expect(actual.macBytes).toEqualBuffer(mac); - expect(actual.dataBytes).toEqualBuffer(data); - }); + expect(actual.encryptionType).toEqual(encType); + expect(actual.ivBytes).toEqualBuffer(iv); + expect(actual.macBytes).toEqualBuffer(mac); + expect(actual.dataBytes).toEqualBuffer(data); + }, + ); it("with AesCbc256_B64", () => { const encType = EncryptionType.AesCbc256_B64; @@ -50,7 +50,6 @@ describe("encArrayBuffer", () => { describe("throws if the buffer has an invalid length", () => { test.each([ - [EncryptionType.AesCbc128_HmacSha256_B64, 50, "AesCbc128_HmacSha256_B64"], [EncryptionType.AesCbc256_HmacSha256_B64, 50, "AesCbc256_HmacSha256_B64"], [EncryptionType.AesCbc256_B64, 18, "AesCbc256_B64"], ])("with %c%c%s", (encType: EncryptionType, minLength: number) => { diff --git a/libs/common/src/platform/models/domain/enc-array-buffer.ts b/libs/common/src/platform/models/domain/enc-array-buffer.ts index 305504f57b7..8b69cb347ba 100644 --- a/libs/common/src/platform/models/domain/enc-array-buffer.ts +++ b/libs/common/src/platform/models/domain/enc-array-buffer.ts @@ -20,7 +20,6 @@ export class EncArrayBuffer implements Encrypted { const encType = encBytes[0]; switch (encType) { - case EncryptionType.AesCbc128_HmacSha256_B64: case EncryptionType.AesCbc256_HmacSha256_B64: { const minimumLength = ENC_TYPE_LENGTH + IV_LENGTH + MAC_LENGTH + MIN_DATA_LENGTH; if (encBytes.length < minimumLength) { diff --git a/libs/common/src/platform/models/domain/enc-string.spec.ts b/libs/common/src/platform/models/domain/enc-string.spec.ts index 3b2586fc22f..c3f257d442a 100644 --- a/libs/common/src/platform/models/domain/enc-string.spec.ts +++ b/libs/common/src/platform/models/domain/enc-string.spec.ts @@ -60,9 +60,7 @@ describe("EncString", () => { const cases = [ "aXY=|Y3Q=", // AesCbc256_B64 w/out header - "aXY=|Y3Q=|cnNhQ3Q=", // AesCbc128_HmacSha256_B64 w/out header "0.QmFzZTY0UGFydA==|QmFzZTY0UGFydA==", // AesCbc256_B64 with header - "1.QmFzZTY0UGFydA==|QmFzZTY0UGFydA==|QmFzZTY0UGFydA==", // AesCbc128_HmacSha256_B64 "2.QmFzZTY0UGFydA==|QmFzZTY0UGFydA==|QmFzZTY0UGFydA==", // AesCbc256_HmacSha256_B64 "3.QmFzZTY0UGFydA==", // Rsa2048_OaepSha256_B64 "4.QmFzZTY0UGFydA==", // Rsa2048_OaepSha1_B64 diff --git a/libs/common/src/platform/models/domain/enc-string.ts b/libs/common/src/platform/models/domain/enc-string.ts index 4ea58a6809e..b0b03e0fb3c 100644 --- a/libs/common/src/platform/models/domain/enc-string.ts +++ b/libs/common/src/platform/models/domain/enc-string.ts @@ -89,7 +89,6 @@ export class EncString implements Encrypted { } switch (encType) { - case EncryptionType.AesCbc128_HmacSha256_B64: case EncryptionType.AesCbc256_HmacSha256_B64: this.iv = encPieces[0]; this.data = encPieces[1]; @@ -132,10 +131,7 @@ export class EncString implements Encrypted { } } else { encPieces = encryptedString.split("|"); - encType = - encPieces.length === 3 - ? EncryptionType.AesCbc128_HmacSha256_B64 - : EncryptionType.AesCbc256_B64; + encType = EncryptionType.AesCbc256_B64; } return { diff --git a/libs/common/src/platform/models/domain/symmetric-crypto-key.spec.ts b/libs/common/src/platform/models/domain/symmetric-crypto-key.spec.ts index e4c43264eaf..58c902ebab6 100644 --- a/libs/common/src/platform/models/domain/symmetric-crypto-key.spec.ts +++ b/libs/common/src/platform/models/domain/symmetric-crypto-key.spec.ts @@ -27,21 +27,6 @@ describe("SymmetricCryptoKey", () => { }); }); - it("AesCbc128_HmacSha256_B64", () => { - const key = makeStaticByteArray(32); - const cryptoKey = new SymmetricCryptoKey(key, EncryptionType.AesCbc128_HmacSha256_B64); - - expect(cryptoKey).toEqual({ - encKey: key.slice(0, 16), - encKeyB64: "AAECAwQFBgcICQoLDA0ODw==", - encType: 1, - key: key, - keyB64: "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=", - macKey: key.slice(16, 32), - macKeyB64: "EBESExQVFhcYGRobHB0eHw==", - }); - }); - it("AesCbc256_HmacSha256_B64", () => { const key = makeStaticByteArray(64); const cryptoKey = new SymmetricCryptoKey(key); diff --git a/libs/common/src/platform/models/domain/symmetric-crypto-key.ts b/libs/common/src/platform/models/domain/symmetric-crypto-key.ts index eab4c7b2114..372b869fd9c 100644 --- a/libs/common/src/platform/models/domain/symmetric-crypto-key.ts +++ b/libs/common/src/platform/models/domain/symmetric-crypto-key.ts @@ -38,9 +38,6 @@ export class SymmetricCryptoKey { if (encType === EncryptionType.AesCbc256_B64 && key.byteLength === 32) { this.encKey = key; this.macKey = null; - } else if (encType === EncryptionType.AesCbc128_HmacSha256_B64 && key.byteLength === 32) { - this.encKey = key.slice(0, 16); - this.macKey = key.slice(16, 32); } else if (encType === EncryptionType.AesCbc256_HmacSha256_B64 && key.byteLength === 64) { this.encKey = key.slice(0, 32); this.macKey = key.slice(32, 64);