From 3896d3e795cd2385beedce06bd80c6c1c7e893fb Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Wed, 29 Nov 2023 16:27:10 +0100 Subject: [PATCH] [15.x] Backport encryption improvements (#312) * Prefer cached `encryptionKey` for encryption when possible (#307) * fix: prefer encryptionKey for encryption when possible * refactor: add test case * Use encryptor `isVaultUpdated` (#310) * chore: update browser-passworder * refactor: remove `updateVault` from `GenericEncryptor` --- jest.config.js | 2 +- package.json | 2 +- src/KeyringController.test.ts | 253 +++++++++++++++++++++------------- src/KeyringController.ts | 24 ++-- src/test/encryptor.mock.ts | 4 + src/types.ts | 14 +- yarn.lock | 10 +- 7 files changed, 192 insertions(+), 117 deletions(-) diff --git a/jest.config.js b/jest.config.js index 1ba7c11d..bddfbd82 100644 --- a/jest.config.js +++ b/jest.config.js @@ -41,7 +41,7 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 79.41, + branches: 79.8, functions: 93.22, lines: 91.5, statements: 91.69, diff --git a/package.json b/package.json index 0f0a080e..2ba9bf5c 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ }, "dependencies": { "@ethereumjs/tx": "^4.2.0", - "@metamask/browser-passworder": "^4.2.0", + "@metamask/browser-passworder": "^4.3.0", "@metamask/eth-hd-keyring": "^7.0.1", "@metamask/eth-sig-util": "^7.0.0", "@metamask/eth-simple-keyring": "^6.0.1", diff --git a/src/KeyringController.test.ts b/src/KeyringController.test.ts index 5ef07f5a..fca3a39c 100644 --- a/src/KeyringController.test.ts +++ b/src/KeyringController.test.ts @@ -78,6 +78,18 @@ async function initializeKeyringController({ return keyringController; } +/** + * Delete the encryption key and salt from the `memStore` of the given keyring controller. + * + * @param keyringController - The keyring controller to delete the encryption key and salt from. + */ +function deleteEncryptionKeyAndSalt(keyringController: KeyringController) { + const keyringControllerState = keyringController.memStore.getState(); + delete keyringControllerState.encryptionKey; + delete keyringControllerState.encryptionSalt; + keyringController.memStore.updateState(keyringControllerState); +} + describe('KeyringController', () => { afterEach(() => { sinon.restore(); @@ -199,83 +211,86 @@ describe('KeyringController', () => { }); describe('when `cacheEncryptionKey` is enabled', () => { - it('should save an up to date encryption salt to the `memStore` when `password` is unset and `encryptionKey` is set', async () => { - const keyringController = await initializeKeyringController({ - password: PASSWORD, - constructorOptions: { - cacheEncryptionKey: true, - }, + describe('when `encryptionKey` is set', () => { + it('should save an up to date encryption salt to the `memStore`', async () => { + const keyringController = await initializeKeyringController({ + password: PASSWORD, + constructorOptions: { + cacheEncryptionKey: true, + }, + }); + const vaultEncryptionKey = '🔑'; + const vaultEncryptionSalt = '🧂'; + const vault = JSON.stringify({ salt: vaultEncryptionSalt }); + keyringController.store.updateState({ vault }); + + await keyringController.unlockKeyrings( + undefined, + vaultEncryptionKey, + vaultEncryptionSalt, + ); + + expect(keyringController.memStore.getState().encryptionKey).toBe( + vaultEncryptionKey, + ); + expect(keyringController.memStore.getState().encryptionSalt).toBe( + vaultEncryptionSalt, + ); + + const response = await keyringController.persistAllKeyrings(); + + expect(response).toBe(true); + expect(keyringController.memStore.getState().encryptionKey).toBe( + vaultEncryptionKey, + ); + expect(keyringController.memStore.getState().encryptionSalt).toBe( + vaultEncryptionSalt, + ); }); - delete keyringController.password; - const vaultEncryptionKey = '🔑'; - const vaultEncryptionSalt = '🧂'; - const vault = JSON.stringify({ salt: vaultEncryptionSalt }); - keyringController.store.updateState({ vault }); - - await keyringController.unlockKeyrings( - undefined, - vaultEncryptionKey, - vaultEncryptionSalt, - ); - - expect(keyringController.memStore.getState().encryptionKey).toBe( - vaultEncryptionKey, - ); - expect(keyringController.memStore.getState().encryptionSalt).toBe( - vaultEncryptionSalt, - ); - - const response = await keyringController.persistAllKeyrings(); - - expect(response).toBe(true); - expect(keyringController.memStore.getState().encryptionKey).toBe( - vaultEncryptionKey, - ); - expect(keyringController.memStore.getState().encryptionSalt).toBe( - vaultEncryptionSalt, - ); }); - it('should save an up to date encryption salt to the `memStore` when `password` is set through `createNewVaultAndKeychain`', async () => { - const keyringController = await initializeKeyringController({ - password: PASSWORD, - constructorOptions: { - cacheEncryptionKey: true, - }, + describe('when `encryptionKey` is not set and `password` is set', () => { + it('should save an up to date encryption salt to the `memStore` when `password` is set through `createNewVaultAndKeychain`', async () => { + const keyringController = await initializeKeyringController({ + password: PASSWORD, + constructorOptions: { + cacheEncryptionKey: true, + }, + }); + await keyringController.createNewVaultAndKeychain(PASSWORD); + deleteEncryptionKeyAndSalt(keyringController); + + const response = await keyringController.persistAllKeyrings(); + + expect(response).toBe(true); + expect(keyringController.memStore.getState().encryptionKey).toBe( + MOCK_HARDCODED_KEY, + ); + expect(keyringController.memStore.getState().encryptionSalt).toBe( + MOCK_ENCRYPTION_SALT, + ); }); - await keyringController.createNewVaultAndKeychain(PASSWORD); - - const response = await keyringController.persistAllKeyrings(); - - expect(response).toBe(true); - expect(keyringController.memStore.getState().encryptionKey).toBe( - MOCK_HARDCODED_KEY, - ); - expect(keyringController.memStore.getState().encryptionSalt).toBe( - MOCK_ENCRYPTION_SALT, - ); - }); - - it('should save an up to date encryption salt to the `memStore` when `password` is set through `submitPassword`', async () => { - const keyringController = await initializeKeyringController({ - password: PASSWORD, - constructorOptions: { - cacheEncryptionKey: true, - }, + it('should save an up to date encryption salt to the `memStore` when `password` is set through `submitPassword`', async () => { + const keyringController = await initializeKeyringController({ + password: PASSWORD, + constructorOptions: { + cacheEncryptionKey: true, + }, + }); + await keyringController.submitPassword(PASSWORD); + deleteEncryptionKeyAndSalt(keyringController); + + const response = await keyringController.persistAllKeyrings(); + + expect(response).toBe(true); + expect(keyringController.memStore.getState().encryptionKey).toBe( + MOCK_HARDCODED_KEY, + ); + expect(keyringController.memStore.getState().encryptionSalt).toBe( + MOCK_ENCRYPTION_SALT, + ); }); - - await keyringController.submitPassword(PASSWORD); - - const response = await keyringController.persistAllKeyrings(); - - expect(response).toBe(true); - expect(keyringController.memStore.getState().encryptionKey).toBe( - MOCK_HARDCODED_KEY, - ); - expect(keyringController.memStore.getState().encryptionSalt).toBe( - MOCK_ENCRYPTION_SALT, - ); }); }); }); @@ -836,29 +851,81 @@ describe('KeyringController', () => { }); describe('with old vault format', () => { - [true, false].forEach((cacheEncryptionKey) => { - describe(`with cacheEncryptionKey = ${cacheEncryptionKey}`, () => { - it('should update the vault', async () => { - const mockEncryptor = new MockEncryptor(); - const keyringController = await initializeKeyringController({ - password: PASSWORD, - constructorOptions: { - cacheEncryptionKey: true, - encryptor: mockEncryptor, - }, - }); - const initialVault = keyringController.store.getState().vault; - const updatedVaultMock = - '{"vault": "updated_vault_detail", "salt": "salt"}'; - sinon.stub(mockEncryptor, 'updateVault').resolves(updatedVaultMock); - sinon.stub(mockEncryptor, 'encrypt').resolves(updatedVaultMock); - - await keyringController.unlockKeyrings(PASSWORD); - const updatedVault = keyringController.store.getState().vault; - - expect(initialVault).not.toBe(updatedVault); - expect(updatedVault).toBe(updatedVaultMock); + describe(`with cacheEncryptionKey = true and encryptionKey is unset`, () => { + it('should update the vault', async () => { + const mockEncryptor = new MockEncryptor(); + const keyringController = await initializeKeyringController({ + password: PASSWORD, + constructorOptions: { + cacheEncryptionKey: true, + encryptor: mockEncryptor, + }, }); + deleteEncryptionKeyAndSalt(keyringController); + const initialVault = keyringController.store.getState().vault; + const mockEncryptionResult = { + data: '0x1234', + iv: 'an iv', + }; + sinon.stub(mockEncryptor, 'isVaultUpdated').returns(false); + sinon + .stub(mockEncryptor, 'encryptWithKey') + .resolves(mockEncryptionResult); + + await keyringController.unlockKeyrings(PASSWORD); + const updatedVault = keyringController.store.getState().vault; + + expect(initialVault).not.toBe(updatedVault); + expect(updatedVault).toBe( + JSON.stringify({ + ...mockEncryptionResult, + salt: MOCK_ENCRYPTION_SALT, + }), + ); + }); + }); + + describe(`with cacheEncryptionKey = true and encryptionKey is set`, () => { + it('should not update the vault', async () => { + const mockEncryptor = new MockEncryptor(); + const keyringController = await initializeKeyringController({ + password: PASSWORD, + constructorOptions: { + cacheEncryptionKey: true, + encryptor: mockEncryptor, + }, + }); + const initialVault = keyringController.store.getState().vault; + sinon.stub(mockEncryptor, 'isVaultUpdated').returns(false); + + await keyringController.unlockKeyrings(PASSWORD); + const updatedVault = keyringController.store.getState().vault; + + expect(initialVault).toBe(updatedVault); + }); + }); + + describe(`with cacheEncryptionKey = false`, () => { + it('should update the vault', async () => { + const mockEncryptor = new MockEncryptor(); + const keyringController = await initializeKeyringController({ + password: PASSWORD, + constructorOptions: { + cacheEncryptionKey: false, + encryptor: mockEncryptor, + }, + }); + const initialVault = keyringController.store.getState().vault; + const updatedVaultMock = + '{"vault": "updated_vault_detail", "salt": "salt"}'; + sinon.stub(mockEncryptor, 'isVaultUpdated').returns(false); + sinon.stub(mockEncryptor, 'encrypt').resolves(updatedVaultMock); + + await keyringController.unlockKeyrings(PASSWORD); + const updatedVault = keyringController.store.getState().vault; + + expect(initialVault).not.toBe(updatedVault); + expect(updatedVault).toBe(updatedVaultMock); }); }); }); diff --git a/src/KeyringController.ts b/src/KeyringController.ts index ed48e288..4978feec 100644 --- a/src/KeyringController.ts +++ b/src/KeyringController.ts @@ -802,7 +802,15 @@ class KeyringController extends EventEmitter { if (this.#cacheEncryptionKey) { assertIsExportableKeyEncryptor(this.#encryptor); - if (this.password) { + if (encryptionKey) { + const key = await this.#encryptor.importKey(encryptionKey); + const vaultJSON = await this.#encryptor.encryptWithKey( + key, + serializedKeyrings, + ); + vaultJSON.salt = encryptionSalt; + vault = JSON.stringify(vaultJSON); + } else if (this.password) { const { vault: newVault, exportedKeyString } = await this.#encryptor.encryptWithDetail( this.password, @@ -811,14 +819,6 @@ class KeyringController extends EventEmitter { vault = newVault; newEncryptionKey = exportedKeyString; - } else if (encryptionKey) { - const key = await this.#encryptor.importKey(encryptionKey); - const vaultJSON = await this.#encryptor.encryptWithKey( - key, - serializedKeyrings, - ); - vaultJSON.salt = encryptionSalt; - vault = JSON.stringify(vaultJSON); } } else { if (typeof this.password !== 'string') { @@ -930,9 +930,9 @@ class KeyringController extends EventEmitter { if ( this.password && - this.#encryptor.updateVault && - (await this.#encryptor.updateVault(encryptedVault, this.password)) !== - encryptedVault + (!this.#cacheEncryptionKey || !encryptionKey) && + this.#encryptor.isVaultUpdated && + !this.#encryptor.isVaultUpdated(encryptedVault) ) { // Re-encrypt the vault with safer method if one is available await this.persistAllKeyrings(); diff --git a/src/test/encryptor.mock.ts b/src/test/encryptor.mock.ts index 0d3a4155..2c97d5ff 100644 --- a/src/test/encryptor.mock.ts +++ b/src/test/encryptor.mock.ts @@ -81,6 +81,10 @@ export class MockEncryptor implements ExportableKeyEncryptor { return _vault; } + isVaultUpdated(_vault: string) { + return true; + } + generateSalt() { return MOCK_ENCRYPTION_SALT; } diff --git a/src/types.ts b/src/types.ts index 67b2385f..4d8feda1 100644 --- a/src/types.ts +++ b/src/types.ts @@ -2,6 +2,7 @@ import type { DetailedDecryptResult, DetailedEncryptionResult, EncryptionResult, + KeyDerivationOptions, } from '@metamask/browser-passworder'; import type { Json, Keyring } from '@metamask/utils'; @@ -63,14 +64,17 @@ export type GenericEncryptor = { */ decrypt: (password: string, encryptedString: string) => Promise; /** - * Optional vault migration helper. Updates the provided vault, re-encrypting - * data with a safer algorithm if one is available. + * Optional vault migration helper. Checks if the provided vault is up to date + * with the desired encryption algorithm. * - * @param vault - The encrypted string to update. - * @param password - The password to decrypt the vault with. + * @param vault - The encrypted string to check. + * @param targetDerivationParams - The desired target derivation params. * @returns The updated encrypted string. */ - updateVault?: (vault: string, password: string) => Promise; + isVaultUpdated?: ( + vault: string, + targetDerivationParams?: KeyDerivationOptions, + ) => boolean; }; /** diff --git a/yarn.lock b/yarn.lock index 0c6fc39b..de6be29d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1127,12 +1127,12 @@ __metadata: languageName: node linkType: hard -"@metamask/browser-passworder@npm:^4.2.0": - version: 4.2.0 - resolution: "@metamask/browser-passworder@npm:4.2.0" +"@metamask/browser-passworder@npm:^4.3.0": + version: 4.3.0 + resolution: "@metamask/browser-passworder@npm:4.3.0" dependencies: "@metamask/utils": ^8.2.0 - checksum: 03b76357942d25a6316d6a03a8bc839cb18e53d9f95fc2787e0fbbcf13eeb2485ece47a2758e928d04635f8dbaa598794f2ecd0313e7c91f989bf11f2a0adec5 + checksum: 7992553a0cd91902aa316a931d36c2628cb5a73fcbc28a31dce4177704036d739214c580ed833079003b2c7dfd60c5648a20734badf91e2c665cfe2f56012a8c languageName: node linkType: hard @@ -1208,7 +1208,7 @@ __metadata: "@lavamoat/allow-scripts": ^2.3.1 "@lavamoat/preinstall-always-fail": ^1.0.0 "@metamask/auto-changelog": ^3.0.0 - "@metamask/browser-passworder": ^4.2.0 + "@metamask/browser-passworder": ^4.3.0 "@metamask/eslint-config": ^12.2.0 "@metamask/eslint-config-jest": ^12.1.0 "@metamask/eslint-config-nodejs": ^12.1.0