From ce3ce17010f68fcd6de0e69e4806fcf92687b241 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com> Date: Tue, 3 Jun 2025 22:12:11 +0200 Subject: [PATCH] [PM-21147] User key transferred over ipc within desktop app without its prototype (#15047) * user key transferred over ipc within desktop app without its prototype. `UserKey` object was transferred over IPC as regular `Object` type and not recreated as `SymmetricCryptoKey` type, losing its original functions and properties. As a result `inner` method did not exist and user key silently failed during decryption of encrypted client key halves during biometric unlock. * ipc biometrics serializable user key type * use encrypt service directly for decryption * moving electron key service to KM * log error when unlock via biometrics fails with exception in lock component * bring back tech debt comment * lock component logging prefix --- .github/codecov.yml | 1 + .../src/app/services/services.module.ts | 2 +- .../renderer-biometrics.service.spec.ts | 34 ++++ .../biometrics/renderer-biometrics.service.ts | 8 +- .../electron-key.service.spec.ts | 188 ++++++++++++++++++ .../electron-key.service.ts | 19 +- apps/desktop/src/key-management/preload.ts | 3 +- .../src/lock/components/lock.component.ts | 2 + 8 files changed, 247 insertions(+), 10 deletions(-) create mode 100644 apps/desktop/src/key-management/electron-key.service.spec.ts rename apps/desktop/src/{platform/services => key-management}/electron-key.service.ts (88%) diff --git a/.github/codecov.yml b/.github/codecov.yml index d9d59f9de2..ba4c4b4816 100644 --- a/.github/codecov.yml +++ b/.github/codecov.yml @@ -44,6 +44,7 @@ component_management: - component_id: key-management-keys name: Key Management - Keys paths: + - apps/desktop/src/key-management/electron-key.service.ts - libs/key-management/src/kdf-config.service.ts - libs/key-management/src/key.service.ts - libs/common/src/key-management/master-password/** diff --git a/apps/desktop/src/app/services/services.module.ts b/apps/desktop/src/app/services/services.module.ts index cfab600505..06c42c5b0b 100644 --- a/apps/desktop/src/app/services/services.module.ts +++ b/apps/desktop/src/app/services/services.module.ts @@ -114,10 +114,10 @@ import { DesktopAutofillService } from "../../autofill/services/desktop-autofill import { DesktopFido2UserInterfaceService } from "../../autofill/services/desktop-fido2-user-interface.service"; import { DesktopBiometricsService } from "../../key-management/biometrics/desktop.biometrics.service"; import { RendererBiometricsService } from "../../key-management/biometrics/renderer-biometrics.service"; +import { ElectronKeyService } from "../../key-management/electron-key.service"; import { DesktopLockComponentService } from "../../key-management/lock/services/desktop-lock-component.service"; import { flagEnabled } from "../../platform/flags"; import { DesktopSettingsService } from "../../platform/services/desktop-settings.service"; -import { ElectronKeyService } from "../../platform/services/electron-key.service"; import { ElectronLogRendererService } from "../../platform/services/electron-log.renderer.service"; import { ELECTRON_SUPPORTS_SECURE_STORAGE, diff --git a/apps/desktop/src/key-management/biometrics/renderer-biometrics.service.spec.ts b/apps/desktop/src/key-management/biometrics/renderer-biometrics.service.spec.ts index 7a3f00c7c4..2901b02ab6 100644 --- a/apps/desktop/src/key-management/biometrics/renderer-biometrics.service.spec.ts +++ b/apps/desktop/src/key-management/biometrics/renderer-biometrics.service.spec.ts @@ -1,3 +1,7 @@ +import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; +import { CsprngArray } from "@bitwarden/common/types/csprng"; +import { UserId } from "@bitwarden/common/types/guid"; +import { UserKey } from "@bitwarden/common/types/key"; import { BiometricsStatus } from "@bitwarden/key-management"; import { RendererBiometricsService } from "./renderer-biometrics.service"; @@ -41,4 +45,34 @@ describe("renderer biometrics service tests", function () { expect(result).toBe(expected); }); }); + + describe("unlockWithBiometricsForUser", () => { + const testUserId = "userId1" as UserId; + const service = new RendererBiometricsService(); + + it("should return null if no user key is returned", async () => { + (global as any).ipc.keyManagement.biometric.unlockWithBiometricsForUser.mockResolvedValue( + null, + ); + + const result = await service.unlockWithBiometricsForUser(testUserId); + + expect(result).toBeNull(); + }); + + it("should return a UserKey object when a user key is returned", async () => { + const mockRandomBytes = new Uint8Array(64) as CsprngArray; + const mockUserKey = new SymmetricCryptoKey(mockRandomBytes) as UserKey; + (global as any).ipc.keyManagement.biometric.unlockWithBiometricsForUser.mockResolvedValue( + mockUserKey.toJSON(), + ); + + const result = await service.unlockWithBiometricsForUser(testUserId); + + expect(result).not.toBeNull(); + expect(result).toBeInstanceOf(SymmetricCryptoKey); + expect(result!.keyB64).toEqual(mockUserKey.keyB64); + expect(result!.inner()).toEqual(mockUserKey.inner()); + }); + }); }); diff --git a/apps/desktop/src/key-management/biometrics/renderer-biometrics.service.ts b/apps/desktop/src/key-management/biometrics/renderer-biometrics.service.ts index db17ee480c..1404d65ae5 100644 --- a/apps/desktop/src/key-management/biometrics/renderer-biometrics.service.ts +++ b/apps/desktop/src/key-management/biometrics/renderer-biometrics.service.ts @@ -1,5 +1,6 @@ import { Injectable } from "@angular/core"; +import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; import { UserId } from "@bitwarden/common/types/guid"; import { UserKey } from "@bitwarden/common/types/key"; import { BiometricsStatus } from "@bitwarden/key-management"; @@ -21,7 +22,12 @@ export class RendererBiometricsService extends DesktopBiometricsService { } async unlockWithBiometricsForUser(userId: UserId): Promise { - return await ipc.keyManagement.biometric.unlockWithBiometricsForUser(userId); + const userKey = await ipc.keyManagement.biometric.unlockWithBiometricsForUser(userId); + if (userKey == null) { + return null; + } + // Objects received over IPC lose their prototype, so they must be recreated to restore methods and properties. + return SymmetricCryptoKey.fromJSON(userKey) as UserKey; } async getBiometricsStatusForUser(id: UserId): Promise { diff --git a/apps/desktop/src/key-management/electron-key.service.spec.ts b/apps/desktop/src/key-management/electron-key.service.spec.ts new file mode 100644 index 0000000000..7a0464f5e2 --- /dev/null +++ b/apps/desktop/src/key-management/electron-key.service.spec.ts @@ -0,0 +1,188 @@ +import { mock } from "jest-mock-extended"; + +import { PinServiceAbstraction } from "@bitwarden/auth/common"; +import { CryptoFunctionService } from "@bitwarden/common/key-management/crypto/abstractions/crypto-function.service"; +import { EncryptService } from "@bitwarden/common/key-management/crypto/abstractions/encrypt.service"; +import { FakeMasterPasswordService } from "@bitwarden/common/key-management/master-password/services/fake-master-password.service"; +import { KeyGenerationService } from "@bitwarden/common/platform/abstractions/key-generation.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; +import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; +import { Utils } from "@bitwarden/common/platform/misc/utils"; +import { CsprngArray } from "@bitwarden/common/types/csprng"; +import { UserId } from "@bitwarden/common/types/guid"; +import { UserKey } from "@bitwarden/common/types/key"; +import { BiometricStateService, KdfConfigService } from "@bitwarden/key-management"; + +import { + makeEncString, + makeStaticByteArray, + makeSymmetricCryptoKey, + FakeAccountService, + mockAccountServiceWith, + FakeStateProvider, +} from "../../../../libs/common/spec"; + +import { DesktopBiometricsService } from "./biometrics/desktop.biometrics.service"; +import { ElectronKeyService } from "./electron-key.service"; + +describe("ElectronKeyService", () => { + let keyService: ElectronKeyService; + + const pinService = mock(); + const keyGenerationService = mock(); + const cryptoFunctionService = mock(); + const encryptService = mock(); + const platformUtilService = mock(); + const logService = mock(); + const stateService = mock(); + const kdfConfigService = mock(); + const biometricStateService = mock(); + const biometricService = mock(); + let stateProvider: FakeStateProvider; + + const mockUserId = Utils.newGuid() as UserId; + let accountService: FakeAccountService; + let masterPasswordService: FakeMasterPasswordService; + + beforeEach(() => { + accountService = mockAccountServiceWith(mockUserId); + masterPasswordService = new FakeMasterPasswordService(); + stateProvider = new FakeStateProvider(accountService); + + keyService = new ElectronKeyService( + pinService, + masterPasswordService, + keyGenerationService, + cryptoFunctionService, + encryptService, + platformUtilService, + logService, + stateService, + accountService, + stateProvider, + biometricStateService, + kdfConfigService, + biometricService, + ); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + describe("setUserKey", () => { + const userKey = makeSymmetricCryptoKey() as UserKey; + + describe("store biometric key", () => { + it("does not set any biometric keys when biometric unlock disabled", async () => { + biometricStateService.getBiometricUnlockEnabled.mockResolvedValue(false); + + await keyService.setUserKey(userKey, mockUserId); + + expect(biometricService.setClientKeyHalfForUser).not.toHaveBeenCalled(); + expect(biometricService.setBiometricProtectedUnlockKeyForUser).not.toHaveBeenCalled(); + expect(biometricStateService.setEncryptedClientKeyHalf).not.toHaveBeenCalled(); + expect(biometricStateService.getBiometricUnlockEnabled).toHaveBeenCalledWith(mockUserId); + }); + + describe("biometric unlock enabled", () => { + beforeEach(() => { + biometricStateService.getBiometricUnlockEnabled.mockResolvedValue(true); + }); + + it("sets null biometric client key half and biometric unlock key when require password on start disabled", async () => { + biometricStateService.getRequirePasswordOnStart.mockResolvedValue(false); + + await keyService.setUserKey(userKey, mockUserId); + + expect(biometricService.setClientKeyHalfForUser).toHaveBeenCalledWith(mockUserId, null); + expect(biometricService.setBiometricProtectedUnlockKeyForUser).toHaveBeenCalledWith( + mockUserId, + userKey.keyB64, + ); + expect(biometricStateService.setEncryptedClientKeyHalf).not.toHaveBeenCalled(); + expect(biometricStateService.getBiometricUnlockEnabled).toHaveBeenCalledWith(mockUserId); + expect(biometricStateService.getRequirePasswordOnStart).toHaveBeenCalledWith(mockUserId); + }); + + describe("require password on start enabled", () => { + beforeEach(() => { + biometricStateService.getRequirePasswordOnStart.mockResolvedValue(true); + }); + + it("sets new biometric client key half and biometric unlock key when no biometric client key half stored", async () => { + const clientKeyHalfBytes = makeStaticByteArray(32); + const clientKeyHalf = Utils.fromBufferToUtf8(clientKeyHalfBytes); + const encryptedClientKeyHalf = makeEncString(); + biometricStateService.getEncryptedClientKeyHalf.mockResolvedValue(null); + cryptoFunctionService.randomBytes.mockResolvedValue( + clientKeyHalfBytes.buffer as CsprngArray, + ); + encryptService.encryptString.mockResolvedValue(encryptedClientKeyHalf); + + await keyService.setUserKey(userKey, mockUserId); + + expect(biometricService.setClientKeyHalfForUser).toHaveBeenCalledWith( + mockUserId, + clientKeyHalf, + ); + expect(biometricService.setBiometricProtectedUnlockKeyForUser).toHaveBeenCalledWith( + mockUserId, + userKey.keyB64, + ); + expect(biometricStateService.setEncryptedClientKeyHalf).toHaveBeenCalledWith( + encryptedClientKeyHalf, + mockUserId, + ); + expect(biometricStateService.getBiometricUnlockEnabled).toHaveBeenCalledWith( + mockUserId, + ); + expect(biometricStateService.getRequirePasswordOnStart).toHaveBeenCalledWith( + mockUserId, + ); + expect(biometricStateService.getEncryptedClientKeyHalf).toHaveBeenCalledWith( + mockUserId, + ); + expect(cryptoFunctionService.randomBytes).toHaveBeenCalledWith(32); + expect(encryptService.encryptString).toHaveBeenCalledWith(clientKeyHalf, userKey); + }); + + it("sets decrypted biometric client key half and biometric unlock key when existing biometric client key half stored", async () => { + const encryptedClientKeyHalf = makeEncString(); + const clientKeyHalf = Utils.fromBufferToUtf8(makeStaticByteArray(32)); + biometricStateService.getEncryptedClientKeyHalf.mockResolvedValue( + encryptedClientKeyHalf, + ); + encryptService.decryptString.mockResolvedValue(clientKeyHalf); + + await keyService.setUserKey(userKey, mockUserId); + + expect(biometricService.setClientKeyHalfForUser).toHaveBeenCalledWith( + mockUserId, + clientKeyHalf, + ); + expect(biometricService.setBiometricProtectedUnlockKeyForUser).toHaveBeenCalledWith( + mockUserId, + userKey.keyB64, + ); + expect(biometricStateService.setEncryptedClientKeyHalf).not.toHaveBeenCalled(); + expect(biometricStateService.getBiometricUnlockEnabled).toHaveBeenCalledWith( + mockUserId, + ); + expect(biometricStateService.getRequirePasswordOnStart).toHaveBeenCalledWith( + mockUserId, + ); + expect(biometricStateService.getEncryptedClientKeyHalf).toHaveBeenCalledWith( + mockUserId, + ); + expect(encryptService.decryptString).toHaveBeenCalledWith( + encryptedClientKeyHalf, + userKey, + ); + }); + }); + }); + }); + }); +}); diff --git a/apps/desktop/src/platform/services/electron-key.service.ts b/apps/desktop/src/key-management/electron-key.service.ts similarity index 88% rename from apps/desktop/src/platform/services/electron-key.service.ts rename to apps/desktop/src/key-management/electron-key.service.ts index 5ecde57ec5..2941276720 100644 --- a/apps/desktop/src/platform/services/electron-key.service.ts +++ b/apps/desktop/src/key-management/electron-key.service.ts @@ -19,8 +19,9 @@ import { BiometricStateService, } from "@bitwarden/key-management"; -import { DesktopBiometricsService } from "../../key-management/biometrics/desktop.biometrics.service"; +import { DesktopBiometricsService } from "./biometrics/desktop.biometrics.service"; +// TODO Remove this class once biometric client key half storage is moved https://bitwarden.atlassian.net/browse/PM-22342 export class ElectronKeyService extends DefaultKeyService { constructor( pinService: PinServiceAbstraction, @@ -77,7 +78,6 @@ export class ElectronKeyService extends DefaultKeyService { private async storeBiometricsProtectedUserKey(userKey: UserKey, userId: UserId): Promise { // May resolve to null, in which case no client key have is required - // TODO: Move to windows implementation const clientEncKeyHalf = await this.getBiometricEncryptionClientKeyHalf(userKey, userId); await this.biometricService.setClientKeyHalfForUser(userId, clientEncKeyHalf); await this.biometricService.setBiometricProtectedUnlockKeyForUser(userId, userKey.keyB64); @@ -102,11 +102,16 @@ export class ElectronKeyService extends DefaultKeyService { } // Retrieve existing key half if it exists - let clientKeyHalf = await this.biometricStateService - .getEncryptedClientKeyHalf(userId) - .then((result) => result?.decrypt(null /* user encrypted */, userKey)) - .then((result) => result as CsprngString); - if (clientKeyHalf == null && userKey != null) { + let clientKeyHalf: CsprngString | null = null; + const encryptedClientKeyHalf = + await this.biometricStateService.getEncryptedClientKeyHalf(userId); + if (encryptedClientKeyHalf != null) { + clientKeyHalf = (await this.encryptService.decryptString( + encryptedClientKeyHalf, + userKey, + )) as CsprngString; + } + if (clientKeyHalf == null) { // Set a key half if it doesn't exist const keyBytes = await this.cryptoFunctionService.randomBytes(32); clientKeyHalf = Utils.fromBufferToUtf8(keyBytes) as CsprngString; diff --git a/apps/desktop/src/key-management/preload.ts b/apps/desktop/src/key-management/preload.ts index c955571697..3e90c27ab0 100644 --- a/apps/desktop/src/key-management/preload.ts +++ b/apps/desktop/src/key-management/preload.ts @@ -1,4 +1,5 @@ import { ipcRenderer } from "electron"; +import { Jsonify } from "type-fest"; import { UserKey } from "@bitwarden/common/types/key"; import { BiometricsStatus } from "@bitwarden/key-management"; @@ -14,7 +15,7 @@ const biometric = { ipcRenderer.invoke("biometric", { action: BiometricAction.GetStatus, } satisfies BiometricMessage), - unlockWithBiometricsForUser: (userId: string): Promise => + unlockWithBiometricsForUser: (userId: string): Promise | null> => ipcRenderer.invoke("biometric", { action: BiometricAction.UnlockForUser, userId: userId, diff --git a/libs/key-management-ui/src/lock/components/lock.component.ts b/libs/key-management-ui/src/lock/components/lock.component.ts index ef91f2a03f..043e2333a9 100644 --- a/libs/key-management-ui/src/lock/components/lock.component.ts +++ b/libs/key-management-ui/src/lock/components/lock.component.ts @@ -388,6 +388,8 @@ export class LockComponent implements OnInit, OnDestroy { return; } + this.logService.error("[LockComponent] Failed to unlock via biometrics.", e); + let biometricTranslatedErrorDesc; if (this.clientType === "browser") {