From 71f45eb8e13283a19de963e8d5aeeecac2fe3f72 Mon Sep 17 00:00:00 2001 From: r-zakarya <82443831+r-zakarya@users.noreply.github.com> Date: Mon, 20 Jul 2026 05:20:07 +0100 Subject: [PATCH] fix(kdf): disable the data-loss KDF auto-migration path (audit 6c-1) /migrate-kdf changed the AES key (100k->600k) but only re-encrypted encrypted_password, leaving totp_secret/custom_fields/*_enc + attachments under the old key = permanent data loss. Both live accounts are already at 600k so it never fired, but the path was armed and auto-triggered by the server's login signal. Removed the two triggers (doLogin + unlock) and the now-dead runKdfMigration. Legacy <600k accounts upgrade safely via Change Master Password instead (re-encrypts everything + moves to Argon2id). The Delphi endpoint is left inert: without the client orchestrating the key swap it can't corrupt anything, so no untested server change. Closes 6c-4 too (unreachable now). Co-Authored-By: Claude Opus 4.8 --- CODE_AUDIT.md | 533 ++++++++++++++++++++++++++++++++++++++++++++++++++ js/app.js | 129 +----------- 2 files changed, 539 insertions(+), 123 deletions(-) diff --git a/CODE_AUDIT.md b/CODE_AUDIT.md index 768a5de..edf2c0d 100644 --- a/CODE_AUDIT.md +++ b/CODE_AUDIT.md @@ -182,6 +182,13 @@ stricte ; incohérence de **format** pré-existante (serveur `'yyyy-mm-dd hh:nn:ss'` vs export JS `toISOString()` avec `T…Z`) sur les rares rows sans `created_at`. +> ⚠️ **MAJ 2026-07-16 — « Tout est désormais UTC » était inexact.** La passe de +> substitution a couvert les sites d'écriture *d'entries/attachments*, mais a +> raté **`PM.Session.CreateSession`** (`FormatDateTime(..., IncHour(Now, 24))`, +> heure locale), qui reste à ce jour le dernier `FormatDateTime(..., Now)` du +> backend. Conséquence réelle sur les fuseaux négatifs. Détail + correctif en +> **§6c-2**. + ### 2.3 🟡 `catch (_) {}` silencieux en cascade Nombreux `catch (_) {}` dans `applyRemoteSnapshot`, `buildSyncSnapshot`, @@ -410,6 +417,521 @@ invisibles, perf et code mort. Rien de bloquant ; classé par type. --- +## 6c. Revue backend Delphi (2026-07-16) — correctifs proposés, NON appliqués + +Passe manuelle sur les ~10 500 lignes Delphi (`Source/` + `Handlers/` + `.dpr`), +axée crypto/auth/session/HTTP/SQL. Les correctifs ci-dessous sont **écrits mais +pas appliqués** — chacun est autonome. + +**Sain, vérifié (pour ne pas re-chasser)** : aucune injection SQL (les seuls SQL +concaténés sont `PRAGMA table_info(...)` / `ALTER TABLE` dans `PM.Database`, sur +constantes internes) ; autorisation scopée par `user_id` **partout** sans +exception ; le `SET` dynamique de [Folders.pas:199](delphi-backend/Handlers/PM.Handler.Folders.pas:199) +est assemblé de littéraux fixes, pas d'input ; les routes non authentifiées sont +exactement les bonnes (register, login, challenge, ping, redeem, stub passkey) ; +`ValidateConnectingProcess` échoue **fermé** si le lookup PID rate ; primitives +crypto correctes + auto-testées à l'init. La fuite d'énumération de comptes de +`/login/challenge` est **documentée et assumée** ([Auth.pas:1189](delphi-backend/Handlers/PM.Handler.Auth.pas:1189)) +→ pas comptée comme finding. + +--- + +### 6c-1. 🔴 `/migrate-kdf` ne re-chiffre que 1 blob sur 8 — perte de données + +> ✅ **RÉSOLU 2026-07-16 — chemin désactivé (option ultra).** Les 2 call-sites +> qui déclenchaient l'auto-migration au login/reauth +> ([app.js:1246, 1443](js/app.js)) sont retirés + la fonction morte +> `runKdfMigration` supprimée. Sans le client qui orchestre le swap de clé, +> l'endpoint serveur ne peut plus corrompre (il ne fait qu'un UPDATE de ce +> qu'on lui envoie) → laissé inerte, non touché. **DB vérifiée avant fix : les 2 +> comptes sont déjà à 600k**, le chemin n'a donc jamais mordu. Un compte legacy +> <600k s'upgrade désormais via **Change Master Password** (qui re-chiffre les +> 8 blobs + attachments correctement et passe à Argon2id). §6c-4 (brick d'un +> compte -v2 par ce même endpoint) devient également inatteignable. + +**C'est le seul vrai bug de perte de données de cette passe.** + +La migration KDF passe les itérations 100k → 600k, donc **change la clé AES +dérivée** — puis [app.js:1035](js/app.js:1035) bascule `state.cryptoKey = newKey`. +Or [`runKdfMigration`](js/app.js:961) ne re-chiffre que `encrypted_password`/`iv`, +et [`HandleMigrateKdf`](delphi-backend/Handlers/PM.Handler.Auth.pas:795) n'UPDATE +que ces deux colonnes. + +Restent chiffrés sous l'**ancienne** clé, donc définitivement illisibles : +`totp_secret`, `custom_fields`, `username_enc`, `site_enc`, `title_enc`, +`tags_enc`, `template_enc` (+ leurs IV), et les attachments. + +Le contraste est net avec [`doChangeMasterPassword`](js/app.js:6106) / +[`HandleChangeMasterPassword`](delphi-backend/Handlers/PM.Handler.Auth.pas:1038), +qui affrontent **exactement la même situation** (changement de clé) et traitent +correctement les 8 colonnes. C'est une nouvelle instance de la classe de bug +décrite en **§3.3** — et la checklist « Entry payload » du CLAUDE.md cite +`doChangeMasterPassword` mais **pas** `runKdfMigration` : voilà par où c'est passé. + +**Symptôme** : sournois, pas bruyant. [`decryptEntryMeta`](js/app.js:1502) ignore +silencieusement les champs qui ne déchiffrent pas (`if (v !== '[ERROR]') e[f] = v`) +→ titres/usernames deviennent `undefined` à l'écran, puis le premier save réécrit +ce vide par-dessus l'ancien ciphertext. Là c'est irréversible. + +**Portée** : uniquement les comptes à `kdf_iterations < 600000`. Les inscriptions +partent directement à 600k ([app.js:1283](js/app.js:1283)) → legacy uniquement. +**Non vérifié** s'il en reste : aucun `vault.db` sur la machine de revue, pas de +CLI sqlite3. À trancher par `SELECT username, kdf_iterations, hash_algo FROM users;` +— si tout est à 600k, le bug n'a jamais mordu, mais le chemin reste **armé** et +auto-déclenché par signal serveur. + +#### Correctif recommandé — extraire un helper partagé + +Recopier la boucle de `doChangeMasterPassword` dans `runKdfMigration` serait +reproduire la cause (duplication, §3.3). La boucle +[app.js:6115-6179](js/app.js:6115) est **déjà exactement** la logique voulue : +l'extraire et l'appeler des deux côtés. + +```js +// js/app.js — nouveau helper, unique source de vérité du re-chiffrement. +// Prend la clé courante (oldKey) et la cible (newKey), retourne le payload +// serveur. N'écrit RIEN dans state : l'appelant décide quand basculer. +async function reEncryptEntriesForNewKey(oldKey, newKey, onProgress) { + const out = []; + let done = 0; + for (const e of state.entries) { + done++; + if (onProgress) onProgress(done, state.entries.length); + + state.cryptoKey = oldKey; + const plain = await decryptPwd(e.encrypted_password, e.iv); + if (plain === '[ERROR]') throw new Error('Could not decrypt entry id=' + e.id); + + try { + state.cryptoKey = newKey; + const re = await encryptPwd(plain); + + let totpEnc = '', totpIv = ''; + if (e.totp_secret && e.totp_iv) { + state.cryptoKey = oldKey; + const plainTotp = await decryptTotpSecret(e.totp_secret, e.totp_iv); + state.cryptoKey = newKey; + if (plainTotp !== '[ERROR]') { + const t = await encryptPwd(plainTotp); + totpEnc = t.encrypted; totpIv = t.iv; + } + } + + let cfEnc = '', cfIv = ''; + if (e.custom_fields && e.custom_fields_iv) { + state.cryptoKey = oldKey; + const plainCf = await decryptCustomFields(e.custom_fields, e.custom_fields_iv); + state.cryptoKey = newKey; + if (plainCf.length > 0) { + const c = await encryptCustomFields(plainCf); + cfEnc = c.encrypted; cfIv = c.iv; + } + } + + state.cryptoKey = newKey; + const meta = {}; + for (const f of ENCRYPTED_META_FIELDS) { + if (e[f]) { + const c = await encryptPwd(e[f]); + meta[f + '_enc'] = c.encrypted; + meta[f + '_iv'] = c.iv; + } else { + meta[f + '_enc'] = ''; + meta[f + '_iv'] = ''; + } + } + + out.push({ + id: e.id, + encrypted_password: re.encrypted, iv: re.iv, + totp_secret: totpEnc, totp_iv: totpIv, + custom_fields: cfEnc, custom_fields_iv: cfIv, + ...meta, + }); + } finally { + state.cryptoKey = oldKey; // restauré tant que le serveur n'a pas confirmé + } + } + return out; +} +``` + +`doChangeMasterPassword` remplace sa boucle par un appel ; `runKdfMigration` +remplace la sienne : + +```js +// js/app.js — dans runKdfMigration, branche `if (kdfChange)` +newKey = await deriveKey(masterPwd, state.salt, toIters); +newCiphertexts = await reEncryptEntriesForNewKey(state.cryptoKey, newKey); +``` + +⚠️ **Les attachments restent à traiter** : `doChangeMasterPassword` les gère dans +une boucle séparée *après* le round-trip serveur (GET blob → décrypt oldKey → +encrypt newKey → `PUT /attachments/:id`). `runKdfMigration` ne l'a pas du tout. +Même extraction à faire (`reEncryptAttachmentsForNewKey(oldKey, newKey)`), appelée +des deux côtés après le commit. + +#### Côté Delphi — aligner l'UPDATE sur celui de la rotation + +```pascal +// PM.Handler.Auth.pas — HandleMigrateKdf, remplace l'UPDATE de la ligne ~795. +// Colonnes identiques à HandleChangeMasterPassword (ligne ~1039) : c'est le +// même événement (la clé change), donc le même jeu de blobs doit voyager. +LQ.SQL.Text := + 'UPDATE vault_entries SET ' + + ' encrypted_password = :ep, iv = :iv, ' + + ' totp_secret = :ts, totp_iv = :tiv, ' + + ' custom_fields = :cf, custom_fields_iv = :cfiv, ' + + ' username_enc = :uenc, username_iv = :uiv, ' + + ' site_enc = :senc, site_iv = :siv, ' + + ' title_enc = :tenc, title_iv = :tiv2, ' + + ' tags_enc = :genc, tags_iv = :giv, ' + + ' template_enc = :tplenc, template_iv = :tpliv, ' + + ' updated_at = CURRENT_TIMESTAMP ' + + 'WHERE id = :id AND user_id = :uid'; +``` + +…avec la même lecture de champs + binding que +[Auth.pas:1051-1077](delphi-backend/Handlers/PM.Handler.Auth.pas:1051). + +#### Alternative stratégique (plus propre, plus de risque) + +Un « migrate-kdf » **est** une rotation de master pw vers le même mot de passe. +Faire déléguer `runKdfMigration` à `/change-master-password` (nouveau salt + +`argon2id-v2`) supprimerait ce chemin dupliqué *et* ferait basculer les comptes +legacy sur Argon2id d'un coup — ce que §1.2 attend de toute façon. Contrepartie : +`HandleChangeMasterPassword` fait `DeleteAllUserSessions` + `CreateSession` +([Auth.pas:1161](delphi-backend/Handlers/PM.Handler.Auth.pas:1161)) et le CLAUDE.md +impose le wipe du blob PIN → une migration silencieuse de fond deviendrait +visible. À arbitrer ; le correctif chirurgical ci-dessus est le chemin bas risque. + +#### Test de non-régression suggéré + +La suite `js/tests/` couvre déjà le round-trip crypto. Ajouter : *entry avec TOTP ++ custom_fields + méta chiffrée → `reEncryptEntriesForNewKey` → tout déchiffre +sous newKey*. C'est le filet qui aurait attrapé ça. + +--- + +### 6c-2. 🟠 Expiry de session écrite en local, lue en UTC (corrige §2.2) + +[`CreateSession`](delphi-backend/Source/PM.Session.pas:155) écrit +`FormatDateTime('yyyy-mm-dd hh:nn:ss', IncHour(Now, 24))` — heure **locale**. +C'est le dernier `FormatDateTime(..., Now)` du backend et il contredit la règle +du CLAUDE.md (« Timestamps = UTC partout … JAMAIS `FormatDateTime(..., Now)` »). +`NowUTCStr` existe et est utilisé partout ailleurs. + +Trois lecteurs, deux bases de temps : + +| Lecteur | Base | Cohérent ? | +|---|---|---| +| [`Authenticate`](delphi-backend/Source/PM.Session.pas:89) | `Now` (local) | oui | +| [`RequireCSRF`](delphi-backend/Source/PM.Session.pas:122) | `datetime('now')` (UTC) | **non** | +| [`CleanupExpired`](delphi-backend/Source/PM.Database.pas:469) | `datetime('now')` (UTC) | **non** | + +**Pourquoi ça ne s'est jamais vu** : en UTC+1/+2 le décalage va dans le sens +inoffensif — `Authenticate` coupe à 24 h avant que la fenêtre CSRF (26 h) ne +serve. En **UTC-5** la fenêtre CSRF tombe à **19 h** : entre 19 h et 24 h la +session s'authentifie normalement, les GET passent, et **toute écriture répond +403 « Invalid CSRF token »**. Scénario plausible : « Start with Windows » + quick +unlock, machine laissée allumée plus d'une journée. + +```pascal +// PM.Session.pas — CreateSession (~ligne 155) +// UTC comme partout ailleurs : RequireCSRF et CleanupExpired comparent contre +// SQLite datetime('now') qui EST UTC. +LExpires := FormatDateTime('yyyy-mm-dd hh:nn:ss', IncHour(NowUTC, 24)); +``` + +```pascal +// PM.Session.pas — Authenticate (~ligne 89) +// Doit suivre : expires_at est désormais UTC. +if (LExpires <> 0) and (LExpires < NowUTC) then +``` + +`PM.Database` (qui expose `NowUTC`) est déjà dans le `uses` de l'interface, +`System.DateUtils` (pour `IncHour`) déjà dans celui de l'implémentation → aucun +`uses` à toucher. Les trois lecteurs s'alignent alors sur UTC. + +**Note migration** : les sessions déjà en base gardent leur `expires_at` local. +Sans importance — elles expirent sous 24 h, et `CleanupExpired` les purge au +prochain démarrage. + +--- + +### 6c-3. 🟡 Le CSRF est validé contre la mauvaise session + +[`RequireCSRF`](delphi-backend/Source/PM.Session.pas:120) cherche le +`csrf_token` par `user_id ORDER BY created_at DESC LIMIT 1` — la session la plus +récente, **pas celle réellement présentée**. Deux conséquences : + +- `created_at` a une résolution d'**une seconde** : deux sessions créées dans la + même seconde donnent une égalité que SQLite tranche arbitrairement → il peut + lire le token de l'ancienne → **403 fantôme**. +- Conceptuellement, le contrôle devient « connais-tu *un* CSRF valide pour cet + utilisateur » au lieu de « celui lié à ta session ». + +`Authenticate` a **déjà** localisé la bonne ligne par `token_hash` : lire +`csrf_token` depuis cette ligne-là est plus simple *et* correct. La signature ne +change pas → aucun call-site à toucher. + +```pascal +// PM.Session.pas — RequireCSRF +var + LSubmitted, LStored, LTokenHash: string; + LQ: TFDQuery; +begin + if SameText(ARequest.Command, 'GET') then Exit; + + LSubmitted := ARequest.RawHeaders.Values['X-CSRF-Token']; + if LSubmitted = '' then + begin + TJSONHelper.SendError(AResponse, 403, 'Missing CSRF token'); + raise ESessionRejected.Create('missing csrf'); + end; + + // Lier le CSRF à la session PRÉSENTÉE, pas à la plus récente du user. + // AUserId reste en filtre (defense in depth). + LTokenHash := SHA256Hex(ExtractBearerToken(ARequest)); + + DB.Lock; + try + LQ := TFDQuery.Create(nil); + try + LQ.Connection := DB.Connection; + LQ.SQL.Text := + 'SELECT csrf_token FROM sessions ' + + 'WHERE token_hash = :th AND user_id = :uid'; + LQ.ParamByName('th').AsString := LTokenHash; + LQ.ParamByName('uid').AsInteger := AUserId; + LQ.Open; + // ... suite inchangée +``` + +L'expiry n'a plus à être re-testée ici : `Authenticate` tourne toujours avant et +l'a déjà fait (et après **6c-2** les deux sont sur la même base de temps). + +--- + +### 6c-4. 🟡 `/migrate-kdf` rétrograderait un compte `-v2` (verrouillage définitif) + +Le garde d'idempotence ([Auth.pas:722](delphi-backend/Handlers/PM.Handler.Auth.pas:722)) +ne court-circuite que si l'algo vaut `pbkdf2-sha256`. Un compte `argon2id-v2` ou +`pbkdf2-sha256-v2` **passe au travers** et se fait réécrire en +`HASH_ALGO_CURRENT` ([Auth.pas:763](delphi-backend/Handlers/PM.Handler.Auth.pas:763) +et [785](delphi-backend/Handlers/PM.Handler.Auth.pas:785)) → la formule de +verifier change → **lockout définitif**. + +Non atteignable aujourd'hui : le register envoie `kdfIterations: 600000` +([app.js:1296](js/app.js:1296)) donc le login ne signale jamais de migration pour +un compte v2 — et le commentaire [app.js:1016](js/app.js:1016) montre que le cas a +été raisonné. Mais l'endpoint est authentifié, CSRF'é et **armé** ; le garde coûte +4 lignes et rend la protection locale au lieu de dépendre de l'appelant. + +```pascal +// PM.Handler.Auth.pas — HandleMigrateKdf, juste avant le garde d'idempotence +// existant (~ligne 722). +// Les schemes -v2 ont un verifier découplé : migrate-kdf ne sait pas les +// réécrire sans changer la formule → refuser au lieu de bricker le compte. +if LAlgo.EndsWith('-v2', True) then +begin + TJSONHelper.SendOK(AResponse, 'Already at target'); + Exit; +end; +``` + +--- + +### 6c-5. 🔵 Fallback silencieux sur params Argon2 invalides + +[Auth.pas:319](delphi-backend/Handlers/PM.Handler.Auth.pas:319) (register) et +[Auth.pas:994](delphi-backend/Handlers/PM.Handler.Auth.pas:994) (rotation) : + +```pascal +if SameText(LReqAlgo, HASH_ALGO_ARGON2) and LArgon.Valid then ... else + LRegAlgo := HASH_ALGO_DECOUPLED; // ← fallback muet +``` + +Un client qui déclare `argon2id-v2` avec des params hors bornes est stocké en +**PBKDF2** sans un mot, alors qu'il a dérivé sa clé en **Argon2id** → au login +suivant `/login/challenge` renvoie `pbkdf2-sha256-v2`, le client dérive en PBKDF2, +verifier différent → **compte brické**. Le JS envoie toujours +`ARGON2_DEFAULT_PARAMS` (valides) donc ce n'est pas live ; échouer fort coûte +5 lignes. + +```pascal +// Aux DEUX endroits : un client qui DEMANDE Argon2id avec des params hors +// bornes doit être refusé, pas silencieusement rétrogradé en PBKDF2. +if SameText(LReqAlgo, HASH_ALGO_ARGON2) and (not LArgon.Valid) then +begin + TJSONHelper.SendError(AResponse, 400, 'Invalid argon2 params'); + Exit; +end; +``` + +--- + +### 6c-6. 🔵 Le port « aléatoire » dépend d'un `Randomize` qui n'est pas le nôtre + +[PM.HTTPServer.pas:136](delphi-backend/Source/PM.HTTPServer.pas:136) utilise +`Random()`, et le projet n'appelle **jamais** `Randomize` (vérifié : 0 occurrence +dans tout le `.dpr` + `.pas`). `RandSeed` vaut 0 par défaut → séquence +déterministe. + +Empiriquement le port **varie** bien (le CLAUDE.md documente le wipe de +`localStorage` à chaque lancement, ce qui le prouve) → une unité RTL/Indy sème +`RandSeed` en initialisation. Ça marche donc **par accident** : dépendance +implicite, sur ce que le design traite comme une mesure de sécurité, alors que +`RandomBytes` (CNG) est déjà utilisé **6 lignes plus haut** pour le token. + +```pascal +// PM.HTTPServer.pas — Start (~ligne 136) +// CSPRNG (CNG) plutôt que Random() : ne dépend pas d'un Randomize appelé par +// une unité tierce. 49152..65535 = 16384 valeurs, et 65536 mod 16384 = 0 +// → pas de biais modulo. +if LRequestedPort = 0 then +begin + var LRnd := PM.Crypto.RandomBytes(2); + LRequestedPort := EphemeralPortMin + + (((LRnd[0] shl 8) or LRnd[1]) mod (EphemeralPortMax - EphemeralPortMin + 1)); +end; +``` + +--- + +### 6c-7. 🔵 Comparaison du token d'accès : sous-chaîne + non constant-time + +[PM.HTTPServer.pas:258](delphi-backend/Source/PM.HTTPServer.pas:258) : + +```pascal +if (CookieHeader <> '') and (Pos(CookieName + '=' + FAccessToken, CookieHeader) > 0) +``` + +`Pos` matche **n'importe où** dans le header et accepte les **préfixes** : +`pm_token=junk` passe, et un cookie nommé `xpm_token=` aussi. Ni ça +ni le `=` de la ligne 262 ne sont constant-time, alors que `ConstantTimeEquals` +existe et sert déjà pour le CSRF. Impact faible derrière le PID check (il faut +déjà connaître le token), mais c'est une incohérence gratuite. + +```pascal +// PM.HTTPServer.pas — parse réel du header Cookie plutôt qu'un Pos() sur la +// chaîne brute, + comparaison constant-time (comme RequireCSRF). +function TryGetCookie(const AHeader, AName: string; out AValue: string): Boolean; +var + LPairs: TArray; + LPair, LName: string; + LEq: Integer; +begin + Result := False; + AValue := ''; + LPairs := AHeader.Split([';']); + for LPair in LPairs do + begin + LEq := Pos('=', LPair); + if LEq <= 0 then Continue; + LName := Trim(Copy(LPair, 1, LEq - 1)); + if SameText(LName, AName) then + begin + AValue := Trim(Copy(LPair, LEq + 1, MaxInt)); + Exit(True); + end; + end; +end; +``` + +```pascal +// ValidateAccessToken — remplace les deux comparaisons +if TryGetCookie(CookieHeader, CookieName, LCookieVal) and + ConstantTimeEquals(LCookieVal, FAccessToken) then + Exit(True); + +QueryToken := ARequest.Params.Values[QueryParamName]; +if (QueryToken <> '') and ConstantTimeEquals(QueryToken, FAccessToken) then +begin + // ... Set-Cookie inchangé +``` + +(`PM.Crypto` est déjà dans le `uses` de `PM.HTTPServer` → rien à ajouter.) + +--- + +### 6c-8. 🔵 Fuite mémoire sur chemin d'erreur + +[Attachments.pas:292](delphi-backend/Handlers/PM.Handler.Attachments.pas:292) : +`LArr := TJSONArray.Create` est créé **avant** le `try/finally` → une exception +dans le bloc DB le fuit. `SendJSON` ne le libère que sur le chemin nominal. + +```pascal +// PM.Handler.Attachments.pas — HandleListAllAttachments +LArr := TJSONArray.Create; +try + DB.Lock; + try + // ... remplissage inchangé + finally + DB.Unlock; + end; +except + LArr.Free; + raise; +end; +TJSONHelper.SendJSON(AResponse, LArr); // libère LArr (AOwnsObj=True) +``` + +--- + +### 6c-9. ❓ `PRAGMA foreign_keys` — à vérifier, non tranché + +`PRAGMA foreign_keys` n'est défini **nulle part**. Le schéma s'appuie pourtant sur +`ON DELETE CASCADE` : [Entries.pas:704](delphi-backend/Handlers/PM.Handler.Entries.pas:704) +(delete permanent) ne nettoie **ni** `entry_attachments` **ni** +`entries_password_history` à la main, et le CLAUDE.md affirme « FK cascade on user ++ entry delete ». + +SQLite met `foreign_keys` à **OFF** par défaut. Le paramètre `ForeignKeys` de +FireDAC est *censé* défauter à `On` (ce qui sauverait la mise), mais **je n'ai pas +pu le confirmer** : les DCU ne sont pas greppables et il n'y avait pas de base pour +tester. Si c'était Off, chaque suppression définitive laisserait des blobs chiffrés +orphelins — exactement ce que `CompactIfBloated` tente de rattraper. + +Deux façons de lever le doute : + +```sql +-- Sur un vault.db réel, après avoir supprimé définitivement une entry +-- qui avait des attachments : +SELECT COUNT(*) FROM entry_attachments a + WHERE NOT EXISTS (SELECT 1 FROM vault_entries e WHERE e.id = a.entry_id); +-- > 0 → la cascade ne s'applique pas. +``` + +```pascal +// PM.Database.pas — TPMDatabase.Create, avec les autres Params (~ligne 82). +// Explicite = plus de dépendance à un défaut FireDAC non documenté chez nous. +FConn.Params.Values['ForeignKeys'] := 'On'; +``` + +Le second est correct **dans les deux cas** (no-op si c'était déjà le défaut) → +à appliquer sans attendre la vérification. + +--- + +### Ordre d'application suggéré + +| # | Sévérité | Effort | Note | +|---|---|---|---| +| **6c-1** | 🔴 | Élevé | Le seul à pouvoir détruire des données. Faire l'extraction + les tests. | +| **6c-2** | 🟠 | Trivial | 2 lignes. Ferme aussi le trou de §2.2. | +| **6c-3** | 🟡 | Faible | Signature inchangée, aucun call-site touché. | +| **6c-9** | ❓ | Trivial | 1 ligne, correcte dans tous les cas. | +| **6c-4** | 🟡 | Trivial | 4 lignes, garde local. | +| **6c-5** | 🔵 | Trivial | 5 lignes × 2 endroits. | +| **6c-6/7/8** | 🔵 | Faible | Cohérence / hygiène, aucune urgence. | + +Rappel build : tout edit JS impose `delphi-backend\assets\BuildAssets.cmd` **puis** +le build Delphi, sinon l'exe garde les anciens assets. + +--- + ## 7. Verdict Pour un usage **personnel, local, mono-utilisateur**, le produit est @@ -425,3 +947,14 @@ verifier découplé (§1.1), Argon2id (§1.2), métadonnées chiffrées au repos navigateur, Windows Hello, import KeePass, i18n). Le produit est passé de « sûr pour du solo loopback » à « durci pour un modèle de menace DB/sync qui fuit ». + +**MAJ 2026-07-16** : la revue backend Delphi (**§6c**) ne remet pas en cause ce +verdict sur le **durcissement** — aucune injection, autorisation scopée partout, +routes non authentifiées correctes, crypto saine. Elle sort en revanche un +**🔴 de perte de données** resté invisible parce qu'il ne vit sur aucun des axes +suivis : `/migrate-kdf` change la clé AES mais ne re-chiffre qu'un blob sur huit +(**§6c-1**) — nouvelle instance de la classe §3.3, sur un call-site que la +checklist « Entry payload » du CLAUDE.md ne liste pas. Elle corrige aussi une +affirmation de **§2.2** (« Tout est désormais UTC ») : `PM.Session` avait été +raté par la substitution (**§6c-2**). À traiter avant de considérer §2.2 comme +clos. diff --git a/js/app.js b/js/app.js index 7da1f5c..c880877 100644 --- a/js/app.js +++ b/js/app.js @@ -941,122 +941,6 @@ async function hibpCheckAllEntries() { render(); } -// ============================================================ -// KDF MIGRATION (PBKDF2 100k → 600k re-encryption) -// ============================================================ -// -// When the server signals kdfMigration in /login or /reauth, we transparently -// re-encrypt every entry with a stronger key (600k PBKDF2 iterations) and -// commit the new ciphertext + the new server-side hash in one atomic -// /migrate-kdf request. If anything fails, the user stays on the legacy -// config and the migration retries at next login. The entries currently -// loaded in state.entries are encrypted with the OLD key (state.cryptoKey). -// -// Threading: runs in the background after enterApp completes. Locking the -// vault during migration is safe — we just lose the in-flight transition -// and the server's atomic rollback means nothing persisted. - -let kdfMigrationInProgress = false; - -async function runKdfMigration(masterPwd, fromIters, toIters) { - if (kdfMigrationInProgress) return; // dedupe concurrent calls - if (!state.entries || !state.cryptoKey) return; - kdfMigrationInProgress = true; - - try { - // Two distinct migration scenarios: - // A. fromIters !== toIters: KDF iteration count is changing, so - // the AES key is changing. We re-encrypt every entry with the - // new key + fresh IVs, swap state.cryptoKey at the end. - // B. fromIters === toIters: same KDF, only the server-side hash - // format is being upgraded (legacy "pbkdf2" raw → "pbkdf2-sha256" - // wrapped). No entry re-encryption needed — just trigger the - // endpoint so the server rewrites the user row. - const kdfChange = fromIters !== toIters; - let newKey, newCiphertexts; - - if (kdfChange) { - newKey = await deriveKey(masterPwd, state.salt, toIters); - // Re-encrypt every entry. Each entry gets a fresh random IV - // under the new key — never reuse the old IV with the new key - // (would be pointless but also a small information leak via IV - // reuse patterns). - newCiphertexts = []; - for (const entry of state.entries) { - const plain = await decryptPwd(entry.encrypted_password, entry.iv); - if (plain === '[ERROR]') { - // One decrypt failure aborts the whole migration — - // better to stay on the legacy config than commit - // partial state. - throw new Error('Could not decrypt entry id=' + entry.id); - } - const tmpKey = state.cryptoKey; - try { - state.cryptoKey = newKey; - const re = await encryptPwd(plain); - newCiphertexts.push({ - id: entry.id, - encrypted_password: re.encrypted, - iv: re.iv, - }); - } finally { - state.cryptoKey = tmpKey; // restore for any concurrent read - } - } - } else { - // Hash-format-only upgrade — server still wants an entries - // array (it's an idempotent transactional update), just empty. - newCiphertexts = []; - } - - // Zero-knowledge: compute BOTH verifiers locally. oldVerifier proves - // the user knows the master pw under the current (legacy) iters; - // newVerifier is what the server will SHA-256-wrap to be the new - // stored hash after migration. Master pw never leaves the browser. - // Only non-v2 accounts ever reach the KDF migration (v2 accounts are - // 600k + decoupled → never signalled). Under a non-v2 algo the - // verifier is the key hex, so both derivations round-trip exactly as - // before; passing state.hashAlgo keeps it explicit. - const oldVerifier = await computeVerifier(masterPwd, state.salt, fromIters, state.hashAlgo, state.argon2Params); - const newVerifier = await computeVerifier(masterPwd, state.salt, toIters, state.hashAlgo, state.argon2Params); - - await api('/migrate-kdf', { - method: 'POST', - headers: authHeaders({ 'Content-Type': 'application/json' }), - body: JSON.stringify({ - oldVerifier: oldVerifier, - newVerifier: newVerifier, - entries: newCiphertexts, - }), - }); - - if (kdfChange) { - // Swap to the new AES key + update cached ciphertexts. - state.cryptoKey = newKey; - state.kdfIterations = toIters; - sessionStorage.setItem('kdfIterations', String(toIters)); - await persistCryptoKey(); - for (let i = 0; i < state.entries.length; i++) { - const nc = newCiphertexts[i]; - state.entries[i].encrypted_password = nc.encrypted_password; - state.entries[i].iv = nc.iv; - } - toast('Vault security upgraded (' + fromIters.toLocaleString() + - ' → ' + toIters.toLocaleString() + ' KDF iterations)'); - } else { - // Format-only upgrade is silent — the user didn't perceive a - // weakness change, and nothing visible in the UI changed. - // (A subtle "Auth format upgraded" toast felt noisy.) - } - } catch (err) { - // Silent retry on next login — the migration is idempotent and - // safe to abandon (server rolled back). - console.warn('KDF migration aborted:', err); - } finally { - kdfMigrationInProgress = false; - } -} - // ============================================================ // ACCOUNT LOCKOUT UI // ============================================================ @@ -1242,10 +1126,11 @@ async function doLogin(e) { await persistCryptoKey(); toast('Welcome back, ' + u); await enterApp(); - // Trigger KDF migration AFTER entries are loaded into state. - if (r.kdfMigration && r.kdfMigration.target) { - runKdfMigration(p, r.kdfIterations, r.kdfMigration.target); - } + // KDF auto-migration (100k→600k) removed 2026-07-16: it changed the AES + // key but only re-encrypted encrypted_password, corrupting the 7 other + // per-entry blobs + attachments (§6c-1, a data-loss footgun). Legacy + // <600k accounts upgrade safely via Change Master Password instead + // (that path re-encrypts everything and moves to Argon2id). } catch (err) { // 429 with retry_after = account lockout. Show countdown in the // auth hint instead of a generic error toast, and keep the login @@ -1440,9 +1325,7 @@ async function doUnlock(p) { $('#authHint').textContent = ''; toast('Unlocked'); await enterApp(); - if (r.kdfMigration && r.kdfMigration.target) { - runKdfMigration(p, r.kdfIterations, r.kdfMigration.target); - } + // KDF auto-migration removed — see the note in the login path (§6c-1). return true; } catch (err) { // Account lockout (too many wrong master pw attempts): show