From 6e370967e115012e1316061cc1df748153b3cfd7 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Tue, 15 Oct 2024 14:50:06 -0400 Subject: [PATCH] Require UserId In CompareHash Method --- .../user-verification.service.spec.ts | 12 ++-- .../user-verification.service.ts | 3 +- .../platform/abstractions/crypto.service.ts | 10 +++- .../platform/services/crypto.service.spec.ts | 59 +++++++++++++++++++ .../src/platform/services/crypto.service.ts | 49 ++++++++------- .../components/password-reprompt.component.ts | 13 +++- 6 files changed, 113 insertions(+), 33 deletions(-) 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 73a97cbc8bbc..c131b6062d97 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 () => { - cryptoService.compareAndUpdateKeyHash.mockResolvedValueOnce(true); + cryptoService.compareKeyHash.mockResolvedValueOnce(true); const result = await sut.verifyUserByMasterPassword( { @@ -227,7 +227,7 @@ describe("UserVerificationService", () => { "email", ); - expect(cryptoService.compareAndUpdateKeyHash).toHaveBeenCalled(); + expect(cryptoService.compareKeyHash).toHaveBeenCalled(); expect(masterPasswordService.setMasterKeyHash).toHaveBeenCalledWith( "localHash", mockUserId, @@ -240,7 +240,7 @@ describe("UserVerificationService", () => { }); it("throws if verification fails", async () => { - cryptoService.compareAndUpdateKeyHash.mockResolvedValueOnce(false); + cryptoService.compareKeyHash.mockResolvedValueOnce(false); await expect( sut.verifyUserByMasterPassword( @@ -253,7 +253,7 @@ describe("UserVerificationService", () => { ), ).rejects.toThrow("Invalid master password"); - expect(cryptoService.compareAndUpdateKeyHash).toHaveBeenCalled(); + expect(cryptoService.compareKeyHash).toHaveBeenCalled(); expect(masterPasswordService.setMasterKeyHash).not.toHaveBeenCalledWith(); expect(masterPasswordService.setMasterKey).not.toHaveBeenCalledWith(); }); @@ -285,7 +285,7 @@ describe("UserVerificationService", () => { "email", ); - expect(cryptoService.compareAndUpdateKeyHash).not.toHaveBeenCalled(); + expect(cryptoService.compareKeyHash).not.toHaveBeenCalled(); expect(masterPasswordService.setMasterKeyHash).toHaveBeenCalledWith( "localHash", mockUserId, @@ -318,7 +318,7 @@ describe("UserVerificationService", () => { ), ).rejects.toThrow("Invalid master password"); - expect(cryptoService.compareAndUpdateKeyHash).not.toHaveBeenCalled(); + expect(cryptoService.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 3b133891c953..a6c347bbb78b 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.cryptoService.compareAndUpdateKeyHash( + const passwordValid = await this.cryptoService.compareKeyHash( verification.secret, masterKey, + userId, ); if (!passwordValid) { throw new Error(this.i18nService.t("invalidMasterPassword")); diff --git a/libs/common/src/platform/abstractions/crypto.service.ts b/libs/common/src/platform/abstractions/crypto.service.ts index 020cfb81754e..4733ed973024 100644 --- a/libs/common/src/platform/abstractions/crypto.service.ts +++ b/libs/common/src/platform/abstractions/crypto.service.ts @@ -202,14 +202,18 @@ export abstract class CryptoService { 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/common/src/platform/services/crypto.service.spec.ts b/libs/common/src/platform/services/crypto.service.spec.ts index 769e6942b05d..f520d9b4e8df 100644 --- a/libs/common/src/platform/services/crypto.service.spec.ts +++ b/libs/common/src/platform/services/crypto.service.spec.ts @@ -735,4 +735,63 @@ describe("cryptoService", () => { }); }); }); + + 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 cryptoService.compareKeyHash( + masterPassword, + masterKey, + mockUserId, + ); + + expect(actualDidMatch).toBe(expectedToMatch); + }, + ); + }); }); diff --git a/libs/common/src/platform/services/crypto.service.ts b/libs/common/src/platform/services/crypto.service.ts index 6b2afdb9806c..e2d6dd03af62 100644 --- a/libs/common/src/platform/services/crypto.service.ts +++ b/libs/common/src/platform/services/crypto.service.ts @@ -319,34 +319,39 @@ export class CryptoService implements CryptoServiceAbstraction { } // 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 (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; + } + + // 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 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 dcc20f7982d3..b4e375a215f9 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 { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -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.cryptoService.getOrDeriveMasterKey( this.formGroup.value.masterPassword, + userId, ); if ( - !(await this.cryptoService.compareAndUpdateKeyHash( + !(await this.cryptoService.compareKeyHash( this.formGroup.value.masterPassword, storedMasterKey, + userId, )) ) { this.platformUtilsService.showToast(