chore: revert iterations (#9336)

MetaMask · Apr 22, 2024 · eebfb67 · eebfb67
1 parent cd81b17
commit eebfb67
Show file tree

Hide file tree

Showing 7 changed files with 54 additions and 166 deletions.
diff --git 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
@@ -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<string> => {
-    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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;