[PM-18697] Remove old symmetric key representations in symmetriccryptokey (#13598)

* Remove AES128CBC-HMAC encryption * Increase test coverage * Refactor symmetric keys and increase test coverage * Re-add type 0 encryption * Fix ts strict warning * Remove old symmetric key representations in symmetriccryptokey * Fix desktop build * Fix test * Fix build * Update libs/common/src/key-management/crypto/services/web-crypto-function.service.ts Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Update libs/node/src/services/node-crypto-function.service.ts Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Undo changes * Remove cast * Undo changes to tests * Fix linting * Undo removing new Uint8Array in aesDecryptFastParameters * Fix merge conflicts * Fix test * Fix another test * Fix test --------- Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
2025-12-19 01:33:33 +00:00 · 2025-04-21 16:57:26 +02:00
parent a250395e6d
commit 43b1f55360
12 changed files with 111 additions and 106 deletions
--- a/libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts
+++ b/libs/common/src/key-management/crypto/services/encrypt.service.implementation.ts
@@ -47,7 +47,7 @@ export class EncryptServiceImplementation implements EncryptService {
    }

    if (this.blockType0) {
-      if (key.encType === EncryptionType.AesCbc256_B64 || key.key.byteLength < 64) {
+      if (key.inner().type === EncryptionType.AesCbc256_B64 || key.key.byteLength < 64) {
        throw new Error("Type 0 encryption is not supported.");
      }
    }
@@ -84,7 +84,7 @@ export class EncryptServiceImplementation implements EncryptService {
    }

    if (this.blockType0) {
-      if (key.encType === EncryptionType.AesCbc256_B64 || key.key.byteLength < 64) {
+      if (key.inner().type === EncryptionType.AesCbc256_B64 || key.key.byteLength < 64) {
        throw new Error("Type 0 encryption is not supported.");
      }
    }
@@ -124,7 +124,7 @@ export class EncryptServiceImplementation implements EncryptService {
    if (encString.encryptionType !== innerKey.type) {
      this.logDecryptError(
        "Key encryption type does not match payload encryption type",
-        key.encType,
+        innerKey.type,
        encString.encryptionType,
        decryptContext,
      );
@@ -148,7 +148,7 @@ export class EncryptServiceImplementation implements EncryptService {
      if (!macsEqual) {
        this.logMacFailed(
          "decryptToUtf8 MAC comparison failed. Key or payload has changed.",
-          key.encType,
+          innerKey.type,
          encString.encryptionType,
          decryptContext,
        );
@@ -191,7 +191,7 @@ export class EncryptServiceImplementation implements EncryptService {
    if (encThing.encryptionType !== inner.type) {
      this.logDecryptError(
        "Encryption key type mismatch",
-        key.encType,
+        inner.type,
        encThing.encryptionType,
        decryptContext,
      );
@@ -200,19 +200,23 @@ export class EncryptServiceImplementation implements EncryptService {

    if (inner.type === EncryptionType.AesCbc256_HmacSha256_B64) {
      if (encThing.macBytes == null) {
-        this.logDecryptError("Mac missing", key.encType, encThing.encryptionType, decryptContext);
+        this.logDecryptError("Mac missing", inner.type, encThing.encryptionType, decryptContext);
        return null;
      }

      const macData = new Uint8Array(encThing.ivBytes.byteLength + encThing.dataBytes.byteLength);
      macData.set(new Uint8Array(encThing.ivBytes), 0);
      macData.set(new Uint8Array(encThing.dataBytes), encThing.ivBytes.byteLength);
-      const computedMac = await this.cryptoFunctionService.hmac(macData, key.macKey, "sha256");
+      const computedMac = await this.cryptoFunctionService.hmac(
+        macData,
+        inner.authenticationKey,
+        "sha256",
+      );
      const macsMatch = await this.cryptoFunctionService.compare(encThing.macBytes, computedMac);
      if (!macsMatch) {
        this.logMacFailed(
          "MAC comparison failed. Key or payload has changed.",
-          key.encType,
+          inner.type,
          encThing.encryptionType,
          decryptContext,
        );
@@ -222,14 +226,14 @@ export class EncryptServiceImplementation implements EncryptService {
      return await this.cryptoFunctionService.aesDecrypt(
        encThing.dataBytes,
        encThing.ivBytes,
-        key.encKey,
+        inner.encryptionKey,
        "cbc",
      );
    } else if (inner.type === EncryptionType.AesCbc256_B64) {
      return await this.cryptoFunctionService.aesDecrypt(
        encThing.dataBytes,
        encThing.ivBytes,
-        key.encKey,
+        inner.encryptionKey,
        "cbc",
      );
    }
--- a/libs/common/src/key-management/crypto/services/encrypt.service.spec.ts
+++ b/libs/common/src/key-management/crypto/services/encrypt.service.spec.ts
@@ -6,7 +6,10 @@ import { EncryptionType } from "@bitwarden/common/platform/enums";
 import { Utils } from "@bitwarden/common/platform/misc/utils";
 import { EncArrayBuffer } from "@bitwarden/common/platform/models/domain/enc-array-buffer";
 import { EncString } from "@bitwarden/common/platform/models/domain/enc-string";
-import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
+import {
+  Aes256CbcHmacKey,
+  SymmetricCryptoKey,
+} from "@bitwarden/common/platform/models/domain/symmetric-crypto-key";
 import { CsprngArray } from "@bitwarden/common/types/csprng";

 import { makeStaticByteArray } from "../../../../spec";
@@ -64,6 +67,10 @@ describe("EncryptService", () => {
      const key = new SymmetricCryptoKey(makeStaticByteArray(32));
      const mock32Key = mock<SymmetricCryptoKey>();
      mock32Key.key = makeStaticByteArray(32);
+      mock32Key.inner.mockReturnValue({
+        type: 0,
+        encryptionKey: mock32Key.key,
+      });

      await expect(encryptService.encrypt(null!, key)).rejects.toThrow(
        "Type 0 encryption is not supported.",
@@ -146,6 +153,10 @@ describe("EncryptService", () => {
      const key = new SymmetricCryptoKey(makeStaticByteArray(32));
      const mock32Key = mock<SymmetricCryptoKey>();
      mock32Key.key = makeStaticByteArray(32);
+      mock32Key.inner.mockReturnValue({
+        type: 0,
+        encryptionKey: mock32Key.key,
+      });

      await expect(encryptService.encryptToBytes(plainValue, key)).rejects.toThrow(
        "Type 0 encryption is not supported.",
@@ -228,7 +239,7 @@ describe("EncryptService", () => {
      expect(cryptoFunctionService.aesDecrypt).toBeCalledWith(
        expect.toEqualBuffer(encBuffer.dataBytes),
        expect.toEqualBuffer(encBuffer.ivBytes),
-        expect.toEqualBuffer(key.encKey),
+        expect.toEqualBuffer(key.inner().encryptionKey),
        "cbc",
      );

@@ -249,7 +260,7 @@ describe("EncryptService", () => {
      expect(cryptoFunctionService.aesDecrypt).toBeCalledWith(
        expect.toEqualBuffer(encBuffer.dataBytes),
        expect.toEqualBuffer(encBuffer.ivBytes),
-        expect.toEqualBuffer(key.encKey),
+        expect.toEqualBuffer(key.inner().encryptionKey),
        "cbc",
      );

@@ -267,7 +278,7 @@ describe("EncryptService", () => {

      expect(cryptoFunctionService.hmac).toBeCalledWith(
        expect.toEqualBuffer(expectedMacData),
-        key.macKey,
+        (key.inner() as Aes256CbcHmacKey).authenticationKey,
        "sha256",
      );

--- a/libs/common/src/key-management/crypto/services/web-crypto-function.service.ts
+++ b/libs/common/src/key-management/crypto/services/web-crypto-function.service.ts
@@ -1,6 +1,7 @@
 import * as argon2 from "argon2-browser";
 import * as forge from "node-forge";

+import { EncryptionType } from "../../../platform/enums";
 import { Utils } from "../../../platform/misc/utils";
 import {
  CbcDecryptParameters,
@@ -247,37 +248,26 @@ export class WebCryptoFunctionService implements CryptoFunctionService {
    mac: string | null,
    key: SymmetricCryptoKey,
  ): CbcDecryptParameters<string> {
-    const p = {} as CbcDecryptParameters<string>;
-    if (key.meta != null) {
-      p.encKey = key.meta.encKeyByteString;
-      p.macKey = key.meta.macKeyByteString;
+    const innerKey = key.inner();
+    if (innerKey.type === EncryptionType.AesCbc256_B64) {
+      return {
+        iv: forge.util.decode64(iv),
+        data: forge.util.decode64(data),
+        encKey: forge.util.createBuffer(innerKey.encryptionKey).getBytes(),
+      } as CbcDecryptParameters<string>;
+    } else if (innerKey.type === EncryptionType.AesCbc256_HmacSha256_B64) {
+      const macData = forge.util.decode64(iv) + forge.util.decode64(data);
+      return {
+        iv: forge.util.decode64(iv),
+        data: forge.util.decode64(data),
+        encKey: forge.util.createBuffer(innerKey.encryptionKey).getBytes(),
+        macKey: forge.util.createBuffer(innerKey.authenticationKey).getBytes(),
+        mac: forge.util.decode64(mac!),
+        macData,
+      } as CbcDecryptParameters<string>;
+    } else {
+      throw new Error("Unsupported encryption type.");
    }
-
-    if (p.encKey == null) {
-      p.encKey = forge.util.decode64(key.encKeyB64);
-    }
-    p.data = forge.util.decode64(data);
-    p.iv = forge.util.decode64(iv);
-    p.macData = p.iv + p.data;
-    if (p.macKey == null && key.macKeyB64 != null) {
-      p.macKey = forge.util.decode64(key.macKeyB64);
-    }
-    if (mac != null) {
-      p.mac = forge.util.decode64(mac);
-    }
-
-    // cache byte string keys for later
-    if (key.meta == null) {
-      key.meta = {};
-    }
-    if (key.meta.encKeyByteString == null) {
-      key.meta.encKeyByteString = p.encKey;
-    }
-    if (p.macKey != null && key.meta.macKeyByteString == null) {
-      key.meta.macKeyByteString = p.macKey;
-    }
-
-    return p;
  }

  aesDecryptFast({
--- a/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts
+++ b/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts
@@ -252,7 +252,9 @@ describe("KeyConnectorService", () => {
      const organization = organizationData(true, true, "https://key-connector-url.com", 2, false);
      const masterKey = getMockMasterKey();
      masterPasswordService.masterKeySubject.next(masterKey);
-      const keyConnectorRequest = new KeyConnectorUserKeyRequest(masterKey.encKeyB64);
+      const keyConnectorRequest = new KeyConnectorUserKeyRequest(
+        Utils.fromBufferToB64(masterKey.inner().encryptionKey),
+      );

      jest.spyOn(keyConnectorService, "getManagingOrganization").mockResolvedValue(organization);
      jest.spyOn(apiService, "postUserKeyToKeyConnector").mockResolvedValue();
@@ -273,7 +275,9 @@ describe("KeyConnectorService", () => {
      // Arrange
      const organization = organizationData(true, true, "https://key-connector-url.com", 2, false);
      const masterKey = getMockMasterKey();
-      const keyConnectorRequest = new KeyConnectorUserKeyRequest(masterKey.encKeyB64);
+      const keyConnectorRequest = new KeyConnectorUserKeyRequest(
+        Utils.fromBufferToB64(masterKey.inner().encryptionKey),
+      );
      const error = new Error("Failed to post user key to key connector");
      organizationService.organizations$.mockReturnValue(of([organization]));

--- a/libs/common/src/key-management/key-connector/services/key-connector.service.ts
+++ b/libs/common/src/key-management/key-connector/services/key-connector.service.ts
@@ -95,7 +95,9 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction {
    userId ??= (await firstValueFrom(this.accountService.activeAccount$))?.id;
    const organization = await this.getManagingOrganization(userId);
    const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
-    const keyConnectorRequest = new KeyConnectorUserKeyRequest(masterKey.encKeyB64);
+    const keyConnectorRequest = new KeyConnectorUserKeyRequest(
+      Utils.fromBufferToB64(masterKey.inner().encryptionKey),
+    );

    try {
      await this.apiService.postUserKeyToKeyConnector(
@@ -157,7 +159,9 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction {
      await this.tokenService.getEmail(),
      kdfConfig,
    );
-    const keyConnectorRequest = new KeyConnectorUserKeyRequest(masterKey.encKeyB64);
+    const keyConnectorRequest = new KeyConnectorUserKeyRequest(
+      Utils.fromBufferToB64(masterKey.inner().encryptionKey),
+    );
    await this.masterPasswordService.setMasterKey(masterKey, userId);

    const userKey = await this.keyService.makeUserKey(masterKey);
--- a/libs/common/src/platform/models/domain/symmetric-crypto-key.spec.ts
+++ b/libs/common/src/platform/models/domain/symmetric-crypto-key.spec.ts
@@ -2,7 +2,7 @@ import { makeStaticByteArray } from "../../../../spec";
 import { EncryptionType } from "../../enums";
 import { Utils } from "../../misc/utils";

-import { SymmetricCryptoKey } from "./symmetric-crypto-key";
+import { Aes256CbcHmacKey, SymmetricCryptoKey } from "./symmetric-crypto-key";

 describe("SymmetricCryptoKey", () => {
  it("errors if no key", () => {
@@ -19,13 +19,8 @@ describe("SymmetricCryptoKey", () => {
      const cryptoKey = new SymmetricCryptoKey(key);

      expect(cryptoKey).toEqual({
-        encKey: key,
-        encKeyB64: "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=",
-        encType: EncryptionType.AesCbc256_B64,
        key: key,
        keyB64: "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=",
-        macKey: null,
-        macKeyB64: undefined,
        innerKey: {
          type: EncryptionType.AesCbc256_B64,
          encryptionKey: key,
@@ -38,14 +33,9 @@ describe("SymmetricCryptoKey", () => {
      const cryptoKey = new SymmetricCryptoKey(key);

      expect(cryptoKey).toEqual({
-        encKey: key.slice(0, 32),
-        encKeyB64: "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=",
-        encType: EncryptionType.AesCbc256_HmacSha256_B64,
        key: key,
        keyB64:
          "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8gISIjJCUmJygpKissLS4vMDEyMzQ1Njc4OTo7PD0+Pw==",
-        macKey: key.slice(32, 64),
-        macKeyB64: "ICEiIyQlJicoKSorLC0uLzAxMjM0NTY3ODk6Ozw9Pj8=",
        innerKey: {
          type: EncryptionType.AesCbc256_HmacSha256_B64,
          encryptionKey: key.slice(0, 32),
@@ -86,8 +76,8 @@ describe("SymmetricCryptoKey", () => {

    expect(actual).toEqual({
      type: EncryptionType.AesCbc256_HmacSha256_B64,
-      encryptionKey: key.encKey,
-      authenticationKey: key.macKey,
+      encryptionKey: key.inner().encryptionKey,
+      authenticationKey: (key.inner() as Aes256CbcHmacKey).authenticationKey,
    });
  });

@@ -95,7 +85,7 @@ describe("SymmetricCryptoKey", () => {
    const key = new SymmetricCryptoKey(makeStaticByteArray(32));
    const actual = key.toEncoded();

-    expect(actual).toEqual(key.encKey);
+    expect(actual).toEqual(key.inner().encryptionKey);
  });

  it("toEncoded returns encoded key for AesCbc256_HmacSha256_B64", () => {
--- a/libs/common/src/platform/models/domain/symmetric-crypto-key.ts
+++ b/libs/common/src/platform/models/domain/symmetric-crypto-key.ts
@@ -25,15 +25,7 @@ export class SymmetricCryptoKey {
  private innerKey: Aes256CbcHmacKey | Aes256CbcKey;

  key: Uint8Array;
-  encKey: Uint8Array;
-  macKey?: Uint8Array;
-  encType: EncryptionType;
-
  keyB64: string;
-  encKeyB64: string;
-  macKeyB64: string;
-
-  meta: any;

  /**
   * @param key The key in one of the permitted serialization formats
@@ -48,30 +40,16 @@ export class SymmetricCryptoKey {
        type: EncryptionType.AesCbc256_B64,
        encryptionKey: key,
      };
-      this.encType = EncryptionType.AesCbc256_B64;
      this.key = key;
-      this.keyB64 = Utils.fromBufferToB64(this.key);
-
-      this.encKey = key;
-      this.encKeyB64 = Utils.fromBufferToB64(this.encKey);
-
-      this.macKey = null;
-      this.macKeyB64 = undefined;
+      this.keyB64 = this.toBase64();
    } else if (key.byteLength === 64) {
      this.innerKey = {
        type: EncryptionType.AesCbc256_HmacSha256_B64,
        encryptionKey: key.slice(0, 32),
        authenticationKey: key.slice(32),
      };
-      this.encType = EncryptionType.AesCbc256_HmacSha256_B64;
      this.key = key;
-      this.keyB64 = Utils.fromBufferToB64(this.key);
-
-      this.encKey = key.slice(0, 32);
-      this.encKeyB64 = Utils.fromBufferToB64(this.encKey);
-
-      this.macKey = key.slice(32);
-      this.macKeyB64 = Utils.fromBufferToB64(this.macKey);
+      this.keyB64 = this.toBase64();
    } else {
      throw new Error(`Unsupported encType/key length ${key.byteLength}`);
    }