fix: use salt_v2 to prevent cache collisions

Author: mathieuartu

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.

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,
+]);

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);

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;