diff --git a/apps/web/src/app/vault/individual-vault/vault-banners/services/vault-banners.service.ts b/apps/web/src/app/vault/individual-vault/vault-banners/services/vault-banners.service.ts index 390b95fa2b1..475cfc2df22 100644 --- a/apps/web/src/app/vault/individual-vault/vault-banners/services/vault-banners.service.ts +++ b/apps/web/src/app/vault/individual-vault/vault-banners/services/vault-banners.service.ts @@ -204,6 +204,7 @@ export class VaultBannersService { private async isLowKdfIteration(userId: UserId) { const kdfConfig = await firstValueFrom(this.kdfConfigService.getKdfConfig$(userId)); return ( + kdfConfig != null && kdfConfig.kdfType === KdfType.PBKDF2_SHA256 && kdfConfig.iterations < PBKDF2KdfConfig.ITERATIONS.defaultValue ); diff --git a/libs/common/src/platform/state/implementations/state-base.ts b/libs/common/src/platform/state/implementations/state-base.ts index 567de957e53..f285963d6e6 100644 --- a/libs/common/src/platform/state/implementations/state-base.ts +++ b/libs/common/src/platform/state/implementations/state-base.ts @@ -28,7 +28,7 @@ import { getStoredValue } from "./util"; // The parts of a KeyDefinition this class cares about to make it work type KeyDefinitionRequirements = { - deserializer: (jsonState: Jsonify) => T; + deserializer: (jsonState: Jsonify) => T | null; cleanupDelayMs: number; debug: Required; }; diff --git a/libs/common/src/platform/state/implementations/util.ts b/libs/common/src/platform/state/implementations/util.ts index f3d57fbafc4..0a9d76f6da5 100644 --- a/libs/common/src/platform/state/implementations/util.ts +++ b/libs/common/src/platform/state/implementations/util.ts @@ -5,12 +5,11 @@ import { AbstractStorageService } from "../../abstractions/storage.service"; export async function getStoredValue( key: string, storage: AbstractStorageService, - deserializer: (jsonValue: Jsonify) => T, + deserializer: (jsonValue: Jsonify) => T | null, ) { if (storage.valuesRequireDeserialization) { const jsonValue = await storage.get>(key); - const value = deserializer(jsonValue); - return value; + return deserializer(jsonValue); } else { const value = await storage.get(key); return value ?? null; diff --git a/libs/common/src/platform/state/key-definition.ts b/libs/common/src/platform/state/key-definition.ts index a270fc3e1a2..519e98ef52d 100644 --- a/libs/common/src/platform/state/key-definition.ts +++ b/libs/common/src/platform/state/key-definition.ts @@ -42,7 +42,7 @@ export type KeyDefinitionOptions = { * @param jsonValue The JSON object representation of your state. * @returns The fully typed version of your state. */ - readonly deserializer: (jsonValue: Jsonify) => T; + readonly deserializer: (jsonValue: Jsonify) => T | null; /** * The number of milliseconds to wait before cleaning up the state after the last subscriber has unsubscribed. * Defaults to 1000ms. diff --git a/libs/key-management/package.json b/libs/key-management/package.json index 083386cbc81..1063e16580e 100644 --- a/libs/key-management/package.json +++ b/libs/key-management/package.json @@ -15,7 +15,8 @@ "scripts": { "clean": "rimraf dist", "build": "npm run clean && tsc", - "build:watch": "npm run clean && tsc -watch" + "build:watch": "npm run clean && tsc -watch", + "test": "jest" }, "dependencies": { "@bitwarden/angular": "file:../angular", diff --git a/libs/key-management/src/abstractions/kdf-config.service.ts b/libs/key-management/src/abstractions/kdf-config.service.ts index d5f613828d4..9cc39561aa8 100644 --- a/libs/key-management/src/abstractions/kdf-config.service.ts +++ b/libs/key-management/src/abstractions/kdf-config.service.ts @@ -7,5 +7,5 @@ import { KdfConfig } from "../models/kdf-config"; export abstract class KdfConfigService { abstract setKdfConfig(userId: UserId, KdfConfig: KdfConfig): Promise; abstract getKdfConfig(): Promise; - abstract getKdfConfig$(userId: UserId): Observable; + abstract getKdfConfig$(userId: UserId): Observable; } diff --git a/libs/key-management/src/kdf-config.service.spec.ts b/libs/key-management/src/kdf-config.service.spec.ts index 76a13fe10fc..986d7abac40 100644 --- a/libs/key-management/src/kdf-config.service.spec.ts +++ b/libs/key-management/src/kdf-config.service.spec.ts @@ -1,18 +1,15 @@ +import { firstValueFrom } from "rxjs"; + +import { Utils } from "@bitwarden/common/platform/misc/utils"; import { FakeAccountService, FakeStateProvider, mockAccountServiceWith, } from "@bitwarden/common/spec"; -// FIXME: remove `src` and fix import -// eslint-disable-next-line no-restricted-imports -import { UserId } from "@bitwarden/common/src/types/guid"; +import { UserId } from "@bitwarden/common/types/guid"; -// FIXME: remove `src` and fix import -// eslint-disable-next-line no-restricted-imports -import { Utils } from "../../common/src/platform/misc/utils"; - -import { DefaultKdfConfigService } from "./kdf-config.service"; -import { Argon2KdfConfig, PBKDF2KdfConfig } from "./models/kdf-config"; +import { DefaultKdfConfigService, KDF_CONFIG } from "./kdf-config.service"; +import { Argon2KdfConfig, KdfConfig, PBKDF2KdfConfig } from "./models/kdf-config"; describe("KdfConfigService", () => { let sutKdfConfigService: DefaultKdfConfigService; @@ -29,36 +26,58 @@ describe("KdfConfigService", () => { sutKdfConfigService = new DefaultKdfConfigService(fakeStateProvider); }); - it("setKdfConfig(): should set the KDF config", async () => { - const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig(600_000); + it("setKdfConfig(): should set the PBKDF2KdfConfig config", async () => { + const kdfConfig: KdfConfig = new PBKDF2KdfConfig(500_000); await sutKdfConfigService.setKdfConfig(mockUserId, kdfConfig); - await expect(sutKdfConfigService.getKdfConfig()).resolves.toEqual(kdfConfig); + expect(fakeStateProvider.mock.setUserState).toHaveBeenCalledWith( + KDF_CONFIG, + kdfConfig, + mockUserId, + ); }); - it("setKdfConfig(): should get the KDF config", async () => { - const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(3, 64, 4); + it("setKdfConfig(): should set the Argon2KdfConfig config", async () => { + const kdfConfig: KdfConfig = new Argon2KdfConfig(2, 63, 3); await sutKdfConfigService.setKdfConfig(mockUserId, kdfConfig); - await expect(sutKdfConfigService.getKdfConfig()).resolves.toEqual(kdfConfig); + expect(fakeStateProvider.mock.setUserState).toHaveBeenCalledWith( + KDF_CONFIG, + kdfConfig, + mockUserId, + ); }); it("setKdfConfig(): should throw error KDF cannot be null", async () => { - const kdfConfig: Argon2KdfConfig = null; try { - await sutKdfConfigService.setKdfConfig(mockUserId, kdfConfig); + await sutKdfConfigService.setKdfConfig(mockUserId, null as unknown as KdfConfig); } catch (e) { expect(e).toEqual(new Error("kdfConfig cannot be null")); } }); it("setKdfConfig(): should throw error userId cannot be null", async () => { - const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(3, 64, 4); + const kdfConfig: KdfConfig = new Argon2KdfConfig(3, 64, 4); try { - await sutKdfConfigService.setKdfConfig(null, kdfConfig); + await sutKdfConfigService.setKdfConfig(null as unknown as UserId, kdfConfig); } catch (e) { expect(e).toEqual(new Error("userId cannot be null")); } }); + it("getKdfConfig(): should get KdfConfig of active user", async () => { + const kdfConfig: KdfConfig = new PBKDF2KdfConfig(500_000); + await fakeStateProvider.setUserState(KDF_CONFIG, kdfConfig, mockUserId); + await expect(sutKdfConfigService.getKdfConfig()).resolves.toEqual(kdfConfig); + }); + + it("getKdfConfig(): should throw error KdfConfig can only be retrieved when there is active user", async () => { + fakeAccountService.activeAccountSubject.next(null); + try { + await sutKdfConfigService.getKdfConfig(); + } catch (e) { + expect(e).toEqual(new Error("KdfConfig can only be retrieved when there is active user")); + } + }); + it("getKdfConfig(): should throw error KdfConfig for active user account state is null", async () => { try { await sutKdfConfigService.getKdfConfig(); @@ -67,75 +86,30 @@ describe("KdfConfigService", () => { } }); - it("validateKdfConfigForSetting(): should validate the PBKDF2 KDF config", () => { - const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig(600_000); - expect(() => kdfConfig.validateKdfConfigForSetting()).not.toThrow(); - }); - - it("validateKdfConfigForSetting(): should validate the Argon2id KDF config", () => { - const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(3, 64, 4); - expect(() => kdfConfig.validateKdfConfigForSetting()).not.toThrow(); - }); - - it("validateKdfConfigForSetting(): should throw an error for invalid PBKDF2 iterations", () => { - const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig(100000); - expect(() => kdfConfig.validateKdfConfigForSetting()).toThrow( - `PBKDF2 iterations must be between ${PBKDF2KdfConfig.ITERATIONS.min} and ${PBKDF2KdfConfig.ITERATIONS.max}`, + it("getKdfConfig$(UserId): should get KdfConfig of provided user", async () => { + await expect(firstValueFrom(sutKdfConfigService.getKdfConfig$(mockUserId))).resolves.toBeNull(); + const kdfConfig: KdfConfig = new PBKDF2KdfConfig(500_000); + await fakeStateProvider.setUserState(KDF_CONFIG, kdfConfig, mockUserId); + await expect(firstValueFrom(sutKdfConfigService.getKdfConfig$(mockUserId))).resolves.toEqual( + kdfConfig, ); }); - it("validateKdfConfigForSetting(): should throw an error for invalid Argon2 iterations", () => { - const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(11, 64, 4); - expect(() => kdfConfig.validateKdfConfigForSetting()).toThrow( - `Argon2 iterations must be between ${Argon2KdfConfig.ITERATIONS.min} and ${Argon2KdfConfig.ITERATIONS.max}`, + it("getKdfConfig$(UserId): should get KdfConfig of provided user after changed", async () => { + await expect(firstValueFrom(sutKdfConfigService.getKdfConfig$(mockUserId))).resolves.toBeNull(); + await fakeStateProvider.setUserState(KDF_CONFIG, new PBKDF2KdfConfig(500_000), mockUserId); + const kdfConfigChanged: KdfConfig = new PBKDF2KdfConfig(500_001); + await fakeStateProvider.setUserState(KDF_CONFIG, kdfConfigChanged, mockUserId); + await expect(firstValueFrom(sutKdfConfigService.getKdfConfig$(mockUserId))).resolves.toEqual( + kdfConfigChanged, ); }); - it("validateKdfConfigForSetting(): should throw an error for invalid Argon2 parallelism", () => { - const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(3, 64, 17); - expect(() => kdfConfig.validateKdfConfigForSetting()).toThrow( - `Argon2 parallelism must be between ${Argon2KdfConfig.PARALLELISM.min} and ${Argon2KdfConfig.PARALLELISM.max}`, - ); - }); - - it("validateKdfConfigForPrelogin(): should validate the PBKDF2 KDF config", () => { - const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig(600_000); - expect(() => kdfConfig.validateKdfConfigForPrelogin()).not.toThrow(); - }); - - it("validateKdfConfigForPrelogin(): should validate the Argon2id KDF config", () => { - const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(3, 64, 4); - expect(() => kdfConfig.validateKdfConfigForPrelogin()).not.toThrow(); - }); - - it("validateKdfConfigForPrelogin(): should throw an error for too low PBKDF2 iterations", () => { - const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig( - PBKDF2KdfConfig.PRELOGIN_ITERATIONS_MIN - 1, - ); - expect(() => kdfConfig.validateKdfConfigForPrelogin()).toThrow( - `PBKDF2 iterations must be at least ${PBKDF2KdfConfig.PRELOGIN_ITERATIONS_MIN}, but was ${kdfConfig.iterations}; possible pre-login downgrade attack detected.`, - ); - }); - - it("validateKdfConfigForPrelogin(): should throw an error for too low Argon2 iterations", () => { - const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig( - Argon2KdfConfig.PRELOGIN_ITERATIONS_MIN - 1, - 64, - 4, - ); - expect(() => kdfConfig.validateKdfConfigForPrelogin()).toThrow( - `Argon2 iterations must be at least ${Argon2KdfConfig.PRELOGIN_ITERATIONS_MIN}, but was ${kdfConfig.iterations}; possible pre-login downgrade attack detected.`, - ); - }); - - it("validateKdfConfigForPrelogin(): should throw an error for too low Argon2 memory", () => { - const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig( - 3, - Argon2KdfConfig.PRELOGIN_MEMORY_MIN - 1, - 4, - ); - expect(() => kdfConfig.validateKdfConfigForPrelogin()).toThrow( - `Argon2 memory must be at least ${Argon2KdfConfig.PRELOGIN_MEMORY_MIN} MiB, but was ${kdfConfig.memory} MiB; possible pre-login downgrade attack detected.`, - ); + it("getKdfConfig$(UserId): should throw error userId cannot be null", async () => { + try { + sutKdfConfigService.getKdfConfig$(null as unknown as UserId); + } catch (e) { + expect(e).toEqual(new Error("userId cannot be null")); + } }); }); diff --git a/libs/key-management/src/kdf-config.service.ts b/libs/key-management/src/kdf-config.service.ts index f0c964c5d2e..efc5310e5a8 100644 --- a/libs/key-management/src/kdf-config.service.ts +++ b/libs/key-management/src/kdf-config.service.ts @@ -1,21 +1,19 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { firstValueFrom, Observable } from "rxjs"; +import { Jsonify } from "type-fest/source/jsonify"; -// FIXME: remove `src` and fix import -// eslint-disable-next-line no-restricted-imports -import { UserId } from "@bitwarden/common/src/types/guid"; - -// FIXME: remove `src` and fix import -// eslint-disable-next-line no-restricted-imports -import { KDF_CONFIG_DISK, StateProvider, UserKeyDefinition } from "../../common/src/platform/state"; +import { + KDF_CONFIG_DISK, + StateProvider, + UserKeyDefinition, +} from "@bitwarden/common/platform/state"; +import { UserId } from "@bitwarden/common/types/guid"; import { KdfConfigService } from "./abstractions/kdf-config.service"; import { KdfType } from "./enums/kdf-type.enum"; import { Argon2KdfConfig, KdfConfig, PBKDF2KdfConfig } from "./models/kdf-config"; export const KDF_CONFIG = new UserKeyDefinition(KDF_CONFIG_DISK, "kdfConfig", { - deserializer: (kdfConfig: KdfConfig) => { + deserializer: (kdfConfig: Jsonify) => { if (kdfConfig == null) { return null; } @@ -28,11 +26,12 @@ export const KDF_CONFIG = new UserKeyDefinition(KDF_CONFIG_DISK, "kdf export class DefaultKdfConfigService implements KdfConfigService { constructor(private stateProvider: StateProvider) {} + async setKdfConfig(userId: UserId, kdfConfig: KdfConfig) { - if (!userId) { + if (userId == null) { throw new Error("userId cannot be null"); } - if (kdfConfig === null) { + if (kdfConfig == null) { throw new Error("kdfConfig cannot be null"); } await this.stateProvider.setUserState(KDF_CONFIG, kdfConfig, userId); @@ -40,14 +39,20 @@ export class DefaultKdfConfigService implements KdfConfigService { async getKdfConfig(): Promise { const userId = await firstValueFrom(this.stateProvider.activeUserId$); + if (userId == null) { + throw new Error("KdfConfig can only be retrieved when there is active user"); + } const state = await firstValueFrom(this.stateProvider.getUser(userId, KDF_CONFIG).state$); - if (state === null) { + if (state == null) { throw new Error("KdfConfig for active user account state is null"); } return state; } - getKdfConfig$(userId: UserId): Observable { + getKdfConfig$(userId: UserId): Observable { + if (userId == null) { + throw new Error("userId cannot be null"); + } return this.stateProvider.getUser(userId, KDF_CONFIG).state$; } } diff --git a/libs/key-management/src/models/kdf-config.spec.ts b/libs/key-management/src/models/kdf-config.spec.ts new file mode 100644 index 00000000000..347f574fc00 --- /dev/null +++ b/libs/key-management/src/models/kdf-config.spec.ts @@ -0,0 +1,75 @@ +import { Argon2KdfConfig, PBKDF2KdfConfig } from "./kdf-config"; + +describe("KdfConfig", () => { + it("validateKdfConfigForSetting(): should validate the PBKDF2 KDF config", () => { + const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig(600_000); + expect(() => kdfConfig.validateKdfConfigForSetting()).not.toThrow(); + }); + + it("validateKdfConfigForSetting(): should validate the Argon2id KDF config", () => { + const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(3, 64, 4); + expect(() => kdfConfig.validateKdfConfigForSetting()).not.toThrow(); + }); + + it("validateKdfConfigForSetting(): should throw an error for invalid PBKDF2 iterations", () => { + const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig(100000); + expect(() => kdfConfig.validateKdfConfigForSetting()).toThrow( + `PBKDF2 iterations must be between ${PBKDF2KdfConfig.ITERATIONS.min} and ${PBKDF2KdfConfig.ITERATIONS.max}`, + ); + }); + + it("validateKdfConfigForSetting(): should throw an error for invalid Argon2 iterations", () => { + const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(11, 64, 4); + expect(() => kdfConfig.validateKdfConfigForSetting()).toThrow( + `Argon2 iterations must be between ${Argon2KdfConfig.ITERATIONS.min} and ${Argon2KdfConfig.ITERATIONS.max}`, + ); + }); + + it("validateKdfConfigForSetting(): should throw an error for invalid Argon2 parallelism", () => { + const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(3, 64, 17); + expect(() => kdfConfig.validateKdfConfigForSetting()).toThrow( + `Argon2 parallelism must be between ${Argon2KdfConfig.PARALLELISM.min} and ${Argon2KdfConfig.PARALLELISM.max}`, + ); + }); + + it("validateKdfConfigForPrelogin(): should validate the PBKDF2 KDF config", () => { + const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig(600_000); + expect(() => kdfConfig.validateKdfConfigForPrelogin()).not.toThrow(); + }); + + it("validateKdfConfigForPrelogin(): should validate the Argon2id KDF config", () => { + const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(3, 64, 4); + expect(() => kdfConfig.validateKdfConfigForPrelogin()).not.toThrow(); + }); + + it("validateKdfConfigForPrelogin(): should throw an error for too low PBKDF2 iterations", () => { + const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig( + PBKDF2KdfConfig.PRELOGIN_ITERATIONS_MIN - 1, + ); + expect(() => kdfConfig.validateKdfConfigForPrelogin()).toThrow( + `PBKDF2 iterations must be at least ${PBKDF2KdfConfig.PRELOGIN_ITERATIONS_MIN}, but was ${kdfConfig.iterations}; possible pre-login downgrade attack detected.`, + ); + }); + + it("validateKdfConfigForPrelogin(): should throw an error for too low Argon2 iterations", () => { + const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig( + Argon2KdfConfig.PRELOGIN_ITERATIONS_MIN - 1, + 64, + 4, + ); + expect(() => kdfConfig.validateKdfConfigForPrelogin()).toThrow( + `Argon2 iterations must be at least ${Argon2KdfConfig.PRELOGIN_ITERATIONS_MIN}, but was ${kdfConfig.iterations}; possible pre-login downgrade attack detected.`, + ); + }); + + it("validateKdfConfigForPrelogin(): should throw an error for too low Argon2 memory", () => { + const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig( + 3, + Argon2KdfConfig.PRELOGIN_MEMORY_MIN - 1, + 4, + ); + expect(() => kdfConfig.validateKdfConfigForPrelogin()).toThrow( + `Argon2 memory must be at least ${Argon2KdfConfig.PRELOGIN_MEMORY_MIN} MiB, but was ${kdfConfig.memory} MiB; possible pre-login downgrade attack detected.`, + ); + }); +}); diff --git a/libs/key-management/src/models/kdf-config.ts b/libs/key-management/src/models/kdf-config.ts index fe9c0de255c..689da77c163 100644 --- a/libs/key-management/src/models/kdf-config.ts +++ b/libs/key-management/src/models/kdf-config.ts @@ -1,8 +1,7 @@ import { Jsonify } from "type-fest"; -// FIXME: remove `src` and fix import -// eslint-disable-next-line no-restricted-imports -import { RangeWithDefault } from "../../../common/src/platform/misc/range-with-default"; +import { RangeWithDefault } from "@bitwarden/common/platform/misc/range-with-default"; + import { KdfType } from "../enums/kdf-type.enum"; /**