From 7bbdee9daa5cd1612026b2015d0189d01c98a1e4 Mon Sep 17 00:00:00 2001 From: Oscar Hinton Date: Tue, 5 Dec 2023 16:55:12 +0100 Subject: [PATCH] [PM-3565] Enforce higher minimum KDF (#6440) Changes minimum iterations for PBKDF2 to 600 000. Also converts the constants into ranges to ensure there is only a single place for all checks. --- .../change-kdf-confirmation.component.ts | 4 ++ .../change-kdf/change-kdf.component.html | 18 ++--- .../change-kdf/change-kdf.component.ts | 23 ++++--- .../vault/individual-vault/vault.component.ts | 6 +- .../platform/abstractions/crypto.service.ts | 8 +++ .../src/platform/enums/kdf-type.enum.ts | 11 +-- .../platform/misc/range-with-default.spec.ts | 26 +++++++ .../src/platform/misc/range-with-default.ts | 24 +++++++ .../src/platform/services/crypto.service.ts | 69 ++++++++++++++----- .../services/vault-export.service.spec.ts | 6 +- 10 files changed, 150 insertions(+), 45 deletions(-) create mode 100644 libs/common/src/platform/misc/range-with-default.spec.ts create mode 100644 libs/common/src/platform/misc/range-with-default.ts diff --git a/apps/web/src/app/auth/settings/security/change-kdf/change-kdf-confirmation.component.ts b/apps/web/src/app/auth/settings/security/change-kdf/change-kdf-confirmation.component.ts index 8e27e6e8f9..0284c665d8 100644 --- a/apps/web/src/app/auth/settings/security/change-kdf/change-kdf-confirmation.component.ts +++ b/apps/web/src/app/auth/settings/security/change-kdf/change-kdf-confirmation.component.ts @@ -73,6 +73,10 @@ export class ChangeKdfConfirmationComponent { const masterKey = await this.cryptoService.getOrDeriveMasterKey(masterPassword); request.masterPasswordHash = await this.cryptoService.hashMasterKey(masterPassword, masterKey); const email = await this.stateService.getEmail(); + + // Ensure the KDF config is valid. + this.cryptoService.validateKdfConfig(this.kdf, this.kdfConfig); + const newMasterKey = await this.cryptoService.makeMasterKey( masterPassword, email, diff --git a/apps/web/src/app/auth/settings/security/change-kdf/change-kdf.component.html b/apps/web/src/app/auth/settings/security/change-kdf/change-kdf.component.html index 0411c37380..5ffe3a5f19 100644 --- a/apps/web/src/app/auth/settings/security/change-kdf/change-kdf.component.html +++ b/apps/web/src/app/auth/settings/security/change-kdf/change-kdf.component.html @@ -31,8 +31,8 @@

- {{ "kdfIterationsDesc" | i18n: (recommendedPbkdf2Iterations | number) }} + {{ "kdfIterationsDesc" | i18n: (PBKDF2_ITERATIONS.defaultValue | number) }}

{{ "kdfIterationsWarning" | i18n: (100000 | number) }} diff --git a/apps/web/src/app/auth/settings/security/change-kdf/change-kdf.component.ts b/apps/web/src/app/auth/settings/security/change-kdf/change-kdf.component.ts index 02bcfa7b19..d91fb8d083 100644 --- a/apps/web/src/app/auth/settings/security/change-kdf/change-kdf.component.ts +++ b/apps/web/src/app/auth/settings/security/change-kdf/change-kdf.component.ts @@ -4,10 +4,10 @@ import { KdfConfig } from "@bitwarden/common/auth/models/domain/kdf-config"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { DEFAULT_KDF_CONFIG, - DEFAULT_PBKDF2_ITERATIONS, - DEFAULT_ARGON2_ITERATIONS, - DEFAULT_ARGON2_MEMORY, - DEFAULT_ARGON2_PARALLELISM, + PBKDF2_ITERATIONS, + ARGON2_ITERATIONS, + ARGON2_MEMORY, + ARGON2_PARALLELISM, KdfType, } from "@bitwarden/common/platform/enums"; import { DialogService } from "@bitwarden/components"; @@ -23,7 +23,12 @@ export class ChangeKdfComponent implements OnInit { kdfConfig: KdfConfig = DEFAULT_KDF_CONFIG; kdfType = KdfType; kdfOptions: any[] = []; - recommendedPbkdf2Iterations = DEFAULT_PBKDF2_ITERATIONS; + + // Default values for template + protected PBKDF2_ITERATIONS = PBKDF2_ITERATIONS; + protected ARGON2_ITERATIONS = ARGON2_ITERATIONS; + protected ARGON2_MEMORY = ARGON2_MEMORY; + protected ARGON2_PARALLELISM = ARGON2_PARALLELISM; constructor( private stateService: StateService, @@ -42,12 +47,12 @@ export class ChangeKdfComponent implements OnInit { async onChangeKdf(newValue: KdfType) { if (newValue === KdfType.PBKDF2_SHA256) { - this.kdfConfig = new KdfConfig(DEFAULT_PBKDF2_ITERATIONS); + this.kdfConfig = new KdfConfig(PBKDF2_ITERATIONS.defaultValue); } else if (newValue === KdfType.Argon2id) { this.kdfConfig = new KdfConfig( - DEFAULT_ARGON2_ITERATIONS, - DEFAULT_ARGON2_MEMORY, - DEFAULT_ARGON2_PARALLELISM, + ARGON2_ITERATIONS.defaultValue, + ARGON2_MEMORY.defaultValue, + ARGON2_PARALLELISM.defaultValue, ); } else { throw new Error("Unknown KDF type."); diff --git a/apps/web/src/app/vault/individual-vault/vault.component.ts b/apps/web/src/app/vault/individual-vault/vault.component.ts index 77fdd1be44..764f5cd6a1 100644 --- a/apps/web/src/app/vault/individual-vault/vault.component.ts +++ b/apps/web/src/app/vault/individual-vault/vault.component.ts @@ -47,7 +47,7 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service" import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; -import { DEFAULT_PBKDF2_ITERATIONS, KdfType } from "@bitwarden/common/platform/enums"; +import { KdfType, PBKDF2_ITERATIONS } from "@bitwarden/common/platform/enums"; import { Utils } from "@bitwarden/common/platform/misc/utils"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CollectionService } from "@bitwarden/common/vault/abstractions/collection.service"; @@ -967,7 +967,9 @@ export class VaultComponent implements OnInit, OnDestroy { async isLowKdfIteration() { const kdfType = await this.stateService.getKdfType(); const kdfOptions = await this.stateService.getKdfConfig(); - return kdfType === KdfType.PBKDF2_SHA256 && kdfOptions.iterations < DEFAULT_PBKDF2_ITERATIONS; + return ( + kdfType === KdfType.PBKDF2_SHA256 && kdfOptions.iterations < PBKDF2_ITERATIONS.defaultValue + ); } protected async repromptCipher(ciphers: CipherView[]) { diff --git a/libs/common/src/platform/abstractions/crypto.service.ts b/libs/common/src/platform/abstractions/crypto.service.ts index 53b65311c8..52849c538a 100644 --- a/libs/common/src/platform/abstractions/crypto.service.ts +++ b/libs/common/src/platform/abstractions/crypto.service.ts @@ -390,6 +390,14 @@ export abstract class CryptoService { privateKey: EncString; }>; + /** + * Validate that the KDF config follows the requirements for the given KDF type. + * + * @remarks + * Should always be called before updating a users KDF config. + */ + validateKdfConfig: (kdf: KdfType, kdfConfig: KdfConfig) => void; + /** * @deprecated Left for migration purposes. Use decryptUserKeyWithPin instead. */ diff --git a/libs/common/src/platform/enums/kdf-type.enum.ts b/libs/common/src/platform/enums/kdf-type.enum.ts index 13f671dfda..97157910f5 100644 --- a/libs/common/src/platform/enums/kdf-type.enum.ts +++ b/libs/common/src/platform/enums/kdf-type.enum.ts @@ -1,14 +1,15 @@ import { KdfConfig } from "../../auth/models/domain/kdf-config"; +import { RangeWithDefault } from "../misc/range-with-default"; export enum KdfType { PBKDF2_SHA256 = 0, Argon2id = 1, } -export const DEFAULT_ARGON2_MEMORY = 64; -export const DEFAULT_ARGON2_PARALLELISM = 4; -export const DEFAULT_ARGON2_ITERATIONS = 3; +export const ARGON2_MEMORY = new RangeWithDefault(16, 1024, 64); +export const ARGON2_PARALLELISM = new RangeWithDefault(1, 16, 4); +export const ARGON2_ITERATIONS = new RangeWithDefault(2, 10, 3); export const DEFAULT_KDF_TYPE = KdfType.PBKDF2_SHA256; -export const DEFAULT_PBKDF2_ITERATIONS = 600000; -export const DEFAULT_KDF_CONFIG = new KdfConfig(DEFAULT_PBKDF2_ITERATIONS); +export const PBKDF2_ITERATIONS = new RangeWithDefault(600_000, 2_000_000, 600_000); +export const DEFAULT_KDF_CONFIG = new KdfConfig(PBKDF2_ITERATIONS.defaultValue); diff --git a/libs/common/src/platform/misc/range-with-default.spec.ts b/libs/common/src/platform/misc/range-with-default.spec.ts new file mode 100644 index 0000000000..b400439c60 --- /dev/null +++ b/libs/common/src/platform/misc/range-with-default.spec.ts @@ -0,0 +1,26 @@ +import { RangeWithDefault } from "./range-with-default"; + +describe("RangeWithDefault", () => { + describe("constructor", () => { + it("should throw an error when min is greater than max", () => { + expect(() => new RangeWithDefault(10, 5, 0)).toThrowError("10 is greater than 5."); + }); + + it("should throw an error when default value is not in range", () => { + expect(() => new RangeWithDefault(0, 10, 20)).toThrowError("Default value is not in range."); + }); + }); + + describe("inRange", () => { + it("should return true when in range", () => { + const range = new RangeWithDefault(0, 10, 5); + expect(range.inRange(5)).toBe(true); + }); + + it("should return false when not in range", () => { + const range = new RangeWithDefault(5, 10, 7); + expect(range.inRange(1)).toBe(false); + expect(range.inRange(20)).toBe(false); + }); + }); +}); diff --git a/libs/common/src/platform/misc/range-with-default.ts b/libs/common/src/platform/misc/range-with-default.ts new file mode 100644 index 0000000000..fbf8aa84f4 --- /dev/null +++ b/libs/common/src/platform/misc/range-with-default.ts @@ -0,0 +1,24 @@ +/** + * A range with a default value. + * + * Enforces constraints to ensure min > default > max. + */ +export class RangeWithDefault { + constructor( + readonly min: number, + readonly max: number, + readonly defaultValue: number, + ) { + if (min > max) { + throw new Error(`${min} is greater than ${max}.`); + } + + if (this.inRange(defaultValue) === false) { + throw new Error("Default value is not in range."); + } + } + + inRange(value: number): boolean { + return value >= this.min && value <= this.max; + } +} diff --git a/libs/common/src/platform/services/crypto.service.ts b/libs/common/src/platform/services/crypto.service.ts index 54e7daabbd..443e7cae87 100644 --- a/libs/common/src/platform/services/crypto.service.ts +++ b/libs/common/src/platform/services/crypto.service.ts @@ -17,10 +17,11 @@ import { KeySuffixOptions, HashPurpose, KdfType, - DEFAULT_ARGON2_ITERATIONS, - DEFAULT_ARGON2_MEMORY, - DEFAULT_ARGON2_PARALLELISM, + ARGON2_ITERATIONS, + ARGON2_MEMORY, + ARGON2_PARALLELISM, EncryptionType, + PBKDF2_ITERATIONS, } from "../enums"; import { sequentialize } from "../misc/sequentialize"; import { EFFLongWordList } from "../misc/wordlist"; @@ -175,6 +176,12 @@ export class CryptoService implements CryptoServiceAbstraction { )); } + /** + * Derive a master key from a password and email. + * + * @remarks + * Does not validate the kdf config to ensure it satisfies the minimum requirements for the given kdf type. + */ async makeMasterKey( password: string, email: string, @@ -841,6 +848,43 @@ export class CryptoService implements CryptoServiceAbstraction { return null; } + /** + * Validate that the KDF config follows the requirements for the given KDF type. + * + * @remarks + * Should always be called before updating a users KDF config. + */ + validateKdfConfig(kdf: KdfType, kdfConfig: KdfConfig): void { + switch (kdf) { + case KdfType.PBKDF2_SHA256: + if (!PBKDF2_ITERATIONS.inRange(kdfConfig.iterations)) { + throw new Error( + `PBKDF2 iterations must be between ${PBKDF2_ITERATIONS.min} and ${PBKDF2_ITERATIONS.max}`, + ); + } + break; + case KdfType.Argon2id: + if (!ARGON2_ITERATIONS.inRange(kdfConfig.iterations)) { + throw new Error( + `Argon2 iterations must be between ${ARGON2_ITERATIONS.min} and ${ARGON2_ITERATIONS.max}`, + ); + } + + if (!ARGON2_MEMORY.inRange(kdfConfig.memory)) { + throw new Error( + `Argon2 memory must be between ${ARGON2_MEMORY.min}mb and ${ARGON2_MEMORY.max}mb`, + ); + } + + if (!ARGON2_PARALLELISM.inRange(kdfConfig.parallelism)) { + throw new Error( + `Argon2 parallelism must be between ${ARGON2_PARALLELISM.min} and ${ARGON2_PARALLELISM.max}.`, + ); + } + break; + } + } + protected async clearAllStoredUserKeys(userId?: string): Promise { await this.stateService.setUserKeyAutoUnlock(null, { userId: userId }); await this.stateService.setPinKeyEncryptedUserKeyEphemeral(null, { userId: userId }); @@ -900,30 +944,21 @@ export class CryptoService implements CryptoServiceAbstraction { let key: Uint8Array = null; if (kdf == null || kdf === KdfType.PBKDF2_SHA256) { if (kdfConfig.iterations == null) { - kdfConfig.iterations = 5000; - } else if (kdfConfig.iterations < 5000) { - throw new Error("PBKDF2 iteration minimum is 5000."); + kdfConfig.iterations = PBKDF2_ITERATIONS.defaultValue; } + key = await this.cryptoFunctionService.pbkdf2(password, salt, "sha256", kdfConfig.iterations); } else if (kdf == KdfType.Argon2id) { if (kdfConfig.iterations == null) { - kdfConfig.iterations = DEFAULT_ARGON2_ITERATIONS; - } else if (kdfConfig.iterations < 2) { - throw new Error("Argon2 iteration minimum is 2."); + kdfConfig.iterations = ARGON2_ITERATIONS.defaultValue; } if (kdfConfig.memory == null) { - kdfConfig.memory = DEFAULT_ARGON2_MEMORY; - } else if (kdfConfig.memory < 16) { - throw new Error("Argon2 memory minimum is 16 MB"); - } else if (kdfConfig.memory > 1024) { - throw new Error("Argon2 memory maximum is 1024 MB"); + kdfConfig.memory = ARGON2_MEMORY.defaultValue; } if (kdfConfig.parallelism == null) { - kdfConfig.parallelism = DEFAULT_ARGON2_PARALLELISM; - } else if (kdfConfig.parallelism < 1) { - throw new Error("Argon2 parallelism minimum is 1."); + kdfConfig.parallelism = ARGON2_PARALLELISM.defaultValue; } const saltHash = await this.cryptoFunctionService.hash(salt, "sha256"); diff --git a/libs/exporter/src/vault-export/services/vault-export.service.spec.ts b/libs/exporter/src/vault-export/services/vault-export.service.spec.ts index 463147c331..b4d0dbb916 100644 --- a/libs/exporter/src/vault-export/services/vault-export.service.spec.ts +++ b/libs/exporter/src/vault-export/services/vault-export.service.spec.ts @@ -5,7 +5,7 @@ import { KdfConfig } from "@bitwarden/common/auth/models/domain/kdf-config"; import { CipherWithIdExport } from "@bitwarden/common/models/export/cipher-with-ids.export"; import { CryptoFunctionService } from "@bitwarden/common/platform/abstractions/crypto-function.service"; import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service"; -import { KdfType, DEFAULT_PBKDF2_ITERATIONS } from "@bitwarden/common/platform/enums"; +import { KdfType, PBKDF2_ITERATIONS } from "@bitwarden/common/platform/enums"; import { Utils } from "@bitwarden/common/platform/misc/utils"; import { EncryptedString, EncString } from "@bitwarden/common/platform/models/domain/enc-string"; import { StateService } from "@bitwarden/common/platform/services/state.service"; @@ -159,7 +159,7 @@ describe("VaultExportService", () => { folderService.getAllDecryptedFromState.mockResolvedValue(UserFolderViews); folderService.getAllFromState.mockResolvedValue(UserFolders); stateService.getKdfType.mockResolvedValue(KdfType.PBKDF2_SHA256); - stateService.getKdfConfig.mockResolvedValue(new KdfConfig(DEFAULT_PBKDF2_ITERATIONS)); + stateService.getKdfConfig.mockResolvedValue(new KdfConfig(PBKDF2_ITERATIONS.defaultValue)); cryptoService.encrypt.mockResolvedValue(new EncString("encrypted")); exportService = new VaultExportService( @@ -240,7 +240,7 @@ describe("VaultExportService", () => { }); it("specifies kdfIterations", () => { - expect(exportObject.kdfIterations).toEqual(DEFAULT_PBKDF2_ITERATIONS); + expect(exportObject.kdfIterations).toEqual(PBKDF2_ITERATIONS.defaultValue); }); it("has kdfType", () => {