From 75b79794fb85ace13e446236306b7b95749f0a17 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Wed, 29 Oct 2025 12:34:00 +0100 Subject: [PATCH] Remove orgid in vault decryption code --- .../vault/models/domain/attachment.spec.ts | 24 +--------- .../src/vault/models/domain/attachment.ts | 31 +++---------- libs/common/src/vault/models/domain/card.ts | 8 +--- libs/common/src/vault/models/domain/cipher.ts | 45 ++++++++----------- .../vault/models/domain/fido2-credential.ts | 15 +++---- libs/common/src/vault/models/domain/field.ts | 4 +- .../src/vault/models/domain/identity.ts | 5 +-- .../src/vault/models/domain/login-uri.spec.ts | 4 +- .../src/vault/models/domain/login-uri.ts | 9 ++-- .../src/vault/models/domain/login.spec.ts | 4 +- libs/common/src/vault/models/domain/login.ts | 11 +++-- .../src/vault/models/domain/password.ts | 4 +- .../common/src/vault/models/domain/ssh-key.ts | 8 +--- 13 files changed, 56 insertions(+), 116 deletions(-) diff --git a/libs/common/src/vault/models/domain/attachment.spec.ts b/libs/common/src/vault/models/domain/attachment.spec.ts index 972c77537ff..5fc4e87edcc 100644 --- a/libs/common/src/vault/models/domain/attachment.spec.ts +++ b/libs/common/src/vault/models/domain/attachment.spec.ts @@ -9,7 +9,7 @@ import { EncryptService } from "../../../key-management/crypto/abstractions/encr import { EncryptedString, EncString } from "../../../key-management/crypto/models/enc-string"; import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key"; import { ContainerService } from "../../../platform/services/container.service"; -import { OrgKey, UserKey } from "../../../types/key"; +import { UserKey } from "../../../types/key"; import { AttachmentData } from "../../models/data/attachment.data"; import { Attachment } from "../../models/domain/attachment"; @@ -110,31 +110,11 @@ describe("Attachment", () => { it("uses the provided key without depending on KeyService", async () => { const providedKey = mock(); - await attachment.decrypt(null, "", providedKey); + await attachment.decrypt(providedKey, ""); expect(keyService.getUserKey).not.toHaveBeenCalled(); expect(encryptService.unwrapSymmetricKey).toHaveBeenCalledWith(attachment.key, providedKey); }); - - it("gets an organization key if required", async () => { - const orgKey = mock(); - keyService.getOrgKey.calledWith("orgId").mockResolvedValue(orgKey); - - await attachment.decrypt("orgId", "", null); - - expect(keyService.getOrgKey).toHaveBeenCalledWith("orgId"); - expect(encryptService.unwrapSymmetricKey).toHaveBeenCalledWith(attachment.key, orgKey); - }); - - it("gets the user's decryption key if required", async () => { - const userKey = mock(); - keyService.getUserKey.mockResolvedValue(userKey); - - await attachment.decrypt(null, "", null); - - expect(keyService.getUserKey).toHaveBeenCalled(); - expect(encryptService.unwrapSymmetricKey).toHaveBeenCalledWith(attachment.key, userKey); - }); }); }); diff --git a/libs/common/src/vault/models/domain/attachment.ts b/libs/common/src/vault/models/domain/attachment.ts index 7b43af9be55..2d7f2f6f3eb 100644 --- a/libs/common/src/vault/models/domain/attachment.ts +++ b/libs/common/src/vault/models/domain/attachment.ts @@ -1,6 +1,5 @@ import { Jsonify } from "type-fest"; -import { OrgKey, UserKey } from "@bitwarden/common/types/key"; import { Attachment as SdkAttachment } from "@bitwarden/sdk-internal"; import { EncString } from "../../../key-management/crypto/models/enc-string"; @@ -34,21 +33,20 @@ export class Attachment extends Domain { } async decrypt( - orgId: string | undefined, + decryptionKey: SymmetricCryptoKey, context = "No Cipher Context", - encKey?: SymmetricCryptoKey, ): Promise { const view = await this.decryptObj( this, new AttachmentView(this), ["fileName"], - orgId ?? null, - encKey, + null, + decryptionKey, "DomainType: Attachment; " + context, ); if (this.key != null) { - view.key = await this.decryptAttachmentKey(orgId, encKey); + view.key = await this.decryptAttachmentKey(decryptionKey); view.encryptedKey = this.key; // Keep the encrypted key for the view } @@ -56,27 +54,15 @@ export class Attachment extends Domain { } private async decryptAttachmentKey( - orgId: string | undefined, - encKey?: SymmetricCryptoKey, + decryptionKey: SymmetricCryptoKey, ): Promise { try { if (this.key == null) { return undefined; } - if (encKey == null) { - const key = await this.getKeyForDecryption(orgId); - - // If we don't have a key, we can't decrypt - if (key == null) { - return undefined; - } - - encKey = key; - } - const encryptService = Utils.getContainerService().getEncryptService(); - const decValue = await encryptService.unwrapSymmetricKey(this.key, encKey); + const decValue = await encryptService.unwrapSymmetricKey(this.key, decryptionKey); return decValue; } catch (e) { // eslint-disable-next-line no-console @@ -85,11 +71,6 @@ export class Attachment extends Domain { } } - private async getKeyForDecryption(orgId: string | undefined): Promise { - const keyService = Utils.getContainerService().getKeyService(); - return orgId != null ? await keyService.getOrgKey(orgId) : await keyService.getUserKey(); - } - toAttachmentData(): AttachmentData { const a = new AttachmentData(); if (this.size != null) { diff --git a/libs/common/src/vault/models/domain/card.ts b/libs/common/src/vault/models/domain/card.ts index b3a087d44fb..4cf93f3abb8 100644 --- a/libs/common/src/vault/models/domain/card.ts +++ b/libs/common/src/vault/models/domain/card.ts @@ -31,16 +31,12 @@ export class Card extends Domain { this.code = conditionalEncString(obj.code); } - async decrypt( - orgId: string | undefined, - context = "No Cipher Context", - encKey?: SymmetricCryptoKey, - ): Promise { + async decrypt(encKey: SymmetricCryptoKey, context = "No Cipher Context"): Promise { return this.decryptObj( this, new CardView(), ["cardholderName", "brand", "number", "expMonth", "expYear", "code"], - orgId ?? null, + null, encKey, "DomainType: Card; " + context, ); diff --git a/libs/common/src/vault/models/domain/cipher.ts b/libs/common/src/vault/models/domain/cipher.ts index 5e284232936..ebf1e15acd5 100644 --- a/libs/common/src/vault/models/domain/cipher.ts +++ b/libs/common/src/vault/models/domain/cipher.ts @@ -1,5 +1,6 @@ import { Jsonify } from "type-fest"; +import { assertNonNullish } from "@bitwarden/common/auth/utils"; import { Cipher as SdkCipher } from "@bitwarden/sdk-internal"; import { EncString } from "../../../key-management/crypto/models/enc-string"; @@ -123,16 +124,22 @@ export class Cipher extends Domain implements Decryptable { // We are passing the organizationId into the EncString.decrypt() method here, but because the encKey will always be // present and so the organizationId will not be used. // We will refactor the EncString.decrypt() in https://bitwarden.atlassian.net/browse/PM-3762 to remove the dependency on the organizationId. - async decrypt(encKey: SymmetricCryptoKey): Promise { + async decrypt(userKeyOrOrgKey: SymmetricCryptoKey): Promise { + assertNonNullish(userKeyOrOrgKey, "userKeyOrOrgKey", "Cipher decryption"); + const model = new CipherView(this); let bypassValidation = true; + // By default, the user/organization key is used for decryption + let cipherDecryptionKey = userKeyOrOrgKey; + + // If there is a cipher key present, unwrap it and use it for decryption if (this.key != null) { const encryptService = Utils.getContainerService().getEncryptService(); try { - const cipherKey = await encryptService.unwrapSymmetricKey(this.key, encKey); - encKey = cipherKey; + const cipherKey = await encryptService.unwrapSymmetricKey(this.key, userKeyOrOrgKey); + cipherDecryptionKey = cipherKey; bypassValidation = false; } catch { model.name = "[error: cannot decrypt]"; @@ -145,18 +152,17 @@ export class Cipher extends Domain implements Decryptable { this, model, ["name", "notes"], - this.organizationId ?? null, - encKey, + null, + cipherDecryptionKey, ); switch (this.type) { case CipherType.Login: if (this.login != null) { model.login = await this.login.decrypt( - this.organizationId, bypassValidation, + userKeyOrOrgKey, `Cipher Id: ${this.id}`, - encKey, ); } break; @@ -167,29 +173,17 @@ export class Cipher extends Domain implements Decryptable { break; case CipherType.Card: if (this.card != null) { - model.card = await this.card.decrypt( - this.organizationId, - `Cipher Id: ${this.id}`, - encKey, - ); + model.card = await this.card.decrypt(userKeyOrOrgKey, `Cipher Id: ${this.id}`); } break; case CipherType.Identity: if (this.identity != null) { - model.identity = await this.identity.decrypt( - this.organizationId, - `Cipher Id: ${this.id}`, - encKey, - ); + model.identity = await this.identity.decrypt(userKeyOrOrgKey, `Cipher Id: ${this.id}`); } break; case CipherType.SshKey: if (this.sshKey != null) { - model.sshKey = await this.sshKey.decrypt( - this.organizationId, - `Cipher Id: ${this.id}`, - encKey, - ); + model.sshKey = await this.sshKey.decrypt(userKeyOrOrgKey, `Cipher Id: ${this.id}`); } break; default: @@ -200,9 +194,8 @@ export class Cipher extends Domain implements Decryptable { const attachments: AttachmentView[] = []; for (const attachment of this.attachments) { const decryptedAttachment = await attachment.decrypt( - this.organizationId, + userKeyOrOrgKey, `Cipher Id: ${this.id}`, - encKey, ); attachments.push(decryptedAttachment); } @@ -212,7 +205,7 @@ export class Cipher extends Domain implements Decryptable { if (this.fields != null && this.fields.length > 0) { const fields: FieldView[] = []; for (const field of this.fields) { - const decryptedField = await field.decrypt(this.organizationId, encKey); + const decryptedField = await field.decrypt(userKeyOrOrgKey); fields.push(decryptedField); } model.fields = fields; @@ -221,7 +214,7 @@ export class Cipher extends Domain implements Decryptable { if (this.passwordHistory != null && this.passwordHistory.length > 0) { const passwordHistory: PasswordHistoryView[] = []; for (const ph of this.passwordHistory) { - const decryptedPh = await ph.decrypt(this.organizationId, encKey); + const decryptedPh = await ph.decrypt(userKeyOrOrgKey); passwordHistory.push(decryptedPh); } model.passwordHistory = passwordHistory; diff --git a/libs/common/src/vault/models/domain/fido2-credential.ts b/libs/common/src/vault/models/domain/fido2-credential.ts index eff95c4d0bd..375918f8a96 100644 --- a/libs/common/src/vault/models/domain/fido2-credential.ts +++ b/libs/common/src/vault/models/domain/fido2-credential.ts @@ -46,10 +46,7 @@ export class Fido2Credential extends Domain { this.creationDate = new Date(obj.creationDate); } - async decrypt( - orgId: string | undefined, - encKey?: SymmetricCryptoKey, - ): Promise { + async decrypt(decryptionKey: SymmetricCryptoKey): Promise { const view = await this.decryptObj( this, new Fido2CredentialView(), @@ -65,8 +62,8 @@ export class Fido2Credential extends Domain { "rpName", "userDisplayName", ], - orgId ?? null, - encKey, + null, + decryptionKey, ); const { counter } = await this.decryptObj< @@ -74,7 +71,7 @@ export class Fido2Credential extends Domain { { counter: string; } - >(this, { counter: "" }, ["counter"], orgId ?? null, encKey); + >(this, { counter: "" }, ["counter"], null, decryptionKey); // Counter will end up as NaN if this fails view.counter = parseInt(counter); @@ -82,8 +79,8 @@ export class Fido2Credential extends Domain { this, { discoverable: "" }, ["discoverable"], - orgId ?? null, - encKey, + null, + decryptionKey, ); view.discoverable = discoverable === "true"; view.creationDate = this.creationDate; diff --git a/libs/common/src/vault/models/domain/field.ts b/libs/common/src/vault/models/domain/field.ts index 2ee3a9af8a5..9819623bf50 100644 --- a/libs/common/src/vault/models/domain/field.ts +++ b/libs/common/src/vault/models/domain/field.ts @@ -28,12 +28,12 @@ export class Field extends Domain { this.value = conditionalEncString(obj.value); } - decrypt(orgId: string | undefined, encKey?: SymmetricCryptoKey): Promise { + decrypt(encKey: SymmetricCryptoKey): Promise { return this.decryptObj( this, new FieldView(this), ["name", "value"], - orgId ?? null, + null, encKey, ); } diff --git a/libs/common/src/vault/models/domain/identity.ts b/libs/common/src/vault/models/domain/identity.ts index e2def3eb386..097ff041b8b 100644 --- a/libs/common/src/vault/models/domain/identity.ts +++ b/libs/common/src/vault/models/domain/identity.ts @@ -56,9 +56,8 @@ export class Identity extends Domain { } decrypt( - orgId: string | undefined, + encKey: SymmetricCryptoKey, context: string = "No Cipher Context", - encKey?: SymmetricCryptoKey, ): Promise { return this.decryptObj( this, @@ -83,7 +82,7 @@ export class Identity extends Domain { "passportNumber", "licenseNumber", ], - orgId ?? null, + null, encKey, "DomainType: Identity; " + context, ); diff --git a/libs/common/src/vault/models/domain/login-uri.spec.ts b/libs/common/src/vault/models/domain/login-uri.spec.ts index 982b435384b..b3893dcc76c 100644 --- a/libs/common/src/vault/models/domain/login-uri.spec.ts +++ b/libs/common/src/vault/models/domain/login-uri.spec.ts @@ -77,7 +77,7 @@ describe("LoginUri", () => { loginUri.uriChecksum = mockEnc("checksum"); encryptService.hash.mockResolvedValue("checksum"); - const actual = await loginUri.validateChecksum("uri", undefined, undefined); + const actual = await loginUri.validateChecksum("uri", undefined); expect(actual).toBe(true); expect(encryptService.hash).toHaveBeenCalledWith("uri", "sha256"); @@ -88,7 +88,7 @@ describe("LoginUri", () => { loginUri.uriChecksum = mockEnc("checksum"); encryptService.hash.mockResolvedValue("incorrect checksum"); - const actual = await loginUri.validateChecksum("uri", undefined, undefined); + const actual = await loginUri.validateChecksum("uri", undefined); expect(actual).toBe(false); }); diff --git a/libs/common/src/vault/models/domain/login-uri.ts b/libs/common/src/vault/models/domain/login-uri.ts index cac487747f8..af3630a8b6e 100644 --- a/libs/common/src/vault/models/domain/login-uri.ts +++ b/libs/common/src/vault/models/domain/login-uri.ts @@ -28,21 +28,20 @@ export class LoginUri extends Domain { } decrypt( - orgId: string | undefined, + encKey: SymmetricCryptoKey, context: string = "No Cipher Context", - encKey?: SymmetricCryptoKey, ): Promise { return this.decryptObj( this, new LoginUriView(this), ["uri"], - orgId ?? null, + null, encKey, context, ); } - async validateChecksum(clearTextUri: string, orgId?: string, encKey?: SymmetricCryptoKey) { + async validateChecksum(clearTextUri: string, encKey: SymmetricCryptoKey) { if (this.uriChecksum == null) { return false; } @@ -50,7 +49,7 @@ export class LoginUri extends Domain { const keyService = Utils.getContainerService().getEncryptService(); const localChecksum = await keyService.hash(clearTextUri, "sha256"); - const remoteChecksum = await this.uriChecksum.decrypt(orgId ?? null, encKey); + const remoteChecksum = await this.uriChecksum.decrypt(null, encKey); return remoteChecksum === localChecksum; } diff --git a/libs/common/src/vault/models/domain/login.spec.ts b/libs/common/src/vault/models/domain/login.spec.ts index 9f03e225b7f..e4606ebd1de 100644 --- a/libs/common/src/vault/models/domain/login.spec.ts +++ b/libs/common/src/vault/models/domain/login.spec.ts @@ -99,7 +99,7 @@ describe("Login DTO", () => { loginUri.validateChecksum.mockResolvedValue(true); login.uris = [loginUri]; - const loginView = await login.decrypt(null, true); + const loginView = await login.decrypt(true, null); expect(loginView).toEqual(expectedView); }); @@ -111,7 +111,7 @@ describe("Login DTO", () => { .mockResolvedValueOnce(true); login.uris = [loginUri, loginUri, loginUri]; - const loginView = await login.decrypt(null, false); + const loginView = await login.decrypt(false, null); expect(loginView).toEqual(expectedView); }); }); diff --git a/libs/common/src/vault/models/domain/login.ts b/libs/common/src/vault/models/domain/login.ts index 13342c69014..04161dda608 100644 --- a/libs/common/src/vault/models/domain/login.ts +++ b/libs/common/src/vault/models/domain/login.ts @@ -44,16 +44,15 @@ export class Login extends Domain { } async decrypt( - orgId: string | undefined, bypassValidation: boolean, + encKey: SymmetricCryptoKey, context: string = "No Cipher Context", - encKey?: SymmetricCryptoKey, ): Promise { const view = await this.decryptObj( this, new LoginView(this), ["username", "password", "totp"], - orgId ?? null, + null, encKey, `DomainType: Login; ${context}`, ); @@ -66,7 +65,7 @@ export class Login extends Domain { continue; } - const uri = await this.uris[i].decrypt(orgId, context, encKey); + const uri = await this.uris[i].decrypt(encKey, context); const uriString = uri.uri; if (uriString == null) { @@ -79,7 +78,7 @@ export class Login extends Domain { // So we bypass the validation if there's no cipher.key or proceed with the validation and // Skip the value if it's been tampered with. const isValidUri = - bypassValidation || (await this.uris[i].validateChecksum(uriString, orgId, encKey)); + bypassValidation || (await this.uris[i].validateChecksum(uriString, encKey)); if (isValidUri) { view.uris.push(uri); @@ -89,7 +88,7 @@ export class Login extends Domain { if (this.fido2Credentials != null) { view.fido2Credentials = await Promise.all( - this.fido2Credentials.map((key) => key.decrypt(orgId, encKey)), + this.fido2Credentials.map((key) => key.decrypt(encKey)), ); } diff --git a/libs/common/src/vault/models/domain/password.ts b/libs/common/src/vault/models/domain/password.ts index 84e8919b905..0f01ecbdac1 100644 --- a/libs/common/src/vault/models/domain/password.ts +++ b/libs/common/src/vault/models/domain/password.ts @@ -22,12 +22,12 @@ export class Password extends Domain { this.lastUsedDate = new Date(obj.lastUsedDate); } - decrypt(orgId: string | undefined, encKey?: SymmetricCryptoKey): Promise { + decrypt(encKey: SymmetricCryptoKey): Promise { return this.decryptObj( this, new PasswordHistoryView(this), ["password"], - orgId ?? null, + null, encKey, "DomainType: PasswordHistory", ); diff --git a/libs/common/src/vault/models/domain/ssh-key.ts b/libs/common/src/vault/models/domain/ssh-key.ts index a7028321a44..1a6e8d0f47d 100644 --- a/libs/common/src/vault/models/domain/ssh-key.ts +++ b/libs/common/src/vault/models/domain/ssh-key.ts @@ -24,16 +24,12 @@ export class SshKey extends Domain { this.keyFingerprint = new EncString(obj.keyFingerprint); } - decrypt( - orgId: string | undefined, - context = "No Cipher Context", - encKey?: SymmetricCryptoKey, - ): Promise { + decrypt(encKey: SymmetricCryptoKey, context = "No Cipher Context"): Promise { return this.decryptObj( this, new SshKeyView(), ["privateKey", "publicKey", "keyFingerprint"], - orgId ?? null, + null, encKey, "DomainType: SshKey; " + context, );