From eebfb675c8542fcc9d0905129bc04a3257116c2c Mon Sep 17 00:00:00 2001 From: Gustavo Antunes <17601467+gantunesr@users.noreply.github.com> Date: Mon, 22 Apr 2024 13:10:29 -0400 Subject: [PATCH] chore: revert iterations (#9336) --- app/core/Encryptor/Encryptor.test.ts | 119 ++------------------------- app/core/Encryptor/Encryptor.ts | 44 +--------- app/core/Encryptor/constants.ts | 32 +++++-- app/core/Encryptor/index.ts | 13 ++- app/core/Engine.ts | 4 +- app/core/SecureKeychain.js | 4 +- app/util/validators/index.js | 4 +- 7 files changed, 54 insertions(+), 166 deletions(-) diff --git a/app/core/Encryptor/Encryptor.test.ts b/app/core/Encryptor/Encryptor.test.ts index 79e19c42548..78e335116a0 100644 --- a/app/core/Encryptor/Encryptor.test.ts +++ b/app/core/Encryptor/Encryptor.test.ts @@ -1,10 +1,6 @@ import { NativeModules } from 'react-native'; import { Encryptor } from './Encryptor'; -import { - ENCRYPTION_LIBRARY, - DERIVATION_PARAMS, - KeyDerivationIteration, -} from './constants'; +import { ENCRYPTION_LIBRARY, LEGACY_DERIVATION_PARAMS } from './constants'; const Aes = NativeModules.Aes; const AesForked = NativeModules.AesForked; @@ -13,26 +9,7 @@ describe('Encryptor', () => { let encryptor: Encryptor; beforeEach(() => { - encryptor = new Encryptor({ derivationParams: DERIVATION_PARAMS }); - }); - - describe('constructor', () => { - it('throws an error if the provided iterations do not meet the minimum required', () => { - const iterations = 100; - expect( - () => - new Encryptor({ - derivationParams: { - algorithm: 'PBKDF2', - params: { - iterations, - }, - }, - }), - ).toThrowError( - `Invalid key derivation iterations: ${iterations}. Recommended number of iterations is ${KeyDerivationIteration.Default}. Minimum required is ${KeyDerivationIteration.Minimum}.`, - ); - }); + encryptor = new Encryptor({ derivationParams: LEGACY_DERIVATION_PARAMS }); }); describe('encrypt', () => { @@ -86,39 +63,23 @@ describe('Encryptor', () => { }); it.each([ - { - lib: ENCRYPTION_LIBRARY.original, - expectedKey: 'mockedAesKey', - expectedPBKDF2Args: ['testPassword', 'mockedSalt', 600000, 256], - description: - 'with original library and default iterations number for key generation', - keyMetadata: DERIVATION_PARAMS, - }, { lib: ENCRYPTION_LIBRARY.original, expectedKey: 'mockedAesKey', expectedPBKDF2Args: ['testPassword', 'mockedSalt', 5000, 256], description: - 'with original library and old iterations number for key generation', - }, - { - lib: 'random-lib', // Assuming not using "original" should lead to AesForked - expectedKey: 'mockedAesForkedKey', - expectedPBKDF2Args: ['testPassword', 'mockedSalt'], - description: - 'with library different to "original" and default iterations number for key generation', - keyMetadata: DERIVATION_PARAMS, + 'with original library and legacy iterations number for key generation', }, { lib: 'random-lib', // Assuming not using "original" should lead to AesForked expectedKey: 'mockedAesForkedKey', expectedPBKDF2Args: ['testPassword', 'mockedSalt'], description: - 'with library different to "original" and old iterations number for key generation', + 'with library different to "original" and legacy iterations number for key generation', }, ])( 'decrypts a string correctly $description', - async ({ lib, expectedKey, expectedPBKDF2Args, keyMetadata }) => { + async ({ lib, expectedKey, expectedPBKDF2Args }) => { const password = 'testPassword'; const mockVault = { cipher: 'mockedCipher', @@ -129,11 +90,7 @@ describe('Encryptor', () => { const decryptedObject = await encryptor.decrypt( password, - JSON.stringify( - keyMetadata !== undefined - ? { ...mockVault, keyMetadata } - : mockVault, - ), + JSON.stringify(mockVault), ); expect(decryptedObject).toEqual(expect.any(Object)); @@ -160,7 +117,7 @@ describe('Encryptor', () => { iv: 'mockedIV', salt: 'mockedSalt', lib: 'original', - keyMetadata: DERIVATION_PARAMS, + keyMetadata: LEGACY_DERIVATION_PARAMS, }), ), ).toBe(true); @@ -179,66 +136,4 @@ describe('Encryptor', () => { ).toBe(false); }); }); - - describe('updateVault', () => { - let encryptSpy: jest.SpyInstance, decryptSpy: jest.SpyInstance; - const expectedKeyMetadata = DERIVATION_PARAMS; - - beforeEach(() => { - encryptSpy = jest - .spyOn(Aes, 'encrypt') - .mockResolvedValue(() => Promise.resolve('mockedCipher')); - decryptSpy = jest - .spyOn(Aes, 'decrypt') - .mockResolvedValue('{"mockData": "mockedPlainText"}'); - }); - - afterEach(() => { - encryptSpy.mockRestore(); - decryptSpy.mockRestore(); - }); - - it('updates a vault correctly if keyMetadata is not present', async () => { - const mockVault = { - cipher: 'mockedCipher', - iv: 'mockedIV', - salt: 'mockedSalt', - lib: 'original', - }; - - const updatedVault = await encryptor.updateVault( - JSON.stringify(mockVault), - 'mockPassword', - ); - - const vault = JSON.parse(updatedVault); - - expect(encryptSpy).toBeCalledTimes(1); - expect(decryptSpy).toBeCalledTimes(1); - expect(vault).toHaveProperty('keyMetadata'); - expect(vault.keyMetadata).toStrictEqual(expectedKeyMetadata); - }); - - it('does not update a vault if algorithm is PBKDF2 and the number of iterations is 600000', async () => { - const mockVault = { - cipher: 'mockedCipher', - iv: 'mockedIV', - salt: 'mockedSalt', - lib: 'original', - keyMetadata: DERIVATION_PARAMS, - }; - - const updatedVault = await encryptor.updateVault( - JSON.stringify(mockVault), - 'mockPassword', - ); - - const vault = JSON.parse(updatedVault); - - expect(encryptSpy).toBeCalledTimes(0); - expect(decryptSpy).toBeCalledTimes(0); - expect(vault).toHaveProperty('keyMetadata'); - expect(vault.keyMetadata).toStrictEqual(expectedKeyMetadata); - }); - }); }); diff --git a/app/core/Encryptor/Encryptor.ts b/app/core/Encryptor/Encryptor.ts index 575c424da14..c0bf38ef5f2 100644 --- a/app/core/Encryptor/Encryptor.ts +++ b/app/core/Encryptor/Encryptor.ts @@ -54,31 +54,16 @@ class Encryptor implements GenericEncryptor { }: { derivationParams: KeyDerivationOptions; }) { - this.checkMinimalRequiredIterations(derivationParams.params.iterations); this.derivationParams = derivationParams; } - /** - * Throws an error if the provided number of iterations does not meet the minimum required for key derivation. - * This method ensures that the key derivation process is secure by enforcing a minimum number of iterations. - * @param iterations - The number of iterations to check. - * @throws Error if the number of iterations is less than the minimum required. - */ - private checkMinimalRequiredIterations = (iterations: number): void => { - if (!this.isMinimalRequiredIterationsMet(iterations)) { - throw new Error( - `Invalid key derivation iterations: ${iterations}. Recommended number of iterations is ${KeyDerivationIteration.Default}. Minimum required is ${KeyDerivationIteration.Minimum}.`, - ); - } - }; - /** * Checks if the provided number of iterations meets the minimum required for key derivation. * @param iterations - The number of iterations to check. * @returns A boolean indicating whether the minimum required iterations are met. */ private isMinimalRequiredIterationsMet = (iterations: number): boolean => - iterations >= KeyDerivationIteration.Minimum; + iterations >= KeyDerivationIteration.OWASP2023Minimum; /** * Generates a random base64-encoded salt string. @@ -208,7 +193,8 @@ class Encryptor implements GenericEncryptor { password, salt: payload.salt, iterations: - payload.keyMetadata?.params.iterations || KeyDerivationIteration.Legacy, + payload.keyMetadata?.params.iterations || + KeyDerivationIteration.Legacy5000, lib: payload.lib, }); const data = await this.decryptWithKey({ @@ -238,30 +224,6 @@ class Encryptor implements GenericEncryptor { keyMetadata.params.iterations === targetDerivationParams.params.iterations ); }; - - /** - * Updates the provided vault, re-encrypting - * data with a safer algorithm if one is available. - * - * If the provided vault is already using the latest available encryption method, - * it is returned as is. - * - * @param vault - The vault to update. - * @param password - The password to use for encryption. - * @param targetDerivationParams - The options to use for key derivation. - * @returns A promise resolving to the updated vault. - */ - updateVault = async ( - vault: string, - password: string, - targetDerivationParams = this.derivationParams, - ): Promise => { - if (this.isVaultUpdated(vault, targetDerivationParams)) { - return vault; - } - - return this.encrypt(password, await this.decrypt(password, vault)); - }; } // eslint-disable-next-line import/prefer-default-export diff --git a/app/core/Encryptor/constants.ts b/app/core/Encryptor/constants.ts index edf9563e5c7..7cf54b319b4 100644 --- a/app/core/Encryptor/constants.ts +++ b/app/core/Encryptor/constants.ts @@ -6,15 +6,37 @@ export const ENCRYPTION_LIBRARY = { original: 'original', }; +/** + * We use "OWASP2023" to indicate the source and year of the recommendation. + * This will help us version the recommend number in case it changes in the future. + * Source: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pbkdf2 + */ export enum KeyDerivationIteration { - Legacy = 5_000, - Minimum = 600_000, - Default = 900_000, + // Legacy, kept for backward compatibility + Legacy5000 = 5_000, + // OWASP's 2023 recommendation for minimum iterations + OWASP2023Minimum = 600_000, + // Default suggested iterations based on OWASP's 2023 recommendation + OWASP2023Default = 900_000, } -export const DERIVATION_PARAMS: KeyDerivationOptions = { +export const LEGACY_DERIVATION_PARAMS: KeyDerivationOptions = { algorithm: 'PBKDF2', params: { - iterations: KeyDerivationIteration.Minimum, + iterations: KeyDerivationIteration.Legacy5000, + }, +}; + +export const DERIVATION_PARAMS_MINIMUM_OWASP2023: KeyDerivationOptions = { + algorithm: 'PBKDF2', + params: { + iterations: KeyDerivationIteration.OWASP2023Minimum, + }, +}; + +export const DERIVATION_PARAMS_DEFAULT_OWASP2023: KeyDerivationOptions = { + algorithm: 'PBKDF2', + params: { + iterations: KeyDerivationIteration.OWASP2023Default, }, }; diff --git a/app/core/Encryptor/index.ts b/app/core/Encryptor/index.ts index e9f4b61263d..8b900afe59f 100644 --- a/app/core/Encryptor/index.ts +++ b/app/core/Encryptor/index.ts @@ -1,4 +1,13 @@ import { Encryptor } from './Encryptor'; -import { DERIVATION_PARAMS } from './constants'; +import { + LEGACY_DERIVATION_PARAMS, + DERIVATION_PARAMS_MINIMUM_OWASP2023, + DERIVATION_PARAMS_DEFAULT_OWASP2023, +} from './constants'; -export { Encryptor, DERIVATION_PARAMS }; +export { + Encryptor, + LEGACY_DERIVATION_PARAMS, + DERIVATION_PARAMS_MINIMUM_OWASP2023, + DERIVATION_PARAMS_DEFAULT_OWASP2023, +}; diff --git a/app/core/Engine.ts b/app/core/Engine.ts index 0dd009d4c91..f66452f2048 100644 --- a/app/core/Engine.ts +++ b/app/core/Engine.ts @@ -121,7 +121,7 @@ import { LoggingControllerActions, } from '@metamask/logging-controller'; import LedgerKeyring from '@consensys/ledgerhq-metamask-keyring'; -import { Encryptor, DERIVATION_PARAMS } from './Encryptor'; +import { Encryptor, LEGACY_DERIVATION_PARAMS } from './Encryptor'; import { isMainnetByChainId, getDecimalChainId, @@ -191,7 +191,7 @@ import { const NON_EMPTY = 'NON_EMPTY'; const encryptor = new Encryptor({ - derivationParams: DERIVATION_PARAMS, + derivationParams: LEGACY_DERIVATION_PARAMS, }); let currentChainId: any; diff --git a/app/core/SecureKeychain.js b/app/core/SecureKeychain.js index 6607c947386..5e37d983cf9 100644 --- a/app/core/SecureKeychain.js +++ b/app/core/SecureKeychain.js @@ -1,5 +1,5 @@ import * as Keychain from 'react-native-keychain'; // eslint-disable-line import/no-namespace -import { Encryptor, DERIVATION_PARAMS } from './Encryptor'; +import { Encryptor, LEGACY_DERIVATION_PARAMS } from './Encryptor'; import { strings } from '../../locales/i18n'; import AsyncStorage from '../store/async-storage-wrapper'; import { Platform } from 'react-native'; @@ -15,7 +15,7 @@ import Device from '../util/device'; const privates = new WeakMap(); const encryptor = new Encryptor({ - derivationParams: DERIVATION_PARAMS, + derivationParams: LEGACY_DERIVATION_PARAMS, }); const defaultOptions = { service: 'com.metamask', diff --git a/app/util/validators/index.js b/app/util/validators/index.js index 596bfd1699b..53caaead348 100644 --- a/app/util/validators/index.js +++ b/app/util/validators/index.js @@ -1,5 +1,5 @@ import { ethers } from 'ethers'; -import { Encryptor, DERIVATION_PARAMS } from '../../core/Encryptor'; +import { Encryptor, LEGACY_DERIVATION_PARAMS } from '../../core/Encryptor'; import { regex } from '../regex'; export const failedSeedPhraseRequirements = (seed) => { @@ -27,7 +27,7 @@ export const parseVaultValue = async (password, vault) => { seedObject?.lib ) { const encryptor = new Encryptor({ - derivationParams: DERIVATION_PARAMS, + derivationParams: LEGACY_DERIVATION_PARAMS, }); const result = await encryptor.decrypt(password, vault); vaultSeed = result[0]?.data?.mnemonic;