From 7cd6fcf265d2a6ea16e1a4ec8f32e9e3d1acbddd Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Tue, 6 Aug 2024 21:35:04 +0200 Subject: [PATCH] [PM-2192] Improve userkey verification on biometric unlock (#10326) * Improve biometric unlock userkey verification * Add early return * Pass activeuserid to cryptoservice functions --- apps/browser/src/_locales/en/messages.json | 6 +++ .../browser/src/background/main.background.ts | 1 + .../background/nativeMessaging.background.ts | 50 ++++++++++++------- .../platform/abstractions/crypto.service.ts | 7 +++ .../src/platform/services/crypto.service.ts | 2 +- 5 files changed, 48 insertions(+), 18 deletions(-) diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index 4e3f7b7abc..03009cdfce 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -2000,6 +2000,12 @@ "nativeMessagingWrongUserTitle": { "message": "Account missmatch" }, + "nativeMessagingWrongUserKeyDesc": { + "message": "Biometric unlock failed. The biometric secret key failed to unlock the vault. Please try to set up biometrics again." + }, + "nativeMessagingWrongUserKeyTitle": { + "message": "Biometric key missmatch" + }, "biometricsNotEnabledTitle": { "message": "Biometrics not set up" }, diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index b1be350f49..f5fd5185bf 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1049,6 +1049,7 @@ export default class MainBackground { this.logService, this.authService, this.biometricStateService, + this.accountService, ); this.commandsBackground = new CommandsBackground( this, diff --git a/apps/browser/src/background/nativeMessaging.background.ts b/apps/browser/src/background/nativeMessaging.background.ts index 3fb943f613..e19485c711 100644 --- a/apps/browser/src/background/nativeMessaging.background.ts +++ b/apps/browser/src/background/nativeMessaging.background.ts @@ -1,5 +1,6 @@ -import { firstValueFrom } from "rxjs"; +import { firstValueFrom, map } from "rxjs"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; @@ -81,6 +82,7 @@ export class NativeMessagingBackground { private logService: LogService, private authService: AuthService, private biometricStateService: BiometricStateService, + private accountService: AccountService, ) { if (chrome?.permissions?.onAdded) { // Reload extension to activate nativeMessaging @@ -223,6 +225,16 @@ export class NativeMessagingBackground { }); } + showIncorrectUserKeyDialog() { + this.messagingService.send("showDialog", { + title: { key: "nativeMessagingWrongUserKeyTitle" }, + content: { key: "nativeMessagingWrongUserKeyDesc" }, + acceptButtonText: { key: "ok" }, + cancelButtonText: null, + type: "danger", + }); + } + async send(message: Message) { if (!this.connected) { await this.connect(); @@ -350,7 +362,26 @@ export class NativeMessagingBackground { const userKey = new SymmetricCryptoKey( Utils.fromB64ToArray(message.userKeyB64), ) as UserKey; - await this.cryptoService.setUserKey(userKey); + const activeUserId = await firstValueFrom( + this.accountService.activeAccount$.pipe(map((a) => a?.id)), + ); + const isUserKeyValid = await this.cryptoService.validateUserKey( + userKey, + activeUserId, + ); + if (isUserKeyValid) { + await this.cryptoService.setUserKey(userKey, activeUserId); + } else { + this.logService.error("Unable to verify biometric unlocked userkey"); + await this.cryptoService.clearKeys(activeUserId); + this.showIncorrectUserKeyDialog(); + + // Exit early + if (this.resolver) { + this.resolver(message); + } + return; + } } else { throw new Error("No key received"); } @@ -371,21 +402,6 @@ export class NativeMessagingBackground { return; } - // Verify key is correct by attempting to decrypt a secret - try { - await this.cryptoService.getFingerprint(await this.stateService.getUserId()); - } catch (e) { - this.logService.error("Unable to verify key: " + e); - await this.cryptoService.clearKeys(); - this.showWrongUserDialog(); - - // Exit early - if (this.resolver) { - this.resolver(message); - } - return; - } - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. // eslint-disable-next-line @typescript-eslint/no-floating-promises this.runtimeBackground.processMessage({ command: "unlocked" }); diff --git a/libs/common/src/platform/abstractions/crypto.service.ts b/libs/common/src/platform/abstractions/crypto.service.ts index 82fa56c32d..b9499c8fd5 100644 --- a/libs/common/src/platform/abstractions/crypto.service.ts +++ b/libs/common/src/platform/abstractions/crypto.service.ts @@ -418,4 +418,11 @@ export abstract class CryptoService { * @throws If an invalid user id is passed in. */ abstract userPublicKey$(userId: UserId): Observable; + + /** + * Validates that a userkey is correct for a given user + * @param key The key to validate + * @param userId The user id for the key + */ + abstract validateUserKey(key: UserKey, userId: UserId): Promise; } diff --git a/libs/common/src/platform/services/crypto.service.ts b/libs/common/src/platform/services/crypto.service.ts index b139775ea4..0fe5268f25 100644 --- a/libs/common/src/platform/services/crypto.service.ts +++ b/libs/common/src/platform/services/crypto.service.ts @@ -620,7 +620,7 @@ export class CryptoService implements CryptoServiceAbstraction { } // ---HELPERS--- - protected async validateUserKey(key: UserKey, userId: UserId): Promise { + async validateUserKey(key: UserKey, userId: UserId): Promise { if (!key) { return false; }