bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2025-12-10 21:33:27 +00:00
Code Issues Releases Wiki Activity

[PM-14445] TS strict for Key Management KDF (#13007)

Browse Source 
* PM-14445: TS strict for Key Management KDF

* state deserializer can return null
This commit is contained in:
Maciej Zieniuk
2025-01-23 10:45:33 +01:00
committed by GitHub
parent 9f524d4b91
commit abb18881b6
10 changed files with 161 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
apps/web/src/app/vault/individual-vault/vault-banners/services/vault-banners.service.ts
View File

@@ -204,6 +204,7 @@ export class VaultBannersService {
private async isLowKdfIteration(userId: UserId) { private async isLowKdfIteration(userId: UserId) {
const kdfConfig = await firstValueFrom(this.kdfConfigService.getKdfConfig$(userId)); const kdfConfig = await firstValueFrom(this.kdfConfigService.getKdfConfig$(userId));
return ( return (
kdfConfig != null &&
kdfConfig.kdfType === KdfType.PBKDF2_SHA256 && kdfConfig.kdfType === KdfType.PBKDF2_SHA256 &&
kdfConfig.iterations < PBKDF2KdfConfig.ITERATIONS.defaultValue kdfConfig.iterations < PBKDF2KdfConfig.ITERATIONS.defaultValue
); );

2
libs/common/src/platform/state/implementations/state-base.ts
View File

@@ -28,7 +28,7 @@ import { getStoredValue } from "./util";
// The parts of a KeyDefinition this class cares about to make it work // The parts of a KeyDefinition this class cares about to make it work
type KeyDefinitionRequirements<T> = { type KeyDefinitionRequirements<T> = {
deserializer: (jsonState: Jsonify<T>) => T; deserializer: (jsonState: Jsonify<T>) => T | null;
cleanupDelayMs: number; cleanupDelayMs: number;
debug: Required<DebugOptions>; debug: Required<DebugOptions>;
}; };

5
libs/common/src/platform/state/implementations/util.ts
View File

@@ -5,12 +5,11 @@ import { AbstractStorageService } from "../../abstractions/storage.service";
export async function getStoredValue<T>( export async function getStoredValue<T>(
key: string, key: string,
storage: AbstractStorageService, storage: AbstractStorageService,
deserializer: (jsonValue: Jsonify<T>) => T, deserializer: (jsonValue: Jsonify<T>) => T | null,
) { ) {
if (storage.valuesRequireDeserialization) { if (storage.valuesRequireDeserialization) {
const jsonValue = await storage.get<Jsonify<T>>(key); const jsonValue = await storage.get<Jsonify<T>>(key);
const value = deserializer(jsonValue); return deserializer(jsonValue);
return value;
} else { } else {
const value = await storage.get<T>(key); const value = await storage.get<T>(key);
return value ?? null; return value ?? null;

2
libs/common/src/platform/state/key-definition.ts
View File

@@ -42,7 +42,7 @@ export type KeyDefinitionOptions<T> = {
* @param jsonValue The JSON object representation of your state. * @param jsonValue The JSON object representation of your state.
* @returns The fully typed version of your state. * @returns The fully typed version of your state.
*/ */
readonly deserializer: (jsonValue: Jsonify<T>) => T; readonly deserializer: (jsonValue: Jsonify<T>) => T | null;
/** /**
* The number of milliseconds to wait before cleaning up the state after the last subscriber has unsubscribed. * The number of milliseconds to wait before cleaning up the state after the last subscriber has unsubscribed.
* Defaults to 1000ms. * Defaults to 1000ms.

3
libs/key-management/package.json
View File

@@ -15,7 +15,8 @@
"scripts": { "scripts": {
"clean": "rimraf dist", "clean": "rimraf dist",
"build": "npm run clean && tsc", "build": "npm run clean && tsc",
"build:watch": "npm run clean && tsc -watch" "build:watch": "npm run clean && tsc -watch",
"test": "jest"
}, },
"dependencies": { "dependencies": {
"@bitwarden/angular": "file:../angular", "@bitwarden/angular": "file:../angular",

2
libs/key-management/src/abstractions/kdf-config.service.ts
View File

@@ -7,5 +7,5 @@ import { KdfConfig } from "../models/kdf-config";
export abstract class KdfConfigService { export abstract class KdfConfigService {
abstract setKdfConfig(userId: UserId, KdfConfig: KdfConfig): Promise<void>; abstract setKdfConfig(userId: UserId, KdfConfig: KdfConfig): Promise<void>;
abstract getKdfConfig(): Promise<KdfConfig>; abstract getKdfConfig(): Promise<KdfConfig>;
abstract getKdfConfig$(userId: UserId): Observable<KdfConfig>; abstract getKdfConfig$(userId: UserId): Observable<KdfConfig | null>;
} }

140
libs/key-management/src/kdf-config.service.spec.ts
View File

@@ -1,18 +1,15 @@
import { firstValueFrom } from "rxjs";
import { Utils } from "@bitwarden/common/platform/misc/utils";
import { import {
FakeAccountService, FakeAccountService,
FakeStateProvider, FakeStateProvider,
mockAccountServiceWith, mockAccountServiceWith,
} from "@bitwarden/common/spec"; } from "@bitwarden/common/spec";
// FIXME: remove `src` and fix import import { UserId } from "@bitwarden/common/types/guid";
// eslint-disable-next-line no-restricted-imports
import { UserId } from "@bitwarden/common/src/types/guid";
// FIXME: remove `src` and fix import import { DefaultKdfConfigService, KDF_CONFIG } from "./kdf-config.service";
// eslint-disable-next-line no-restricted-imports import { Argon2KdfConfig, KdfConfig, PBKDF2KdfConfig } from "./models/kdf-config";
import { Utils } from "../../common/src/platform/misc/utils";
import { DefaultKdfConfigService } from "./kdf-config.service";
import { Argon2KdfConfig, PBKDF2KdfConfig } from "./models/kdf-config";
describe("KdfConfigService", () => { describe("KdfConfigService", () => {
let sutKdfConfigService: DefaultKdfConfigService; let sutKdfConfigService: DefaultKdfConfigService;
@@ -29,36 +26,58 @@ describe("KdfConfigService", () => {
sutKdfConfigService = new DefaultKdfConfigService(fakeStateProvider); sutKdfConfigService = new DefaultKdfConfigService(fakeStateProvider);
}); });
it("setKdfConfig(): should set the KDF config", async () => { it("setKdfConfig(): should set the PBKDF2KdfConfig config", async () => {
const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig(600_000); const kdfConfig: KdfConfig = new PBKDF2KdfConfig(500_000);
await sutKdfConfigService.setKdfConfig(mockUserId, kdfConfig); 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 () => { it("setKdfConfig(): should set the Argon2KdfConfig config", async () => {
const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(3, 64, 4); const kdfConfig: KdfConfig = new Argon2KdfConfig(2, 63, 3);
await sutKdfConfigService.setKdfConfig(mockUserId, kdfConfig); 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 () => { it("setKdfConfig(): should throw error KDF cannot be null", async () => {
const kdfConfig: Argon2KdfConfig = null;
try { try {
await sutKdfConfigService.setKdfConfig(mockUserId, kdfConfig); await sutKdfConfigService.setKdfConfig(mockUserId, null as unknown as KdfConfig);
} catch (e) { } catch (e) {
expect(e).toEqual(new Error("kdfConfig cannot be null")); expect(e).toEqual(new Error("kdfConfig cannot be null"));
} }
}); });
it("setKdfConfig(): should throw error userId cannot be null", async () => { 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 { try {
await sutKdfConfigService.setKdfConfig(null, kdfConfig); await sutKdfConfigService.setKdfConfig(null as unknown as UserId, kdfConfig);
} catch (e) { } catch (e) {
expect(e).toEqual(new Error("userId cannot be null")); 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 () => { it("getKdfConfig(): should throw error KdfConfig for active user account state is null", async () => {
try { try {
await sutKdfConfigService.getKdfConfig(); await sutKdfConfigService.getKdfConfig();
@@ -67,75 +86,30 @@ describe("KdfConfigService", () => {
} }
}); });
it("validateKdfConfigForSetting(): should validate the PBKDF2 KDF config", () => { it("getKdfConfig$(UserId): should get KdfConfig of provided user", async () => {
const kdfConfig: PBKDF2KdfConfig = new PBKDF2KdfConfig(600_000); await expect(firstValueFrom(sutKdfConfigService.getKdfConfig$(mockUserId))).resolves.toBeNull();
expect(() => kdfConfig.validateKdfConfigForSetting()).not.toThrow(); const kdfConfig: KdfConfig = new PBKDF2KdfConfig(500_000);
}); await fakeStateProvider.setUserState(KDF_CONFIG, kdfConfig, mockUserId);
await expect(firstValueFrom(sutKdfConfigService.getKdfConfig$(mockUserId))).resolves.toEqual(
it("validateKdfConfigForSetting(): should validate the Argon2id KDF config", () => { kdfConfig,
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", () => { it("getKdfConfig$(UserId): should get KdfConfig of provided user after changed", async () => {
const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(11, 64, 4); await expect(firstValueFrom(sutKdfConfigService.getKdfConfig$(mockUserId))).resolves.toBeNull();
expect(() => kdfConfig.validateKdfConfigForSetting()).toThrow( await fakeStateProvider.setUserState(KDF_CONFIG, new PBKDF2KdfConfig(500_000), mockUserId);
`Argon2 iterations must be between ${Argon2KdfConfig.ITERATIONS.min} and ${Argon2KdfConfig.ITERATIONS.max}`, 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", () => { it("getKdfConfig$(UserId): should throw error userId cannot be null", async () => {
const kdfConfig: Argon2KdfConfig = new Argon2KdfConfig(3, 64, 17); try {
expect(() => kdfConfig.validateKdfConfigForSetting()).toThrow( sutKdfConfigService.getKdfConfig$(null as unknown as UserId);
`Argon2 parallelism must be between ${Argon2KdfConfig.PARALLELISM.min} and ${Argon2KdfConfig.PARALLELISM.max}`, } catch (e) {
); expect(e).toEqual(new Error("userId cannot be null"));
}); }
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.`,
);
}); });
}); });

33
libs/key-management/src/kdf-config.service.ts
View File

@@ -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 { firstValueFrom, Observable } from "rxjs";
import { Jsonify } from "type-fest/source/jsonify";
// FIXME: remove `src` and fix import import {
// eslint-disable-next-line no-restricted-imports KDF_CONFIG_DISK,
import { UserId } from "@bitwarden/common/src/types/guid"; StateProvider,
UserKeyDefinition,
// FIXME: remove `src` and fix import } from "@bitwarden/common/platform/state";
// eslint-disable-next-line no-restricted-imports import { UserId } from "@bitwarden/common/types/guid";
import { KDF_CONFIG_DISK, StateProvider, UserKeyDefinition } from "../../common/src/platform/state";
import { KdfConfigService } from "./abstractions/kdf-config.service"; import { KdfConfigService } from "./abstractions/kdf-config.service";
import { KdfType } from "./enums/kdf-type.enum"; import { KdfType } from "./enums/kdf-type.enum";
import { Argon2KdfConfig, KdfConfig, PBKDF2KdfConfig } from "./models/kdf-config"; import { Argon2KdfConfig, KdfConfig, PBKDF2KdfConfig } from "./models/kdf-config";
export const KDF_CONFIG = new UserKeyDefinition<KdfConfig>(KDF_CONFIG_DISK, "kdfConfig", { export const KDF_CONFIG = new UserKeyDefinition<KdfConfig>(KDF_CONFIG_DISK, "kdfConfig", {
deserializer: (kdfConfig: KdfConfig) => { deserializer: (kdfConfig: Jsonify<KdfConfig>) => {
if (kdfConfig == null) { if (kdfConfig == null) {
return null; return null;
} }
@@ -28,11 +26,12 @@ export const KDF_CONFIG = new UserKeyDefinition<KdfConfig>(KDF_CONFIG_DISK, "kdf
export class DefaultKdfConfigService implements KdfConfigService { export class DefaultKdfConfigService implements KdfConfigService {
constructor(private stateProvider: StateProvider) {} constructor(private stateProvider: StateProvider) {}
async setKdfConfig(userId: UserId, kdfConfig: KdfConfig) { async setKdfConfig(userId: UserId, kdfConfig: KdfConfig) {
if (!userId) { if (userId == null) {
throw new Error("userId cannot be null"); throw new Error("userId cannot be null");
} }
if (kdfConfig === null) { if (kdfConfig == null) {
throw new Error("kdfConfig cannot be null"); throw new Error("kdfConfig cannot be null");
} }
await this.stateProvider.setUserState(KDF_CONFIG, kdfConfig, userId); await this.stateProvider.setUserState(KDF_CONFIG, kdfConfig, userId);
@@ -40,14 +39,20 @@ export class DefaultKdfConfigService implements KdfConfigService {
async getKdfConfig(): Promise<KdfConfig> { async getKdfConfig(): Promise<KdfConfig> {
const userId = await firstValueFrom(this.stateProvider.activeUserId$); 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$); 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"); throw new Error("KdfConfig for active user account state is null");
} }
return state; return state;
} }
getKdfConfig$(userId: UserId): Observable<KdfConfig> { getKdfConfig$(userId: UserId): Observable<KdfConfig | null> {
if (userId == null) {
throw new Error("userId cannot be null");
}
return this.stateProvider.getUser(userId, KDF_CONFIG).state$; return this.stateProvider.getUser(userId, KDF_CONFIG).state$;
} }
} }

75
libs/key-management/src/models/kdf-config.spec.ts Normal file
View File

@@ -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.`,
);
});
});

5
libs/key-management/src/models/kdf-config.ts
View File

@@ -1,8 +1,7 @@
import { Jsonify } from "type-fest"; import { Jsonify } from "type-fest";
// FIXME: remove `src` and fix import import { RangeWithDefault } from "@bitwarden/common/platform/misc/range-with-default";
// eslint-disable-next-line no-restricted-imports
import { RangeWithDefault } from "../../../common/src/platform/misc/range-with-default";
import { KdfType } from "../enums/kdf-type.enum"; import { KdfType } from "../enums/kdf-type.enum";
/** /**