From 5ba1416679f112e42e4b5dbd5c47949dcd8d0b56 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Wed, 9 Jun 2021 15:53:54 -0500 Subject: [PATCH] Authenticate with secure storage service (#402) * Split secure key into use case Allows us to push authentication for key access as late as possible. * Do not reload if biometric locked * Linter fixes * Fix key upgrade scenario * Fix boolean value message parsing * Handle systems which don't support biometrics * Do not fail key retrieval on secret upgrade * Ensure old key is removed regardless of upgrade success * Log errors --- angular/src/components/lock.component.ts | 8 +- common/src/abstractions/biometric.main.ts | 2 +- common/src/abstractions/crypto.service.ts | 7 +- common/src/abstractions/storage.service.ts | 13 +- .../src/abstractions/vaultTimeout.service.ts | 2 + common/src/services/crypto.service.ts | 113 +++++++++++++----- common/src/services/system.service.ts | 4 +- common/src/services/vaultTimeout.service.ts | 22 ++-- electron/src/biometric.darwin.main.ts | 4 +- electron/src/biometric.windows.main.ts | 4 +- electron/src/keytarStorageListener.ts | 44 +++++-- .../electronRendererSecureStorage.service.ts | 20 +++- .../electronRendererStorage.service.ts | 7 ++ .../src/services/electronStorage.service.ts | 7 ++ node/src/services/lowdbStorage.service.ts | 4 + 15 files changed, 188 insertions(+), 73 deletions(-) diff --git a/angular/src/components/lock.component.ts b/angular/src/components/lock.component.ts index 0db27a6239..48ec2eb022 100644 --- a/angular/src/components/lock.component.ts +++ b/angular/src/components/lock.component.ts @@ -51,7 +51,8 @@ export class LockComponent implements OnInit { this.pinSet = await this.vaultTimeoutService.isPinLockSet(); this.pinLock = (this.pinSet[0] && this.vaultTimeoutService.pinProtectedKey != null) || this.pinSet[1]; this.supportsBiometric = await this.platformUtilsService.supportsBiometric(); - this.biometricLock = await this.vaultTimeoutService.isBiometricLockSet() && (await this.cryptoService.hasKey() || !this.platformUtilsService.supportsSecureStorage()); + this.biometricLock = await this.vaultTimeoutService.isBiometricLockSet() && + (await this.cryptoService.hasKeyStored('biometric') || !this.platformUtilsService.supportsSecureStorage()); this.biometricText = await this.storageService.get(ConstantsService.biometricText); this.email = await this.userService.getEmail(); let vaultUrl = this.environmentService.getWebVaultUrl(); @@ -157,7 +158,8 @@ export class LockComponent implements OnInit { if (!this.biometricLock) { return; } - const success = await this.platformUtilsService.authenticateBiometric(); + + const success = (await this.cryptoService.getKey('biometric')) != null; if (success) { await this.doContinue(); @@ -176,6 +178,8 @@ export class LockComponent implements OnInit { private async doContinue() { this.vaultTimeoutService.biometricLocked = false; + this.vaultTimeoutService.everBeenUnlocked = true; + this.vaultTimeoutService.manuallyOrTimerLocked = false; const disableFavicon = await this.storageService.get(ConstantsService.disableFaviconKey); await this.stateService.save(ConstantsService.disableFaviconKey, !!disableFavicon); this.messagingService.send('unlocked'); diff --git a/common/src/abstractions/biometric.main.ts b/common/src/abstractions/biometric.main.ts index eff805ea32..65a4eb5cc4 100644 --- a/common/src/abstractions/biometric.main.ts +++ b/common/src/abstractions/biometric.main.ts @@ -2,5 +2,5 @@ export abstract class BiometricMain { isError: boolean; init: () => Promise; supportsBiometric: () => Promise; - requestCreate: () => Promise; + authenticateBiometric: () => Promise; } diff --git a/common/src/abstractions/crypto.service.ts b/common/src/abstractions/crypto.service.ts index 42e91c1851..8172a7c983 100644 --- a/common/src/abstractions/crypto.service.ts +++ b/common/src/abstractions/crypto.service.ts @@ -5,6 +5,7 @@ import { SymmetricCryptoKey } from '../models/domain/symmetricCryptoKey'; import { ProfileOrganizationResponse } from '../models/response/profileOrganizationResponse'; import { KdfType } from '../enums/kdfType'; +import { KeySuffixOptions } from './storage.service'; export abstract class CryptoService { setKey: (key: SymmetricCryptoKey) => Promise; @@ -12,7 +13,7 @@ export abstract class CryptoService { setEncKey: (encKey: string) => Promise<{}>; setEncPrivateKey: (encPrivateKey: string) => Promise<{}>; setOrgKeys: (orgs: ProfileOrganizationResponse[]) => Promise<{}>; - getKey: () => Promise; + getKey: (keySuffix?: KeySuffixOptions) => Promise; getKeyHash: () => Promise; getEncKey: (key?: SymmetricCryptoKey) => Promise; getPublicKey: () => Promise; @@ -21,8 +22,10 @@ export abstract class CryptoService { getOrgKeys: () => Promise>; getOrgKey: (orgId: string) => Promise; hasKey: () => Promise; + hasKeyInMemory: () => boolean; + hasKeyStored: (keySuffix?: KeySuffixOptions) => Promise; hasEncKey: () => Promise; - clearKey: () => Promise; + clearKey: (clearSecretStorage?: boolean) => Promise; clearKeyHash: () => Promise; clearEncKey: (memoryOnly?: boolean) => Promise; clearKeyPair: (memoryOnly?: boolean) => Promise; diff --git a/common/src/abstractions/storage.service.ts b/common/src/abstractions/storage.service.ts index 02fe9d9818..cfedb2d05b 100644 --- a/common/src/abstractions/storage.service.ts +++ b/common/src/abstractions/storage.service.ts @@ -1,5 +1,12 @@ export abstract class StorageService { - get: (key: string) => Promise; - save: (key: string, obj: any) => Promise; - remove: (key: string) => Promise; + get: (key: string, options?: StorageServiceOptions) => Promise; + has: (key: string, options?: StorageServiceOptions) => Promise; + save: (key: string, obj: any, options?: StorageServiceOptions) => Promise; + remove: (key: string, options?: StorageServiceOptions) => Promise; } + +export interface StorageServiceOptions { + keySuffix: KeySuffixOptions; +} + +export type KeySuffixOptions = 'auto' | 'biometric'; diff --git a/common/src/abstractions/vaultTimeout.service.ts b/common/src/abstractions/vaultTimeout.service.ts index ce1da3c3c3..719acf3b71 100644 --- a/common/src/abstractions/vaultTimeout.service.ts +++ b/common/src/abstractions/vaultTimeout.service.ts @@ -2,6 +2,8 @@ import { EncString } from '../models/domain/encString'; export abstract class VaultTimeoutService { biometricLocked: boolean; + manuallyOrTimerLocked: boolean; + everBeenUnlocked: boolean; pinProtectedKey: EncString; isLocked: () => Promise; checkVaultTimeout: () => Promise; diff --git a/common/src/services/crypto.service.ts b/common/src/services/crypto.service.ts index 1b98315cf3..7d1b94a4f3 100644 --- a/common/src/services/crypto.service.ts +++ b/common/src/services/crypto.service.ts @@ -13,7 +13,10 @@ import { CryptoService as CryptoServiceAbstraction } from '../abstractions/crypt import { CryptoFunctionService } from '../abstractions/cryptoFunction.service'; import { LogService } from '../abstractions/log.service'; import { PlatformUtilsService } from '../abstractions/platformUtils.service'; -import { StorageService } from '../abstractions/storage.service'; +import { + KeySuffixOptions, + StorageService, +} from '../abstractions/storage.service'; import { ConstantsService } from './constants.service'; @@ -46,11 +49,17 @@ export class CryptoService implements CryptoServiceAbstraction { async setKey(key: SymmetricCryptoKey): Promise { this.key = key; - if (!await this.shouldStoreKey()) { - return; + if (await this.shouldStoreKey('auto')) { + await this.secureStorageService.save(Keys.key, key.keyB64, { keySuffix: 'auto' }); + } else { + this.clearStoredKey('auto'); } - return this.secureStorageService.save(Keys.key, key.keyB64); + if (await this.shouldStoreKey('biometric')) { + await this.secureStorageService.save(Keys.key, key.keyB64, { keySuffix: 'biometric' }); + } else { + this.clearStoredKey('biometric'); + } } setKeyHash(keyHash: string): Promise<{}> { @@ -86,28 +95,23 @@ export class CryptoService implements CryptoServiceAbstraction { return this.storageService.save(Keys.encOrgKeys, orgKeys); } - async getKey(): Promise { + async getKey(keySuffix?: KeySuffixOptions): Promise { if (this.key != null) { return this.key; } - - const key = await this.secureStorageService.get(Keys.key); + keySuffix ||= 'auto'; + const key = await this.retrieveKeyFromStorage(keySuffix); if (key != null) { - if (!await this.shouldStoreKey()) { - this.logService.warning('Throwing away stored key since settings have changed'); - this.secureStorageService.remove(Keys.key); - return null; - } const symmetricKey = new SymmetricCryptoKey(Utils.fromB64ToArray(key).buffer); if (!await this.validateKey(symmetricKey)) { this.logService.warning('Wrong key, throwing away stored key'); - this.secureStorageService.remove(Keys.key); + this.secureStorageService.remove(Keys.key, { keySuffix: keySuffix }); return null; } - this.key = symmetricKey; + this.setKey(symmetricKey); } return key == null ? null : this.key; @@ -247,7 +251,16 @@ export class CryptoService implements CryptoServiceAbstraction { } async hasKey(): Promise { - return (await this.getKey()) != null; + return this.hasKeyInMemory() || await this.hasKeyStored('auto') || await this.hasKeyStored('biometric'); + } + + hasKeyInMemory(): boolean { + return this.key != null; + } + + async hasKeyStored(keySuffix: KeySuffixOptions): Promise { + await this.upgradeSecurelyStoredKey(); + return await this.secureStorageService.has(Keys.key, { keySuffix: keySuffix }); } async hasEncKey(): Promise { @@ -255,9 +268,16 @@ export class CryptoService implements CryptoServiceAbstraction { return encKey != null; } - clearKey(): Promise { + async clearKey(clearSecretStorage: boolean = true): Promise { this.key = this.legacyEtmKey = null; - return this.secureStorageService.remove(Keys.key); + if (clearSecretStorage) { + this.clearStoredKey('auto'); + this.clearStoredKey('biometric'); + } + } + + async clearStoredKey(keySuffix: KeySuffixOptions) { + await this.secureStorageService.remove(Keys.key, { keySuffix: keySuffix }); } clearKeyHash(): Promise { @@ -305,14 +325,6 @@ export class CryptoService implements CryptoServiceAbstraction { async toggleKey(): Promise { const key = await this.getKey(); - const option = await this.storageService.get(ConstantsService.vaultTimeoutKey); - const biometric = await this.storageService.get(ConstantsService.biometricUnlockKey); - if ((!biometric && this.platformUtilService.supportsSecureStorage()) && (option != null || option === 0)) { - // if we have a lock option set, clear the key - await this.clearKey(); - this.key = key; - return; - } await this.setKey(key); } @@ -592,11 +604,11 @@ export class CryptoService implements CryptoServiceAbstraction { async validateKey(key: SymmetricCryptoKey) { try { const encPrivateKey = await this.storageService.get(Keys.encPrivateKey); - if (encPrivateKey == null) { + const encKey = await this.getEncKey(key); + if (encPrivateKey == null || encKey == null) { return false; } - const encKey = await this.getEncKey(key); const privateKey = await this.decryptToBytes(new EncString(encPrivateKey), encKey); await this.cryptoFunctionService.rsaExtractPublicKey(privateKey); } catch (e) { @@ -608,14 +620,49 @@ export class CryptoService implements CryptoServiceAbstraction { // Helpers - private async shouldStoreKey() { - const vaultTimeout = await this.storageService.get(ConstantsService.vaultTimeoutKey); - const biometricUnlock = await this.storageService.get(ConstantsService.biometricUnlockKey); + private async shouldStoreKey(keySuffix: KeySuffixOptions) { + let shouldStoreKey = false; + if (keySuffix === 'auto') { + const vaultTimeout = await this.storageService.get(ConstantsService.vaultTimeoutKey); + shouldStoreKey = vaultTimeout == null; + } else if (keySuffix === 'biometric') { + const biometricUnlock = await this.storageService.get(ConstantsService.biometricUnlockKey); + shouldStoreKey = biometricUnlock && this.platformUtilService.supportsSecureStorage(); + } + return shouldStoreKey; + } - const biometricsEnabled = biometricUnlock && this.platformUtilService.supportsSecureStorage(); - const noVaultTimeout = vaultTimeout == null; + private async retrieveKeyFromStorage(keySuffix: KeySuffixOptions) { + await this.upgradeSecurelyStoredKey(); - return noVaultTimeout || biometricsEnabled; + return await this.secureStorageService.get(Keys.key, { keySuffix: keySuffix }); + } + + /** + * @deprecated 4 Jun 2021 This is temporary upgrade method to move from a single shared stored key to + * multiple, unique stored keys for each use, e.g. never logout vs. biometric authentication. + */ + private async upgradeSecurelyStoredKey() { + // attempt key upgrade, but if we fail just delete it. Keys will be stored property upon unlock anyway. + const key = await this.secureStorageService.get(Keys.key); + + if (key == null) { + return; + } + + try { + if (await this.shouldStoreKey('auto')) { + await this.secureStorageService.save(Keys.key, key, { keySuffix: 'auto' }); + } + if (await this.shouldStoreKey('biometric')) { + await this.secureStorageService.save(Keys.key, key, { keySuffix: 'biometric' }); + } + } catch (e) { + this.logService.error(`Encountered error while upgrading obsolete Bitwarden secure storage item:`); + this.logService.error(e); + } + + await this.secureStorageService.remove(Keys.key); } private async aesEncrypt(data: ArrayBuffer, key: SymmetricCryptoKey): Promise { diff --git a/common/src/services/system.service.ts b/common/src/services/system.service.ts index 053dd50de4..ada918132a 100644 --- a/common/src/services/system.service.ts +++ b/common/src/services/system.service.ts @@ -19,7 +19,9 @@ export class SystemService implements SystemServiceAbstraction { } startProcessReload(): void { - if (this.vaultTimeoutService.pinProtectedKey != null || this.reloadInterval != null) { + if (this.vaultTimeoutService.pinProtectedKey != null || + this.vaultTimeoutService.biometricLocked || + this.reloadInterval != null) { return; } this.cancelProcessReload(); diff --git a/common/src/services/vaultTimeout.service.ts b/common/src/services/vaultTimeout.service.ts index 0039690fc9..328ba06820 100644 --- a/common/src/services/vaultTimeout.service.ts +++ b/common/src/services/vaultTimeout.service.ts @@ -17,6 +17,8 @@ import { EncString } from '../models/domain/encString'; export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { pinProtectedKey: EncString = null; biometricLocked: boolean = true; + everBeenUnlocked: boolean = false; + manuallyOrTimerLocked: boolean = false; private inited = false; @@ -46,9 +48,13 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { // Keys aren't stored for a device that is locked or logged out. async isLocked(): Promise { + if (await this.cryptoService.hasKeyStored('auto') && !this.everBeenUnlocked) { + await this.cryptoService.getKey('auto'); + } + const hasKey = await this.cryptoService.hasKey(); if (hasKey) { - if (await this.isBiometricLockSet() && this.biometricLocked) { + if ((await this.isBiometricLockSet() && this.biometricLocked) || this.manuallyOrTimerLocked) { return true; } } @@ -102,18 +108,8 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { } this.biometricLocked = true; - if (allowSoftLock) { - const biometricLocked = await this.isBiometricLockSet(); - if (biometricLocked && this.platformUtilsService.supportsSecureStorage()) { - this.messagingService.send('locked'); - if (this.lockedCallback != null) { - await this.lockedCallback(); - } - return; - } - } - - await this.cryptoService.clearKey(); + this.manuallyOrTimerLocked = true; + await this.cryptoService.clearKey(false); await this.cryptoService.clearOrgKeys(true); await this.cryptoService.clearKeyPair(true); await this.cryptoService.clearEncKey(true); diff --git a/electron/src/biometric.darwin.main.ts b/electron/src/biometric.darwin.main.ts index 4dcecb05e4..480e69f494 100644 --- a/electron/src/biometric.darwin.main.ts +++ b/electron/src/biometric.darwin.main.ts @@ -16,7 +16,7 @@ export default class BiometricDarwinMain implements BiometricMain { this.storageService.save(ElectronConstants.noAutoPromptBiometricsText, 'noAutoPromptTouchId'); ipcMain.on('biometric', async (event: any, message: any) => { - event.returnValue = await this.requestCreate(); + event.returnValue = await this.authenticateBiometric(); }); } @@ -24,7 +24,7 @@ export default class BiometricDarwinMain implements BiometricMain { return Promise.resolve(systemPreferences.canPromptTouchID()); } - async requestCreate(): Promise { + async authenticateBiometric(): Promise { try { await systemPreferences.promptTouchID(this.i18nservice.t('touchIdConsentMessage')); return true; diff --git a/electron/src/biometric.windows.main.ts b/electron/src/biometric.windows.main.ts index 85cee8819d..5ec7cb64ed 100644 --- a/electron/src/biometric.windows.main.ts +++ b/electron/src/biometric.windows.main.ts @@ -30,7 +30,7 @@ export default class BiometricWindowsMain implements BiometricMain { this.storageService.save(ElectronConstants.noAutoPromptBiometricsText, 'noAutoPromptWindowsHello'); ipcMain.on('biometric', async (event: any, message: any) => { - event.returnValue = await this.requestCreate(); + event.returnValue = await this.authenticateBiometric(); }); } @@ -40,7 +40,7 @@ export default class BiometricWindowsMain implements BiometricMain { return this.getAllowedAvailabilities().includes(availability); } - async requestCreate(): Promise { + async authenticateBiometric(): Promise { const module = this.getWindowsSecurityCredentialsUiModule(); if (module == null) { return false; diff --git a/electron/src/keytarStorageListener.ts b/electron/src/keytarStorageListener.ts index 633e0264f5..fb263cb353 100644 --- a/electron/src/keytarStorageListener.ts +++ b/electron/src/keytarStorageListener.ts @@ -6,27 +6,51 @@ import { setPassword, } from 'keytar'; +import { BiometricMain } from 'jslib-common/abstractions/biometric.main'; + +const AuthRequiredSuffix = '_biometric'; +const AuthenticatedActions = ['getPassword']; + export class KeytarStorageListener { - constructor(private serviceName: string) { } + constructor(private serviceName: string, private biometricService: BiometricMain) { } init() { ipcMain.on('keytar', async (event: any, message: any) => { try { - let val: string = null; - if (message.action && message.key) { - if (message.action === 'getPassword') { - val = await getPassword(this.serviceName, message.key); - } else if (message.action === 'setPassword' && message.value) { - await setPassword(this.serviceName, message.key, message.value); - } else if (message.action === 'deletePassword') { - await deletePassword(this.serviceName, message.key); - } + let serviceName = this.serviceName; + message.keySuffix = '_' + (message.keySuffix ?? ''); + if (message.keySuffix !== '_') { + serviceName += message.keySuffix; } + const authenticationRequired = AuthenticatedActions.includes(message.action) && + AuthRequiredSuffix === message.keySuffix; + const authenticated = !authenticationRequired || await this.authenticateBiometric(); + + let val: string | boolean = null; + if (authenticated && message.action && message.key) { + if (message.action === 'getPassword') { + val = await getPassword(serviceName, message.key); + } else if (message.action === 'hasPassword') { + const result = await getPassword(serviceName, message.key); + val = result != null; + } else if (message.action === 'setPassword' && message.value) { + await setPassword(serviceName, message.key, message.value); + } else if (message.action === 'deletePassword') { + await deletePassword(serviceName, message.key); + } + } event.returnValue = val; } catch { event.returnValue = null; } }); } + + private async authenticateBiometric(): Promise { + if (this.biometricService) { + return await this.biometricService.authenticateBiometric(); + } + return false; + } } diff --git a/electron/src/services/electronRendererSecureStorage.service.ts b/electron/src/services/electronRendererSecureStorage.service.ts index 4c2354b1e5..fb71805811 100644 --- a/electron/src/services/electronRendererSecureStorage.service.ts +++ b/electron/src/services/electronRendererSecureStorage.service.ts @@ -1,29 +1,41 @@ import { ipcRenderer } from 'electron'; -import { StorageService } from 'jslib-common/abstractions/storage.service'; +import { StorageService, StorageServiceOptions } from 'jslib-common/abstractions/storage.service'; export class ElectronRendererSecureStorageService implements StorageService { - async get(key: string): Promise { + async get(key: string, options?: StorageServiceOptions): Promise { const val = ipcRenderer.sendSync('keytar', { action: 'getPassword', key: key, + keySuffix: options?.keySuffix ?? '', }); return Promise.resolve(val != null ? JSON.parse(val) as T : null); } - async save(key: string, obj: any): Promise { + async has(key: string, options?: StorageServiceOptions): Promise { + const val = ipcRenderer.sendSync('keytar', { + action: 'hasPassword', + key: key, + keySuffix: options?.keySuffix ?? '', + }); + return Promise.resolve(!!val); + } + + async save(key: string, obj: any, options?: StorageServiceOptions): Promise { ipcRenderer.sendSync('keytar', { action: 'setPassword', key: key, + keySuffix: options?.keySuffix ?? '', value: JSON.stringify(obj), }); return Promise.resolve(); } - async remove(key: string): Promise { + async remove(key: string, options?: StorageServiceOptions): Promise { ipcRenderer.sendSync('keytar', { action: 'deletePassword', key: key, + keySuffix: options?.keySuffix ?? '', }); return Promise.resolve(); } diff --git a/electron/src/services/electronRendererStorage.service.ts b/electron/src/services/electronRendererStorage.service.ts index a12048ed38..e1554a7ec8 100644 --- a/electron/src/services/electronRendererStorage.service.ts +++ b/electron/src/services/electronRendererStorage.service.ts @@ -10,6 +10,13 @@ export class ElectronRendererStorageService implements StorageService { }); } + has(key: string): Promise { + return ipcRenderer.invoke('storageService', { + action: 'has', + key: key, + }); + } + save(key: string, obj: any): Promise { return ipcRenderer.invoke('storageService', { action: 'save', diff --git a/electron/src/services/electronStorage.service.ts b/electron/src/services/electronStorage.service.ts index ac8875ff2c..5bf8d26cbb 100644 --- a/electron/src/services/electronStorage.service.ts +++ b/electron/src/services/electronStorage.service.ts @@ -25,6 +25,8 @@ export class ElectronStorageService implements StorageService { switch (options.action) { case 'get': return this.get(options.key); + case 'has': + return this.has(options.key); case 'save': return this.save(options.key, options.obj); case 'remove': @@ -38,6 +40,11 @@ export class ElectronStorageService implements StorageService { return Promise.resolve(val != null ? val : null); } + has(key: string): Promise { + const val = this.store.get(key); + return Promise.resolve(val != null); + } + save(key: string, obj: any): Promise { if (obj instanceof Set) { obj = Array.from(obj); diff --git a/node/src/services/lowdbStorage.service.ts b/node/src/services/lowdbStorage.service.ts index 8b305ea244..99b327a1d7 100644 --- a/node/src/services/lowdbStorage.service.ts +++ b/node/src/services/lowdbStorage.service.ts @@ -84,6 +84,10 @@ export class LowdbStorageService implements StorageService { }); } + has(key: string): Promise { + return this.get(key).then(v => v != null); + } + save(key: string, obj: any): Promise { return this.lockDbFile(() => { this.readForNoCache();