fix: use a shared salt instead of an empty one

packages/profile-sync-controller/src/controllers/user-storage/services.test.ts

@@ -1,4 +1,5 @@
 import encryption, { createSHA256Hash } from '../../shared/encryption';
+import { SHARED_SALT } from '../../shared/encryption/constants';
 import type { UserStorageFeatureKeys } from '../../shared/storage-schema';
 import { USER_STORAGE_FEATURE_NAMES } from '../../shared/storage-schema';
 import { createMockGetStorageResponse } from './__fixtures__';
@@ -84,13 +85,13 @@ describe('user-storage/services.ts - getUserStorage() tests', () => {
     expect(result).toBeNull();
   });
 
-  it('re-encrypts data if received entry was encrypted with a non-empty salt, 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 DECRYPED_DATA = 'data1';
     const INITIAL_ENCRYPTED_DATA = {
       HashedKey: 'entry1',
       Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}',
     };
-    // Encrypted with a non-empty salt
+    // Encrypted with a random salt
     const mockResponse = INITIAL_ENCRYPTED_DATA;
 
     const mockGetUserStorage = await mockEndpointGetUserStorage(
@@ -109,11 +110,11 @@ describe('user-storage/services.ts - getUserStorage() tests', () => {
           return;
         }
 
-        const isNewSaltEmpty =
-          encryption.getSalt(requestBody.data).length === 0;
+        const isEncryptedUsingSharedSalt =
+          encryption.getSalt(requestBody.data).toString() ===
+          SHARED_SALT.toString();
 
-        expect(isNewSaltEmpty).toBe(true);
-        expect(JSON.parse(requestBody.data).saltLen).toBe(0);
+        expect(isEncryptedUsingSharedSalt).toBe(true);
       },
     );
 
@@ -145,10 +146,10 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', (
     expect(result).toStrictEqual([MOCK_STORAGE_DATA]);
   });
 
