From fb4d0ad1661c0b9e0d28fabb390e980477b82819 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 10 Jul 2025 13:00:25 +0200 Subject: [PATCH 01/20] feat: dumb down user storage KDF --- .../src/sdk/user-storage.test.ts | 29 +++++--------- .../src/sdk/user-storage.ts | 30 +++++++------- .../src/shared/encryption/constants.ts | 1 + .../src/shared/encryption/encryption.ts | 40 +++++++++++++++---- 4 files changed, 57 insertions(+), 43 deletions(-) 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..2023f9c32b0 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.test.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.test.ts @@ -20,7 +20,6 @@ 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 { Env } from '../shared/env'; import { USER_STORAGE_FEATURE_NAMES } from '../shared/storage-schema'; @@ -92,12 +91,11 @@ describe('User Storage', () => { expect(response).toBe(data); }); - it('re-encrypts data if received entry was encrypted with a random salt, and saves it back to user storage', async () => { + it('re-encrypts data if received entry was encrypted with v1 encryption params, and saves it back to user storage', async () => { const { auth } = arrangeAuth('SRP', MOCK_SRP); const { userStorage } = arrangeUserStorage(auth); // This corresponds to 'data1' - // Encrypted with a random 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}', @@ -115,11 +113,9 @@ describe('User Storage', () => { return; } - const isEncryptedUsingSharedSalt = - encryption.getSalt(requestBody.data).toString() === - SHARED_SALT.toString(); - - expect(isEncryptedUsingSharedSalt).toBe(true); + // Verify that the data was re-encrypted to V2 format + const parsed = JSON.parse(requestBody.data); + expect(parsed.v).toBe('2'); }, ); @@ -152,7 +148,7 @@ describe('User Storage', () => { expect(responseAllFeatureEntries).toStrictEqual([data]); }); - it('re-encrypts data if received entries were encrypted with random salts, and saves it back to user storage', async () => { + it('re-encrypts data if received entries were encrypted with v1 encryption, 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 @@ -184,20 +180,13 @@ 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( + // All re-encrypted entries should be V2 + const doEntriesUseV2Encryption = Object.entries(requestBody.data).every( ([_entryKey, entryValue]) => - encryption.getSalt(entryValue as string).toString() === - SHARED_SALT.toString(), + JSON.parse(entryValue as string).v === '2', ); - expect(doEntriesUseSharedSalt).toBe(true); + expect(doEntriesUseV2Encryption).toBe(true); const wereOnlyNonEmptySaltEntriesUploaded = Object.entries(requestBody.data).length === 2; diff --git a/packages/profile-sync-controller/src/sdk/user-storage.ts b/packages/profile-sync-controller/src/sdk/user-storage.ts index 6384878b0d1..3ac8f0203a2 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -1,7 +1,6 @@ import type { IBaseAuth } from './authentication-jwt-bearer/types'; import { NotFoundError, UserStorageError } from './errors'; 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 { @@ -318,10 +317,13 @@ export class UserStorage { options?.nativeScryptCrypto, ); - // Re-encrypt the entry if it was encrypted with a random salt - const salt = encryption.getSalt(encryptedData); - if (salt.toString() !== SHARED_SALT.toString()) { - await this.#upsertUserStorage(path, decryptedData, options); + // Migrate data from v1 to v2 encryption + if (JSON.parse(encryptedData).v === '1') { + const reEncryptedData = await encryption.encryptString( + decryptedData, + storageKey, + ); + await this.#upsertUserStorage(path, reEncryptedData, options); } return decryptedData; @@ -388,17 +390,13 @@ export class UserStorage { ); 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, - ), - ]); + // Migrate data from v1 to v2 encryption + if (JSON.parse(entry.Data).v === '1') { + const reEncryptedData = await encryption.encryptString( + data, + storageKey, + ); + reEncryptedEntries.push([entry.HashedKey, reEncryptedData]); } } catch { // do nothing diff --git a/packages/profile-sync-controller/src/shared/encryption/constants.ts b/packages/profile-sync-controller/src/shared/encryption/constants.ts index e5a04f74783..d2cf5b7ffd9 100644 --- a/packages/profile-sync-controller/src/shared/encryption/constants.ts +++ b/packages/profile-sync-controller/src/shared/encryption/constants.ts @@ -6,6 +6,7 @@ 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 diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index b69615c7662..c53d3dcc7c7 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -13,6 +13,7 @@ import { ALGORITHM_KEY_SIZE, ALGORITHM_NONCE_SIZE, SCRYPT_N, + SCRYPT_N_V2, SCRYPT_p, SCRYPT_r, SCRYPT_SALT_SIZE, @@ -28,7 +29,7 @@ import type { NativeScrypt } from '../types/encryption'; export type EncryptedPayload = { // version - v: '1'; + v: '1' | '2'; // key derivation function algorithm - scrypt t: 'scrypt'; @@ -55,11 +56,23 @@ class EncryptorDecryptor { nativeScryptCrypto?: NativeScrypt, ): Promise { try { - return await this.#encryptStringV1( + const startTimestamp = Date.now(); + console.warn( + `Scrypt removal: Encrypting string with password: ${password} at ${new Date(startTimestamp).toISOString()}`, + ); + const encryptedData = await this.#encryptStringV1( plaintext, password, nativeScryptCrypto, + { + N: SCRYPT_N_V2, + }, + ); + console.warn( + `Scrypt removal: Encryption completed in ${Date.now() - startTimestamp} ms`, ); + + return encryptedData; } catch (e) { const errorMessage = e instanceof Error ? e.message : JSON.stringify(e); throw new Error(`Unable to encrypt string - ${errorMessage}`); @@ -72,14 +85,24 @@ class EncryptorDecryptor { nativeScryptCrypto?: NativeScrypt, ): Promise { try { + const startTimestamp = Date.now(); + console.warn( + `Scrypt removal: Decrypting string with password: ${password} at ${new Date(startTimestamp).toISOString()}`, + ); const encryptedData: EncryptedPayload = JSON.parse(encryptedDataStr); - if (encryptedData.v === '1') { + + if (['1', '2'].includes(encryptedData.v)) { if (encryptedData.t === 'scrypt') { - return await this.#decryptStringV1( + const decryptedData = await this.#decryptStringV1( encryptedData, password, nativeScryptCrypto, ); + + console.warn( + `Scrypt removal: Decryption completed in ${Date.now() - startTimestamp} ms (using V${encryptedData.v} encryption)`, + ); + return decryptedData; } } throw new Error( @@ -95,11 +118,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, @@ -119,11 +145,11 @@ class EncryptorDecryptor { const encryptedData = byteArrayToBase64(ciphertextAndNonceAndSalt); const encryptedPayload: EncryptedPayload = { - v: '1', + v: '2', t: 'scrypt', d: encryptedData, o: { - N: SCRYPT_N, + N: scryptOverrides.N, r: SCRYPT_r, p: SCRYPT_p, dkLen: ALGORITHM_KEY_SIZE, From 2bb8758eed499aedd05a248c526e146a6dc1266a Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 10 Jul 2025 14:02:44 +0200 Subject: [PATCH 02/20] refresh hash From 8ebaf2b122e0e3755b0a2ff506a5e9c2a45026f1 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 11 Jul 2025 11:37:33 +0200 Subject: [PATCH 03/20] fix: stay backwards compatible --- .../src/sdk/user-storage.test.ts | 110 ++++++++++++++++-- .../src/sdk/user-storage.ts | 14 +-- .../src/shared/encryption/encryption.test.ts | 33 +++--- .../src/shared/encryption/encryption.ts | 46 ++------ 4 files changed, 128 insertions(+), 75 deletions(-) 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 2023f9c32b0..3a0e5be8b9f 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,7 +18,9 @@ import { import type { StorageOptions } from './user-storage'; import { STORAGE_URL, UserStorage } from './user-storage'; import encryption, { createSHA256Hash } from '../shared/encryption'; +import { SCRYPT_N_V2, SHARED_SALT } 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'; @@ -91,11 +91,12 @@ describe('User Storage', () => { expect(response).toBe(data); }); - it('re-encrypts data if received entry was encrypted with v1 encryption params, and saves it back to user storage', async () => { + it('re-encrypts data if received entry was encrypted with a random salt, and saves it back to user storage', async () => { const { auth } = arrangeAuth('SRP', MOCK_SRP); const { userStorage } = arrangeUserStorage(auth); // This corresponds to 'data1' + // Encrypted with a random 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}', @@ -113,9 +114,48 @@ describe('User Storage', () => { return; } - // Verify that the data was re-encrypted to V2 format - const parsed = JSON.parse(requestBody.data); - expect(parsed.v).toBe('2'); + const isEncryptedUsingSharedSalt = + encryption.getSalt(requestBody.data).toString() === + SHARED_SALT.toString(); + + expect(isEncryptedUsingSharedSalt).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}', + }; + + 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); }, ); @@ -148,7 +188,7 @@ describe('User Storage', () => { expect(responseAllFeatureEntries).toStrictEqual([data]); }); - it('re-encrypts data if received entries were encrypted with v1 encryption, and saves it back to user storage', async () => { + it('re-encrypts data if received entries were encrypted with random salts, 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 @@ -180,13 +220,13 @@ describe('User Storage', () => { return; } - // All re-encrypted entries should be V2 - const doEntriesUseV2Encryption = Object.entries(requestBody.data).every( + const doEntriesUseSharedSalt = Object.entries(requestBody.data).every( ([_entryKey, entryValue]) => - JSON.parse(entryValue as string).v === '2', + encryption.getSalt(entryValue as string).toString() === + SHARED_SALT.toString(), ); - expect(doEntriesUseV2Encryption).toBe(true); + expect(doEntriesUseSharedSalt).toBe(true); const wereOnlyNonEmptySaltEntriesUploaded = Object.entries(requestBody.data).length === 2; @@ -202,6 +242,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 3ac8f0203a2..106b512b129 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -317,13 +317,9 @@ export class UserStorage { options?.nativeScryptCrypto, ); - // Migrate data from v1 to v2 encryption - if (JSON.parse(encryptedData).v === '1') { - const reEncryptedData = await encryption.encryptString( - decryptedData, - storageKey, - ); - await this.#upsertUserStorage(path, reEncryptedData, options); + // Data migration + if (encryption.doesEntryNeedReEncryption(encryptedData)) { + await this.#upsertUserStorage(path, decryptedData, options); } return decryptedData; @@ -390,8 +386,8 @@ export class UserStorage { ); decryptedData.push(data); - // Migrate data from v1 to v2 encryption - if (JSON.parse(entry.Data).v === '1') { + // Data migration + if (encryption.doesEntryNeedReEncryption(entry.Data)) { const reEncryptedData = await encryption.encryptString( data, storageKey, 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 434706b58da..d83022e3986 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts @@ -1,3 +1,4 @@ +import { SCRYPT_N, SCRYPT_N_V2 } from './constants'; import encryption, { createSHA256Hash } from './encryption'; describe('encryption tests', () => { @@ -81,34 +82,26 @@ 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}', - ]; + 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.getIfEntriesHaveDifferentSalts(entries); + 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 the shared salt and the new scrypt N parameter', () => { + const entry = `{"v":"1","t":"scrypt","d":"AAECAwQFBgcICQoLDA0ODzGY11N8GhRoaD6YMqLp/sZnRHaG9gbNEyASWZSok/xBZhAOw2hKOH6qieSQSCYmtZZjqZ6lxfEfsYyS2ivGhUrVQmJYSXpr78As4Bc7pnLQACPfLJqiFwDVRG4Lf/k+DpfKzBmdS1h+nOiTHaN8MmMY6jKkfjVqnJSEkvKcQBnOBw27+PW8L1OQBXITaImO1GOE4OOBjfD4XX7uezBrsv0TuFWeDumSzYqDnw==","o":{"N":${SCRYPT_N_V2},"r":8,"p":1,"dkLen":16},"saltLen":16}`; - 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 c53d3dcc7c7..6a08792afd8 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -29,7 +29,7 @@ import type { NativeScrypt } from '../types/encryption'; export type EncryptedPayload = { // version - v: '1' | '2'; + v: '1'; // key derivation function algorithm - scrypt t: 'scrypt'; @@ -56,11 +56,7 @@ class EncryptorDecryptor { nativeScryptCrypto?: NativeScrypt, ): Promise { try { - const startTimestamp = Date.now(); - console.warn( - `Scrypt removal: Encrypting string with password: ${password} at ${new Date(startTimestamp).toISOString()}`, - ); - const encryptedData = await this.#encryptStringV1( + return await this.#encryptStringV1( plaintext, password, nativeScryptCrypto, @@ -68,11 +64,6 @@ class EncryptorDecryptor { N: SCRYPT_N_V2, }, ); - console.warn( - `Scrypt removal: Encryption completed in ${Date.now() - startTimestamp} ms`, - ); - - return encryptedData; } catch (e) { const errorMessage = e instanceof Error ? e.message : JSON.stringify(e); throw new Error(`Unable to encrypt string - ${errorMessage}`); @@ -85,24 +76,15 @@ class EncryptorDecryptor { nativeScryptCrypto?: NativeScrypt, ): Promise { try { - const startTimestamp = Date.now(); - console.warn( - `Scrypt removal: Decrypting string with password: ${password} at ${new Date(startTimestamp).toISOString()}`, - ); const encryptedData: EncryptedPayload = JSON.parse(encryptedDataStr); - if (['1', '2'].includes(encryptedData.v)) { + if (encryptedData.v === '1') { if (encryptedData.t === 'scrypt') { - const decryptedData = await this.#decryptStringV1( + return await this.#decryptStringV1( encryptedData, password, nativeScryptCrypto, ); - - console.warn( - `Scrypt removal: Decryption completed in ${Date.now() - startTimestamp} ms (using V${encryptedData.v} encryption)`, - ); - return decryptedData; } } throw new Error( @@ -145,7 +127,7 @@ class EncryptorDecryptor { const encryptedData = byteArrayToBase64(ciphertextAndNonceAndSalt); const encryptedPayload: EncryptedPayload = { - v: '2', + v: '1', t: 'scrypt', d: encryptedData, o: { @@ -222,19 +204,13 @@ 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 { + const doesEntryHaveRandomSalt = + this.getSalt(encryptedDataStr).toString() !== SHARED_SALT.toString(); + const doesEntryUseOldScryptN = + JSON.parse(encryptedDataStr).o?.N !== SCRYPT_N_V2; - const strSet = new Set(salts.map((arr) => arr.toString())); - return strSet.size === salts.length; + return doesEntryHaveRandomSalt || doesEntryUseOldScryptN; } #encrypt(plaintext: Uint8Array, key: Uint8Array): Uint8Array { From 58eee07e74cd016f35f4ae8cdd3135a608f2b1ba Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 11 Jul 2025 12:30:13 +0200 Subject: [PATCH 04/20] feat: add encryption callbacks --- .../UserStorageController.test.ts | 63 +++++++++++++++++++ .../user-storage/UserStorageController.ts | 13 ++++ .../src/sdk/user-storage.ts | 39 +++++++----- .../src/shared/encryption/encryption.test.ts | 45 ++++++++++++- .../src/shared/encryption/encryption.ts | 33 ++++++++-- 5 files changed, 171 insertions(+), 22 deletions(-) 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 588fff2e15a..c801d82e161 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 @@ -17,6 +17,13 @@ import * as AccountSyncControllerIntegrationModule from './account-syncing/contr 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('user-storage/user-storage-controller - constructor() tests', () => { @@ -936,3 +943,59 @@ describe('user-storage/user-storage-controller - snap handling', () => { expect(await controller.getStorageKey()).toBe(MOCK_STORAGE_KEY); }); }); + +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 27f44d30b76..24496e01244 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -44,6 +44,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, @@ -147,6 +148,10 @@ const metadata: StateMetadata = { type ControllerConfig = { env: Env; + encryption?: { + onEncrypt?: (encryptedData: Omit) => Promise; + onDecrypt?: (encryptedData: Omit) => Promise; + }; accountSyncing?: { maxNumberOfAccountsToAdd?: number; /** @@ -490,6 +495,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, }); } @@ -508,6 +515,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, }); } @@ -528,6 +537,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, }); } @@ -548,6 +559,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.ts b/packages/profile-sync-controller/src/sdk/user-storage.ts index 106b512b129..3a414e20c1c 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -1,5 +1,6 @@ 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 type { Env } from '../shared/env'; import { getEnvUrls } from '../shared/env'; @@ -38,6 +39,8 @@ export type GetUserStorageAllFeatureEntriesResponse = { export type UserStorageMethodOptions = { nativeScryptCrypto?: NativeScrypt; entropySourceId?: string; + onEncrypt?: (encryptedData: Omit) => Promise; + onDecrypt?: (encryptedData: Omit) => Promise; }; type ErrorMessage = { @@ -140,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)); @@ -195,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, + }), ]; }), ); @@ -314,7 +315,10 @@ export class UserStorage { const decryptedData = await encryption.decryptString( encryptedData, storageKey, - options?.nativeScryptCrypto, + { + nativeScryptCrypto: options?.nativeScryptCrypto, + onDecrypt: options?.onDecrypt, + }, ); // Data migration @@ -379,11 +383,10 @@ 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); // Data migration @@ -391,6 +394,10 @@ export class UserStorage { const reEncryptedData = await encryption.encryptString( data, storageKey, + { + nativeScryptCrypto: options?.nativeScryptCrypto, + onEncrypt: options?.onEncrypt, + }, ); reEncryptedEntries.push([entry.HashedKey, reEncryptedData]); } 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 d83022e3986..b14e618b862 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,11 @@ -import { SCRYPT_N, SCRYPT_N_V2 } from './constants'; +import { + ALGORITHM_KEY_SIZE, + SCRYPT_N, + SCRYPT_N_V2, + SCRYPT_p, + SCRYPT_r, + SCRYPT_SALT_SIZE, +} from './constants'; import encryption, { createSHA256Hash } from './encryption'; describe('encryption tests', () => { @@ -82,6 +89,42 @@ describe('encryption tests', () => { expect(decryptedData).toBe(ACCOUNTS_DECRYPTED_DATA); }); + 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); + }); + describe('doesEntryNeedReEncryption()', () => { it('should return true if entry uses a random salt', () => { const entry = diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index 6a08792afd8..b7d42f6e54a 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -53,17 +53,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}`); @@ -73,17 +86,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, ); } } From 7b31eceb261b5270ccbaeac857dcadf04873f252 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 11 Jul 2025 12:33:39 +0200 Subject: [PATCH 05/20] fix: update changelog --- packages/profile-sync-controller/CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index afdda630926..e8bdfec03f5 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Dumb down the `N` `scrypt` parameter in order to increase performance on clients + - This decreases the impact of the KDF on all clients to around 99% + - Add backwards compatible migration logic + - Add encryption callbacks so we can measure the impact of future migrations through analytics + ## [21.0.0] ### Added From ea7474e2f69a4ac072e21e70ecc3aa8e73b42b3e Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 16 Jul 2025 13:55:12 +0200 Subject: [PATCH 06/20] fix: better changelog entry --- packages/profile-sync-controller/CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index e8bdfec03f5..57d7b0473cc 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -9,8 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Dumb down the `N` `scrypt` parameter in order to increase performance on clients - - This decreases the impact of the KDF on all clients to around 99% +- Reduce the `N` `scrypt` parameter used to compute the encryption key in order to significantly increase performance on clients. + - 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 From a384bb9d54a1fa0796cd2d11e022d70999fb30a2 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 16 Jul 2025 21:10:40 +0200 Subject: [PATCH 07/20] fix: add more precision to the KDF cache index --- .../profile-sync-controller/src/shared/encryption/encryption.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index b7d42f6e54a..bebedac0bff 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -263,7 +263,7 @@ class EncryptorDecryptor { salt?: Uint8Array, nativeScryptCrypto?: NativeScrypt, ) { - const hashedPassword = createSHA256Hash(password); + const hashedPassword = `${createSHA256Hash(password)}.${o.N}.${o.r}.${o.p}.${o.dkLen}`; const cachedKey = salt ? getCachedKeyBySalt(hashedPassword, salt) : getCachedKeyGeneratedWithSharedSalt(hashedPassword); From c2d112dc330ff55622ae1126555b6ab20ed115cd Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 17 Jul 2025 09:30:22 +0200 Subject: [PATCH 08/20] fix: add cache index UT for entries with same password but different encryption parameters --- .../src/shared/encryption/encryption.test.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) 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 b14e618b862..36aa6741a2b 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts @@ -40,6 +40,25 @@ 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 () => { + const data1EncryptedWithOldN = + '{"v":"1","t":"scrypt","d":"AAECAwQFBgcICQoLDA0OD5rMiKCTz61h5abF98yn50UPflCq6Ozov3NAk+y4h6o5bp0jJLJ0rw==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}'; + const data1EncryptedWithNewN = + '{"v":"1","t":"scrypt","d":"AAECAwQFBgcICQoLDA0ODx8V+QwHALZFtAw/DhXD9ev78fT327jgGXlB7/kXeZQc6zaRqw6VgA==","o":{"N":2,"r":8,"p":1,"dkLen":16},"saltLen":16}'; + + 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 = From f4a7cba082b57e6b2519246fbc5c60c5938b5c6c Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 17 Jul 2025 12:25:43 +0200 Subject: [PATCH 09/20] Update packages/profile-sync-controller/src/shared/encryption/encryption.ts Co-authored-by: Mircea Nistor --- .../profile-sync-controller/src/shared/encryption/encryption.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index bebedac0bff..04ea8cea3e0 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -263,7 +263,7 @@ class EncryptorDecryptor { salt?: Uint8Array, nativeScryptCrypto?: NativeScrypt, ) { - const hashedPassword = `${createSHA256Hash(password)}.${o.N}.${o.r}.${o.p}.${o.dkLen}`; + const hashedPassword = createSHA256Hash(`${password}.${o.N}.${o.r}.${o.p}.${o.dkLen}`); const cachedKey = salt ? getCachedKeyBySalt(hashedPassword, salt) : getCachedKeyGeneratedWithSharedSalt(hashedPassword); From 78a4fef213ddd89c970b5aaa2ef65904e42c473a Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 17 Jul 2025 13:00:48 +0200 Subject: [PATCH 10/20] fix: do not hash cache indexes --- .../profile-sync-controller/src/shared/encryption/encryption.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index 04ea8cea3e0..a04b45a1d5c 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -263,7 +263,7 @@ class EncryptorDecryptor { salt?: Uint8Array, nativeScryptCrypto?: NativeScrypt, ) { - const hashedPassword = createSHA256Hash(`${password}.${o.N}.${o.r}.${o.p}.${o.dkLen}`); + const hashedPassword = `${password}.${o.N}.${o.r}.${o.p}.${o.dkLen}`; const cachedKey = salt ? getCachedKeyBySalt(hashedPassword, salt) : getCachedKeyGeneratedWithSharedSalt(hashedPassword); From 052e332edb0fb8132ddb2e91ed077e304c5b37ea Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 17 Jul 2025 13:21:50 +0200 Subject: [PATCH 11/20] fix: cursor feedback --- .../src/shared/encryption/encryption.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index a04b45a1d5c..e749ec80438 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -228,12 +228,16 @@ class EncryptorDecryptor { } doesEntryNeedReEncryption(encryptedDataStr: string): boolean { - const doesEntryHaveRandomSalt = - this.getSalt(encryptedDataStr).toString() !== SHARED_SALT.toString(); - const doesEntryUseOldScryptN = - JSON.parse(encryptedDataStr).o?.N !== SCRYPT_N_V2; - - return doesEntryHaveRandomSalt || doesEntryUseOldScryptN; + try { + const doesEntryHaveRandomSalt = + this.getSalt(encryptedDataStr).toString() !== SHARED_SALT.toString(); + const doesEntryUseOldScryptN = + JSON.parse(encryptedDataStr).o?.N !== SCRYPT_N_V2; + + return doesEntryHaveRandomSalt || doesEntryUseOldScryptN; + } catch { + return false; + } } #encrypt(plaintext: Uint8Array, key: Uint8Array): Uint8Array { From f584cf030cd7efb60e1cfe15b6b13170dea2d34d Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 17 Jul 2025 20:54:26 +0200 Subject: [PATCH 12/20] fix: use salt_v2 to prevent cache collisions --- .../src/shared/encryption/cache.ts | 26 ---------------- .../src/shared/encryption/constants.ts | 4 +++ .../src/shared/encryption/encryption.test.ts | 11 +++++-- .../src/shared/encryption/encryption.ts | 30 +++++++++---------- 4 files changed, 27 insertions(+), 44 deletions(-) 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 d2cf5b7ffd9..ed751bbc65a 100644 --- a/packages/profile-sync-controller/src/shared/encryption/constants.ts +++ b/packages/profile-sync-controller/src/shared/encryption/constants.ts @@ -13,3 +13,7 @@ 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, +]); 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 36aa6741a2b..14763d7f240 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts @@ -41,10 +41,15 @@ describe('encryption tests', () => { }); 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 = - '{"v":"1","t":"scrypt","d":"AAECAwQFBgcICQoLDA0ODx8V+QwHALZFtAw/DhXD9ev78fT327jgGXlB7/kXeZQc6zaRqw6VgA==","o":{"N":2,"r":8,"p":1,"dkLen":16},"saltLen":16}'; + + // Generate encrypted data with new format dynamically + const data1EncryptedWithNewN = await encryption.encryptString( + DATA1, + PASSWORD, + ); const decrypted1 = await encryption.decryptString( data1EncryptedWithOldN, @@ -160,7 +165,7 @@ describe('encryption tests', () => { expect(result).toBe(true); }); - it('should return false if entry uses the shared salt and the new scrypt N parameter', () => { + it('should return false if entry uses new scrypt N parameter', () => { const entry = `{"v":"1","t":"scrypt","d":"AAECAwQFBgcICQoLDA0ODzGY11N8GhRoaD6YMqLp/sZnRHaG9gbNEyASWZSok/xBZhAOw2hKOH6qieSQSCYmtZZjqZ6lxfEfsYyS2ivGhUrVQmJYSXpr78As4Bc7pnLQACPfLJqiFwDVRG4Lf/k+DpfKzBmdS1h+nOiTHaN8MmMY6jKkfjVqnJSEkvKcQBnOBw27+PW8L1OQBXITaImO1GOE4OOBjfD4XX7uezBrsv0TuFWeDumSzYqDnw==","o":{"N":${SCRYPT_N_V2},"r":8,"p":1,"dkLen":16},"saltLen":16}`; const result = encryption.doesEntryNeedReEncryption(entry); diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index e749ec80438..ba09edc0cbf 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -4,11 +4,7 @@ import { scryptAsync } from '@noble/hashes/scrypt'; import { sha256 } from '@noble/hashes/sha256'; 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, @@ -18,6 +14,7 @@ import { SCRYPT_r, SCRYPT_SALT_SIZE, SHARED_SALT, + SHARED_SALT_V2, } from './constants'; import { base64ToByteArray, @@ -229,12 +226,12 @@ class EncryptorDecryptor { doesEntryNeedReEncryption(encryptedDataStr: string): boolean { try { - const doesEntryHaveRandomSalt = - this.getSalt(encryptedDataStr).toString() !== SHARED_SALT.toString(); - const doesEntryUseOldScryptN = - JSON.parse(encryptedDataStr).o?.N !== SCRYPT_N_V2; + const encryptedData: EncryptedPayload = JSON.parse(encryptedDataStr); - return doesEntryHaveRandomSalt || doesEntryUseOldScryptN; + // Only check N value - in production, only two valid scenarios exist: + // 1. N:2**17 + SHARED_SALT (old code) + // 2. N:2 + SHARED_SALT_V2 (new code) + return encryptedData.o?.N !== SCRYPT_N_V2; } catch { return false; } @@ -267,10 +264,13 @@ class EncryptorDecryptor { salt?: Uint8Array, nativeScryptCrypto?: NativeScrypt, ) { - const hashedPassword = `${password}.${o.N}.${o.r}.${o.p}.${o.dkLen}`; - const cachedKey = salt - ? getCachedKeyBySalt(hashedPassword, salt) - : getCachedKeyGeneratedWithSharedSalt(hashedPassword); + const hashedPassword = createSHA256Hash(password); + + // Determine which salt to use (for both lookup and generation) + const targetSalt = + salt ?? (o.N === SCRYPT_N_V2 ? SHARED_SALT_V2 : SHARED_SALT); + + const cachedKey = getCachedKeyBySalt(hashedPassword, targetSalt); if (cachedKey) { return { @@ -279,7 +279,7 @@ class EncryptorDecryptor { }; } - const newSalt = salt ?? SHARED_SALT; + const newSalt = targetSalt; let newKey: Uint8Array; From 227bac3113ae0b1beaf917118a10e0d6977877de Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 17 Jul 2025 21:19:22 +0200 Subject: [PATCH 13/20] fix: UTs --- .../src/sdk/user-storage.test.ts | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) 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 3a0e5be8b9f..6dffcd4d442 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.test.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.test.ts @@ -18,7 +18,11 @@ import { import type { StorageOptions } from './user-storage'; import { STORAGE_URL, UserStorage } from './user-storage'; import encryption, { createSHA256Hash } from '../shared/encryption'; -import { SCRYPT_N_V2, SHARED_SALT } from '../shared/encryption/constants'; +import { + SCRYPT_N_V2, + SHARED_SALT, + 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'; @@ -114,11 +118,15 @@ 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(isEncryptedUsingSharedSalt).toBe(true); + expect(isEncryptedUsingSharedSaltV2).toBe(true); + expect(isEncryptedUsingNewScryptN).toBe(true); }, ); @@ -220,13 +228,19 @@ describe('User Storage', () => { return; } - 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(), ); - expect(doEntriesUseSharedSalt).toBe(true); + const doEntriesUseNewScryptN = Object.entries(requestBody.data).every( + ([_entryKey, entryValue]) => + JSON.parse(entryValue as string).o.N === SCRYPT_N_V2, + ); + + expect(doEntriesUseSharedSaltV2).toBe(true); + expect(doEntriesUseNewScryptN).toBe(true); const wereOnlyNonEmptySaltEntriesUploaded = Object.entries(requestBody.data).length === 2; From 00abdb8d8d613e647308f5868c6d9c66d6cdfef2 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Thu, 17 Jul 2025 21:24:52 +0200 Subject: [PATCH 14/20] fix: lint --- .../profile-sync-controller/src/sdk/user-storage.test.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) 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 6dffcd4d442..0e6086fff21 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.test.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.test.ts @@ -18,11 +18,7 @@ import { import type { StorageOptions } from './user-storage'; import { STORAGE_URL, UserStorage } from './user-storage'; import encryption, { createSHA256Hash } from '../shared/encryption'; -import { - SCRYPT_N_V2, - SHARED_SALT, - SHARED_SALT_V2, -} 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'; From 1982229e66697f98fd55ba07cb53d718c8059f43 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 18 Jul 2025 10:29:13 +0200 Subject: [PATCH 15/20] fix: tests --- .../src/shared/encryption/encryption.test.ts | 4 ++-- .../src/shared/encryption/encryption.ts | 11 +++++------ 2 files changed, 7 insertions(+), 8 deletions(-) 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 0bfdefae343..2f461dd7b78 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts @@ -164,8 +164,8 @@ describe('encryption tests', () => { expect(result).toBe(true); }); - it('should return false if entry uses new scrypt N parameter', () => { - const entry = `{"v":"1","t":"scrypt","d":"AAECAwQFBgcICQoLDA0ODzGY11N8GhRoaD6YMqLp/sZnRHaG9gbNEyASWZSok/xBZhAOw2hKOH6qieSQSCYmtZZjqZ6lxfEfsYyS2ivGhUrVQmJYSXpr78As4Bc7pnLQACPfLJqiFwDVRG4Lf/k+DpfKzBmdS1h+nOiTHaN8MmMY6jKkfjVqnJSEkvKcQBnOBw27+PW8L1OQBXITaImO1GOE4OOBjfD4XX7uezBrsv0TuFWeDumSzYqDnw==","o":{"N":${SCRYPT_N_V2},"r":8,"p":1,"dkLen":16},"saltLen":16}`; + it('should return false if entry uses new scrypt N parameter', async () => { + const entry = await encryption.encryptString(DATA1, PASSWORD); 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 cab98a9281a..6f1866c410c 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -228,10 +228,10 @@ class EncryptorDecryptor { try { const encryptedData: EncryptedPayload = JSON.parse(encryptedDataStr); - // Only check N value - in production, only two valid scenarios exist: - // 1. N:2**17 + SHARED_SALT (old code) - // 2. N:2 + SHARED_SALT_V2 (new code) - return encryptedData.o?.N !== SCRYPT_N_V2; + return ( + encryptedData.o?.N !== SCRYPT_N_V2 || + this.getSalt(encryptedDataStr).toString() !== SHARED_SALT_V2.toString() + ); } catch { return false; } @@ -266,8 +266,7 @@ class EncryptorDecryptor { ) { const hashedPassword = createSHA256Hash(password); - const targetSalt = - salt ?? (o.N === SCRYPT_N_V2 ? SHARED_SALT_V2 : SHARED_SALT); + const targetSalt = salt ?? SHARED_SALT_V2; const cachedKey = getCachedKeyBySalt(hashedPassword, targetSalt); From 7b3f699c57f386ce6c3532b10bc18d3366d7baca Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 18 Jul 2025 11:06:35 +0200 Subject: [PATCH 16/20] fix: lint --- .../profile-sync-controller/src/shared/encryption/encryption.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index 6f1866c410c..c16d9542ec8 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -13,7 +13,6 @@ import { SCRYPT_p, SCRYPT_r, SCRYPT_SALT_SIZE, - SHARED_SALT, SHARED_SALT_V2, } from './constants'; import { From 03ea8543cb5ead752c49eba6f3bf4b89e5ac830e Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 18 Jul 2025 12:15:37 +0200 Subject: [PATCH 17/20] fix: add specificity to cache indexes --- .../src/shared/encryption/encryption.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index c16d9542ec8..0c6a8eaff86 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -263,7 +263,9 @@ class EncryptorDecryptor { salt?: Uint8Array, nativeScryptCrypto?: NativeScrypt, ) { - const hashedPassword = createSHA256Hash(password); + const hashedPassword = createSHA256Hash( + `${password}.${o.N}.${o.r}.${o.p}.${o.dkLen}`, + ); const targetSalt = salt ?? SHARED_SALT_V2; From 4e89f5381cea2f5c1c14b991c91c398e39272c22 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 8 Oct 2025 14:55:06 +0200 Subject: [PATCH 18/20] fix: improve logic --- .../src/shared/encryption/cache.ts | 26 ---------- .../src/shared/encryption/encryption.ts | 52 ++++++++----------- 2 files changed, 21 insertions(+), 57 deletions(-) diff --git a/packages/profile-sync-controller/src/shared/encryption/cache.ts b/packages/profile-sync-controller/src/shared/encryption/cache.ts index 9fb9c666a91..628716ed722 100644 --- a/packages/profile-sync-controller/src/shared/encryption/cache.ts +++ b/packages/profile-sync-controller/src/shared/encryption/cache.ts @@ -1,5 +1,4 @@ import { byteArrayToBase64 } from './utils'; -import { SHARED_SALT } from './constants'; type CachedEntry = { salt: Uint8Array; @@ -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/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index 116cc6bb576..66e35207de1 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -1,14 +1,10 @@ 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, @@ -275,18 +271,18 @@ class EncryptorDecryptor { salt?: Uint8Array, nativeScryptCrypto?: NativeScrypt, ) { - // Use enhanced password hashing that includes scrypt params to prevent cache collisions - // between different scrypt configurations (important for security when supporting multiple N values) - const hashedPassword = createSHA256Hash( - `${password}.${o.N}.${o.r}.${o.p}.${o.dkLen}`, - ); - const targetSalt = salt ?? SHARED_SALT_V2; - // Check if we already have the key cached - const cachedKey = salt - ? getCachedKeyBySalt(hashedPassword, salt) - : getCachedKeyBySalt(hashedPassword, targetSalt); + // 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 { @@ -295,15 +291,6 @@ class EncryptorDecryptor { }; } - // Create a unique cache key for this KDF operation to prevent duplicate work - const newSalt = targetSalt; - 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) { @@ -323,8 +310,8 @@ class EncryptorDecryptor { const kdfPromise = this.#performKdfOperation( password, o, - newSalt, - hashedPassword, + targetSalt, + cacheKey, nativeScryptCrypto, ); @@ -341,21 +328,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; @@ -378,7 +368,7 @@ class EncryptorDecryptor { }); } - setCachedKey(hashedPassword, salt, newKey); + setCachedKey(cacheKey, salt, newKey); return { key: newKey, From b17c41d8186bbf51035653a22ede9dc8b15d9b8b Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 8 Oct 2025 15:13:37 +0200 Subject: [PATCH 19/20] fix: lint issue --- .../profile-sync-controller/src/shared/encryption/encryption.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index 66e35207de1..a9d645de0d8 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -14,7 +14,6 @@ import { SCRYPT_p, SCRYPT_r, SCRYPT_SALT_SIZE, - SHARED_SALT, SHARED_SALT_V2, } from './constants'; import { From b5439f2c94a50d0f43bfd9cea4cebcce8988464a Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Wed, 8 Oct 2025 15:28:36 +0200 Subject: [PATCH 20/20] fix: update changelog --- packages/profile-sync-controller/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 9ff3062c03c..a8bbefbc0e3 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Reduce the `N` `scrypt` parameter used to compute the encryption key in order to significantly increase performance on clients. +- 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