[PM-16699] Add decrypt trace for decrypt failures (#12749)

* Improve decrypt failure logging * Rename decryptcontext to decrypttrace * Improve docs * Revert changes to decrypt logic * Revert keyservice decryption logic change * Undo one more change to decrypt logic
2025-12-12 22:33:35 +00:00 · 2025-01-09 20:23:55 +01:00
parent bb8e649048
commit 8cabb36c99
18 changed files with 165 additions and 43 deletions
--- a/libs/common/src/auth/services/master-password/master-password.service.ts
+++ b/libs/common/src/auth/services/master-password/master-password.service.ts
@@ -180,10 +180,18 @@ export class MasterPasswordService implements InternalMasterPasswordServiceAbstr
    let decUserKey: Uint8Array;

    if (userKey.encryptionType === EncryptionType.AesCbc256_B64) {
-      decUserKey = await this.encryptService.decryptToBytes(userKey, masterKey);
+      decUserKey = await this.encryptService.decryptToBytes(
+        userKey,
+        masterKey,
+        "Content: User Key; Encrypting Key: Master Key",
+      );
    } else if (userKey.encryptionType === EncryptionType.AesCbc256_HmacSha256_B64) {
      const newKey = await this.keyGenerationService.stretchKey(masterKey);
-      decUserKey = await this.encryptService.decryptToBytes(userKey, newKey);
+      decUserKey = await this.encryptService.decryptToBytes(
+        userKey,
+        newKey,
+        "Content: User Key; Encrypting Key: Stretched Master Key",
+      );
    } else {
      throw new Error("Unsupported encryption type.");
    }
--- a/libs/common/src/platform/abstractions/encrypt.service.ts
+++ b/libs/common/src/platform/abstractions/encrypt.service.ts
@@ -8,12 +8,32 @@ import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key";
 export abstract class EncryptService {
  abstract encrypt(plainValue: string | Uint8Array, key: SymmetricCryptoKey): Promise<EncString>;
  abstract encryptToBytes(plainValue: Uint8Array, key: SymmetricCryptoKey): Promise<EncArrayBuffer>;
+  /**
+   * Decrypts an EncString to a string
+   * @param encString - The EncString to decrypt
+   * @param key - The key to decrypt the EncString with
+   * @param decryptTrace - A string to identify the context of the object being decrypted. This can include: field name, encryption type, cipher id, key type, but should not include
+   * sensitive information like encryption keys or data. This is used for logging when decryption errors occur in order to identify what failed to decrypt
+   * @returns The decrypted string
+   */
  abstract decryptToUtf8(
    encString: EncString,
    key: SymmetricCryptoKey,
-    decryptContext?: string,
+    decryptTrace?: string,
  ): Promise<string>;
-  abstract decryptToBytes(encThing: Encrypted, key: SymmetricCryptoKey): Promise<Uint8Array>;
+  /**
+   * Decrypts an Encrypted object to a Uint8Array
+   * @param encThing - The Encrypted object to decrypt
+   * @param key - The key to decrypt the Encrypted object with
+   * @param decryptTrace - A string to identify the context of the object being decrypted. This can include: field name, encryption type, cipher id, key type, but should not include
+   * sensitive information like encryption keys or data. This is used for logging when decryption errors occur in order to identify what failed to decrypt
+   * @returns The decrypted Uint8Array
+   */
+  abstract decryptToBytes(
+    encThing: Encrypted,
+    key: SymmetricCryptoKey,
+    decryptTrace?: string,
+  ): Promise<Uint8Array>;
  abstract rsaEncrypt(data: Uint8Array, publicKey: Uint8Array): Promise<EncString>;
  abstract rsaDecrypt(data: EncString, privateKey: Uint8Array): Promise<Uint8Array>;
  abstract resolveLegacyKey(key: SymmetricCryptoKey, encThing: Encrypted): SymmetricCryptoKey;
--- a/libs/common/src/platform/models/domain/domain-base.ts
+++ b/libs/common/src/platform/models/domain/domain-base.ts
@@ -63,6 +63,7 @@ export default class Domain {
    map: any,
    orgId: string,
    key: SymmetricCryptoKey = null,
+    objectContext: string = "No Domain Context",
  ): Promise<T> {
    const promises = [];
    const self: any = this;
@@ -78,7 +79,11 @@ export default class Domain {
          .then(() => {
            const mapProp = map[theProp] || theProp;
            if (self[mapProp]) {
-              return self[mapProp].decrypt(orgId, key);
+              return self[mapProp].decrypt(
+                orgId,
+                key,
+                `Property: ${prop}; ObjectContext: ${objectContext}`,
+              );
            }
            return null;
          })
@@ -114,12 +119,21 @@ export default class Domain {
    key: SymmetricCryptoKey,
    encryptService: EncryptService,
    _: Constructor<TThis> = this.constructor as Constructor<TThis>,
+    objectContext: string = "No Domain Context",
  ): Promise<DecryptedObject<TThis, TEncryptedKeys>> {
    const promises = [];

    for (const prop of encryptedProperties) {
      const value = (this as any)[prop] as EncString;
-      promises.push(this.decryptProperty(prop, value, key, encryptService));
+      promises.push(
+        this.decryptProperty(
+          prop,
+          value,
+          key,
+          encryptService,
+          `Property: ${prop.toString()}; ObjectContext: ${objectContext}`,
+        ),
+      );
    }

    const decryptedObjects = await Promise.all(promises);
@@ -137,10 +151,11 @@ export default class Domain {
    value: EncString,
    key: SymmetricCryptoKey,
    encryptService: EncryptService,
+    decryptTrace: string,
  ) {
    let decrypted: string = null;
    if (value) {
-      decrypted = await value.decryptWithKey(key, encryptService);
+      decrypted = await value.decryptWithKey(key, encryptService, decryptTrace);
    } else {
      decrypted = null;
    }
--- a/libs/common/src/platform/models/domain/enc-string.ts
+++ b/libs/common/src/platform/models/domain/enc-string.ts
@@ -156,21 +156,21 @@ export class EncString implements Encrypted {
    return EXPECTED_NUM_PARTS_BY_ENCRYPTION_TYPE[encType] === encPieces.length;
  }

-  async decrypt(orgId: string, key: SymmetricCryptoKey = null): Promise<string> {
+  async decrypt(orgId: string, key: SymmetricCryptoKey = null, context?: string): Promise<string> {
    if (this.decryptedValue != null) {
      return this.decryptedValue;
    }

-    let keyContext = "provided-key";
+    let decryptTrace = "provided-key";
    try {
      if (key == null) {
        key = await this.getKeyForDecryption(orgId);
-        keyContext = orgId == null ? `domain-orgkey-${orgId}` : "domain-userkey|masterkey";
+        decryptTrace = orgId == null ? `domain-orgkey-${orgId}` : "domain-userkey|masterkey";
        if (orgId != null) {
-          keyContext = `domain-orgkey-${orgId}`;
+          decryptTrace = `domain-orgkey-${orgId}`;
        } else {
          const cryptoService = Utils.getContainerService().getKeyService();
-          keyContext =
+          decryptTrace =
            (await cryptoService.getUserKey()) == null
              ? "domain-withlegacysupport-masterkey"
              : "domain-withlegacysupport-userkey";
@@ -181,20 +181,28 @@ export class EncString implements Encrypted {
      }

      const encryptService = Utils.getContainerService().getEncryptService();
-      this.decryptedValue = await encryptService.decryptToUtf8(this, key, keyContext);
+      this.decryptedValue = await encryptService.decryptToUtf8(
+        this,
+        key,
+        decryptTrace == null ? context : `${decryptTrace}${context || ""}`,
+      );
    } catch (e) {
      this.decryptedValue = DECRYPT_ERROR;
    }
    return this.decryptedValue;
  }

-  async decryptWithKey(key: SymmetricCryptoKey, encryptService: EncryptService) {
+  async decryptWithKey(
+    key: SymmetricCryptoKey,
+    encryptService: EncryptService,
+    decryptTrace: string = "domain-withkey",
+  ): Promise<string> {
    try {
      if (key == null) {
        throw new Error("No key to decrypt EncString");
      }

-      this.decryptedValue = await encryptService.decryptToUtf8(this, key, "domain-withkey");
+      this.decryptedValue = await encryptService.decryptToUtf8(this, key, decryptTrace);
    } catch (e) {
      this.decryptedValue = DECRYPT_ERROR;
    }
--- a/libs/common/src/platform/services/cryptography/encrypt.service.implementation.ts
+++ b/libs/common/src/platform/services/cryptography/encrypt.service.implementation.ts
@@ -114,7 +114,7 @@ export class EncryptServiceImplementation implements EncryptService {
      const macsEqual = await this.cryptoFunctionService.compareFast(fastParams.mac, computedMac);
      if (!macsEqual) {
        this.logMacFailed(
-          "[Encrypt service] MAC comparison failed. Key or payload has changed. Key type " +
+          "[Encrypt service] decryptToUtf8 MAC comparison failed. Key or payload has changed. Key type " +
            encryptionTypeName(key.encType) +
            "Payload type " +
            encryptionTypeName(encString.encryptionType) +
@@ -128,7 +128,11 @@ export class EncryptServiceImplementation implements EncryptService {
    return await this.cryptoFunctionService.aesDecryptFast(fastParams, "cbc");
  }

-  async decryptToBytes(encThing: Encrypted, key: SymmetricCryptoKey): Promise<Uint8Array> {
+  async decryptToBytes(
+    encThing: Encrypted,
+    key: SymmetricCryptoKey,
+    decryptContext: string = "no context",
+  ): Promise<Uint8Array> {
    if (key == null) {
      throw new Error("No encryption key provided.");
    }
@@ -145,7 +149,9 @@ export class EncryptServiceImplementation implements EncryptService {
        "[Encrypt service] Key has mac key but payload is missing mac bytes. Key type " +
          encryptionTypeName(key.encType) +
          " Payload type " +
-          encryptionTypeName(encThing.encryptionType),
+          encryptionTypeName(encThing.encryptionType) +
+          " Decrypt context: " +
+          decryptContext,
      );
      return null;
    }
@@ -155,7 +161,9 @@ export class EncryptServiceImplementation implements EncryptService {
        "[Encrypt service] Key encryption type does not match payload encryption type. Key type " +
          encryptionTypeName(key.encType) +
          " Payload type " +
-          encryptionTypeName(encThing.encryptionType),
+          encryptionTypeName(encThing.encryptionType) +
+          " Decrypt context: " +
+          decryptContext,
      );
      return null;
    }
@@ -167,11 +175,13 @@ export class EncryptServiceImplementation implements EncryptService {
      const computedMac = await this.cryptoFunctionService.hmac(macData, key.macKey, "sha256");
      if (computedMac === null) {
        this.logMacFailed(
-          "[Encrypt service] Failed to compute MAC." +
+          "[Encrypt service#decryptToBytes] Failed to compute MAC." +
            " Key type " +
            encryptionTypeName(key.encType) +
            " Payload type " +
-            encryptionTypeName(encThing.encryptionType),
+            encryptionTypeName(encThing.encryptionType) +
+            " Decrypt context: " +
+            decryptContext,
        );
        return null;
      }
@@ -179,11 +189,13 @@ export class EncryptServiceImplementation implements EncryptService {
      const macsMatch = await this.cryptoFunctionService.compare(encThing.macBytes, computedMac);
      if (!macsMatch) {
        this.logMacFailed(
-          "[Encrypt service] MAC comparison failed. Key or payload has changed." +
+          "[Encrypt service#decryptToBytes]: MAC comparison failed. Key or payload has changed." +
            " Key type " +
            encryptionTypeName(key.encType) +
            " Payload type " +
-            encryptionTypeName(encThing.encryptionType),
+            encryptionTypeName(encThing.encryptionType) +
+            " Decrypt context: " +
+            decryptContext,
        );
        return null;
      }
--- a/libs/common/src/tools/send/models/domain/send.spec.ts
+++ b/libs/common/src/tools/send/models/domain/send.spec.ts
@@ -123,7 +123,12 @@ describe("Send", () => {
    const view = await send.decrypt();

    expect(text.decrypt).toHaveBeenNthCalledWith(1, "cryptoKey");
-    expect(send.name.decrypt).toHaveBeenNthCalledWith(1, null, "cryptoKey");
+    expect(send.name.decrypt).toHaveBeenNthCalledWith(
+      1,
+      null,
+      "cryptoKey",
+      "Property: name; ObjectContext: No Domain Context",
+    );

    expect(view).toMatchObject({
      id: "id",
--- a/libs/common/src/vault/models/domain/attachment.spec.ts
+++ b/libs/common/src/vault/models/domain/attachment.spec.ts
@@ -101,7 +101,7 @@ describe("Attachment", () => {
      it("uses the provided key without depending on KeyService", async () => {
        const providedKey = mock<SymmetricCryptoKey>();

-        await attachment.decrypt(null, providedKey);
+        await attachment.decrypt(null, "", providedKey);

        expect(keyService.getUserKeyWithLegacySupport).not.toHaveBeenCalled();
        expect(encryptService.decryptToBytes).toHaveBeenCalledWith(attachment.key, providedKey);
@@ -111,7 +111,7 @@ describe("Attachment", () => {
        const orgKey = mock<OrgKey>();
        keyService.getOrgKey.calledWith("orgId").mockResolvedValue(orgKey);

-        await attachment.decrypt("orgId", null);
+        await attachment.decrypt("orgId", "", null);

        expect(keyService.getOrgKey).toHaveBeenCalledWith("orgId");
        expect(encryptService.decryptToBytes).toHaveBeenCalledWith(attachment.key, orgKey);
@@ -121,7 +121,7 @@ describe("Attachment", () => {
        const userKey = mock<UserKey>();
        keyService.getUserKeyWithLegacySupport.mockResolvedValue(userKey);

-        await attachment.decrypt(null, null);
+        await attachment.decrypt(null, "", null);

        expect(keyService.getUserKeyWithLegacySupport).toHaveBeenCalled();
        expect(encryptService.decryptToBytes).toHaveBeenCalledWith(attachment.key, userKey);
--- a/libs/common/src/vault/models/domain/attachment.ts
+++ b/libs/common/src/vault/models/domain/attachment.ts
@@ -38,7 +38,11 @@ export class Attachment extends Domain {
    );
  }

-  async decrypt(orgId: string, encKey?: SymmetricCryptoKey): Promise<AttachmentView> {
+  async decrypt(
+    orgId: string,
+    context = "No Cipher Context",
+    encKey?: SymmetricCryptoKey,
+  ): Promise<AttachmentView> {
    const view = await this.decryptObj(
      new AttachmentView(this),
      {
@@ -46,6 +50,7 @@ export class Attachment extends Domain {
      },
      orgId,
      encKey,
+      "DomainType: Attachment; " + context,
    );

    if (this.key != null) {
--- a/libs/common/src/vault/models/domain/card.ts
+++ b/libs/common/src/vault/models/domain/card.ts
@@ -37,7 +37,11 @@ export class Card extends Domain {
    );
  }

-  decrypt(orgId: string, encKey?: SymmetricCryptoKey): Promise<CardView> {
+  async decrypt(
+    orgId: string,
+    context = "No Cipher Context",
+    encKey?: SymmetricCryptoKey,
+  ): Promise<CardView> {
    return this.decryptObj(
      new CardView(),
      {
@@ -50,6 +54,7 @@ export class Card extends Domain {
      },
      orgId,
      encKey,
+      "DomainType: Card; " + context,
    );
  }

--- a/libs/common/src/vault/models/domain/cipher.ts
+++ b/libs/common/src/vault/models/domain/cipher.ts
@@ -136,7 +136,11 @@ export class Cipher extends Domain implements Decryptable<CipherView> {

    if (this.key != null) {
      const encryptService = Utils.getContainerService().getEncryptService();
-      const keyBytes = await encryptService.decryptToBytes(this.key, encKey);
+      const keyBytes = await encryptService.decryptToBytes(
+        this.key,
+        encKey,
+        `Cipher Id: ${this.id}; Content: CipherKey; IsEncryptedByOrgKey: ${this.organizationId != null}`,
+      );
      if (keyBytes == null) {
        model.name = "[error: cannot decrypt]";
        model.decryptionFailure = true;
@@ -158,19 +162,36 @@ export class Cipher extends Domain implements Decryptable<CipherView> {

    switch (this.type) {
      case CipherType.Login:
-        model.login = await this.login.decrypt(this.organizationId, bypassValidation, encKey);
+        model.login = await this.login.decrypt(
+          this.organizationId,
+          bypassValidation,
+          `Cipher Id: ${this.id}`,
+          encKey,
+        );
        break;
      case CipherType.SecureNote:
-        model.secureNote = await this.secureNote.decrypt(this.organizationId, encKey);
+        model.secureNote = await this.secureNote.decrypt(
+          this.organizationId,
+          `Cipher Id: ${this.id}`,
+          encKey,
+        );
        break;
      case CipherType.Card:
-        model.card = await this.card.decrypt(this.organizationId, encKey);
+        model.card = await this.card.decrypt(this.organizationId, `Cipher Id: ${this.id}`, encKey);
        break;
      case CipherType.Identity:
-        model.identity = await this.identity.decrypt(this.organizationId, encKey);
+        model.identity = await this.identity.decrypt(
+          this.organizationId,
+          `Cipher Id: ${this.id}`,
+          encKey,
+        );
        break;
      case CipherType.SshKey:
-        model.sshKey = await this.sshKey.decrypt(this.organizationId, encKey);
+        model.sshKey = await this.sshKey.decrypt(
+          this.organizationId,
+          `Cipher Id: ${this.id}`,
+          encKey,
+        );
        break;
      default:
        break;
@@ -181,7 +202,7 @@ export class Cipher extends Domain implements Decryptable<CipherView> {
      await this.attachments.reduce((promise, attachment) => {
        return promise
          .then(() => {
-            return attachment.decrypt(this.organizationId, encKey);
+            return attachment.decrypt(this.organizationId, `Cipher Id: ${this.id}`, encKey);
          })
          .then((decAttachment) => {
            attachments.push(decAttachment);
--- a/libs/common/src/vault/models/domain/identity.ts
+++ b/libs/common/src/vault/models/domain/identity.ts
@@ -61,7 +61,11 @@ export class Identity extends Domain {
    );
  }

-  decrypt(orgId: string, encKey?: SymmetricCryptoKey): Promise<IdentityView> {
+  decrypt(
+    orgId: string,
+    context: string = "No Cipher Context",
+    encKey?: SymmetricCryptoKey,
+  ): Promise<IdentityView> {
    return this.decryptObj(
      new IdentityView(),
      {
@@ -86,6 +90,7 @@ export class Identity extends Domain {
      },
      orgId,
      encKey,
+      "DomainType: Identity; " + context,
    );
  }

--- a/libs/common/src/vault/models/domain/login-uri.ts
+++ b/libs/common/src/vault/models/domain/login-uri.ts
@@ -33,7 +33,11 @@ export class LoginUri extends Domain {
    );
  }

-  decrypt(orgId: string, encKey?: SymmetricCryptoKey): Promise<LoginUriView> {
+  decrypt(
+    orgId: string,
+    context: string = "No Cipher Context",
+    encKey?: SymmetricCryptoKey,
+  ): Promise<LoginUriView> {
    return this.decryptObj(
      new LoginUriView(this),
      {
@@ -41,6 +45,7 @@ export class LoginUri extends Domain {
      },
      orgId,
      encKey,
+      context,
    );
  }

--- a/libs/common/src/vault/models/domain/login.ts
+++ b/libs/common/src/vault/models/domain/login.ts
@@ -55,6 +55,7 @@ export class Login extends Domain {
  async decrypt(
    orgId: string,
    bypassValidation: boolean,
+    context: string = "No Cipher Context",
    encKey?: SymmetricCryptoKey,
  ): Promise<LoginView> {
    const view = await this.decryptObj(
@@ -66,6 +67,7 @@ export class Login extends Domain {
      },
      orgId,
      encKey,
+      `DomainType: Login; ${context}`,
    );

    if (this.uris != null) {
@@ -76,7 +78,7 @@ export class Login extends Domain {
          continue;
        }

-        const uri = await this.uris[i].decrypt(orgId, encKey);
+        const uri = await this.uris[i].decrypt(orgId, context, encKey);
        // URIs are shared remotely after decryption
        // we need to validate that the string hasn't been changed by a compromised server
        // This validation is tied to the existence of cypher.key for backwards compatibility
--- a/libs/common/src/vault/models/domain/password.ts
+++ b/libs/common/src/vault/models/domain/password.ts
@@ -32,6 +32,7 @@ export class Password extends Domain {
      },
      orgId,
      encKey,
+      "DomainType: PasswordHistory",
    );
  }

--- a/libs/common/src/vault/models/domain/secure-note.ts
+++ b/libs/common/src/vault/models/domain/secure-note.ts
@@ -20,8 +20,12 @@ export class SecureNote extends Domain {
    this.type = obj.type;
  }

-  decrypt(orgId: string, encKey?: SymmetricCryptoKey): Promise<SecureNoteView> {
-    return Promise.resolve(new SecureNoteView(this));
+  async decrypt(
+    orgId: string,
+    context = "No Cipher Context",
+    encKey?: SymmetricCryptoKey,
+  ): Promise<SecureNoteView> {
+    return new SecureNoteView(this);
  }

  toSecureNoteData(): SecureNoteData {
--- a/libs/common/src/vault/models/domain/ssh-key.ts
+++ b/libs/common/src/vault/models/domain/ssh-key.ts
@@ -32,7 +32,11 @@ export class SshKey extends Domain {
    );
  }

-  decrypt(orgId: string, encKey?: SymmetricCryptoKey): Promise<SshKeyView> {
+  decrypt(
+    orgId: string,
+    context = "No Cipher Context",
+    encKey?: SymmetricCryptoKey,
+  ): Promise<SshKeyView> {
    return this.decryptObj(
      new SshKeyView(),
      {
@@ -42,6 +46,7 @@ export class SshKey extends Domain {
      },
      orgId,
      encKey,
+      "DomainType: SshKey; " + context,
    );
  }

--- a/libs/key-management/src/key.service.spec.ts
+++ b/libs/key-management/src/key.service.spec.ts
@@ -462,6 +462,7 @@ describe("keyService", () => {
      expect(encryptService.decryptToBytes).toHaveBeenCalledWith(
        fakeEncryptedUserPrivateKey,
        userKey,
+        "Content: Encrypted Private Key",
      );

      expect(userPrivateKey).toBe(fakeDecryptedUserPrivateKey);
--- a/libs/key-management/src/key.service.ts
+++ b/libs/key-management/src/key.service.ts
@@ -382,7 +382,6 @@ export class DefaultKeyService implements KeyServiceAbstraction {
          key: org.key,
        };
      });
-
      return encOrgKeyData;
    });
  }
@@ -891,6 +890,7 @@ export class DefaultKeyService implements KeyServiceAbstraction {
    return (await this.encryptService.decryptToBytes(
      new EncString(encryptedPrivateKey),
      key,
+      "Content: Encrypted Private Key",
    )) as UserPrivateKey;
  }