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

@@ -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<boolean>(ConstantsService.disableFaviconKey);
         await this.stateService.save(ConstantsService.disableFaviconKey, !!disableFavicon);
         this.messagingService.send('unlocked');

common/src/abstractions/biometric.main.ts

@@ -2,5 +2,5 @@ export abstract class BiometricMain {
     isError: boolean;
     init: () => Promise<void>;
     supportsBiometric: () => Promise<boolean>;
-    requestCreate: () => Promise<boolean>;
+    authenticateBiometric: () => Promise<boolean>;
 }

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<any>;
@@ -12,7 +13,7 @@ export abstract class CryptoService {
     setEncKey: (encKey: string) => Promise<{}>;
     setEncPrivateKey: (encPrivateKey: string) => Promise<{}>;
     setOrgKeys: (orgs: ProfileOrganizationResponse[]) => Promise<{}>;
-    getKey: () => Promise<SymmetricCryptoKey>;
+    getKey: (keySuffix?: KeySuffixOptions) => Promise<SymmetricCryptoKey>;
     getKeyHash: () => Promise<string>;
     getEncKey: (key?: SymmetricCryptoKey) => Promise<SymmetricCryptoKey>;
     getPublicKey: () => Promise<ArrayBuffer>;
@@ -21,8 +22,10 @@ export abstract class CryptoService {
     getOrgKeys: () => Promise<Map<string, SymmetricCryptoKey>>;
     getOrgKey: (orgId: string) => Promise<SymmetricCryptoKey>;
     hasKey: () => Promise<boolean>;
+    hasKeyInMemory: () => boolean;
+    hasKeyStored: (keySuffix?: KeySuffixOptions) => Promise<boolean>;
     hasEncKey: () => Promise<boolean>;
-    clearKey: () => Promise<any>;
+    clearKey: (clearSecretStorage?: boolean) => Promise<any>;
     clearKeyHash: () => Promise<any>;
     clearEncKey: (memoryOnly?: boolean) => Promise<any>;
     clearKeyPair: (memoryOnly?: boolean) => Promise<any>;

common/src/abstractions/storage.service.ts

@@ -1,5 +1,12 @@
 export abstract class StorageService {
-    get: <T>(key: string) => Promise<T>;
+    get: <T>(key: string, options?: StorageServiceOptions) => Promise<T>;
-    save: (key: string, obj: any) => Promise<any>;
+    has: (key: string, options?: StorageServiceOptions) => Promise<boolean>;
-    remove: (key: string) => Promise<any>;
+    save: (key: string, obj: any, options?: StorageServiceOptions) => Promise<any>;
+    remove: (key: string, options?: StorageServiceOptions) => Promise<any>;
 }
+
+export interface StorageServiceOptions {
+    keySuffix: KeySuffixOptions;
+}
+
+export type KeySuffixOptions = 'auto' | 'biometric';

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<boolean>;
     checkVaultTimeout: () => Promise<void>;

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<any> {
         this.key = key;
 
-        if (!await this.shouldStoreKey()) {
+        if (await this.shouldStoreKey('auto')) {
-            return;
+            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<SymmetricCryptoKey> {
+    async getKey(keySuffix?: KeySuffixOptions): Promise<SymmetricCryptoKey> {
         if (this.key != null) {
             return this.key;
         }
-
+        keySuffix ||= 'auto';
-        const key = await this.secureStorageService.get<string>(Keys.key);
+        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<boolean> {
-        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<boolean> {
+        await this.upgradeSecurelyStoredKey();
+        return await this.secureStorageService.has(Keys.key, { keySuffix: keySuffix });
     }
 
     async hasEncKey(): Promise<boolean> {
@@ -255,9 +268,16 @@ export class CryptoService implements CryptoServiceAbstraction {
         return encKey != null;
     }
 
-    clearKey(): Promise<any> {
+    async clearKey(clearSecretStorage: boolean = true): Promise<any> {
         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<any> {
@@ -305,14 +325,6 @@ export class CryptoService implements CryptoServiceAbstraction {
 
     async toggleKey(): Promise<any> {
         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<string>(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() {
+    private async shouldStoreKey(keySuffix: KeySuffixOptions) {
+        let shouldStoreKey = false;
+        if (keySuffix === 'auto') {
             const vaultTimeout = await this.storageService.get<number>(ConstantsService.vaultTimeoutKey);
+            shouldStoreKey = vaultTimeout == null;
+        } else if (keySuffix === 'biometric') {
             const biometricUnlock = await this.storageService.get<boolean>(ConstantsService.biometricUnlockKey);
+            shouldStoreKey = biometricUnlock && this.platformUtilService.supportsSecureStorage();
+        }
+        return shouldStoreKey;
+    }
 
-        const biometricsEnabled = biometricUnlock && this.platformUtilService.supportsSecureStorage();
+    private async retrieveKeyFromStorage(keySuffix: KeySuffixOptions) {
-        const noVaultTimeout = vaultTimeout == null;
+        await this.upgradeSecurelyStoredKey();
 
-        return noVaultTimeout || biometricsEnabled;
+        return await this.secureStorageService.get<string>(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<string>(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<EncryptedObject> {

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();

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<boolean> {
+        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) {
+        this.manuallyOrTimerLocked = true;
-            const biometricLocked = await this.isBiometricLockSet();
+        await this.cryptoService.clearKey(false);
-            if (biometricLocked && this.platformUtilsService.supportsSecureStorage()) {
-                this.messagingService.send('locked');
-                if (this.lockedCallback != null) {
-                    await this.lockedCallback();
-                }
-                return;
-            }
-        }
-
-        await this.cryptoService.clearKey();
         await this.cryptoService.clearOrgKeys(true);
         await this.cryptoService.clearKeyPair(true);
         await this.cryptoService.clearEncKey(true);

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<boolean> {
+    async authenticateBiometric(): Promise<boolean> {
         try {
             await systemPreferences.promptTouchID(this.i18nservice.t('touchIdConsentMessage'));
             return true;

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<boolean> {
+    async authenticateBiometric(): Promise<boolean> {
         const module = this.getWindowsSecurityCredentialsUiModule();
         if (module == null) {
             return false;

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;
+                let serviceName = this.serviceName;
-                if (message.action && message.key) {
+                message.keySuffix = '_' + (message.keySuffix ?? '');
-                    if (message.action === 'getPassword') {
+                if (message.keySuffix !== '_') {
-                        val = await getPassword(this.serviceName, message.key);
+                    serviceName += message.keySuffix;
-                    } 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);
-                    }
                 }
 
+                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<boolean> {
+        if (this.biometricService) {
+            return await this.biometricService.authenticateBiometric();
+        }
+        return false;
+    }
 }

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<T>(key: string): Promise<T> {
+    async get<T>(key: string, options?: StorageServiceOptions): Promise<T> {
         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<any> {
+    async has(key: string, options?: StorageServiceOptions): Promise<boolean> {
+        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<any> {
         ipcRenderer.sendSync('keytar', {
             action: 'setPassword',
             key: key,
+            keySuffix: options?.keySuffix ?? '',
             value: JSON.stringify(obj),
         });
         return Promise.resolve();
     }
 
-    async remove(key: string): Promise<any> {
+    async remove(key: string, options?: StorageServiceOptions): Promise<any> {
         ipcRenderer.sendSync('keytar', {
             action: 'deletePassword',
             key: key,
+            keySuffix: options?.keySuffix ?? '',
         });
         return Promise.resolve();
     }

electron/src/services/electronRendererStorage.service.ts

@@ -10,6 +10,13 @@ export class ElectronRendererStorageService implements StorageService {
         });
     }
 
+    has(key: string): Promise<boolean> {
+        return ipcRenderer.invoke('storageService', {
+            action: 'has',
+            key: key,
+        });
+    }
+
     save(key: string, obj: any): Promise<any> {
         return ipcRenderer.invoke('storageService', {
             action: 'save',

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<boolean> {
+        const val = this.store.get(key);
+        return Promise.resolve(val != null);
+    }
+
     save(key: string, obj: any): Promise<any> {
         if (obj instanceof Set) {
             obj = Array.from(obj);

node/src/services/lowdbStorage.service.ts

@@ -84,6 +84,10 @@ export class LowdbStorageService implements StorageService {
         });
     }
 
+    has(key: string): Promise<boolean> {
+        return this.get(key).then(v => v != null);
+    }
+
     save(key: string, obj: any): Promise<any> {
         return this.lockDbFile(() => {
             this.readForNoCache();