diff --git a/packages/sync-core/README.md b/packages/sync-core/README.md index 0c57e9ce15..0be7ef4ec5 100644 --- a/packages/sync-core/README.md +++ b/packages/sync-core/README.md @@ -29,6 +29,11 @@ const plain = await decrypt(cipher, password); All ciphertexts are base64-encoded for transport. The format is discriminated by length: `< 28` bytes is invalid, `< 44` bytes is unambiguously legacy, `>= 44` bytes is treated as Argon2id with a legacy fallback on auth failure. Do not change this without a versioning migration. +### Salt and IV semantics + +- The **IV** (12 bytes) is freshly random per call. AES-GCM security under a fixed key reduces to IV uniqueness, which this guarantees. +- The **salt** (16 bytes) is derived once per `(process session, password)` pair and reused across every `encrypt`/`encryptWithDerivedKey`/`encryptBatch` call in that session. This is intentional — it lets the session cache amortize the ~500 ms–2 s Argon2id derivation. Two encryptions of the same plaintext within a session therefore share the salt prefix and differ only in IV and ciphertext. Do not assert per-call salt uniqueness in tests. + ### Session key caching `encrypt`/`decrypt`/`encryptBatch`/`decryptBatch` all share three in-memory caches (encrypt key, decrypt key by salt, legacy PBKDF2 key) that survive across sync cycles. Argon2id derivation is expensive (~500–2000 ms on mobile with the default 64 MiB / 3 iterations); the cache turns repeated syncs from minutes into seconds. diff --git a/packages/sync-core/src/encryption.ts b/packages/sync-core/src/encryption.ts index a54a111410..26a8995e86 100644 --- a/packages/sync-core/src/encryption.ts +++ b/packages/sync-core/src/encryption.ts @@ -17,6 +17,21 @@ * by length: < 28 bytes is invalid, < 44 bytes is unambiguously legacy, * >= 44 bytes is treated as Argon2id with a legacy fallback on auth failure. * + * ## Salt and IV semantics + * + * - **IV** is freshly random per call (12 bytes from CSPRNG). AES-GCM + * confidentiality and integrity depend on IV uniqueness under a given key, + * which this guarantees. + * - **Salt** is derived once per (process session, password) and reused + * across every `encrypt()` / `encryptWithDerivedKey()` / `encryptBatch()` + * call in that session. This is intentional: it lets us amortize the + * ~500ms-2s Argon2id derivation across many calls via the session cache. + * A session-stable salt is safe because the derived key never changes for + * the same (salt, password) pair, and AES-GCM security under a fixed key + * reduces to IV uniqueness. Two encryptions of the same plaintext within + * a session therefore share the salt prefix and differ only in IV + + * ciphertext — do not assert otherwise in tests. + * * ## Legacy-decrypt diagnostics * * Two complementary mechanisms surface legacy ciphertext to callers; both are @@ -49,7 +64,6 @@ import { type DerivedKey, deriveKeyFromPassword } from './encryption/argon2'; import { getDecryptCache, getOrDeriveEncryptKey, - hasDecryptCache, setDecryptCache, } from './encryption/session-cache'; import { decryptLegacy } from './encryption/legacy'; @@ -266,8 +280,7 @@ export const decryptBatch = async ( interface ArgonItem { index: number; format: 'argon2'; - cacheKey: string; - salt: Uint8Array; + saltBase64: string; buffer: ArrayBuffer; } interface LegacyItem { @@ -276,9 +289,9 @@ export const decryptBatch = async ( data: string; } - // Phase 1: analyze items, decode once, collect unique salts needing derivation. + // Phase 1: analyze items, decode once, collect every unique salt. const itemAnalysis: Array = []; - const saltsNeedingDerivation = new Map(); + const uniqueSalts = new Map(); for (let i = 0; i < dataItems.length; i++) { const data = dataItems[i]; @@ -296,36 +309,40 @@ export const decryptBatch = async ( const salt = new Uint8Array(buffer, 0, SALT_LENGTH); const saltBase64 = encodeBase64(salt); - const cacheKey = `${passwordHash}:${saltBase64}`; - itemAnalysis.push({ index: i, format: 'argon2', cacheKey, salt, buffer }); + itemAnalysis.push({ index: i, format: 'argon2', saltBase64, buffer }); - if (!hasDecryptCache(cacheKey) && !saltsNeedingDerivation.has(saltBase64)) { - saltsNeedingDerivation.set(saltBase64, salt); + if (!uniqueSalts.has(saltBase64)) { + uniqueSalts.set(saltBase64, salt); } } - // Phase 2: derive keys for unique salts SERIALLY. - // Argon2id is single-threaded and allocates 64MB per derivation; parallel - // derivations via Promise.all only interleave microtasks (no real parallelism) - // and risk OOM on mobile. - for (const [saltBase64, salt] of saltsNeedingDerivation) { + // Phase 2: derive keys for unique salts SERIALLY and keep them in a + // batch-local map. Argon2id is single-threaded and allocates 64MB per + // derivation; parallel derivations via Promise.all only interleave microtasks + // (no real parallelism) and risk OOM on mobile. Holding keys locally also + // ensures Phase 3 cannot see an entry evicted by the LRU session cache + // (capped at SESSION_DECRYPT_CACHE_MAX_SIZE) when a batch contains more + // unique salts than the cache can hold. + const batchKeys = new Map(); + for (const [saltBase64, salt] of uniqueSalts) { const cacheKey = `${passwordHash}:${saltBase64}`; - if (hasDecryptCache(cacheKey)) { - continue; + let key = getDecryptCache(cacheKey); + if (!key) { + key = await deriveKeyFromPassword(password, salt); + setDecryptCache(cacheKey, key); } - const key = await deriveKeyFromPassword(password, salt); - setDecryptCache(cacheKey, key); + batchKeys.set(saltBase64, key); } - // Phase 3: decrypt all items in parallel using cached keys, reusing the - // buffer decoded in phase 1 to avoid a second base64 decode. + // Phase 3: decrypt all items in parallel using batch-local keys, reusing + // the buffer decoded in phase 1 to avoid a second base64 decode. const decryptionPromises = itemAnalysis.map(async (item) => { if (item.format === 'legacy') { return { index: item.index, result: await decryptLegacy(item.data, password) }; } - const key = getDecryptCache(item.cacheKey)!; + const key = batchKeys.get(item.saltBase64)!; try { return { index: item.index, diff --git a/packages/sync-core/src/encryption/session-cache.ts b/packages/sync-core/src/encryption/session-cache.ts index eb767c6d69..46172197c0 100644 --- a/packages/sync-core/src/encryption/session-cache.ts +++ b/packages/sync-core/src/encryption/session-cache.ts @@ -63,9 +63,6 @@ export const getOrDeriveEncryptKey = async (password: string): Promise sessionDecryptKeyCache.get(cacheKey); -export const hasDecryptCache = (cacheKey: string): boolean => - sessionDecryptKeyCache.has(cacheKey); - export const setDecryptCache = (cacheKey: string, key: DerivedKey): void => { if (sessionDecryptKeyCache.size >= SESSION_DECRYPT_CACHE_MAX_SIZE) { const firstKey = sessionDecryptKeyCache.keys().next().value; diff --git a/packages/sync-core/src/encryption/web-crypto.ts b/packages/sync-core/src/encryption/web-crypto.ts index 33a1f03b9b..9662b4814d 100644 --- a/packages/sync-core/src/encryption/web-crypto.ts +++ b/packages/sync-core/src/encryption/web-crypto.ts @@ -136,13 +136,17 @@ export const detectFormat = ( }; /** - * Simple hash of password for cache key comparison (djb2). - * NOT for security — just for cache invalidation when password changes. + * Returns an injective string identifier for a password, used as a cache-map + * key (and as a prefix in decrypt-cache keys like `${id}:${saltBase64}`). + * + * Length-prefixed so concatenation stays unambiguous even if the password + * contains the `:` separator used downstream. Holding the password verbatim + * is safe: these caches live in-memory in the same process that already holds + * the plaintext password. + * + * Earlier versions used a 32-bit djb2 hash; collisions would silently return + * a key derived from a different password, producing undecryptable ciphertext. */ export const hashPasswordForCache = (password: string): string => { - let hash = 5381; - for (let i = 0; i < password.length; i++) { - hash = (hash * 33) ^ password.charCodeAt(i); - } - return (hash >>> 0).toString(16); + return `${password.length}:${password}`; }; diff --git a/packages/sync-core/tests/encryption.spec.ts b/packages/sync-core/tests/encryption.spec.ts index d4f3f0a50a..974b532e99 100644 --- a/packages/sync-core/tests/encryption.spec.ts +++ b/packages/sync-core/tests/encryption.spec.ts @@ -284,6 +284,24 @@ describe('encryption', () => { expect(decrypted[0]).toBe('item-0'); expect(decrypted[49]).toBe('item-49'); }); + + // SESSION_DECRYPT_CACHE_MAX_SIZE is 100; if a batch contains more unique + // salts than the LRU cache can hold, early derivations get evicted before + // Phase 3 reads them. The implementation keeps derived keys in a + // batch-local map to keep this safe. + it('decrypts batches with more unique salts than the LRU cache holds', async () => { + clearSessionKeyCache(); + const COUNT = 120; + const items = Array.from({ length: COUNT }, (_, i) => `item-${i}`); + const encrypted: string[] = []; + for (let i = 0; i < COUNT; i++) { + // Fresh derivation per item → unique salt per ciphertext + const key = await deriveKeyFromPassword(PASSWORD); + encrypted.push(await encryptWithDerivedKey(items[i], key)); + } + const decrypted = await decryptBatch(encrypted, PASSWORD); + expect(decrypted).toEqual(items); + }); }); describe('Session key caching', () => { diff --git a/packages/sync-providers/src/super-sync/super-sync.ts b/packages/sync-providers/src/super-sync/super-sync.ts index c52cecce07..323e26d19b 100644 --- a/packages/sync-providers/src/super-sync/super-sync.ts +++ b/packages/sync-providers/src/super-sync/super-sync.ts @@ -853,8 +853,10 @@ export class SuperSyncProvider }); if (!response.ok) { - clearTimeout(timeoutId); + // Keep the abort timer running across response.text() — a server/proxy + // that sends an error status and stalls the body must still be aborted. const errorText = await response.text().catch(() => ''); + clearTimeout(timeoutId); // Check for auth failure FIRST before throwing generic error this._checkHttpStatus(response.status, errorText); const reason = this._extractServerErrorReason(errorText, response.status);