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 02cd6056ef..1538f571cf 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 @@ -216,7 +216,7 @@ describe("UserVerificationService", () => { }); it("returns if verification is successful", async () => { - keyService.compareAndUpdateKeyHash.mockResolvedValueOnce(true); + keyService.compareKeyHash.mockResolvedValueOnce(true); const result = await sut.verifyUserByMasterPassword( { @@ -227,7 +227,7 @@ describe("UserVerificationService", () => { "email", ); - expect(keyService.compareAndUpdateKeyHash).toHaveBeenCalled(); + expect(keyService.compareKeyHash).toHaveBeenCalled(); expect(masterPasswordService.setMasterKeyHash).toHaveBeenCalledWith( "localHash", mockUserId, @@ -240,7 +240,7 @@ describe("UserVerificationService", () => { }); it("throws if verification fails", async () => { - keyService.compareAndUpdateKeyHash.mockResolvedValueOnce(false); + keyService.compareKeyHash.mockResolvedValueOnce(false); await expect( sut.verifyUserByMasterPassword( @@ -253,7 +253,7 @@ describe("UserVerificationService", () => { ), ).rejects.toThrow("Invalid master password"); - expect(keyService.compareAndUpdateKeyHash).toHaveBeenCalled(); + expect(keyService.compareKeyHash).toHaveBeenCalled(); expect(masterPasswordService.setMasterKeyHash).not.toHaveBeenCalledWith(); expect(masterPasswordService.setMasterKey).not.toHaveBeenCalledWith(); }); @@ -285,7 +285,7 @@ describe("UserVerificationService", () => { "email", ); - expect(keyService.compareAndUpdateKeyHash).not.toHaveBeenCalled(); + expect(keyService.compareKeyHash).not.toHaveBeenCalled(); expect(masterPasswordService.setMasterKeyHash).toHaveBeenCalledWith( "localHash", mockUserId, @@ -318,7 +318,7 @@ describe("UserVerificationService", () => { ), ).rejects.toThrow("Invalid master password"); - expect(keyService.compareAndUpdateKeyHash).not.toHaveBeenCalled(); + expect(keyService.compareKeyHash).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 b31ba59c98..5446558a54 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 @@ -206,9 +206,10 @@ export class UserVerificationService implements UserVerificationServiceAbstracti let policyOptions: MasterPasswordPolicyResponse | null; // Client-side verification if (await this.hasMasterPasswordAndMasterKeyHash(userId)) { - const passwordValid = await this.keyService.compareAndUpdateKeyHash( + const passwordValid = await this.keyService.compareKeyHash( verification.secret, masterKey, + userId, ); if (!passwordValid) { throw new Error(this.i18nService.t("invalidMasterPassword")); diff --git a/libs/key-management/src/abstractions/key.service.ts b/libs/key-management/src/abstractions/key.service.ts index 55ffea9db7..0ec3aaafdc 100644 --- a/libs/key-management/src/abstractions/key.service.ts +++ b/libs/key-management/src/abstractions/key.service.ts @@ -204,14 +204,18 @@ export abstract class KeyService { hashPurpose?: HashPurpose, ): Promise; /** - * Compares the provided master password to the stored password hash and server password hash. - * Updates the stored hash if outdated. + * Compares the provided master password to the stored password hash. * @param masterPassword The user's master password * @param key The user's master key + * @param userId The id of the user to do the operation for. * @returns True if the provided master password matches either the stored * key hash or the server key hash */ - abstract compareAndUpdateKeyHash(masterPassword: string, masterKey: MasterKey): Promise; + 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 263779f59b..2b2c6514eb 100644 --- a/libs/key-management/src/key.service.spec.ts +++ b/libs/key-management/src/key.service.spec.ts @@ -733,4 +733,63 @@ describe("keyService", () => { }); }); }); + + describe("compareKeyHash", () => { + type TestCase = { + masterKey: MasterKey; + masterPassword: string | null; + storedMasterKeyHash: string; + mockReturnedHash: string; + expectedToMatch: boolean; + }; + + const data: TestCase[] = [ + { + masterKey: makeSymmetricCryptoKey(64), + masterPassword: "my_master_password", + storedMasterKeyHash: "bXlfaGFzaA==", + mockReturnedHash: "bXlfaGFzaA==", + expectedToMatch: true, + }, + { + masterKey: makeSymmetricCryptoKey(64), + masterPassword: null, + storedMasterKeyHash: "bXlfaGFzaA==", + mockReturnedHash: "bXlfaGFzaA==", + expectedToMatch: false, + }, + { + masterKey: makeSymmetricCryptoKey(64), + masterPassword: null, + storedMasterKeyHash: null, + mockReturnedHash: "bXlfaGFzaA==", + 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.key, masterPassword, "sha256", 2) + .mockResolvedValue(Utils.fromB64ToArray(mockReturnedHash)); + + const actualDidMatch = await keyService.compareKeyHash( + masterPassword, + masterKey, + mockUserId, + ); + + expect(actualDidMatch).toBe(expectedToMatch); + }, + ); + }); }); diff --git a/libs/key-management/src/key.service.ts b/libs/key-management/src/key.service.ts index b12db176ce..f2ba24ef5d 100644 --- a/libs/key-management/src/key.service.ts +++ b/libs/key-management/src/key.service.ts @@ -319,34 +319,43 @@ export class DefaultKeyService implements KeyServiceAbstraction { } // TODO: move to MasterPasswordService - async compareAndUpdateKeyHash(masterPassword: string, masterKey: MasterKey): Promise { - const userId = await firstValueFrom(this.stateProvider.activeUserId$); + 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 (masterPassword != null && storedPasswordHash != null) { - const localKeyHash = await this.hashMasterKey( - masterPassword, - masterKey, - HashPurpose.LocalAuthorization, - ); - if (localKeyHash != null && storedPasswordHash === localKeyHash) { - return true; - } - // TODO: remove serverKeyHash check in 1-2 releases after everyone's keyHash has been updated - const serverKeyHash = await this.hashMasterKey( - masterPassword, - masterKey, - HashPurpose.ServerAuthorization, - ); - if (serverKeyHash != null && storedPasswordHash === serverKeyHash) { - await this.masterPasswordService.setMasterKeyHash(localKeyHash, userId); - return true; - } + if (storedPasswordHash == null) { + return false; } - 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( diff --git a/libs/vault/src/components/password-reprompt.component.ts b/libs/vault/src/components/password-reprompt.component.ts index 3cbdfa1416..21646d6e6a 100644 --- a/libs/vault/src/components/password-reprompt.component.ts +++ b/libs/vault/src/components/password-reprompt.component.ts @@ -1,8 +1,10 @@ import { DialogRef } from "@angular/cdk/dialog"; import { Component } from "@angular/core"; import { FormBuilder, ReactiveFormsModule, Validators } from "@angular/forms"; +import { firstValueFrom, map } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { @@ -43,16 +45,25 @@ export class PasswordRepromptComponent { protected i18nService: I18nService, protected formBuilder: FormBuilder, protected dialogRef: DialogRef, + protected accountService: AccountService, ) {} submit = async () => { + const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(map((a) => a?.id))); + + if (userId == null) { + 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.compareAndUpdateKeyHash( + !(await this.keyService.compareKeyHash( this.formGroup.value.masterPassword, storedMasterKey, + userId, )) ) { this.platformUtilsService.showToast(