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 <noreply@anthropic.com>
This commit is contained in:
+533
@@ -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
|
hh:nn:ss'` vs export JS `toISOString()` avec `T…Z`) sur les rares rows sans
|
||||||
`created_at`.
|
`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
|
### 2.3 🟡 `catch (_) {}` silencieux en cascade
|
||||||
|
|
||||||
Nombreux `catch (_) {}` dans `applyRemoteSnapshot`, `buildSyncSnapshot`,
|
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=<token>junk` passe, et un cookie nommé `xpm_token=<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<string>;
|
||||||
|
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
|
## 7. Verdict
|
||||||
|
|
||||||
Pour un usage **personnel, local, mono-utilisateur**, le produit est
|
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
|
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
|
« sûr pour du solo loopback » à « durci pour un modèle de menace DB/sync qui
|
||||||
fuit ».
|
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.
|
||||||
|
|||||||
@@ -941,122 +941,6 @@ async function hibpCheckAllEntries() {
|
|||||||
render();
|
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
|
// ACCOUNT LOCKOUT UI
|
||||||
// ============================================================
|
// ============================================================
|
||||||
@@ -1242,10 +1126,11 @@ async function doLogin(e) {
|
|||||||
await persistCryptoKey();
|
await persistCryptoKey();
|
||||||
toast('Welcome back, ' + u);
|
toast('Welcome back, ' + u);
|
||||||
await enterApp();
|
await enterApp();
|
||||||
// Trigger KDF migration AFTER entries are loaded into state.
|
// KDF auto-migration (100k→600k) removed 2026-07-16: it changed the AES
|
||||||
if (r.kdfMigration && r.kdfMigration.target) {
|
// key but only re-encrypted encrypted_password, corrupting the 7 other
|
||||||
runKdfMigration(p, r.kdfIterations, r.kdfMigration.target);
|
// 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) {
|
} catch (err) {
|
||||||
// 429 with retry_after = account lockout. Show countdown in the
|
// 429 with retry_after = account lockout. Show countdown in the
|
||||||
// auth hint instead of a generic error toast, and keep the login
|
// auth hint instead of a generic error toast, and keep the login
|
||||||
@@ -1440,9 +1325,7 @@ async function doUnlock(p) {
|
|||||||
$('#authHint').textContent = '';
|
$('#authHint').textContent = '';
|
||||||
toast('Unlocked');
|
toast('Unlocked');
|
||||||
await enterApp();
|
await enterApp();
|
||||||
if (r.kdfMigration && r.kdfMigration.target) {
|
// KDF auto-migration removed — see the note in the login path (§6c-1).
|
||||||
runKdfMigration(p, r.kdfIterations, r.kdfMigration.target);
|
|
||||||
}
|
|
||||||
return true;
|
return true;
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
// Account lockout (too many wrong master pw attempts): show
|
// Account lockout (too many wrong master pw attempts): show
|
||||||
|
|||||||
Reference in New Issue
Block a user