From 04489108068a86704e8e2ec6e38dc1a93f97a175 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Thu, 7 Sep 2023 11:42:35 -0400 Subject: [PATCH] [PM-3732] Use subtle to make aes keys (#6162) * Provide `aesGenerateKey` to make aes keys * Use aesGenerateKey when generating a key data * Fix device test --- .../service-accounts/access/access.service.ts | 2 +- ...ice-trust-crypto.service.implementation.ts | 2 +- .../device-trust-crypto.service.spec.ts | 8 +++---- .../auth/services/key-connector.service.ts | 2 +- .../abstractions/crypto-function.service.ts | 9 ++++++++ .../src/platform/services/crypto.service.ts | 10 ++++----- .../web-crypto-function.service.spec.ts | 14 ++++++++++++ .../services/web-crypto-function.service.ts | 22 +++++++++++++++---- .../src/tools/send/services/send.service.ts | 2 +- .../node-crypto-function.service.spec.ts | 9 ++++++++ .../services/node-crypto-function.service.ts | 4 ++++ 11 files changed, 67 insertions(+), 17 deletions(-) diff --git a/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/access/access.service.ts b/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/access/access.service.ts index 75b6473fd1..d087fb9ef6 100644 --- a/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/access/access.service.ts +++ b/bitwarden_license/bit-web/src/app/secrets-manager/service-accounts/access/access.service.ts @@ -53,7 +53,7 @@ export class AccessService { serviceAccountId: string, accessTokenView: AccessTokenView ): Promise { - const keyMaterial = await this.cryptoFunctionService.randomBytes(16); + const keyMaterial = await this.cryptoFunctionService.aesGenerateKey(128); const key = await this.cryptoFunctionService.hkdf( keyMaterial, "bitwarden-accesstoken", diff --git a/libs/common/src/auth/services/device-trust-crypto.service.implementation.ts b/libs/common/src/auth/services/device-trust-crypto.service.implementation.ts index e7e1349eae..fe45c5e208 100644 --- a/libs/common/src/auth/services/device-trust-crypto.service.implementation.ts +++ b/libs/common/src/auth/services/device-trust-crypto.service.implementation.ts @@ -167,7 +167,7 @@ export class DeviceTrustCryptoService implements DeviceTrustCryptoServiceAbstrac private async makeDeviceKey(): Promise { // Create 512-bit device key - const randomBytes: CsprngArray = await this.cryptoFunctionService.randomBytes(64); + const randomBytes: CsprngArray = await this.cryptoFunctionService.aesGenerateKey(512); const deviceKey = new SymmetricCryptoKey(randomBytes) as DeviceKey; return deviceKey; diff --git a/libs/common/src/auth/services/device-trust-crypto.service.spec.ts b/libs/common/src/auth/services/device-trust-crypto.service.spec.ts index 1ba2000892..b56c8b922a 100644 --- a/libs/common/src/auth/services/device-trust-crypto.service.spec.ts +++ b/libs/common/src/auth/services/device-trust-crypto.service.spec.ts @@ -168,16 +168,16 @@ describe("deviceTrustCryptoService", () => { it("creates a new non-null 64 byte device key, securely stores it, and returns it", async () => { const mockRandomBytes = new Uint8Array(deviceKeyBytesLength) as CsprngArray; - const cryptoFuncSvcRandomBytesSpy = jest - .spyOn(cryptoFunctionService, "randomBytes") + const cryptoFuncSvcGenerateKeySpy = jest + .spyOn(cryptoFunctionService, "aesGenerateKey") .mockResolvedValue(mockRandomBytes); // TypeScript will allow calling private methods if the object is of type 'any' // This is a hacky workaround, but it allows for cleaner tests const deviceKey = await (deviceTrustCryptoService as any).makeDeviceKey(); - expect(cryptoFuncSvcRandomBytesSpy).toHaveBeenCalledTimes(1); - expect(cryptoFuncSvcRandomBytesSpy).toHaveBeenCalledWith(deviceKeyBytesLength); + expect(cryptoFuncSvcGenerateKeySpy).toHaveBeenCalledTimes(1); + expect(cryptoFuncSvcGenerateKeySpy).toHaveBeenCalledWith(deviceKeyBytesLength * 8); expect(deviceKey).not.toBeNull(); expect(deviceKey).toBeInstanceOf(SymmetricCryptoKey); diff --git a/libs/common/src/auth/services/key-connector.service.ts b/libs/common/src/auth/services/key-connector.service.ts index 7aa41a9857..e277537cbd 100644 --- a/libs/common/src/auth/services/key-connector.service.ts +++ b/libs/common/src/auth/services/key-connector.service.ts @@ -93,7 +93,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { keyConnectorUrl: legacyKeyConnectorUrl, userDecryptionOptions, } = tokenResponse; - const password = await this.cryptoFunctionService.randomBytes(64); + const password = await this.cryptoFunctionService.aesGenerateKey(512); const kdfConfig = new KdfConfig(kdfIterations, kdfMemory, kdfParallelism); const masterKey = await this.cryptoService.makeMasterKey( diff --git a/libs/common/src/platform/abstractions/crypto-function.service.ts b/libs/common/src/platform/abstractions/crypto-function.service.ts index 7c76702532..9f171872ae 100644 --- a/libs/common/src/platform/abstractions/crypto-function.service.ts +++ b/libs/common/src/platform/abstractions/crypto-function.service.ts @@ -66,5 +66,14 @@ export abstract class CryptoFunctionService { ) => Promise; rsaExtractPublicKey: (privateKey: Uint8Array) => Promise; rsaGenerateKeyPair: (length: 1024 | 2048 | 4096) => Promise<[Uint8Array, Uint8Array]>; + /** + * Generates a key of the given length suitable for use in AES encryption + */ + aesGenerateKey: (bitLength: 128 | 192 | 256 | 512) => Promise; + /** + * Generates a random array of bytes of the given length. Uses a cryptographically secure random number generator. + * + * Do not use this for generating encryption keys. Use aesGenerateKey or rsaGenerateKeyPair instead. + */ randomBytes: (length: number) => Promise; } diff --git a/libs/common/src/platform/services/crypto.service.ts b/libs/common/src/platform/services/crypto.service.ts index 7be0738f68..1f5cd10edc 100644 --- a/libs/common/src/platform/services/crypto.service.ts +++ b/libs/common/src/platform/services/crypto.service.ts @@ -119,7 +119,7 @@ export class CryptoService implements CryptoServiceAbstraction { throw new Error("No Master Key found."); } - const newUserKey = await this.cryptoFunctionService.randomBytes(64); + const newUserKey = await this.cryptoFunctionService.aesGenerateKey(512); return this.buildProtectedSymmetricKey(masterKey, newUserKey); } @@ -367,7 +367,7 @@ export class CryptoService implements CryptoServiceAbstraction { throw new Error("No key provided"); } - const newSymKey = await this.cryptoFunctionService.randomBytes(64); + const newSymKey = await this.cryptoFunctionService.aesGenerateKey(512); return this.buildProtectedSymmetricKey(key, newSymKey); } @@ -458,7 +458,7 @@ export class CryptoService implements CryptoServiceAbstraction { } async makeOrgKey(): Promise<[EncString, T]> { - const shareKey = await this.cryptoFunctionService.randomBytes(64); + const shareKey = await this.cryptoFunctionService.aesGenerateKey(512); const publicKey = await this.getPublicKey(); const encShareKey = await this.rsaEncrypt(shareKey, publicKey); return [encShareKey, new SymmetricCryptoKey(shareKey) as T]; @@ -731,8 +731,8 @@ export class CryptoService implements CryptoServiceAbstraction { publicKey: string; privateKey: EncString; }> { - const randomBytes = await this.cryptoFunctionService.randomBytes(64); - const userKey = new SymmetricCryptoKey(randomBytes) as UserKey; + const rawKey = await this.cryptoFunctionService.aesGenerateKey(512); + const userKey = new SymmetricCryptoKey(rawKey) as UserKey; const [publicKey, privateKey] = await this.makeKeyPair(userKey); await this.setUserKey(userKey); await this.stateService.setEncryptedPrivateKey(privateKey.encryptedString); diff --git a/libs/common/src/platform/services/web-crypto-function.service.spec.ts b/libs/common/src/platform/services/web-crypto-function.service.spec.ts index 1f88a41982..a4b7a3ccaf 100644 --- a/libs/common/src/platform/services/web-crypto-function.service.spec.ts +++ b/libs/common/src/platform/services/web-crypto-function.service.spec.ts @@ -354,6 +354,20 @@ describe("WebCrypto Function Service", () => { ).toBeTruthy(); }); }); + + describe("aesGenerateKey", () => { + it.each([128, 192, 256, 512])("Should make a key of %s bits long", async (length) => { + const cryptoFunctionService = getWebCryptoFunctionService(); + const key = await cryptoFunctionService.aesGenerateKey(length); + expect(key.byteLength * 8).toBe(length); + }); + + it("should not repeat itself for 512 length special case", async () => { + const cryptoFunctionService = getWebCryptoFunctionService(); + const key = await cryptoFunctionService.aesGenerateKey(512); + expect(key.slice(0, 32)).not.toEqual(key.slice(32, 64)); + }); + }); }); function testPbkdf2( diff --git a/libs/common/src/platform/services/web-crypto-function.service.ts b/libs/common/src/platform/services/web-crypto-function.service.ts index 4cf8636a09..7c52cae543 100644 --- a/libs/common/src/platform/services/web-crypto-function.service.ts +++ b/libs/common/src/platform/services/web-crypto-function.service.ts @@ -347,6 +347,23 @@ export class WebCryptoFunctionService implements CryptoFunctionService { return new Uint8Array(buffer); } + async aesGenerateKey(bitLength = 128 | 192 | 256 | 512): Promise { + if (bitLength === 512) { + // 512 bit keys are not supported in WebCrypto, so we concat two 256 bit keys + const key1 = await this.aesGenerateKey(256); + const key2 = await this.aesGenerateKey(256); + return new Uint8Array([...key1, ...key2]) as CsprngArray; + } + const aesParams = { + name: "AES-CBC", + length: bitLength, + }; + + const key = await this.subtle.generateKey(aesParams, true, ["encrypt", "decrypt"]); + const rawKey = await this.subtle.exportKey("raw", key); + return new Uint8Array(rawKey) as CsprngArray; + } + async rsaGenerateKeyPair(length: 1024 | 2048 | 4096): Promise<[Uint8Array, Uint8Array]> { const rsaParams = { name: "RSA-OAEP", @@ -355,10 +372,7 @@ export class WebCryptoFunctionService implements CryptoFunctionService { // Have to specify some algorithm hash: { name: this.toWebCryptoAlgorithm("sha1") }, }; - const keyPair = (await this.subtle.generateKey(rsaParams, true, [ - "encrypt", - "decrypt", - ])) as CryptoKeyPair; + const keyPair = await this.subtle.generateKey(rsaParams, true, ["encrypt", "decrypt"]); const publicKey = await this.subtle.exportKey("spki", keyPair.publicKey); const privateKey = await this.subtle.exportKey("pkcs8", keyPair.privateKey); return [new Uint8Array(publicKey), new Uint8Array(privateKey)]; diff --git a/libs/common/src/tools/send/services/send.service.ts b/libs/common/src/tools/send/services/send.service.ts index e1ddeae708..fc1985067f 100644 --- a/libs/common/src/tools/send/services/send.service.ts +++ b/libs/common/src/tools/send/services/send.service.ts @@ -70,7 +70,7 @@ export class SendService implements InternalSendServiceAbstraction { send.hideEmail = model.hideEmail; send.maxAccessCount = model.maxAccessCount; if (model.key == null) { - model.key = await this.cryptoFunctionService.randomBytes(16); + model.key = await this.cryptoFunctionService.aesGenerateKey(128); model.cryptoKey = await this.cryptoService.makeSendKey(model.key); } if (password != null) { diff --git a/libs/node/src/services/node-crypto-function.service.spec.ts b/libs/node/src/services/node-crypto-function.service.spec.ts index 5a7f369d27..051c45712f 100644 --- a/libs/node/src/services/node-crypto-function.service.spec.ts +++ b/libs/node/src/services/node-crypto-function.service.spec.ts @@ -271,6 +271,15 @@ describe("NodeCrypto Function Service", () => { ).toBeTruthy(); }); }); + + describe("aesGenerateKey", () => { + it("should delegate to randomBytes", async () => { + const nodeCryptoFunctionService = new NodeCryptoFunctionService(); + const spy = jest.spyOn(nodeCryptoFunctionService, "randomBytes"); + await nodeCryptoFunctionService.aesGenerateKey(256); + expect(spy).toHaveBeenCalledWith(32); + }); + }); }); function testPbkdf2( diff --git a/libs/node/src/services/node-crypto-function.service.ts b/libs/node/src/services/node-crypto-function.service.ts index b824fe1b8c..9c0e49d00d 100644 --- a/libs/node/src/services/node-crypto-function.service.ts +++ b/libs/node/src/services/node-crypto-function.service.ts @@ -271,6 +271,10 @@ export class NodeCryptoFunctionService implements CryptoFunctionService { }); } + aesGenerateKey(bitLength: 128 | 192 | 256 | 512): Promise { + return this.randomBytes(bitLength / 8); + } + randomBytes(length: number): Promise { return new Promise((resolve, reject) => { crypto.randomBytes(length, (error, bytes) => {