[PM-16831] TS Strict crypto function service (#12737)

* strict types in crypto function services * Improve aesDecrypt types
bitwarden · Jan 9, 2025 · 6ef3e9a · 6ef3e9a
1 parent 8cabb36
commit 6ef3e9a
Show file tree

Hide file tree

Showing 8 changed files with 117 additions and 67 deletions.
diff --git a/libs/common/src/platform/abstractions/crypto-function.service.ts b/libs/common/src/platform/abstractions/crypto-function.service.ts
@@ -1,5 +1,5 @@
 import { CsprngArray } from "../../types/csprng";
-import { DecryptParameters } from "../models/domain/decrypt-parameters";
+import { CbcDecryptParameters, EcbDecryptParameters } from "../models/domain/decrypt-parameters";
 import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key";
 
 export abstract class CryptoFunctionService {
@@ -51,11 +51,13 @@ export abstract class CryptoFunctionService {
     iv: string,
     mac: string,
     key: SymmetricCryptoKey,
-  ): DecryptParameters<Uint8Array | string>;
-  abstract aesDecryptFast(
-    parameters: DecryptParameters<Uint8Array | string>,
-    mode: "cbc" | "ecb",
-  ): Promise<string>;
+  ): CbcDecryptParameters<Uint8Array | string>;
+  abstract aesDecryptFast({
+    mode,
+    parameters,
+  }:
+    | { mode: "cbc"; parameters: CbcDecryptParameters<Uint8Array | string> }
+    | { mode: "ecb"; parameters: EcbDecryptParameters<Uint8Array | string> }): Promise<string>;
   abstract aesDecrypt(
     data: Uint8Array,
     iv: Uint8Array,

diff --git a/libs/common/src/platform/models/domain/decrypt-parameters.ts b/libs/common/src/platform/models/domain/decrypt-parameters.ts
@@ -1,10 +1,13 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
-export class DecryptParameters<T> {
+export type CbcDecryptParameters<T> = {
   encKey: T;
   data: T;
   iv: T;
-  macKey: T;
-  mac: T;
+  macKey?: T;
+  mac?: T;
   macData: T;
-}
+};
+
+export type EcbDecryptParameters<T> = {
+  encKey: T;
+  data: T;
+};
diff --git a/libs/common/src/platform/models/domain/symmetric-crypto-key.ts b/libs/common/src/platform/models/domain/symmetric-crypto-key.ts
@@ -7,7 +7,7 @@ import { EncryptionType } from "../../enums";
 
 export class SymmetricCryptoKey {
   key: Uint8Array;
-  encKey?: Uint8Array;
+  encKey: Uint8Array;
   macKey?: Uint8Array;
   encType: EncryptionType;
 
@@ -48,12 +48,8 @@ export class SymmetricCryptoKey {
       throw new Error("Unsupported encType/key length.");
     }
 
-    if (this.key != null) {
-      this.keyB64 = Utils.fromBufferToB64(this.key);
-    }
-    if (this.encKey != null) {
-      this.encKeyB64 = Utils.fromBufferToB64(this.encKey);
-    }
+    this.keyB64 = Utils.fromBufferToB64(this.key);
+    this.encKeyB64 = Utils.fromBufferToB64(this.encKey);
     if (this.macKey != null) {
       this.macKeyB64 = Utils.fromBufferToB64(this.macKey);
     }

diff --git a/libs/common/src/platform/services/cryptography/encrypt.service.implementation.ts b/libs/common/src/platform/services/cryptography/encrypt.service.implementation.ts
@@ -125,7 +125,7 @@ export class EncryptServiceImplementation implements EncryptService {
       }
     }
 
-    return await this.cryptoFunctionService.aesDecryptFast(fastParams, "cbc");
+    return await this.cryptoFunctionService.aesDecryptFast({ mode: "cbc", parameters: fastParams });
   }
 
   async decryptToBytes(

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
@@ -2,7 +2,7 @@ import { mock } from "jest-mock-extended";
 
 import { Utils } from "../../platform/misc/utils";
 import { PlatformUtilsService } from "../abstractions/platform-utils.service";
-import { DecryptParameters } from "../models/domain/decrypt-parameters";
+import { EcbDecryptParameters } from "../models/domain/decrypt-parameters";
 import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key";
 
 import { WebCryptoFunctionService } from "./web-crypto-function.service";
@@ -253,8 +253,13 @@ describe("WebCrypto Function Service", () => {
       const encData = Utils.fromBufferToB64(encValue);
       const b64Iv = Utils.fromBufferToB64(iv);
       const symKey = new SymmetricCryptoKey(key);
-      const params = cryptoFunctionService.aesDecryptFastParameters(encData, b64Iv, null, symKey);
-      const decValue = await cryptoFunctionService.aesDecryptFast(params, "cbc");
+      const parameters = cryptoFunctionService.aesDecryptFastParameters(
+        encData,
+        b64Iv,
+        null,
+        symKey,
+      );
+      const decValue = await cryptoFunctionService.aesDecryptFast({ mode: "cbc", parameters });
       expect(decValue).toBe(value);
     });
 
@@ -276,8 +281,8 @@ describe("WebCrypto Function Service", () => {
       const iv = Utils.fromBufferToB64(makeStaticByteArray(16));
       const symKey = new SymmetricCryptoKey(makeStaticByteArray(32));
       const data = "ByUF8vhyX4ddU9gcooznwA==";
-      const params = cryptoFunctionService.aesDecryptFastParameters(data, iv, null, symKey);
-      const decValue = await cryptoFunctionService.aesDecryptFast(params, "cbc");
+      const parameters = cryptoFunctionService.aesDecryptFastParameters(data, iv, null, symKey);
+      const decValue = await cryptoFunctionService.aesDecryptFast({ mode: "cbc", parameters });
       expect(decValue).toBe("EncryptMe!");
     });
   });
