From 5e84c630a80321f21c65d3cb5e8ea5411d11a667 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Wed, 13 Jul 2022 07:08:07 +1000 Subject: [PATCH] [EC-135] Delay decryption of provider-encrypted org keys (#2902) --- .../services/stateMigration.service.spec.ts | 50 ++++++++++++++++- libs/common/src/abstractions/state.service.ts | 7 ++- libs/common/src/enums/stateVersion.ts | 3 +- .../data/encryptedOrganizationKeyData.ts | 14 +++++ libs/common/src/models/domain/account.ts | 8 ++- .../models/domain/encryptedOrganizationKey.ts | 56 +++++++++++++++++++ libs/common/src/services/crypto.service.ts | 50 ++++++++++------- libs/common/src/services/state.service.ts | 7 ++- .../src/services/stateMigration.service.ts | 34 +++++++++++ 9 files changed, 199 insertions(+), 30 deletions(-) create mode 100644 libs/common/src/models/data/encryptedOrganizationKeyData.ts create mode 100644 libs/common/src/models/domain/encryptedOrganizationKey.ts diff --git a/libs/common/spec/services/stateMigration.service.spec.ts b/libs/common/spec/services/stateMigration.service.spec.ts index 46fb13d70a..7ac12f51e3 100644 --- a/libs/common/spec/services/stateMigration.service.spec.ts +++ b/libs/common/spec/services/stateMigration.service.spec.ts @@ -9,6 +9,9 @@ import { StateMigrationService } from "@bitwarden/common/services/stateMigration const userId = "USER_ID"; +// Note: each test calls the private migration method for that migration, +// so that we don't accidentally run all following migrations as well + describe("State Migration Service", () => { let storageService: SubstituteOf; let secureStorageService: SubstituteOf; @@ -66,13 +69,13 @@ describe("State Migration Service", () => { storageService.get(userId, Arg.any()).resolves(accountVersion3); - await stateMigrationService.migrate(); + await (stateMigrationService as any).migrateStateFrom3To4(); storageService.received(1).save(userId, expectedAccountVersion4, Arg.any()); }); it("updates StateVersion number", async () => { - await stateMigrationService.migrate(); + await (stateMigrationService as any).migrateStateFrom3To4(); storageService.received(1).save( "global", @@ -81,4 +84,47 @@ describe("State Migration Service", () => { ); }); }); + + describe("StateVersion 4 to 5 migration", () => { + it("migrates organization keys to new format", async () => { + const accountVersion4 = new Account({ + keys: { + organizationKeys: { + encrypted: { + orgOneId: "orgOneEncKey", + orgTwoId: "orgTwoEncKey", + orgThreeId: "orgThreeEncKey", + }, + }, + }, + } as any); + + const expectedAccount = new Account({ + keys: { + organizationKeys: { + encrypted: { + orgOneId: { + type: "organization", + key: "orgOneEncKey", + }, + orgTwoId: { + type: "organization", + key: "orgTwoEncKey", + }, + orgThreeId: { + type: "organization", + key: "orgThreeEncKey", + }, + }, + }, + }, + }); + + const migratedAccount = await (stateMigrationService as any).migrateAccountFrom4To5( + accountVersion4 + ); + + expect(migratedAccount).toEqual(expectedAccount); + }); + }); }); diff --git a/libs/common/src/abstractions/state.service.ts b/libs/common/src/abstractions/state.service.ts index 5d3a7880a5..3b6758ae34 100644 --- a/libs/common/src/abstractions/state.service.ts +++ b/libs/common/src/abstractions/state.service.ts @@ -5,6 +5,7 @@ import { ThemeType } from "../enums/themeType"; import { UriMatchType } from "../enums/uriMatchType"; import { CipherData } from "../models/data/cipherData"; import { CollectionData } from "../models/data/collectionData"; +import { EncryptedOrganizationKeyData } from "../models/data/encryptedOrganizationKeyData"; import { EventData } from "../models/data/eventData"; import { FolderData } from "../models/data/folderData"; import { OrganizationData } from "../models/data/organizationData"; @@ -191,9 +192,11 @@ export abstract class StateService { value: { [id: string]: FolderData }, options?: StorageOptions ) => Promise; - getEncryptedOrganizationKeys: (options?: StorageOptions) => Promise; + getEncryptedOrganizationKeys: ( + options?: StorageOptions + ) => Promise<{ [orgId: string]: EncryptedOrganizationKeyData }>; setEncryptedOrganizationKeys: ( - value: Map, + value: { [orgId: string]: EncryptedOrganizationKeyData }, options?: StorageOptions ) => Promise; getEncryptedPasswordGenerationHistory: ( diff --git a/libs/common/src/enums/stateVersion.ts b/libs/common/src/enums/stateVersion.ts index 5aeb02e540..cc36f53f57 100644 --- a/libs/common/src/enums/stateVersion.ts +++ b/libs/common/src/enums/stateVersion.ts @@ -3,5 +3,6 @@ export enum StateVersion { Two = 2, // Move to a typed State object Three = 3, // Fix migration of users' premium status Four = 4, // Fix 'Never Lock' option by removing stale data - Latest = Four, + Five = 5, // Migrate to new storage of encrypted organization keys + Latest = Five, } diff --git a/libs/common/src/models/data/encryptedOrganizationKeyData.ts b/libs/common/src/models/data/encryptedOrganizationKeyData.ts new file mode 100644 index 0000000000..8ecbeefb81 --- /dev/null +++ b/libs/common/src/models/data/encryptedOrganizationKeyData.ts @@ -0,0 +1,14 @@ +export type EncryptedOrganizationKeyData = + | OrganizationEncryptedOrganizationKeyData + | ProviderEncryptedOrganizationKeyData; + +type OrganizationEncryptedOrganizationKeyData = { + type: "organization"; + key: string; +}; + +type ProviderEncryptedOrganizationKeyData = { + type: "provider"; + key: string; + providerId: string; +}; diff --git a/libs/common/src/models/domain/account.ts b/libs/common/src/models/domain/account.ts index 525acf1527..ff4b60345d 100644 --- a/libs/common/src/models/domain/account.ts +++ b/libs/common/src/models/domain/account.ts @@ -3,6 +3,7 @@ import { KdfType } from "../../enums/kdfType"; import { UriMatchType } from "../../enums/uriMatchType"; import { CipherData } from "../data/cipherData"; import { CollectionData } from "../data/collectionData"; +import { EncryptedOrganizationKeyData } from "../data/encryptedOrganizationKeyData"; import { EventData } from "../data/eventData"; import { FolderData } from "../data/folderData"; import { OrganizationData } from "../data/organizationData"; @@ -69,8 +70,11 @@ export class AccountKeys { string, SymmetricCryptoKey >(); - organizationKeys?: EncryptionPair> = new EncryptionPair< - any, + organizationKeys?: EncryptionPair< + { [orgId: string]: EncryptedOrganizationKeyData }, + Map + > = new EncryptionPair< + { [orgId: string]: EncryptedOrganizationKeyData }, Map >(); providerKeys?: EncryptionPair> = new EncryptionPair< diff --git a/libs/common/src/models/domain/encryptedOrganizationKey.ts b/libs/common/src/models/domain/encryptedOrganizationKey.ts new file mode 100644 index 0000000000..25a49aab14 --- /dev/null +++ b/libs/common/src/models/domain/encryptedOrganizationKey.ts @@ -0,0 +1,56 @@ +import { CryptoService } from "../../abstractions/crypto.service"; +import { EncryptedOrganizationKeyData } from "../../models/data/encryptedOrganizationKeyData"; + +import { EncString } from "./encString"; +import { SymmetricCryptoKey } from "./symmetricCryptoKey"; + +export abstract class BaseEncryptedOrganizationKey { + decrypt: (cryptoService: CryptoService) => Promise; + + static fromData(data: EncryptedOrganizationKeyData) { + switch (data.type) { + case "organization": + return new EncryptedOrganizationKey(data.key); + + case "provider": + return new ProviderEncryptedOrganizationKey(data.key, data.providerId); + + default: + return null; + } + } +} + +export class EncryptedOrganizationKey implements BaseEncryptedOrganizationKey { + constructor(private key: string) {} + + async decrypt(cryptoService: CryptoService) { + const decValue = await cryptoService.rsaDecrypt(this.key); + return new SymmetricCryptoKey(decValue); + } + + toData(): EncryptedOrganizationKeyData { + return { + type: "organization", + key: this.key, + }; + } +} + +export class ProviderEncryptedOrganizationKey implements BaseEncryptedOrganizationKey { + constructor(private key: string, private providerId: string) {} + + async decrypt(cryptoService: CryptoService) { + const providerKey = await cryptoService.getProviderKey(this.providerId); + const decValue = await cryptoService.decryptToBytes(new EncString(this.key), providerKey); + return new SymmetricCryptoKey(decValue); + } + + toData(): EncryptedOrganizationKeyData { + return { + type: "provider", + key: this.key, + providerId: this.providerId, + }; + } +} diff --git a/libs/common/src/services/crypto.service.ts b/libs/common/src/services/crypto.service.ts index d4f9bd1de1..0d625295e4 100644 --- a/libs/common/src/services/crypto.service.ts +++ b/libs/common/src/services/crypto.service.ts @@ -13,9 +13,11 @@ import { KeySuffixOptions } from "../enums/keySuffixOptions"; import { sequentialize } from "../misc/sequentialize"; import { Utils } from "../misc/utils"; import { EEFLongWordList } from "../misc/wordlist"; +import { EncryptedOrganizationKeyData } from "../models/data/encryptedOrganizationKeyData"; import { EncArrayBuffer } from "../models/domain/encArrayBuffer"; import { EncString } from "../models/domain/encString"; import { EncryptedObject } from "../models/domain/encryptedObject"; +import { BaseEncryptedOrganizationKey } from "../models/domain/encryptedOrganizationKey"; import { SymmetricCryptoKey } from "../models/domain/symmetricCryptoKey"; import { ProfileOrganizationResponse } from "../models/response/profileOrganizationResponse"; import { ProfileProviderOrganizationResponse } from "../models/response/profileProviderOrganizationResponse"; @@ -58,23 +60,28 @@ export class CryptoService implements CryptoServiceAbstraction { } async setOrgKeys( - orgs: ProfileOrganizationResponse[], - providerOrgs: ProfileProviderOrganizationResponse[] + orgs: ProfileOrganizationResponse[] = [], + providerOrgs: ProfileProviderOrganizationResponse[] = [] ): Promise { - const orgKeys: any = {}; + const encOrgKeyData: { [orgId: string]: EncryptedOrganizationKeyData } = {}; + orgs.forEach((org) => { - orgKeys[org.id] = org.key; + encOrgKeyData[org.id] = { + type: "organization", + key: org.key, + }; }); - for (const providerOrg of providerOrgs) { - // Convert provider encrypted keys to user encrypted. - const providerKey = await this.getProviderKey(providerOrg.providerId); - const decValue = await this.decryptToBytes(new EncString(providerOrg.key), providerKey); - orgKeys[providerOrg.id] = (await this.rsaEncrypt(decValue)).encryptedString; - } + providerOrgs.forEach((org) => { + encOrgKeyData[org.id] = { + type: "provider", + providerId: org.providerId, + key: org.key, + }; + }); await this.stateService.setDecryptedOrganizationKeys(null); - return await this.stateService.setEncryptedOrganizationKeys(orgKeys); + return await this.stateService.setEncryptedOrganizationKeys(encOrgKeyData); } async setProviderKeys(providers: ProfileProviderResponse[]): Promise { @@ -211,35 +218,36 @@ export class CryptoService implements CryptoServiceAbstraction { @sequentialize(() => "getOrgKeys") async getOrgKeys(): Promise> { - const orgKeys: Map = new Map(); + const result: Map = new Map(); const decryptedOrganizationKeys = await this.stateService.getDecryptedOrganizationKeys(); if (decryptedOrganizationKeys != null && decryptedOrganizationKeys.size > 0) { return decryptedOrganizationKeys; } - const encOrgKeys = await this.stateService.getEncryptedOrganizationKeys(); - if (encOrgKeys == null) { + const encOrgKeyData = await this.stateService.getEncryptedOrganizationKeys(); + if (encOrgKeyData == null) { return null; } let setKey = false; - for (const orgId in encOrgKeys) { - // eslint-disable-next-line - if (!encOrgKeys.hasOwnProperty(orgId)) { + for (const orgId of Object.keys(encOrgKeyData)) { + if (result.has(orgId)) { continue; } - const decValue = await this.rsaDecrypt(encOrgKeys[orgId]); - orgKeys.set(orgId, new SymmetricCryptoKey(decValue)); + const encOrgKey = BaseEncryptedOrganizationKey.fromData(encOrgKeyData[orgId]); + const decOrgKey = await encOrgKey.decrypt(this); + result.set(orgId, decOrgKey); + setKey = true; } if (setKey) { - await this.stateService.setDecryptedOrganizationKeys(orgKeys); + await this.stateService.setDecryptedOrganizationKeys(result); } - return orgKeys; + return result; } async getOrgKey(orgId: string): Promise { diff --git a/libs/common/src/services/state.service.ts b/libs/common/src/services/state.service.ts index d4901810f3..332e482934 100644 --- a/libs/common/src/services/state.service.ts +++ b/libs/common/src/services/state.service.ts @@ -13,6 +13,7 @@ import { StateFactory } from "../factories/stateFactory"; import { Utils } from "../misc/utils"; import { CipherData } from "../models/data/cipherData"; import { CollectionData } from "../models/data/collectionData"; +import { EncryptedOrganizationKeyData } from "../models/data/encryptedOrganizationKeyData"; import { EventData } from "../models/data/eventData"; import { FolderData } from "../models/data/folderData"; import { OrganizationData } from "../models/data/organizationData"; @@ -1344,14 +1345,16 @@ export class StateService< ); } - async getEncryptedOrganizationKeys(options?: StorageOptions): Promise { + async getEncryptedOrganizationKeys( + options?: StorageOptions + ): Promise<{ [orgId: string]: EncryptedOrganizationKeyData }> { return ( await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())) )?.keys?.organizationKeys.encrypted; } async setEncryptedOrganizationKeys( - value: Map, + value: { [orgId: string]: EncryptedOrganizationKeyData }, options?: StorageOptions ): Promise { const account = await this.getAccount( diff --git a/libs/common/src/services/stateMigration.service.ts b/libs/common/src/services/stateMigration.service.ts index 0fc38aa297..0d723e9847 100644 --- a/libs/common/src/services/stateMigration.service.ts +++ b/libs/common/src/services/stateMigration.service.ts @@ -155,6 +155,15 @@ export class StateMigrationService< case StateVersion.Three: await this.migrateStateFrom3To4(); break; + case StateVersion.Four: { + const authenticatedAccounts = await this.getAuthenticatedAccounts(); + for (const account of authenticatedAccounts) { + const migratedAccount = await this.migrateAccountFrom4To5(account); + await this.set(account.profile.userId, migratedAccount); + } + await this.setCurrentStateVersion(StateVersion.Five); + break; + } } currentStateVersion += 1; @@ -488,6 +497,20 @@ export class StateMigrationService< await this.set(keys.global, globals); } + protected async migrateAccountFrom4To5(account: TAccount): Promise { + const encryptedOrgKeys = account.keys?.organizationKeys?.encrypted; + if (encryptedOrgKeys != null) { + for (const [orgId, encKey] of Object.entries(encryptedOrgKeys)) { + encryptedOrgKeys[orgId] = { + type: "organization", + key: encKey as unknown as string, // Account v4 does not reflect the current account model so we have to cast + }; + } + } + + return account; + } + protected get options(): StorageOptions { return { htmlStorageLocation: HtmlStorageLocation.Local }; } @@ -510,4 +533,15 @@ export class StateMigrationService< protected async getCurrentStateVersion(): Promise { return (await this.getGlobals())?.stateVersion ?? StateVersion.One; } + + protected async setCurrentStateVersion(newVersion: StateVersion): Promise { + const globals = await this.getGlobals(); + globals.stateVersion = newVersion; + await this.set(keys.global, globals); + } + + protected async getAuthenticatedAccounts(): Promise { + const authenticatedUserIds = await this.get(keys.authenticatedAccounts); + return Promise.all(authenticatedUserIds.map((id) => this.get(id))); + } }