From 457c0795bec0d9ebcd3ad62e3ba37db221dec928 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Mon, 22 Jul 2024 15:40:19 +0200 Subject: [PATCH] Remove old biometrics masterkey logic (#9943) --- .../browser/src/background/main.background.ts | 2 - .../background/nativeMessaging.background.ts | 27 +--------- .../safari/SafariWebExtensionHandler.swift | 6 --- .../services/electron-crypto.service.spec.ts | 8 --- .../services/electron-crypto.service.ts | 50 +------------------ .../src/services/native-messaging.service.ts | 9 ---- .../pin/pin.service.implementation.ts | 3 -- .../common/services/pin/pin.service.spec.ts | 2 - .../platform/abstractions/state.service.ts | 12 ----- .../src/platform/models/domain/account.ts | 2 - .../src/platform/services/state.service.ts | 50 ------------------- 11 files changed, 3 insertions(+), 168 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 4c9a51a388d..2fb8aa203b0 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1026,8 +1026,6 @@ export default class MainBackground { this.accountService, ); this.nativeMessagingBackground = new NativeMessagingBackground( - this.accountService, - this.masterPasswordService, this.cryptoService, this.cryptoFunctionService, this.runtimeBackground, diff --git a/apps/browser/src/background/nativeMessaging.background.ts b/apps/browser/src/background/nativeMessaging.background.ts index 534a239a811..3fb943f613b 100644 --- a/apps/browser/src/background/nativeMessaging.background.ts +++ b/apps/browser/src/background/nativeMessaging.background.ts @@ -1,8 +1,6 @@ import { firstValueFrom } from "rxjs"; -import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; -import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; import { CryptoFunctionService } from "@bitwarden/common/platform/abstractions/crypto-function.service"; @@ -15,7 +13,7 @@ import { BiometricStateService } from "@bitwarden/common/platform/biometrics/bio import { Utils } from "@bitwarden/common/platform/misc/utils"; import { EncString } from "@bitwarden/common/platform/models/domain/enc-string"; import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; -import { UserKey, MasterKey } from "@bitwarden/common/types/key"; +import { UserKey } from "@bitwarden/common/types/key"; import { BrowserApi } from "../platform/browser/browser-api"; @@ -73,8 +71,6 @@ export class NativeMessagingBackground { private validatingFingerprint: boolean; constructor( - private accountService: AccountService, - private masterPasswordService: InternalMasterPasswordServiceAbstraction, private cryptoService: CryptoService, private cryptoFunctionService: CryptoFunctionService, private runtimeBackground: RuntimeBackground, @@ -355,27 +351,6 @@ export class NativeMessagingBackground { Utils.fromB64ToArray(message.userKeyB64), ) as UserKey; await this.cryptoService.setUserKey(userKey); - } else if (message.keyB64) { - const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id; - // Backwards compatibility to support cases in which the user hasn't updated their desktop app - // TODO: Remove after 2023.10 release (https://bitwarden.atlassian.net/browse/PM-3472) - const encUserKeyPrim = await this.stateService.getEncryptedCryptoSymmetricKey(); - const encUserKey = - encUserKeyPrim != null - ? new EncString(encUserKeyPrim) - : await this.masterPasswordService.getMasterKeyEncryptedUserKey(userId); - if (!encUserKey) { - throw new Error("No encrypted user key found"); - } - const masterKey = new SymmetricCryptoKey( - Utils.fromB64ToArray(message.keyB64), - ) as MasterKey; - const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey( - masterKey, - encUserKey, - ); - await this.masterPasswordService.setMasterKey(masterKey, userId); - await this.cryptoService.setUserKey(userKey); } else { throw new Error("No key received"); } diff --git a/apps/browser/src/safari/safari/SafariWebExtensionHandler.swift b/apps/browser/src/safari/safari/SafariWebExtensionHandler.swift index 95369453409..b0688e3bebb 100644 --- a/apps/browser/src/safari/safari/SafariWebExtensionHandler.swift +++ b/apps/browser/src/safari/safari/SafariWebExtensionHandler.swift @@ -133,12 +133,6 @@ class SafariWebExtensionHandler: NSObject, NSExtensionRequestHandling { status = SecKeychainFindGenericPassword(nil, UInt32(ServiceNameBiometric.utf8.count), ServiceNameBiometric, UInt32(fallbackName.utf8.count), fallbackName, &passwordLength, &passwordPtr, nil) } - // TODO: Remove after 2023.10 release (https://bitwarden.atlassian.net/browse/PM-3473) - if status != errSecSuccess { - let secondaryFallbackName = "_masterkey_biometric" - status = SecKeychainFindGenericPassword(nil, UInt32(ServiceNameBiometric.utf8.count), ServiceNameBiometric, UInt32(secondaryFallbackName.utf8.count), secondaryFallbackName, &passwordLength, &passwordPtr, nil) - } - if status == errSecSuccess { let result = NSString(bytes: passwordPtr!, length: Int(passwordLength), encoding: String.Encoding.utf8.rawValue) as String? SecKeychainItemFreeContent(nil, passwordPtr) diff --git a/apps/desktop/src/platform/services/electron-crypto.service.spec.ts b/apps/desktop/src/platform/services/electron-crypto.service.spec.ts index 0deeca2d41d..debbd0aa9b4 100644 --- a/apps/desktop/src/platform/services/electron-crypto.service.spec.ts +++ b/apps/desktop/src/platform/services/electron-crypto.service.spec.ts @@ -109,14 +109,6 @@ describe("electronCryptoService", () => { userId: mockUserId, }); }); - - it("clears the old deprecated Biometric key whenever a User Key is set", async () => { - await sut.setUserKey(mockUserKey, mockUserId); - - expect(stateService.setCryptoMasterKeyBiometric).toHaveBeenCalledWith(null, { - userId: mockUserId, - }); - }); }); }); }); diff --git a/apps/desktop/src/platform/services/electron-crypto.service.ts b/apps/desktop/src/platform/services/electron-crypto.service.ts index 1bbd02ab8b9..10982e7270f 100644 --- a/apps/desktop/src/platform/services/electron-crypto.service.ts +++ b/apps/desktop/src/platform/services/electron-crypto.service.ts @@ -13,13 +13,12 @@ import { StateService } from "@bitwarden/common/platform/abstractions/state.serv import { BiometricStateService } from "@bitwarden/common/platform/biometrics/biometric-state.service"; import { KeySuffixOptions } from "@bitwarden/common/platform/enums"; import { Utils } from "@bitwarden/common/platform/misc/utils"; -import { EncString } from "@bitwarden/common/platform/models/domain/enc-string"; import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; import { CryptoService } from "@bitwarden/common/platform/services/crypto.service"; import { StateProvider } from "@bitwarden/common/platform/state"; import { CsprngString } from "@bitwarden/common/types/csprng"; import { UserId } from "@bitwarden/common/types/guid"; -import { UserKey, MasterKey } from "@bitwarden/common/types/key"; +import { UserKey } from "@bitwarden/common/types/key"; export class ElectronCryptoService extends CryptoService { constructor( @@ -53,9 +52,7 @@ export class ElectronCryptoService extends CryptoService { override async hasUserKeyStored(keySuffix: KeySuffixOptions, userId?: UserId): Promise { if (keySuffix === KeySuffixOptions.Biometric) { - // TODO: Remove after 2023.10 release (https://bitwarden.atlassian.net/browse/PM-3474) - const oldKey = await this.stateService.hasCryptoMasterKeyBiometric({ userId: userId }); - return oldKey || (await this.stateService.hasUserKeyBiometric({ userId: userId })); + return await this.stateService.hasUserKeyBiometric({ userId: userId }); } return super.hasUserKeyStored(keySuffix, userId); } @@ -90,7 +87,6 @@ export class ElectronCryptoService extends CryptoService { userId?: UserId, ): Promise { if (keySuffix === KeySuffixOptions.Biometric) { - await this.migrateBiometricKeyIfNeeded(userId); const userKey = await this.stateService.getUserKeyBiometric({ userId: userId }); return userKey == null ? null @@ -149,46 +145,4 @@ export class ElectronCryptoService extends CryptoService { return biometricKey; } - - // --LEGACY METHODS-- - // We previously used the master key for additional keys, but now we use the user key. - // These methods support migrating the old keys to the new ones. - // TODO: Remove after 2023.10 release (https://bitwarden.atlassian.net/browse/PM-3475) - - override async clearDeprecatedKeys(keySuffix: KeySuffixOptions, userId?: UserId) { - if (keySuffix === KeySuffixOptions.Biometric) { - await this.stateService.setCryptoMasterKeyBiometric(null, { userId: userId }); - } - - // 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 - super.clearDeprecatedKeys(keySuffix, userId); - } - - private async migrateBiometricKeyIfNeeded(userId?: UserId) { - if (await this.stateService.hasCryptoMasterKeyBiometric({ userId })) { - const oldBiometricKey = await this.stateService.getCryptoMasterKeyBiometric({ userId }); - // decrypt - const masterKey = new SymmetricCryptoKey(Utils.fromB64ToArray(oldBiometricKey)) as MasterKey; - userId ??= (await firstValueFrom(this.accountService.activeAccount$))?.id; - const encUserKeyPrim = await this.stateService.getEncryptedCryptoSymmetricKey({ - userId: userId, - }); - const encUserKey = - encUserKeyPrim != null - ? new EncString(encUserKeyPrim) - : await this.masterPasswordService.getMasterKeyEncryptedUserKey(userId); - if (!encUserKey) { - throw new Error("No user key found during biometric migration"); - } - const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey( - masterKey, - encUserKey, - userId, - ); - // migrate - await this.storeBiometricKey(userKey, userId); - await this.stateService.setCryptoMasterKeyBiometric(null, { userId }); - } - } } diff --git a/apps/desktop/src/services/native-messaging.service.ts b/apps/desktop/src/services/native-messaging.service.ts index 7f6d39b2e8d..2a5c341ee7b 100644 --- a/apps/desktop/src/services/native-messaging.service.ts +++ b/apps/desktop/src/services/native-messaging.service.ts @@ -3,7 +3,6 @@ import { firstValueFrom, map } from "rxjs"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; -import { MasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { CryptoFunctionService } from "@bitwarden/common/platform/abstractions/crypto-function.service"; import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service"; @@ -34,7 +33,6 @@ export class NativeMessagingService { private sharedSecrets = new Map(); constructor( - private masterPasswordService: MasterPasswordServiceAbstraction, private cryptoFunctionService: CryptoFunctionService, private cryptoService: CryptoService, private platformUtilService: PlatformUtilsService, @@ -177,19 +175,12 @@ export class NativeMessagingService { KeySuffixOptions.Biometric, message.userId, ); - const masterKey = await firstValueFrom( - this.masterPasswordService.masterKey$(message.userId as UserId), - ); if (userKey != null) { - // we send the master key still for backwards compatibility - // with older browser extensions - // TODO: Remove after 2023.10 release (https://bitwarden.atlassian.net/browse/PM-3472) await this.send( { command: "biometricUnlock", response: "unlocked", - keyB64: masterKey?.keyB64, userKeyB64: userKey.keyB64, }, appId, diff --git a/libs/auth/src/common/services/pin/pin.service.implementation.ts b/libs/auth/src/common/services/pin/pin.service.implementation.ts index 61586fae462..ac2493a8c48 100644 --- a/libs/auth/src/common/services/pin/pin.service.implementation.ts +++ b/libs/auth/src/common/services/pin/pin.service.implementation.ts @@ -405,9 +405,6 @@ export class PinService implements PinServiceAbstraction { await this.clearOldPinKeyEncryptedMasterKey(userId); - // This also clears the old Biometrics key since the new Biometrics key will be created when the user key is set. - await this.stateService.setCryptoMasterKeyBiometric(null, { userId: userId }); - return userKey; } diff --git a/libs/auth/src/common/services/pin/pin.service.spec.ts b/libs/auth/src/common/services/pin/pin.service.spec.ts index b40d37d4246..81009993d2e 100644 --- a/libs/auth/src/common/services/pin/pin.service.spec.ts +++ b/libs/auth/src/common/services/pin/pin.service.spec.ts @@ -455,8 +455,6 @@ describe("PinService", () => { await sut.setUserKeyEncryptedPin(mockUserKeyEncryptedPin, mockUserId); await sut.clearOldPinKeyEncryptedMasterKey(mockUserId); - - await stateService.setCryptoMasterKeyBiometric(null, { userId: mockUserId }); } function mockDecryptUserKeyFn() { diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index 5c06fdda508..d9498905bbe 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -56,18 +56,6 @@ export abstract class StateService { * @deprecated For migration purposes only, use setUserKeyAuto instead */ setCryptoMasterKeyAuto: (value: string, options?: StorageOptions) => Promise; - /** - * @deprecated For migration purposes only, use getUserKeyBiometric instead - */ - getCryptoMasterKeyBiometric: (options?: StorageOptions) => Promise; - /** - * @deprecated For migration purposes only, use hasUserKeyBiometric instead - */ - hasCryptoMasterKeyBiometric: (options?: StorageOptions) => Promise; - /** - * @deprecated For migration purposes only, use setUserKeyBiometric instead - */ - setCryptoMasterKeyBiometric: (value: BiometricKey, options?: StorageOptions) => Promise; getDuckDuckGoSharedKey: (options?: StorageOptions) => Promise; setDuckDuckGoSharedKey: (value: string, options?: StorageOptions) => Promise; getIsAuthenticated: (options?: StorageOptions) => Promise; diff --git a/libs/common/src/platform/models/domain/account.ts b/libs/common/src/platform/models/domain/account.ts index 79ba4058a41..b367d617dac 100644 --- a/libs/common/src/platform/models/domain/account.ts +++ b/libs/common/src/platform/models/domain/account.ts @@ -49,8 +49,6 @@ export class AccountKeys { /** @deprecated July 2023, left for migration purposes*/ cryptoMasterKeyAuto?: string; /** @deprecated July 2023, left for migration purposes*/ - cryptoMasterKeyBiometric?: string; - /** @deprecated July 2023, left for migration purposes*/ cryptoSymmetricKey?: EncryptionPair = new EncryptionPair< string, SymmetricCryptoKey diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index e799471241f..ba5abecaac6 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -33,7 +33,6 @@ const partialKeys = { userBiometricKey: "_user_biometric", autoKey: "_masterkey_auto", - biometricKey: "_masterkey_biometric", masterKey: "_masterkey", }; @@ -254,54 +253,6 @@ export class StateService< await this.saveSecureStorageKey(partialKeys.masterKey, value, options); } - /** - * @deprecated Use UserKeyBiometric instead - */ - async getCryptoMasterKeyBiometric(options?: StorageOptions): Promise { - options = this.reconcileOptions( - this.reconcileOptions(options, { keySuffix: "biometric" }), - await this.defaultSecureStorageOptions(), - ); - if (options?.userId == null) { - return null; - } - return await this.secureStorageService.get( - `${options.userId}${partialKeys.biometricKey}`, - options, - ); - } - - /** - * @deprecated Use UserKeyBiometric instead - */ - async hasCryptoMasterKeyBiometric(options?: StorageOptions): Promise { - options = this.reconcileOptions( - this.reconcileOptions(options, { keySuffix: "biometric" }), - await this.defaultSecureStorageOptions(), - ); - if (options?.userId == null) { - return false; - } - return await this.secureStorageService.has( - `${options.userId}${partialKeys.biometricKey}`, - options, - ); - } - - /** - * @deprecated Use UserKeyBiometric instead - */ - async setCryptoMasterKeyBiometric(value: BiometricKey, options?: StorageOptions): Promise { - options = this.reconcileOptions( - this.reconcileOptions(options, { keySuffix: "biometric" }), - await this.defaultSecureStorageOptions(), - ); - if (options?.userId == null) { - return; - } - await this.saveSecureStorageKey(partialKeys.biometricKey, value, options); - } - async getDuckDuckGoSharedKey(options?: StorageOptions): Promise { options = this.reconcileOptions(options, await this.defaultSecureStorageOptions()); if (options?.userId == null) { @@ -678,7 +629,6 @@ export class StateService< await this.setUserKeyAutoUnlock(null, { userId: userId }); await this.setUserKeyBiometric(null, { userId: userId }); await this.setCryptoMasterKeyAuto(null, { userId: userId }); - await this.setCryptoMasterKeyBiometric(null, { userId: userId }); await this.setCryptoMasterKeyB64(null, { userId: userId }); }