@@ -287,10 +292,11 @@ describe("WebCrypto Function Service", () => {
       const cryptoFunctionService = getWebCryptoFunctionService();
       const key = makeStaticByteArray(32);
       const data = Utils.fromB64ToArray("z5q2XSxYCdQFdI+qK2yLlw==");
-      const params = new DecryptParameters<string>();
-      params.encKey = Utils.fromBufferToByteString(key);
-      params.data = Utils.fromBufferToByteString(data);
-      const decValue = await cryptoFunctionService.aesDecryptFast(params, "ecb");
+      const parameters: EcbDecryptParameters<string> = {
+        encKey: Utils.fromBufferToByteString(key),
+        data: Utils.fromBufferToByteString(data),
+      };
+      const decValue = await cryptoFunctionService.aesDecryptFast({ mode: "ecb", parameters });
       expect(decValue).toBe("EncryptMe!");
     });
   });
@@ -304,6 +310,15 @@ describe("WebCrypto Function Service", () => {
       const decValue = await cryptoFunctionService.aesDecrypt(data, iv, key, "cbc");
       expect(Utils.fromBufferToUtf8(decValue)).toBe("EncryptMe!");
     });
+
+    it("throws if iv is not provided", async () => {
+      const cryptoFunctionService = getWebCryptoFunctionService();
+      const key = makeStaticByteArray(32);
+      const data = Utils.fromB64ToArray("ByUF8vhyX4ddU9gcooznwA==");
+      await expect(() => cryptoFunctionService.aesDecrypt(data, null, key, "cbc")).rejects.toThrow(
+        "IV is required for CBC mode",
+      );
+    });
   });
 
   describe("aesDecrypt ECB mode", () => {

diff --git a/libs/common/src/platform/services/web-crypto-function.service.ts b/libs/common/src/platform/services/web-crypto-function.service.ts
@@ -1,23 +1,25 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
 import * as argon2 from "argon2-browser";
 import * as forge from "node-forge";
 
 import { Utils } from "../../platform/misc/utils";
 import { CsprngArray } from "../../types/csprng";
 import { CryptoFunctionService } from "../abstractions/crypto-function.service";
-import { DecryptParameters } from "../models/domain/decrypt-parameters";
+import { CbcDecryptParameters, EcbDecryptParameters } from "../models/domain/decrypt-parameters";
 import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key";
 
 export class WebCryptoFunctionService implements CryptoFunctionService {
   private crypto: Crypto;
   private subtle: SubtleCrypto;
   private wasmSupported: boolean;
 
-  constructor(globalContext: Window | typeof global) {
-    this.crypto = typeof globalContext.crypto !== "undefined" ? globalContext.crypto : null;
-    this.subtle =
-      !!this.crypto && typeof this.crypto.subtle !== "undefined" ? this.crypto.subtle : null;
+  constructor(globalContext: { crypto: Crypto }) {
+    if (globalContext?.crypto?.subtle == null) {
+      throw new Error(
+        "Could not instantiate WebCryptoFunctionService. Could not locate Subtle crypto.",
+      );
+    }
+    this.crypto = globalContext.crypto;
+    this.subtle = this.crypto.subtle;
     this.wasmSupported = this.checkIfWasmSupported();
   }
 
@@ -220,7 +222,7 @@ export class WebCryptoFunctionService implements CryptoFunctionService {
     hmac.update(a);
     const mac1 = hmac.digest().getBytes();
 
-    hmac.start(null, null);
+    hmac.start("sha256", null);
     hmac.update(b);
     const mac2 = hmac.digest().getBytes();
 
@@ -239,10 +241,10 @@ export class WebCryptoFunctionService implements CryptoFunctionService {
   aesDecryptFastParameters(
     data: string,
     iv: string,
-    mac: string,
+    mac: string | null,
     key: SymmetricCryptoKey,
-  ): DecryptParameters<string> {
-    const p = new DecryptParameters<string>();
+  ): CbcDecryptParameters<string> {
+    const p = {} as CbcDecryptParameters<string>;
     if (key.meta != null) {
       p.encKey = key.meta.encKeyByteString;
       p.macKey = key.meta.macKeyByteString;
@@ -275,7 +277,12 @@ export class WebCryptoFunctionService implements CryptoFunctionService {
     return p;
   }
 
-  aesDecryptFast(parameters: DecryptParameters<string>, mode: "cbc" | "ecb"): Promise<string> {
+  aesDecryptFast({
+    mode,
+    parameters,
+  }:
+    | { mode: "cbc"; parameters: CbcDecryptParameters<string> }
+    | { mode: "ecb"; parameters: EcbDecryptParameters<string> }): Promise<string> {
     const decipher = (forge as any).cipher.createDecipher(
       this.toWebCryptoAesMode(mode),
       parameters.encKey,
@@ -294,21 +301,27 @@ export class WebCryptoFunctionService implements CryptoFunctionService {
 
   async aesDecrypt(
     data: Uint8Array,
-    iv: Uint8Array,
+    iv: Uint8Array | null,
     key: Uint8Array,
     mode: "cbc" | "ecb",
   ): Promise<Uint8Array> {
     if (mode === "ecb") {
       // Web crypto does not support AES-ECB mode, so we need to do this in forge.
-      const params = new DecryptParameters<string>();
-      params.data = this.toByteString(data);
-      params.encKey = this.toByteString(key);
-      const result = await this.aesDecryptFast(params, "ecb");
+      const parameters: EcbDecryptParameters<string> = {
+        data: this.toByteString(data),
+        encKey: this.toByteString(key),
+      };
+      const result = await this.aesDecryptFast({ mode: "ecb", parameters });
       return Utils.fromByteStringToArray(result);
     }
     const impKey = await this.subtle.importKey("raw", key, { name: "AES-CBC" } as any, false, [
       "decrypt",
     ]);
+
+    // CBC
+    if (iv == null) {
+      throw new Error("IV is required for CBC mode.");
+    }
     const buffer = await this.subtle.decrypt({ name: "AES-CBC", iv: iv }, impKey, data);
     return new Uint8Array(buffer);
   }

diff --git a/libs/node/src/services/node-crypto-function.service.spec.ts b/libs/node/src/services/node-crypto-function.service.spec.ts
@@ -1,5 +1,5 @@
 import { Utils } from "@bitwarden/common/platform/misc/utils";
-import { DecryptParameters } from "@bitwarden/common/platform/models/domain/decrypt-parameters";
+import { EcbDecryptParameters } from "@bitwarden/common/platform/models/domain/decrypt-parameters";
 import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
 
 import { NodeCryptoFunctionService } from "./node-crypto-function.service";
@@ -193,19 +193,20 @@ describe("NodeCrypto Function Service", () => {
       const iv = Utils.fromBufferToB64(makeStaticByteArray(16));
       const symKey = new SymmetricCryptoKey(makeStaticByteArray(32));
       const data = "ByUF8vhyX4ddU9gcooznwA==";
-      const params = nodeCryptoFunctionService.aesDecryptFastParameters(data, iv, null, symKey);
-      const decValue = await nodeCryptoFunctionService.aesDecryptFast(params, "cbc");
+      const parameters = nodeCryptoFunctionService.aesDecryptFastParameters(data, iv, null, symKey);
+      const decValue = await nodeCryptoFunctionService.aesDecryptFast({ mode: "cbc", parameters });
       expect(decValue).toBe("EncryptMe!");
     });
   });
 
   describe("aesDecryptFast ECB mode", () => {
     it("should successfully decrypt data", async () => {
       const nodeCryptoFunctionService = new NodeCryptoFunctionService();
-      const params = new DecryptParameters<Uint8Array>();
-      params.encKey = makeStaticByteArray(32);
-      params.data = Utils.fromB64ToArray("z5q2XSxYCdQFdI+qK2yLlw==");
-      const decValue = await nodeCryptoFunctionService.aesDecryptFast(params, "ecb");
+      const parameters: EcbDecryptParameters<Uint8Array> = {
+        encKey: makeStaticByteArray(32),
+        data: Utils.fromB64ToArray("z5q2XSxYCdQFdI+qK2yLlw=="),
+      };
+      const decValue = await nodeCryptoFunctionService.aesDecryptFast({ mode: "ecb", parameters });
       expect(decValue).toBe("EncryptMe!");
     });
   });
@@ -219,6 +220,15 @@ describe("NodeCrypto Function Service", () => {
       const decValue = await nodeCryptoFunctionService.aesDecrypt(data, iv, key, "cbc");
       expect(Utils.fromBufferToUtf8(decValue)).toBe("EncryptMe!");
     });
+
+    it("throws if IV is not provided", async () => {
+      const nodeCryptoFunctionService = new NodeCryptoFunctionService();
+      const key = makeStaticByteArray(32);
+      const data = Utils.fromB64ToArray("ByUF8vhyX4ddU9gcooznwA==");
+      await expect(
+        async () => await nodeCryptoFunctionService.aesDecrypt(data, null, key, "cbc"),
+      ).rejects.toThrow("Invalid initialization vector");
+    });
   });
 
   describe("aesDecrypt ECB mode", () => {
@@ -454,7 +464,7 @@ function testHmac(algorithm: "sha1" | "sha256" | "sha512", mac: string, fast = f
     const cryptoFunctionService = new NodeCryptoFunctionService();
     const value = Utils.fromUtf8ToArray("SignMe!!");
     const key = Utils.fromUtf8ToArray("secretkey");
-    let computedMac: ArrayBuffer = null;
+    let computedMac: ArrayBuffer;
     if (fast) {
       computedMac = await cryptoFunctionService.hmacFast(value, key, algorithm);
     } else {

diff --git a/libs/node/src/services/node-crypto-function.service.ts b/libs/node/src/services/node-crypto-function.service.ts
@@ -1,12 +1,13 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
 import * as crypto from "crypto";
 
 import * as forge from "node-forge";
 
 import { CryptoFunctionService } from "@bitwarden/common/platform/abstractions/crypto-function.service";
 import { Utils } from "@bitwarden/common/platform/misc/utils";
-import { DecryptParameters } from "@bitwarden/common/platform/models/domain/decrypt-parameters";
+import {
+  CbcDecryptParameters,
+  EcbDecryptParameters,
+} from "@bitwarden/common/platform/models/domain/decrypt-parameters";
 import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
 import { CsprngArray } from "@bitwarden/common/types/csprng";
 
@@ -168,10 +169,10 @@ export class NodeCryptoFunctionService implements CryptoFunctionService {
   aesDecryptFastParameters(
     data: string,
     iv: string,
-    mac: string,
+    mac: string | null,
     key: SymmetricCryptoKey,
-  ): DecryptParameters<Uint8Array> {
-    const p = new DecryptParameters<Uint8Array>();
+  ): CbcDecryptParameters<Uint8Array> {
+    const p = {} as CbcDecryptParameters<Uint8Array>;
     p.encKey = key.encKey;
     p.data = Utils.fromB64ToArray(data);
     p.iv = Utils.fromB64ToArray(iv);
@@ -191,22 +192,25 @@ export class NodeCryptoFunctionService implements CryptoFunctionService {
     return p;
   }
 
-  async aesDecryptFast(
-    parameters: DecryptParameters<Uint8Array>,
-    mode: "cbc" | "ecb",
-  ): Promise<string> {
-    const decBuf = await this.aesDecrypt(parameters.data, parameters.iv, parameters.encKey, mode);
+  async aesDecryptFast({
+    mode,
+    parameters,
+  }:
+    | { mode: "cbc"; parameters: CbcDecryptParameters<Uint8Array> }
+    | { mode: "ecb"; parameters: EcbDecryptParameters<Uint8Array> }): Promise<string> {
+    const iv = mode === "cbc" ? parameters.iv : null;
+    const decBuf = await this.aesDecrypt(parameters.data, iv, parameters.encKey, mode);
     return Utils.fromBufferToUtf8(decBuf);
   }
 
   aesDecrypt(
     data: Uint8Array,
-    iv: Uint8Array,
+    iv: Uint8Array | null,
     key: Uint8Array,
     mode: "cbc" | "ecb",
   ): Promise<Uint8Array> {
     const nodeData = this.toNodeBuffer(data);
-    const nodeIv = mode === "ecb" ? null : this.toNodeBuffer(iv);
+    const nodeIv = this.toNodeBufferOrNull(iv);
     const nodeKey = this.toNodeBuffer(key);
     const decipher = crypto.createDecipheriv(this.toNodeCryptoAesMode(mode), nodeKey, nodeIv);
     const decBuf = Buffer.concat([decipher.update(nodeData), decipher.final()]);
@@ -311,6 +315,13 @@ export class NodeCryptoFunctionService implements CryptoFunctionService {
     return Buffer.from(value);
   }
 
+  private toNodeBufferOrNull(value: Uint8Array | null): Buffer | null {
+    if (value == null) {
+      return null;
+    }
+    return this.toNodeBuffer(value);
+  }
+
   private toUint8Buffer(value: Buffer | string | Uint8Array): Uint8Array {
     let buf: Uint8Array;
     if (typeof value === "string") {
-Original file line number
+Diff line change
@@ Expand Up @@
           }
         }
-        return await this.cryptoFunctionService.aesDecryptFast(fastParams, "cbc");
+        return await this.cryptoFunctionService.aesDecryptFast({ mode: "cbc", parameters: fastParams });
       }
       async decryptToBytes(
@@ Expand Down @@