-  it('re-encrypts data if received entries were encrypted with non-empty salts, 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 non-empty salt, except for the last entry
-    // The last entry is used to test if the function can handle entries with either empty or non-empty salts
+    // 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 entries with both random salts and the shared salt
     const mockResponse = [
       {
         HashedKey: 'entry1',
@@ -185,12 +186,13 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', (
 
         expect(doEntriesHaveDifferentSalts).toBe(false);
 
-        const doEntriesHaveEmptySalts = Object.entries(requestBody.data).every(
+        const doEntriesUseSharedSalt = Object.entries(requestBody.data).every(
           ([_entryKey, entryValue]) =>
-            encryption.getSalt(entryValue as string).length === 0,
+            encryption.getSalt(entryValue as string).toString() ===
+            SHARED_SALT.toString(),
         );
 
-        expect(doEntriesHaveEmptySalts).toBe(true);
+        expect(doEntriesUseSharedSalt).toBe(true);
 
         const wereOnlyNonEmptySaltEntriesUploaded =
           Object.entries(requestBody.data).length === 2;

packages/profile-sync-controller/src/controllers/user-storage/services.ts

@@ -1,6 +1,7 @@
 import log from 'loglevel';
 
 import encryption, { createSHA256Hash } from '../../shared/encryption';
+import { SHARED_SALT } from '../../shared/encryption/constants';
 import { Env, getEnvUrls } from '../../shared/env';
 import type {
   UserStoragePathWithFeatureAndKey,
@@ -93,9 +94,9 @@ export async function getUserStorage(
       nativeScryptCrypto,
     );
 
-    // Re-encrypt and re-upload the entry if the salt is non-empty
+    // Re-encrypt and re-upload the entry if the salt is random
     const salt = encryption.getSalt(encryptedData);
-    if (salt.length) {
+    if (salt.toString() !== SHARED_SALT.toString()) {
       await upsertUserStorage(decryptedData, opts);
     }
 
@@ -159,9 +160,9 @@ export async function getUserStorageAllFeatureEntries(
         );
         decryptedData.push(data);
 
-        // Re-encrypt the entry if the salt is non-empty
+        // Re-encrypt the entry if the salt is different from the shared one
         const salt = encryption.getSalt(entry.Data);
-        if (salt.length) {
+        if (salt.toString() !== SHARED_SALT.toString()) {
           reEncryptedEntries.push([
             entry.HashedKey,
             await encryption.encryptString(

packages/profile-sync-controller/src/sdk/user-storage.test.ts

@@ -1,4 +1,5 @@
 import encryption, { createSHA256Hash } from '../shared/encryption';
+import { SHARED_SALT } from '../shared/encryption/constants';
 import { Env } from '../shared/env';
 import type { UserStorageFeatureKeys } from '../shared/storage-schema';
 import { USER_STORAGE_FEATURE_NAMES } from '../shared/storage-schema';
@@ -88,12 +89,12 @@ describe('User Storage', () => {
     expect(response).toBe(data);
   });
 
-  it('re-encrypts data if received entry was encrypted with a non-empty salt, 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 non-empty salt
+    // 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}',
@@ -110,10 +111,11 @@ describe('User Storage', () => {
           return;
         }
 
-        const isNewSaltEmpty =
-          encryption.getSalt(requestBody.data).length === 0;
+        const isEncryptedUsingSharedSalt =
+          encryption.getSalt(requestBody.data).toString() ===
+          SHARED_SALT.toString();
 
-        expect(isNewSaltEmpty).toBe(true);
+        expect(isEncryptedUsingSharedSalt).toBe(true);
       },
     );
 
@@ -138,10 +140,10 @@ describe('User Storage', () => {
     expect(responseAllFeatureEntries).toStrictEqual([data]);
   });
 
-  it('re-encrypts data if received entries were encrypted with non-empty salts, 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 non-empty salt, except for the last entry
-    // The last entry is used to test if the function can handle entries with either empty or non-empty salts
+    // 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',
@@ -176,12 +178,13 @@ describe('User Storage', () => {
 
         expect(doEntriesHaveDifferentSalts).toBe(false);
 
-        const doEntriesHaveEmptySalts = Object.entries(requestBody.data).every(
+        const doEntriesUseSharedSalt = Object.entries(requestBody.data).every(
           ([_entryKey, entryValue]) =>
-            encryption.getSalt(entryValue as string).length === 0,
+            encryption.getSalt(entryValue as string).toString() ===
+            SHARED_SALT.toString(),
         );
 
-        expect(doEntriesHaveEmptySalts).toBe(true);
+        expect(doEntriesUseSharedSalt).toBe(true);
 
         const wereOnlyNonEmptySaltEntriesUploaded =
           Object.entries(requestBody.data).length === 2;

packages/profile-sync-controller/src/sdk/user-storage.ts

@@ -1,4 +1,5 @@
 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 {
@@ -277,7 +278,7 @@ export class UserStorage {
         storageKey,
       );
 
-      // Re-encrypt the entry if the salt is non-empty
+      // Re-encrypt the entry if it was encrypted with a random salt
       const salt = encryption.getSalt(encryptedData);
       if (salt.length) {
         await this.#upsertUserStorage(path, decryptedData);
@@ -345,9 +346,9 @@ export class UserStorage {
           const data = await encryption.decryptString(entry.Data, storageKey);
           decryptedData.push(data);
 
-          // Re-encrypt the entry if the salt is non-empty
+          // Re-encrypt the entry was encrypted with a random salt
           const salt = encryption.getSalt(entry.Data);
-          if (salt.length) {
+          if (salt.toString() !== SHARED_SALT.toString()) {
             reEncryptedEntries.push([
               entry.HashedKey,
               await encryption.encryptString(data, storageKey),

packages/profile-sync-controller/src/shared/encryption/cache.ts

@@ -1,3 +1,4 @@
+import { SHARED_SALT } from './constants';
 import { byteArrayToBase64 } from './utils';
 
 type CachedEntry = {
@@ -53,19 +54,20 @@ export function getCachedKeyBySalt(
  * @param hashedPassword - hashed password for cache lookup
  * @returns the cached key
  */
-export function getCachedKeyGeneratedWithoutSalt(
+export function getCachedKeyGeneratedWithSharedSalt(
   hashedPassword: string,
 ): CachedEntry | undefined {
   const cache = getPasswordCache(hashedPassword);
-  const cachedKey = cache.get('');
+  const base64Salt = byteArrayToBase64(SHARED_SALT);
+  const cachedKey = cache.get(base64Salt);
 
   if (!cachedKey) {
     return undefined;
   }
 
   return {
-    salt: new Uint8Array(),
-    base64Salt: '',
+    salt: SHARED_SALT,
+    base64Salt,
     key: cachedKey,
   };
 }

packages/profile-sync-controller/src/shared/encryption/constants.ts

@@ -0,0 +1,14 @@
+// Nonce/Key Sizes
+export const ALGORITHM_NONCE_SIZE = 12; // 12 bytes
+export const ALGORITHM_KEY_SIZE = 16; // 16 bytes
+
+// Scrypt settings
+// 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)
+// eslint-disable-next-line @typescript-eslint/naming-convention
+export const SCRYPT_r = 8; // Block size parameter
+// eslint-disable-next-line @typescript-eslint/naming-convention
+export const SCRYPT_p = 1; // Parallelization parameter
+
+export const SHARED_SALT = new Uint8Array([...Array(SCRYPT_SALT_SIZE).keys()]);

packages/profile-sync-controller/src/shared/encryption/encryption.ts

@@ -7,9 +7,18 @@ import { utf8ToBytes, concatBytes, bytesToHex } from '@noble/hashes/utils';
 import type { NativeScrypt } from '../types/encryption';
 import {
   getCachedKeyBySalt,
-  getCachedKeyGeneratedWithoutSalt,
+  getCachedKeyGeneratedWithSharedSalt,
   setCachedKey,
 } from './cache';
+import {
+  ALGORITHM_KEY_SIZE,
+  ALGORITHM_NONCE_SIZE,
+  SCRYPT_N,
+  SCRYPT_p,
+  SCRYPT_r,
+  SCRYPT_SALT_SIZE,
+  SHARED_SALT,
+} from './constants';
 import {
   base64ToByteArray,
   byteArrayToBase64,
@@ -40,19 +49,6 @@ export type EncryptedPayload = {
   saltLen: number;
 };
 
-// Nonce/Key Sizes
-const ALGORITHM_NONCE_SIZE = 12; // 12 bytes
-const ALGORITHM_KEY_SIZE = 16; // 16 bytes
-
-// Scrypt settings
-// see: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#scrypt
-const SCRYPT_SALT_SIZE = 0; // We don't use a salt
-const SCRYPT_N = 2 ** 17; // CPU/memory cost parameter (must be a power of 2, > 1)
-// eslint-disable-next-line @typescript-eslint/naming-convention
-const SCRYPT_r = 8; // Block size parameter
-// eslint-disable-next-line @typescript-eslint/naming-convention
-const SCRYPT_p = 1; // Parallelization parameter
-
 class EncryptorDecryptor {
   async encryptString(
     plaintext: string,
@@ -246,7 +242,7 @@ class EncryptorDecryptor {
     const hashedPassword = createSHA256Hash(password);
     const cachedKey = salt
       ? getCachedKeyBySalt(hashedPassword, salt)
-      : getCachedKeyGeneratedWithoutSalt(hashedPassword);
+      : getCachedKeyGeneratedWithSharedSalt(hashedPassword);
 
     if (cachedKey) {
       return {
@@ -255,7 +251,7 @@ class EncryptorDecryptor {
       };
     }
 
-    const newSalt = salt ?? new Uint8Array(SCRYPT_SALT_SIZE);
+    const newSalt = salt ?? SHARED_SALT;
 
     let newKey: Uint8Array;