diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 2ce826ff3a2..a8bbefbc0e3 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Reduce the `N` `scrypt` parameter used to compute the encryption key in order to significantly increase performance on clients ([#6097](https://github.com/MetaMask/core/pull/6097)) + - The input to the KDF is already a long, high entropy non-user-generated string so it does not make sense to use a KDF with high compute cost. + - This decreases the impact of the KDF on all clients by around 99% + - Add backwards compatible migration logic + - Add encryption callbacks so we can measure the impact of future migrations through analytics + ## [25.1.0] ### Changed diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts index 54d70c58800..e8ff29160e4 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts @@ -14,6 +14,13 @@ import { import { BACKUPANDSYNC_FEATURES } from './constants'; import { MOCK_STORAGE_DATA, MOCK_STORAGE_KEY } from './mocks/mockStorage'; import UserStorageController, { defaultState } from './UserStorageController'; +import { + ALGORITHM_KEY_SIZE, + SCRYPT_N_V2, + SCRYPT_p, + SCRYPT_r, + SCRYPT_SALT_SIZE, +} from '../../shared/encryption/constants'; import { USER_STORAGE_FEATURE_NAMES } from '../../shared/storage-schema'; describe('UserStorageController', () => { @@ -835,3 +842,59 @@ describe('metadata', () => { `); }); }); + +describe('user-storage/user-storage-controller - encryption callbacks', () => { + const arrangeMocks = async () => { + const messengerMocks = mockUserStorageMessenger(); + const mockOnDecrypt = jest.fn(); + const mockOnEncrypt = jest.fn(); + const controller = new UserStorageController({ + messenger: messengerMocks.messenger, + config: { + encryption: { + onDecrypt: mockOnDecrypt, + onEncrypt: mockOnEncrypt, + }, + }, + }); + + await Promise.all([ + mockEndpointGetUserStorage( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + ), + mockEndpointUpsertUserStorage( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + ), + ]); + + return { mockOnDecrypt, mockOnEncrypt, controller }; + }; + + it('calls encryption callbacks if they are set', async () => { + const { mockOnEncrypt, mockOnDecrypt, controller } = await arrangeMocks(); + + await controller.performSetStorage( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + 'new data', + ); + + await controller.performGetStorage( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + ); + + const scryptParams = { + o: { + N: SCRYPT_N_V2, + dkLen: ALGORITHM_KEY_SIZE, + p: SCRYPT_p, + r: SCRYPT_r, + }, + saltLen: SCRYPT_SALT_SIZE, + t: 'scrypt', + v: '1', + }; + + expect(mockOnEncrypt).toHaveBeenCalledWith(scryptParams); + expect(mockOnDecrypt).toHaveBeenCalledWith(scryptParams); + }); +}); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts index 7d231a4c5b8..74c42ca834f 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -30,6 +30,7 @@ import { BACKUPANDSYNC_FEATURES } from './constants'; import { syncContactsWithUserStorage } from './contact-syncing/controller-integration'; import { setupContactSyncingSubscriptions } from './contact-syncing/setup-subscriptions'; import type { + EncryptedPayload, UserStorageGenericFeatureKey, UserStorageGenericPathWithFeatureAndKey, UserStorageGenericPathWithFeatureOnly, @@ -114,6 +115,10 @@ const metadata: StateMetadata = { type ControllerConfig = { env: Env; + encryption?: { + onEncrypt?: (encryptedData: Omit) => Promise; + onDecrypt?: (encryptedData: Omit) => Promise; + }; contactSyncing?: { /** * Callback that fires when contact sync updates a contact. @@ -418,6 +423,8 @@ export default class UserStorageController extends BaseController< return await this.#userStorage.getItem(path, { nativeScryptCrypto: this.#nativeScryptCrypto, entropySourceId, + onDecrypt: this.#config.encryption?.onDecrypt, + onEncrypt: this.#config.encryption?.onEncrypt, }); } @@ -436,6 +443,8 @@ export default class UserStorageController extends BaseController< return await this.#userStorage.getAllFeatureItems(path, { nativeScryptCrypto: this.#nativeScryptCrypto, entropySourceId, + onDecrypt: this.#config.encryption?.onDecrypt, + onEncrypt: this.#config.encryption?.onEncrypt, }); } @@ -456,6 +465,8 @@ export default class UserStorageController extends BaseController< return await this.#userStorage.setItem(path, value, { nativeScryptCrypto: this.#nativeScryptCrypto, entropySourceId, + onDecrypt: this.#config.encryption?.onDecrypt, + onEncrypt: this.#config.encryption?.onEncrypt, }); } @@ -476,6 +487,8 @@ export default class UserStorageController extends BaseController< return await this.#userStorage.batchSetItems(path, values, { nativeScryptCrypto: this.#nativeScryptCrypto, entropySourceId, + onDecrypt: this.#config.encryption?.onDecrypt, + onEncrypt: this.#config.encryption?.onEncrypt, }); } diff --git a/packages/profile-sync-controller/src/sdk/user-storage.test.ts b/packages/profile-sync-controller/src/sdk/user-storage.test.ts index 3ae2bdbb5a4..0e6086fff21 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.test.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.test.ts @@ -1,5 +1,3 @@ -import type { UserStorageGenericFeatureKey } from 'src/shared/storage-schema'; - import { arrangeAuthAPIs } from './__fixtures__/auth'; import { arrangeAuth, typedMockFn } from './__fixtures__/test-utils'; import { @@ -20,8 +18,9 @@ import { import type { StorageOptions } from './user-storage'; import { STORAGE_URL, UserStorage } from './user-storage'; import encryption, { createSHA256Hash } from '../shared/encryption'; -import { SHARED_SALT } from '../shared/encryption/constants'; +import { SCRYPT_N_V2, SHARED_SALT_V2 } from '../shared/encryption/constants'; import { Env } from '../shared/env'; +import type { UserStorageGenericFeatureKey } from '../shared/storage-schema'; import { USER_STORAGE_FEATURE_NAMES } from '../shared/storage-schema'; const MOCK_SRP = '0x6265617665726275696c642e6f7267'; @@ -115,11 +114,52 @@ describe('User Storage', () => { return; } - const isEncryptedUsingSharedSalt = + const isEncryptedUsingSharedSaltV2 = encryption.getSalt(requestBody.data).toString() === - SHARED_SALT.toString(); + SHARED_SALT_V2.toString(); + + const isEncryptedUsingNewScryptN = + JSON.parse(requestBody.data).o.N === SCRYPT_N_V2; + + expect(isEncryptedUsingSharedSaltV2).toBe(true); + expect(isEncryptedUsingNewScryptN).toBe(true); + }, + ); + + await userStorage.getItem( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + ); + expect(mockGet.isDone()).toBe(true); + expect(mockPut.isDone()).toBe(true); + }); + + it('re-encrypts data if received entry was encrypted with the old scrypt N param, and saves it back to user storage', async () => { + const { auth } = arrangeAuth('SRP', MOCK_SRP); + const { userStorage } = arrangeUserStorage(auth); + + // This corresponds to 'data1' + // Encrypted with the old scrypt N param + const mockResponse = { + HashedKey: 'entry1', + Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }; - expect(isEncryptedUsingSharedSalt).toBe(true); + const mockGet = await handleMockUserStorageGet({ + status: 200, + body: JSON.stringify(mockResponse), + }); + const mockPut = handleMockUserStoragePut( + undefined, + async (_, requestBody) => { + // eslint-disable-next-line jest/no-conditional-in-test + if (typeof requestBody === 'string') { + return; + } + + const isEncryptedUsingNewScryptN = + JSON.parse(requestBody.data).o.N === SCRYPT_N_V2; + + expect(isEncryptedUsingNewScryptN).toBe(true); }, ); @@ -184,20 +224,19 @@ describe('User Storage', () => { return; } - const doEntriesHaveDifferentSalts = - encryption.getIfEntriesHaveDifferentSalts( - Object.entries(requestBody.data).map((entry) => entry[1] as string), - ); - - expect(doEntriesHaveDifferentSalts).toBe(false); - - const doEntriesUseSharedSalt = Object.entries(requestBody.data).every( + const doEntriesUseSharedSaltV2 = Object.entries(requestBody.data).every( ([_entryKey, entryValue]) => encryption.getSalt(entryValue as string).toString() === - SHARED_SALT.toString(), + SHARED_SALT_V2.toString(), + ); + + const doEntriesUseNewScryptN = Object.entries(requestBody.data).every( + ([_entryKey, entryValue]) => + JSON.parse(entryValue as string).o.N === SCRYPT_N_V2, ); - expect(doEntriesUseSharedSalt).toBe(true); + expect(doEntriesUseSharedSaltV2).toBe(true); + expect(doEntriesUseNewScryptN).toBe(true); const wereOnlyNonEmptySaltEntriesUploaded = Object.entries(requestBody.data).length === 2; @@ -213,6 +252,54 @@ describe('User Storage', () => { expect(mockPut.isDone()).toBe(true); }); + it('re-encrypts data if received entries were encrypted with the old scrypt N param, and saves it back to user storage', async () => { + // This corresponds to [['entry1', 'data1'], ['entry2', 'data2'], ['HASHED_KEY', '{ "hello": "world" }']] + // Each entry has been encrypted with a random salt, except for the last entry + // The last entry is used to test if the function can handle payloads that contain both random salts and the shared salt + const mockResponse = [ + { + HashedKey: 'entry1', + Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + { + HashedKey: 'entry2', + Data: '{"v":"1","t":"scrypt","d":"3ioo9bxhjDjTmJWIGQMnOlnfa4ysuUNeLYTTmJ+qrq7gwI6hURH3ooUcBldJkHtvuQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + await MOCK_STORAGE_RESPONSE('data3'), + ]; + + const { auth } = arrangeAuth('SRP', MOCK_SRP); + const { userStorage } = arrangeUserStorage(auth); + + const mockGetAll = await handleMockUserStorageGetAllFeatureEntries({ + status: 200, + body: mockResponse, + }); + + const mockPut = handleMockUserStoragePut( + undefined, + async (_, requestBody) => { + // eslint-disable-next-line jest/no-conditional-in-test + if (typeof requestBody === 'string') { + return; + } + + const doEntriesUseNewScryptN = Object.entries(requestBody.data).every( + ([_entryKey, entryValue]) => + JSON.parse(entryValue as string).o.N === SCRYPT_N_V2, + ); + + expect(doEntriesUseNewScryptN).toBe(true); + }, + ); + + await userStorage.getAllFeatureItems( + USER_STORAGE_FEATURE_NAMES.notifications, + ); + expect(mockGetAll.isDone()).toBe(true); + expect(mockPut.isDone()).toBe(true); + }); + it('batch set items', async () => { const dataToStore: [UserStorageGenericFeatureKey, string][] = [ ['0x123', JSON.stringify(MOCK_NOTIFICATIONS_DATA)], diff --git a/packages/profile-sync-controller/src/sdk/user-storage.ts b/packages/profile-sync-controller/src/sdk/user-storage.ts index 6384878b0d1..3a414e20c1c 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -1,7 +1,7 @@ import type { IBaseAuth } from './authentication-jwt-bearer/types'; import { NotFoundError, UserStorageError } from './errors'; +import type { EncryptedPayload } from '../shared/encryption'; import encryption, { createSHA256Hash } from '../shared/encryption'; -import { SHARED_SALT } from '../shared/encryption/constants'; import type { Env } from '../shared/env'; import { getEnvUrls } from '../shared/env'; import type { @@ -39,6 +39,8 @@ export type GetUserStorageAllFeatureEntriesResponse = { export type UserStorageMethodOptions = { nativeScryptCrypto?: NativeScrypt; entropySourceId?: string; + onEncrypt?: (encryptedData: Omit) => Promise; + onDecrypt?: (encryptedData: Omit) => Promise; }; type ErrorMessage = { @@ -141,11 +143,10 @@ export class UserStorage { try { const headers = await this.#getAuthorizationHeader(entropySourceId); const storageKey = await this.getStorageKey(entropySourceId); - const encryptedData = await encryption.encryptString( - data, - storageKey, - options?.nativeScryptCrypto, - ); + const encryptedData = await encryption.encryptString(data, storageKey, { + nativeScryptCrypto: options?.nativeScryptCrypto, + onEncrypt: options?.onEncrypt, + }); const encryptedPath = createEntryPath(path, storageKey); const url = new URL(STORAGE_URL(this.env, encryptedPath)); @@ -196,11 +197,10 @@ export class UserStorage { data.map(async (d) => { return [ this.#createEntryKey(d[0], storageKey), - await encryption.encryptString( - d[1], - storageKey, - options?.nativeScryptCrypto, - ), + await encryption.encryptString(d[1], storageKey, { + nativeScryptCrypto: options?.nativeScryptCrypto, + onEncrypt: options?.onEncrypt, + }), ]; }), ); @@ -315,12 +315,14 @@ export class UserStorage { const decryptedData = await encryption.decryptString( encryptedData, storageKey, - options?.nativeScryptCrypto, + { + nativeScryptCrypto: options?.nativeScryptCrypto, + onDecrypt: options?.onDecrypt, + }, ); - // Re-encrypt the entry if it was encrypted with a random salt - const salt = encryption.getSalt(encryptedData); - if (salt.toString() !== SHARED_SALT.toString()) { + // Data migration + if (encryption.doesEntryNeedReEncryption(encryptedData)) { await this.#upsertUserStorage(path, decryptedData, options); } @@ -381,24 +383,23 @@ export class UserStorage { } try { - const data = await encryption.decryptString( - entry.Data, - storageKey, - options?.nativeScryptCrypto, - ); + const data = await encryption.decryptString(entry.Data, storageKey, { + nativeScryptCrypto: options?.nativeScryptCrypto, + onDecrypt: options?.onDecrypt, + }); decryptedData.push(data); - // Re-encrypt the entry was encrypted with a random salt - const salt = encryption.getSalt(entry.Data); - if (salt.toString() !== SHARED_SALT.toString()) { - reEncryptedEntries.push([ - entry.HashedKey, - await encryption.encryptString( - data, - storageKey, - options?.nativeScryptCrypto, - ), - ]); + // Data migration + if (encryption.doesEntryNeedReEncryption(entry.Data)) { + const reEncryptedData = await encryption.encryptString( + data, + storageKey, + { + nativeScryptCrypto: options?.nativeScryptCrypto, + onEncrypt: options?.onEncrypt, + }, + ); + reEncryptedEntries.push([entry.HashedKey, reEncryptedData]); } } catch { // do nothing diff --git a/packages/profile-sync-controller/src/shared/encryption/cache.ts b/packages/profile-sync-controller/src/shared/encryption/cache.ts index 7552ef60d1a..628716ed722 100644 --- a/packages/profile-sync-controller/src/shared/encryption/cache.ts +++ b/packages/profile-sync-controller/src/shared/encryption/cache.ts @@ -1,4 +1,3 @@ -import { SHARED_SALT } from './constants'; import { byteArrayToBase64 } from './utils'; type CachedEntry = { @@ -47,31 +46,6 @@ export function getCachedKeyBySalt( }; } -/** - * Gets the cached key that was generated without a salt, if it exists. - * This is unique per hashed password. - * - * @param hashedPassword - hashed password for cache lookup - * @returns the cached key - */ -export function getCachedKeyGeneratedWithSharedSalt( - hashedPassword: string, -): CachedEntry | undefined { - const cache = getPasswordCache(hashedPassword); - const base64Salt = byteArrayToBase64(SHARED_SALT); - const cachedKey = cache.get(base64Salt); - - if (!cachedKey) { - return undefined; - } - - return { - salt: SHARED_SALT, - base64Salt, - key: cachedKey, - }; -} - /** * Sets a key to the in memory cache. * We have set an arbitrary size of 10 cached keys per hashed password. diff --git a/packages/profile-sync-controller/src/shared/encryption/constants.ts b/packages/profile-sync-controller/src/shared/encryption/constants.ts index 6539a63ebeb..947df6167c7 100644 --- a/packages/profile-sync-controller/src/shared/encryption/constants.ts +++ b/packages/profile-sync-controller/src/shared/encryption/constants.ts @@ -6,11 +6,16 @@ export const ALGORITHM_KEY_SIZE = 16; // 16 bytes // see: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#scrypt export const SCRYPT_SALT_SIZE = 16; // 16 bytes export const SCRYPT_N = 2 ** 17; // CPU/memory cost parameter (must be a power of 2, > 1) +export const SCRYPT_N_V2 = 2; export const SCRYPT_r = 8; // Block size parameter export const SCRYPT_p = 1; // Parallelization parameter export const SHARED_SALT = new Uint8Array([ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, ]); +// Different salt for SCRYPT_N_V2 to prevent cache collisions on outdated clients +export const SHARED_SALT_V2 = new Uint8Array([ + 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, +]); export const MAX_KDF_PROMISE_CACHE_SIZE = 20; diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts index 3517725ca3a..21e082ed6d6 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts @@ -1,4 +1,12 @@ -import { MAX_KDF_PROMISE_CACHE_SIZE } from './constants'; +import { + ALGORITHM_KEY_SIZE, + SCRYPT_N, + SCRYPT_N_V2, + SCRYPT_p, + SCRYPT_r, + SCRYPT_SALT_SIZE, + MAX_KDF_PROMISE_CACHE_SIZE, +} from './constants'; import encryption, { createSHA256Hash } from './encryption'; describe('encryption tests', () => { @@ -33,6 +41,29 @@ describe('encryption tests', () => { expect(result).toBe(DATA1); }); + it('should derive and use a different key for entries that have the same password but different encryption options', async () => { + // Test decryption of old format (N:2^17 with SHARED_SALT) + const data1EncryptedWithOldN = + '{"v":"1","t":"scrypt","d":"AAECAwQFBgcICQoLDA0OD5rMiKCTz61h5abF98yn50UPflCq6Ozov3NAk+y4h6o5bp0jJLJ0rw==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}'; + + const data1EncryptedWithNewN = await encryption.encryptString( + DATA1, + PASSWORD, + ); + + const decrypted1 = await encryption.decryptString( + data1EncryptedWithOldN, + PASSWORD, + ); + const decrypted2 = await encryption.decryptString( + data1EncryptedWithNewN, + PASSWORD, + ); + + expect(decrypted1).toBe(DATA1); + expect(decrypted2).toBe(DATA1); + }); + it('should sha-256 hash a value and should be deterministic', () => { const DATA = 'Hello World'; const EXPECTED_HASH = @@ -82,34 +113,62 @@ describe('encryption tests', () => { expect(decryptedData).toBe(ACCOUNTS_DECRYPTED_DATA); }); - describe('getIfEntriesHaveDifferentSalts()', () => { - it('should return true if entries have different salts', () => { - const entries = [ - '{"v":"1","t":"scrypt","d":"1yC/ZXarV57HbqEZ46nH0JWgXfPl86nTHD7kai2g5gm290FM9tw5QjOaAAwIuQESEE8TIM/J9pIj7nmlGi+BZrevTtK3DXWXwnUQsCP7amKd5Q4gs3EEQgXpA0W+WJUgyElj869rwIv/C6tl5E2pK4j/0EAjMSIm1TGoj9FPohyRgZsOIt8VhZfb7w0GODsjPwPIkN6zazvJ3gAFYFPh7yRtebFs86z3fzqCWZ9zakdCHntchC2oZiaApXR9yzaPlGgnPg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - '{"v":"1","t":"scrypt","d":"x7QqsdqsdEtUo7q/jG+UNkD/HOxQARGGRXsGPrLsDlkwDfgfoYlPI0To/M3pJRBlKD0RLEFIPHtHBEA5bv/2izB21VljvhMnhHfo0KgQ+e8Uq1t7grwa+r+ge3qbPNY+w78Xt8GtC+Hkrw5fORKvCn+xjzaCHYV6RxKYbp1TpyCJq7hDrr1XiyL8kqbpE0hAHALrrQOoV9/WXJi9pC5J118kquXx8CNA1P5wO/BXKp1AbryGR6kVW3lsp1sy3lYE/TApa5lTj+","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - ]; + it('should fire the onEncrypt callback if provided', async () => { + const onEncrypt = jest.fn(); + const encryptedData = await encryption.encryptString(DATA1, PASSWORD, { + onEncrypt, + }); + expect(onEncrypt).toHaveBeenCalledWith({ + v: '1', + t: 'scrypt', + o: { + N: SCRYPT_N_V2, + r: SCRYPT_r, + p: SCRYPT_p, + dkLen: ALGORITHM_KEY_SIZE, + }, + saltLen: SCRYPT_SALT_SIZE, + }); + expect(encryptedData).toBeDefined(); + }); + + it('should fire the onDecrypt callback if provided', async () => { + const onDecrypt = jest.fn(); + const decryptedData = await encryption.decryptString( + ACCOUNTS_ENCRYPTED_DATA_WITH_SALT, + ACCOUNTS_PASSWORD, + { onDecrypt }, + ); + const encryptedData = JSON.parse(ACCOUNTS_ENCRYPTED_DATA_WITH_SALT); + expect(onDecrypt).toHaveBeenCalledWith({ + v: encryptedData.v, + t: encryptedData.t, + o: encryptedData.o, + saltLen: encryptedData.saltLen, + }); + expect(decryptedData).toBe(ACCOUNTS_DECRYPTED_DATA); + }); - const result = encryption.getIfEntriesHaveDifferentSalts(entries); + describe('doesEntryNeedReEncryption()', () => { + it('should return true if entry uses a random salt', () => { + const entry = + '{"v":"1","t":"scrypt","d":"1yC/ZXarV57HbqEZ46nH0JWgXfPl86nTHD7kai2g5gm290FM9tw5QjOaAAwIuQESEE8TIM/J9pIj7nmlGi+BZrevTtK3DXWXwnUQsCP7amKd5Q4gs3EEQgXpA0W+WJUgyElj869rwIv/C6tl5E2pK4j/0EAjMSIm1TGoj9FPohyRgZsOIt8VhZfb7w0GODsjPwPIkN6zazvJ3gAFYFPh7yRtebFs86z3fzqCWZ9zakdCHntchC2oZiaApXR9yzaPlGgnPg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}'; + + const result = encryption.doesEntryNeedReEncryption(entry); expect(result).toBe(true); }); - it('should return false if entries have the same salts', () => { - const entries = [ - '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4dIzFL/hGR4Y6hb2qUGrPb/hjxHVJUk1jcJAyHP9eUzgZQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4XYxpF0N3IXuhCpPM9dAyw5pO2gcqcXNucJs60rBtgKttA==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - ]; + it('should return true if entry uses the old scrypt N parameter', () => { + const entry = `{"v":"1","t":"scrypt","d":"x7QqsdqsdEtUo7q/jG+UNkD/HOxQARGGRXsGPrLsDlkwDfgfoYlPI0To/M3pJRBlKD0RLEFIPHtHBEA5bv/2izB21VljvhMnhHfo0KgQ+e8Uq1t7grwa+r+ge3qbPNY+w78Xt8GtC+Hkrw5fORKvCn+xjzaCHYV6RxKYbp1TpyCJq7hDrr1XiyL8kqbpE0hAHALrrQOoV9/WXJi9pC5J118kquXx8CNA1P5wO/BXKp1AbryGR6kVW3lsp1sy3lYE/TApa5lTj+","o":{"N":${SCRYPT_N},"r":8,"p":1,"dkLen":16},"saltLen":16}`; - const result = encryption.getIfEntriesHaveDifferentSalts(entries); - expect(result).toBe(false); + const result = encryption.doesEntryNeedReEncryption(entry); + expect(result).toBe(true); }); - it('should return false if entries do not have salts', () => { - const entries = [ - '{"v":"1","t":"scrypt","d":"CgHcOM6xCaaNFnPCr0etqyxCq4xoJNQ9gfP9+GRn94hGtKurbOuXzyDoHJgzaJxDKd1zQHJhDwLjnH6oCZvC8XKvZZ6RcrN9BicZHpzpojon+HwpcPHceM/pvoMabYfiXqbokYHXZymGTxE5X+TjFo+HB7/Y6xOCU1usz47bru9vfyZrdQ66qGlMO2MUFx00cnh8xHOksDNC","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":0}', - '{"v":"1","t":"scrypt","d":"OCrYnCFkt7a33cjaAL65D/WypM+oVxIiGVwMk+mjijcpnG4r3vzPl6OzFpx2LNKHj6YN59wcLje3QK2hISU0R8iXyZubdkeAiY89SsI7owLda96ysF+q6PuyxnWfNfWe+5a1+4O8BVkR8p/9PYimwTN0QGhX2lkfLt5r0aYgsLnWld/5k9G7cB4yqoduIopzpojS5ZGI8PFW","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":0}', - ]; + it('should return false if entry uses new scrypt N parameter', async () => { + const entry = await encryption.encryptString(DATA1, PASSWORD); - const result = encryption.getIfEntriesHaveDifferentSalts(entries); + const result = encryption.doesEntryNeedReEncryption(entry); expect(result).toBe(false); }); }); diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index 3aedfdb77c1..a9d645de0d8 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -1,23 +1,20 @@ import { gcm } from '@noble/ciphers/aes'; import { randomBytes } from '@noble/ciphers/webcrypto'; import { scryptAsync } from '@noble/hashes/scrypt'; -import { sha256 } from '@noble/hashes/sha256'; +import { sha256 } from '@noble/hashes/sha2'; import { utf8ToBytes, concatBytes, bytesToHex } from '@noble/hashes/utils'; -import { - getCachedKeyBySalt, - getCachedKeyGeneratedWithSharedSalt, - setCachedKey, -} from './cache'; +import { getCachedKeyBySalt, setCachedKey } from './cache'; import { ALGORITHM_KEY_SIZE, ALGORITHM_NONCE_SIZE, MAX_KDF_PROMISE_CACHE_SIZE, SCRYPT_N, + SCRYPT_N_V2, SCRYPT_p, SCRYPT_r, SCRYPT_SALT_SIZE, - SHARED_SALT, + SHARED_SALT_V2, } from './constants'; import { base64ToByteArray, @@ -59,14 +56,30 @@ class EncryptorDecryptor { async encryptString( plaintext: string, password: string, - nativeScryptCrypto?: NativeScrypt, + options?: { + nativeScryptCrypto?: NativeScrypt; + onEncrypt?: (encryptedData: Omit) => Promise; + }, ): Promise { try { - return await this.#encryptStringV1( + const encryptedString = await this.#encryptStringV1( plaintext, password, - nativeScryptCrypto, + options?.nativeScryptCrypto, + { + N: SCRYPT_N_V2, + }, ); + + const encryptedData: EncryptedPayload = JSON.parse(encryptedString); + await options?.onEncrypt?.({ + v: encryptedData.v, + t: encryptedData.t, + o: encryptedData.o, + saltLen: encryptedData.saltLen, + }); + + return encryptedString; } catch (e) { const errorMessage = e instanceof Error ? e.message : JSON.stringify(e); throw new Error(`Unable to encrypt string - ${errorMessage}`); @@ -76,16 +89,27 @@ class EncryptorDecryptor { async decryptString( encryptedDataStr: string, password: string, - nativeScryptCrypto?: NativeScrypt, + options?: { + nativeScryptCrypto?: NativeScrypt; + onDecrypt?: (encryptedData: Omit) => Promise; + }, ): Promise { try { const encryptedData: EncryptedPayload = JSON.parse(encryptedDataStr); + + await options?.onDecrypt?.({ + v: encryptedData.v, + t: encryptedData.t, + o: encryptedData.o, + saltLen: encryptedData.saltLen, + }); + if (encryptedData.v === '1') { if (encryptedData.t === 'scrypt') { return await this.#decryptStringV1( encryptedData, password, - nativeScryptCrypto, + options?.nativeScryptCrypto, ); } } @@ -102,11 +126,14 @@ class EncryptorDecryptor { plaintext: string, password: string, nativeScryptCrypto?: NativeScrypt, + scryptOverrides = { + N: SCRYPT_N, + }, ): Promise { const { key, salt } = await this.#getOrGenerateScryptKey( password, { - N: SCRYPT_N, + N: scryptOverrides.N, r: SCRYPT_r, p: SCRYPT_p, dkLen: ALGORITHM_KEY_SIZE, @@ -130,7 +157,7 @@ class EncryptorDecryptor { t: 'scrypt', d: encryptedData, o: { - N: SCRYPT_N, + N: scryptOverrides.N, r: SCRYPT_r, p: SCRYPT_p, dkLen: ALGORITHM_KEY_SIZE, @@ -203,19 +230,17 @@ class EncryptorDecryptor { } } - getIfEntriesHaveDifferentSalts(entries: string[]): boolean { - const salts = entries - .map((e) => { - try { - return this.getSalt(e); - } catch { - return undefined; - } - }) - .filter((s): s is Uint8Array => s !== undefined); + doesEntryNeedReEncryption(encryptedDataStr: string): boolean { + try { + const encryptedData: EncryptedPayload = JSON.parse(encryptedDataStr); - const strSet = new Set(salts.map((arr) => arr.toString())); - return strSet.size === salts.length; + return ( + encryptedData.o?.N !== SCRYPT_N_V2 || + this.getSalt(encryptedDataStr).toString() !== SHARED_SALT_V2.toString() + ); + } catch { + return false; + } } #encrypt(plaintext: Uint8Array, key: Uint8Array): Uint8Array { @@ -245,12 +270,18 @@ class EncryptorDecryptor { salt?: Uint8Array, nativeScryptCrypto?: NativeScrypt, ) { - const hashedPassword = createSHA256Hash(password); + const targetSalt = salt ?? SHARED_SALT_V2; - // Check if we already have the key cached - const cachedKey = salt - ? getCachedKeyBySalt(hashedPassword, salt) - : getCachedKeyGeneratedWithSharedSalt(hashedPassword); + // Create a unique cache key for this KDF operation that includes all parameters + const cacheKey = this.#createKdfCacheKey( + password, + o, + targetSalt, + nativeScryptCrypto, + ); + + // Check if we already have the key cached using the cache key as identifier + const cachedKey = getCachedKeyBySalt(cacheKey, targetSalt); if (cachedKey) { return { @@ -259,15 +290,6 @@ class EncryptorDecryptor { }; } - // Create a unique cache key for this KDF operation - const newSalt = salt ?? SHARED_SALT; - const cacheKey = this.#createKdfCacheKey( - hashedPassword, - o, - newSalt, - nativeScryptCrypto, - ); - // Check if there's already an ongoing KDF operation with the same parameters const existingPromise = this.#kdfPromiseCache.get(cacheKey); if (existingPromise) { @@ -287,8 +309,8 @@ class EncryptorDecryptor { const kdfPromise = this.#performKdfOperation( password, o, - newSalt, - hashedPassword, + targetSalt, + cacheKey, nativeScryptCrypto, ); @@ -305,21 +327,24 @@ class EncryptorDecryptor { } #createKdfCacheKey( - hashedPassword: string, + password: string, o: EncryptedPayload['o'], salt: Uint8Array, nativeScryptCrypto?: NativeScrypt, ): string { + const hashedPassword = createSHA256Hash( + `${password}.${o.N}.${o.r}.${o.p}.${o.dkLen}`, + ); const saltStr = byteArrayToBase64(salt); const hasNative = Boolean(nativeScryptCrypto); - return `${hashedPassword}:${o.N}:${o.r}:${o.p}:${o.dkLen}:${saltStr}:${hasNative}`; + return `${hashedPassword}:${saltStr}:${hasNative}`; } async #performKdfOperation( password: string, o: EncryptedPayload['o'], salt: Uint8Array, - hashedPassword: string, + cacheKey: string, nativeScryptCrypto?: NativeScrypt, ): Promise<{ key: Uint8Array; salt: Uint8Array }> { let newKey: Uint8Array; @@ -342,7 +367,7 @@ class EncryptorDecryptor { }); } - setCachedKey(hashedPassword, salt, newKey); + setCachedKey(cacheKey, salt, newKey); return { key: newKey,