From cf7f9cfc7e526bddc0eddab52d08dc842421c579 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Wed, 25 Feb 2026 17:02:04 +0100 Subject: [PATCH] [BEEEP|PM-32521] Remove compare key hash and move to proof of decryption (#19101) * Remove compare key hash and move to proof of decryption * Fix cli build * Fix mv2 * Fix provider * Prettier --- .../browser/src/background/main.background.ts | 10 ++ .../service-container/service-container.ts | 1 + .../src/services/jslib-services.module.ts | 1 + .../user-verification.service.spec.ts | 22 +++-- .../user-verification.service.ts | 18 ++-- .../src/abstractions/key.service.ts | 15 --- libs/key-management/src/key.service.spec.ts | 98 ------------------- libs/key-management/src/key.service.ts | 39 -------- .../components/password-reprompt.component.ts | 11 +-- 9 files changed, 40 insertions(+), 175 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index c55674d2d4f..a45b371a9da 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -111,7 +111,9 @@ import { DeviceTrustServiceAbstraction } from "@bitwarden/common/key-management/ import { DeviceTrustService } from "@bitwarden/common/key-management/device-trust/services/device-trust.service.implementation"; import { KeyConnectorService as KeyConnectorServiceAbstraction } from "@bitwarden/common/key-management/key-connector/abstractions/key-connector.service"; import { KeyConnectorService } from "@bitwarden/common/key-management/key-connector/services/key-connector.service"; +import { MasterPasswordUnlockService } from "@bitwarden/common/key-management/master-password/abstractions/master-password-unlock.service"; import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/key-management/master-password/abstractions/master-password.service.abstraction"; +import { DefaultMasterPasswordUnlockService } from "@bitwarden/common/key-management/master-password/services/default-master-password-unlock.service"; import { MasterPasswordService } from "@bitwarden/common/key-management/master-password/services/master-password.service"; import { PinStateService } from "@bitwarden/common/key-management/pin/pin-state.service.implementation"; import { PinServiceAbstraction } from "@bitwarden/common/key-management/pin/pin.service.abstraction"; @@ -373,6 +375,7 @@ export default class MainBackground { keyService: KeyServiceAbstraction; cryptoFunctionService: CryptoFunctionServiceAbstraction; masterPasswordService: InternalMasterPasswordServiceAbstraction; + masterPasswordUnlockService: MasterPasswordUnlockService; tokenService: TokenServiceAbstraction; appIdService: AppIdServiceAbstraction; apiService: ApiServiceAbstraction; @@ -732,6 +735,12 @@ export default class MainBackground { this.accountCryptographicStateService, ); + this.masterPasswordUnlockService = new DefaultMasterPasswordUnlockService( + this.masterPasswordService, + this.keyService, + this.logService, + ); + const pinStateService = new PinStateService(this.stateProvider); this.appIdService = new AppIdService(this.storageService, this.logService); @@ -1022,6 +1031,7 @@ export default class MainBackground { this.pinService, this.kdfConfigService, this.biometricsService, + this.masterPasswordUnlockService, ); this.vaultSettingsService = new VaultSettingsService( diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index fa6c0e99c42..6bbb7614c78 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/apps/cli/src/service-container/service-container.ts @@ -861,6 +861,7 @@ export class ServiceContainer { this.pinService, this.kdfConfigService, new CliBiometricsService(), + this.masterPasswordUnlockService, ); const biometricService = new CliBiometricsService(); diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 93facfd5be7..964df445f17 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -1199,6 +1199,7 @@ const safeProviders: SafeProvider[] = [ PinServiceAbstraction, KdfConfigService, BiometricsService, + MasterPasswordUnlockService, ], }), safeProvider({ diff --git a/libs/common/src/auth/services/user-verification/user-verification.service.spec.ts b/libs/common/src/auth/services/user-verification/user-verification.service.spec.ts index e570c0f4a43..ddfb8e75148 100644 --- a/libs/common/src/auth/services/user-verification/user-verification.service.spec.ts +++ b/libs/common/src/auth/services/user-verification/user-verification.service.spec.ts @@ -15,6 +15,7 @@ import { } from "@bitwarden/key-management"; import { FakeAccountService, mockAccountServiceWith } from "../../../../spec"; +import { MasterPasswordUnlockService } from "../../../key-management/master-password/abstractions/master-password-unlock.service"; import { InternalMasterPasswordServiceAbstraction } from "../../../key-management/master-password/abstractions/master-password.service.abstraction"; import { PinLockType } from "../../../key-management/pin/pin-lock-type"; import { PinServiceAbstraction } from "../../../key-management/pin/pin.service.abstraction"; @@ -43,6 +44,7 @@ describe("UserVerificationService", () => { const vaultTimeoutSettingsService = mock(); const kdfConfigService = mock(); const biometricsService = mock(); + const masterPasswordUnlockService = mock(); const mockUserId = Utils.newGuid() as UserId; let accountService: FakeAccountService; @@ -61,6 +63,7 @@ describe("UserVerificationService", () => { pinService, kdfConfigService, biometricsService, + masterPasswordUnlockService, ); }); @@ -328,11 +331,10 @@ describe("UserVerificationService", () => { describe("client-side verification", () => { beforeEach(() => { setMasterPasswordAvailability(true); + masterPasswordUnlockService.proofOfDecryption.mockResolvedValue(true); }); it("returns if verification is successful", async () => { - keyService.compareKeyHash.mockResolvedValueOnce(true); - const result = await sut.verifyUserByMasterPassword( { type: VerificationType.MasterPassword, @@ -342,7 +344,10 @@ describe("UserVerificationService", () => { "email", ); - expect(keyService.compareKeyHash).toHaveBeenCalled(); + expect(masterPasswordUnlockService.proofOfDecryption).toHaveBeenCalledWith( + "password", + mockUserId, + ); expect(masterPasswordService.setMasterKeyHash).toHaveBeenCalledWith( "localHash", mockUserId, @@ -356,7 +361,7 @@ describe("UserVerificationService", () => { }); it("throws if verification fails", async () => { - keyService.compareKeyHash.mockResolvedValueOnce(false); + masterPasswordUnlockService.proofOfDecryption.mockResolvedValueOnce(false); await expect( sut.verifyUserByMasterPassword( @@ -369,7 +374,10 @@ describe("UserVerificationService", () => { ), ).rejects.toThrow("Invalid master password"); - expect(keyService.compareKeyHash).toHaveBeenCalled(); + expect(masterPasswordUnlockService.proofOfDecryption).toHaveBeenCalledWith( + "password", + mockUserId, + ); expect(masterPasswordService.setMasterKeyHash).not.toHaveBeenCalledWith(); expect(masterPasswordService.setMasterKey).not.toHaveBeenCalledWith(); }); @@ -401,7 +409,7 @@ describe("UserVerificationService", () => { "email", ); - expect(keyService.compareKeyHash).not.toHaveBeenCalled(); + expect(masterPasswordUnlockService.proofOfDecryption).not.toHaveBeenCalled(); expect(masterPasswordService.setMasterKeyHash).toHaveBeenCalledWith( "localHash", mockUserId, @@ -435,7 +443,7 @@ describe("UserVerificationService", () => { ), ).rejects.toThrow("Invalid master password"); - expect(keyService.compareKeyHash).not.toHaveBeenCalled(); + expect(masterPasswordUnlockService.proofOfDecryption).not.toHaveBeenCalled(); expect(masterPasswordService.setMasterKeyHash).not.toHaveBeenCalledWith(); expect(masterPasswordService.setMasterKey).not.toHaveBeenCalledWith(); }); diff --git a/libs/common/src/auth/services/user-verification/user-verification.service.ts b/libs/common/src/auth/services/user-verification/user-verification.service.ts index 7d93120148b..46c4b86a9c2 100644 --- a/libs/common/src/auth/services/user-verification/user-verification.service.ts +++ b/libs/common/src/auth/services/user-verification/user-verification.service.ts @@ -14,6 +14,7 @@ import { KeyService, } from "@bitwarden/key-management"; +import { MasterPasswordUnlockService } from "../../../key-management/master-password/abstractions/master-password-unlock.service"; import { InternalMasterPasswordServiceAbstraction } from "../../../key-management/master-password/abstractions/master-password.service.abstraction"; import { PinServiceAbstraction } from "../../../key-management/pin/pin.service.abstraction"; import { I18nService } from "../../../platform/abstractions/i18n.service"; @@ -54,6 +55,7 @@ export class UserVerificationService implements UserVerificationServiceAbstracti private pinService: PinServiceAbstraction, private kdfConfigService: KdfConfigService, private biometricsService: BiometricsService, + private masterPasswordUnlockService: MasterPasswordUnlockService, ) {} async getAvailableVerificationOptions( @@ -202,9 +204,8 @@ export class UserVerificationService implements UserVerificationServiceAbstracti let policyOptions: MasterPasswordPolicyResponse | null; // Client-side verification if (await this.hasMasterPasswordAndMasterKeyHash(userId)) { - const passwordValid = await this.keyService.compareKeyHash( + const passwordValid = await this.masterPasswordUnlockService.proofOfDecryption( verification.secret, - masterKey, userId, ); if (!passwordValid) { @@ -214,12 +215,13 @@ export class UserVerificationService implements UserVerificationServiceAbstracti } else { // Server-side verification const request = new SecretVerificationRequest(); - const serverKeyHash = await this.keyService.hashMasterKey( - verification.secret, - masterKey, - HashPurpose.ServerAuthorization, - ); - request.masterPasswordHash = serverKeyHash; + const authenticationData = + await this.masterPasswordService.makeMasterPasswordAuthenticationData( + verification.secret, + kdfConfig, + await firstValueFrom(this.masterPasswordService.saltForUser$(userId)), + ); + request.authenticateWith(authenticationData); try { policyOptions = await this.userVerificationApiService.postAccountVerifyPassword(request); // FIXME: Remove when updating file. Eslint update diff --git a/libs/key-management/src/abstractions/key.service.ts b/libs/key-management/src/abstractions/key.service.ts index e9844ede4bb..9c705a8d0cd 100644 --- a/libs/key-management/src/abstractions/key.service.ts +++ b/libs/key-management/src/abstractions/key.service.ts @@ -174,21 +174,6 @@ export abstract class KeyService { key: MasterKey, hashPurpose?: HashPurpose, ): Promise; - /** - * Compares the provided master password to the stored password hash. - * @deprecated Interacting with the master key directly is prohibited. Use a high level function from MasterPasswordService instead. - * @param masterPassword The user's master password - * @param masterKey The user's master key - * @param userId The id of the user to do the operation for. - * @throws Error when master key is null/undefined. - * @returns True if the derived master password hash matches the stored - * key hash, false otherwise. - */ - abstract compareKeyHash( - masterPassword: string, - masterKey: MasterKey, - userId: UserId, - ): Promise; /** * Stores the encrypted organization keys and clears any decrypted * organization keys currently in memory diff --git a/libs/key-management/src/key.service.spec.ts b/libs/key-management/src/key.service.spec.ts index 8fe77a665bc..0eb49070d6d 100644 --- a/libs/key-management/src/key.service.spec.ts +++ b/libs/key-management/src/key.service.spec.ts @@ -843,104 +843,6 @@ describe("keyService", () => { ); }); - describe("compareKeyHash", () => { - type TestCase = { - masterKey: MasterKey; - masterPassword: string; - storedMasterKeyHash: string | null; - mockReturnedHash: string; - expectedToMatch: boolean; - }; - - const data: TestCase[] = [ - { - masterKey: makeSymmetricCryptoKey(32), - masterPassword: "my_master_password", - storedMasterKeyHash: "bXlfaGFzaA==", - mockReturnedHash: "bXlfaGFzaA==", - expectedToMatch: true, - }, - { - masterKey: makeSymmetricCryptoKey(32), - masterPassword: null as unknown as string, - storedMasterKeyHash: "bXlfaGFzaA==", - mockReturnedHash: "bXlfaGFzaA==", - expectedToMatch: false, - }, - { - masterKey: makeSymmetricCryptoKey(32), - masterPassword: null as unknown as string, - storedMasterKeyHash: null, - mockReturnedHash: "bXlfaGFzaA==", - expectedToMatch: false, - }, - { - masterKey: makeSymmetricCryptoKey(32), - masterPassword: "my_master_password", - storedMasterKeyHash: "bXlfaGFzaA==", - mockReturnedHash: "zxccbXlfaGFzaA==", - expectedToMatch: false, - }, - ]; - - it.each(data)( - "returns expected match value when calculated hash equals stored hash", - async ({ - masterKey, - masterPassword, - storedMasterKeyHash, - mockReturnedHash, - expectedToMatch, - }) => { - masterPasswordService.masterKeyHashSubject.next(storedMasterKeyHash); - - cryptoFunctionService.pbkdf2 - .calledWith(masterKey.inner().encryptionKey, masterPassword, "sha256", 2) - .mockResolvedValue(Utils.fromB64ToArray(mockReturnedHash)); - - const actualDidMatch = await keyService.compareKeyHash( - masterPassword, - masterKey, - mockUserId, - ); - - expect(actualDidMatch).toBe(expectedToMatch); - }, - ); - - test.each([null as unknown as MasterKey, undefined as unknown as MasterKey])( - "throws an error if masterKey is %s", - async (masterKey) => { - await expect( - keyService.compareKeyHash("my_master_password", masterKey, mockUserId), - ).rejects.toThrow("'masterKey' is required to be non-null."); - }, - ); - - test.each([null as unknown as string, undefined as unknown as string])( - "returns false when masterPassword is %s", - async (masterPassword) => { - const result = await keyService.compareKeyHash( - masterPassword, - makeSymmetricCryptoKey(32), - mockUserId, - ); - expect(result).toBe(false); - }, - ); - - it("returns false when storedMasterKeyHash is null", async () => { - masterPasswordService.masterKeyHashSubject.next(null); - - const result = await keyService.compareKeyHash( - "my_master_password", - makeSymmetricCryptoKey(32), - mockUserId, - ); - expect(result).toBe(false); - }); - }); - describe("makeOrgKey", () => { const mockUserPublicKey = new Uint8Array(64) as UserPublicKey; const shareKey = new SymmetricCryptoKey(new Uint8Array(64)); diff --git a/libs/key-management/src/key.service.ts b/libs/key-management/src/key.service.ts index 7258857d889..02784deb4ae 100644 --- a/libs/key-management/src/key.service.ts +++ b/libs/key-management/src/key.service.ts @@ -303,45 +303,6 @@ export class DefaultKeyService implements KeyServiceAbstraction { return Utils.fromBufferToB64(hash); } - async compareKeyHash( - masterPassword: string, - masterKey: MasterKey, - userId: UserId, - ): Promise { - if (masterKey == null) { - throw new Error("'masterKey' is required to be non-null."); - } - - if (masterPassword == null) { - // If they don't give us a master password, we can't hash it, and therefore - // it will never match what we have stored. - return false; - } - - // Retrieve the current password hash - const storedPasswordHash = await firstValueFrom( - this.masterPasswordService.masterKeyHash$(userId), - ); - - if (storedPasswordHash == null) { - return false; - } - - // Hash the key for local use - const localKeyHash = await this.hashMasterKey( - masterPassword, - masterKey, - HashPurpose.LocalAuthorization, - ); - - // Check if the stored hash is already equal to the hash we create locally - if (localKeyHash == null || storedPasswordHash !== localKeyHash) { - return false; - } - - return true; - } - async setOrgKeys( orgs: ProfileOrganizationResponse[], providerOrgs: ProfileProviderOrganizationResponse[], diff --git a/libs/vault/src/components/password-reprompt.component.ts b/libs/vault/src/components/password-reprompt.component.ts index f5245f5cad6..98f7a90f620 100644 --- a/libs/vault/src/components/password-reprompt.component.ts +++ b/libs/vault/src/components/password-reprompt.component.ts @@ -5,6 +5,7 @@ import { firstValueFrom } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { getUserId } from "@bitwarden/common/auth/services/account.service"; +import { MasterPasswordUnlockService } from "@bitwarden/common/key-management/master-password/abstractions/master-password-unlock.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { @@ -16,7 +17,6 @@ import { IconButtonModule, ToastService, } from "@bitwarden/components"; -import { KeyService } from "@bitwarden/key-management"; /** * Used to verify the user's Master Password for the "Master Password Re-prompt" feature only. @@ -43,7 +43,7 @@ export class PasswordRepromptComponent { }); constructor( - protected keyService: KeyService, + protected masterPasswordUnlockService: MasterPasswordUnlockService, protected platformUtilsService: PlatformUtilsService, protected i18nService: I18nService, protected formBuilder: FormBuilder, @@ -65,14 +65,9 @@ export class PasswordRepromptComponent { throw new Error("An active user is expected while doing password reprompt."); } - const storedMasterKey = await this.keyService.getOrDeriveMasterKey( - this.formGroup.value.masterPassword, - userId, - ); if ( - !(await this.keyService.compareKeyHash( + !(await this.masterPasswordUnlockService.proofOfDecryption( this.formGroup.value.masterPassword, - storedMasterKey, userId, )) ) {