From 370432f6381bfcbcb022d32115834942200e1f57 Mon Sep 17 00:00:00 2001 From: r-zakarya <82443831+r-zakarya@users.noreply.github.com> Date: Mon, 20 Jul 2026 05:43:39 +0100 Subject: [PATCH] fix(kdf): neutralize the /migrate-kdf endpoint server-side (6c-1 / 6c-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 6c-1 removed the client caller, but the server handler stayed armed: a stray authed+CSRF'd call could still re-encrypt only 1 blob of 8 (data loss) or rewrite a -v2 account's hash_algo (permanent lockout, 6c-4). Gutted the ~165 destructive lines and replaced the body with an auth+CSRF gate then HTTP 410. Both risks are now closed server-side, not merely unreachable by convention. The vestigial kdfMigration login signal is left (client ignores it, endpoint 410s) — removing it would cascade through a login-path param for no benefit. Fixed the now-false comments pointing at migrate-kdf. Co-Authored-By: Claude Opus 4.8 --- CODE_AUDIT.md | 15 +- delphi-backend/Handlers/PM.Handler.Auth.pas | 200 ++------------------ 2 files changed, 27 insertions(+), 188 deletions(-) diff --git a/CODE_AUDIT.md b/CODE_AUDIT.md index 213d722..969692b 100644 --- a/CODE_AUDIT.md +++ b/CODE_AUDIT.md @@ -729,11 +729,14 @@ 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) -> ⛔ **SANS OBJET depuis 6c-1 (2026-07-16).** Le chemin migrate-kdf est -> désactivé côté client (fonction `runKdfMigration` supprimée, triggers -> retirés). L'endpoint reste inerte (rien ne l'appelle) → ce scénario de brick -> n'est plus atteignable. Garde à ré-ajouter uniquement si migrate-kdf revenait -> un jour. +> ✅ **RÉSOLU 2026-07-16.** Au-delà de 6c-1 (client), l'endpoint serveur +> `HandleMigrateKdf` a été **vidé de ses ~165 lignes destructrices** et remplacé +> par un reject `410` après auth+CSRF ([Auth.pas:656](delphi-backend/Handlers/PM.Handler.Auth.pas)). +> Un appel isolé ne peut donc plus **ni corrompre** (6c-1) **ni brick** un compte +> -v2 (6c-4) : les deux sont fermés côté serveur, pas seulement « inatteignables +> par convention ». Le signal `kdfMigration` du login reste (vestigial, client +> l'ignore, endpoint = 410) — non retiré car câblé via un paramètre sur le +> hot-path login, churn sans bénéfice. 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 @@ -950,7 +953,7 @@ Le second est correct **dans les deux cas** (no-op si c'était déjà le défaut | **6c-2** | 🟠 | — | ✅ **Fait** — `expires_at` en UTC. Ferme §2.2. | | **6c-3** | 🟡 | — | ✅ **Fait** — CSRF lié à la session présentée (token_hash). | | **6c-9** | ❓ | — | ✅ **Fait** — `ForeignKeys=On` explicite. | -| **6c-4** | 🟡 | — | ⛔ **Sans objet** — l'endpoint migrate-kdf est inatteignable depuis 6c-1. | +| **6c-4** | 🟡 | — | ✅ **Fait** — endpoint migrate-kdf vidé + reject 410 (ferme 6c-1 & 6c-4 côté serveur). | | **6c-5** | 🔵 | Trivial | À faire — 5 lignes × 2 endroits (fail-fast params Argon2). | | **6c-6/7/8** | 🔵 | Faible | À faire — cohérence / hygiène, aucune urgence. | diff --git a/delphi-backend/Handlers/PM.Handler.Auth.pas b/delphi-backend/Handlers/PM.Handler.Auth.pas index fe7ddbe..704e49e 100644 --- a/delphi-backend/Handlers/PM.Handler.Auth.pas +++ b/delphi-backend/Handlers/PM.Handler.Auth.pas @@ -206,8 +206,10 @@ begin LObj.AddPair('csrfToken', ACSRFToken); // kdfIterations is the iteration count the client must use when deriving // the AES-GCM key for THIS session — matches the count under which the - // existing entries are encrypted. If the server signals migration, the - // client should re-encrypt with the new target and call /migrate-kdf. + // existing entries are encrypted. NOTE: the kdfMigration signal below is + // now VESTIGIAL — /migrate-kdf was disabled (§6c-1) and the client ignores + // it. Legacy accounts upgrade via /change-master-password instead. Kept to + // avoid churning the login hot path; harmless (endpoint returns 410). LObj.AddPair('kdfIterations', TJSONNumber.Create(AKdfIterations)); if ANeedsMigration then begin @@ -643,26 +645,18 @@ begin end; // ===== /migrate-kdf ========================================================== -// Atomic transition from an old PBKDF2 iteration count to the current target. -// Client side: derive both old and new AES keys, decrypt each entry with old, -// re-encrypt with new, then POST the new ciphertext blob to this endpoint -// along with the master password (so we can recompute the new server hash). -// Server side: verify the master pw with the old hash, then in a single -// transaction: update users.password_hash to the new PBKDF2 output, set -// kdf_iterations to TARGET, and replace each entry's encrypted_password/iv. -// All-or-nothing: if anything fails, the user stays on the old config. +// DISABLED 2026-07-16 (§6c-1 / §6c-4). This path re-derived the AES key +// (100k -> 600k) but re-encrypted ONLY encrypted_password — totp_secret, +// custom_fields, the *_enc metadata + attachments stayed under the old key = +// data loss — and it would rewrite a -v2 account's hash_algo, permanently +// locking it out. The client no longer calls it; any stray call is rejected so +// it can neither corrupt nor brick a vault. Legacy <600k accounts upgrade +// safely via /change-master-password (re-encrypts everything + moves to +// Argon2id). procedure HandleMigrateKdf(ARequest: TIdHTTPRequestInfo; AResponse: TIdHTTPResponseInfo; const AParams: TArray); var - LUserId, LOldIters, I: Integer; - LBody, LEntry: TJSONObject; - LEntries: TJSONArray; - LUser, LPwd, LOldVerifier, LNewVerifier, LSalt, LStoredHash, LAlgo, LIP, - LComputed, LNewHash: string; - LQ: TFDQuery; - LValid: Boolean; - LEntryId: Integer; - LEncPwd, LIv: string; + LUserId: Integer; begin try LUserId := Authenticate(ARequest, AResponse); @@ -670,166 +664,8 @@ begin except on ESessionRejected do Exit; end; - - LIP := GetClientIP(ARequest); - - LBody := TJSONHelper.ReadBody(ARequest); - try - LPwd := LBody.GetValue('masterPassword', ''); - // ZK path: client provides PBKDF2 hex at the OLD iter count (oldVerifier, - // for current-hash verification) AND at the new TARGET iter count - // (newVerifier, for the post-migration stored hash). - LOldVerifier := LBody.GetValue('oldVerifier', ''); - LNewVerifier := LBody.GetValue('newVerifier', ''); - LEntries := LBody.GetValue('entries'); - if LEntries = nil then - begin - TJSONHelper.SendError(AResponse, 400, 'Missing entries array'); - Exit; - end; - - DB.Lock; - try - // Step 1: load current user state. - LQ := TFDQuery.Create(nil); - try - LQ.Connection := DB.Connection; - LQ.SQL.Text := - 'SELECT username, password_hash, salt, hash_algo, kdf_iterations ' + - 'FROM users WHERE id = :uid'; - LQ.ParamByName('uid').AsInteger := LUserId; - LQ.Open; - if LQ.IsEmpty then - begin - TJSONHelper.SendError(AResponse, 401, 'User not found'); - Exit; - end; - LUser := LQ.FieldByName('username').AsString; - LStoredHash := LQ.FieldByName('password_hash').AsString; - LSalt := LQ.FieldByName('salt').AsString; - LAlgo := LQ.FieldByName('hash_algo').AsString; - LOldIters := LQ.FieldByName('kdf_iterations').AsInteger; - if LAlgo = '' then LAlgo := 'pbkdf2'; - if LOldIters <= 0 then LOldIters := PBKDF2_ITERATIONS; - finally - LQ.Free; - end; - - // Idempotency: nothing to do if BOTH iter count is at target AND - // hash format is current. Previously we short-circuited on iter - // count alone, which would have skipped the hash-format upgrade for - // users who migrated KDF before this commit landed. - if (LOldIters >= PBKDF2_ITERATIONS_TARGET) and - SameText(LAlgo, HASH_ALGO_CURRENT) then - begin - TJSONHelper.SendOK(AResponse, 'Already at target'); - Exit; - end; - - // Step 2: verify the master pw against the CURRENT (old) hash. Prefer - // the ZK verifier path; fall back to plaintext for legacy clients. - LValid := False; - if LOldVerifier <> '' then - LValid := CheckVerifier(LOldVerifier, LStoredHash, LAlgo) - else if SameText(LAlgo, HASH_ALGO_LEGACY) then - begin - LComputed := PBKDF2_SHA256_Hex(LPwd, LSalt, LOldIters); - LValid := ConstantTimeEquals(LComputed, LStoredHash); - end - else if SameText(LAlgo, HASH_ALGO_CURRENT) then - begin - LComputed := ComputeAuthHashCurrent(LPwd, LSalt, LOldIters); - LValid := ConstantTimeEquals(LComputed, LStoredHash); - end; - if not LValid then - begin - RecordFailedAccountAttempt(LUser, LIP); - LogAudit(LUserId, 'failed_migrate_kdf', LIP); - TJSONHelper.SendError(AResponse, 401, 'Invalid password'); - Exit; - end; - - // Step 3: compute the new password hash under the current scheme - // (SHA-256 wrap) at the target iter count. ZK path takes the - // newVerifier (PBKDF2 at the target iters, computed client-side) and - // just wraps it; plaintext path runs PBKDF2 server-side. - if LNewVerifier <> '' then - begin - if not IsValidVerifier(LNewVerifier) then - begin - TJSONHelper.SendError(AResponse, 400, 'Malformed newVerifier'); - Exit; - end; - LNewHash := VerifierToStoredHash(LNewVerifier, HASH_ALGO_CURRENT); - end - else - LNewHash := ComputeAuthHashCurrent(LPwd, LSalt, PBKDF2_ITERATIONS_TARGET); - - // Step 4: atomic transaction — update user hash AND every entry's - // ciphertext together. Any failure rolls back, leaving the user on - // the legacy config (safe to retry next login). - DB.Connection.StartTransaction; - try - LQ := TFDQuery.Create(nil); - try - LQ.Connection := DB.Connection; - // Update hash, iter count, AND hash_algo all in one row update. - // hash_algo := HASH_ALGO_CURRENT is what completes the migration - // away from the "stored hash IS the AES key" architectural flaw. - LQ.SQL.Text := - 'UPDATE users SET password_hash = :h, kdf_iterations = :it, ' + - ' hash_algo = :algo ' + - 'WHERE id = :uid'; - LQ.ParamByName('h').AsString := LNewHash; - LQ.ParamByName('it').AsInteger := PBKDF2_ITERATIONS_TARGET; - LQ.ParamByName('algo').AsString := HASH_ALGO_CURRENT; - LQ.ParamByName('uid').AsInteger := LUserId; - LQ.ExecSQL; - finally - LQ.Free; - end; - - LQ := TFDQuery.Create(nil); - try - LQ.Connection := DB.Connection; - LQ.SQL.Text := - 'UPDATE vault_entries ' + - 'SET encrypted_password = :ep, iv = :iv, updated_at = CURRENT_TIMESTAMP ' + - 'WHERE id = :id AND user_id = :uid'; - - for I := 0 to LEntries.Count - 1 do - begin - LEntry := LEntries.Items[I] as TJSONObject; - LEntryId := LEntry.GetValue('id', 0); - LEncPwd := LEntry.GetValue('encrypted_password', ''); - LIv := LEntry.GetValue('iv', ''); - if (LEntryId <= 0) or (LEncPwd = '') or (LIv = '') then - raise Exception.CreateFmt('Invalid entry payload at index %d', [I]); - - LQ.ParamByName('id').AsInteger := LEntryId; - LQ.ParamByName('uid').AsInteger := LUserId; - LQ.ParamByName('ep').AsString := LEncPwd; - LQ.ParamByName('iv').AsString := LIv; - LQ.ExecSQL; - end; - finally - LQ.Free; - end; - - DB.Connection.Commit; - except - DB.Connection.Rollback; - raise; - end; - finally - DB.Unlock; - end; - finally - LBody.Free; - end; - - LogAudit(LUserId, Format('migrate_kdf %d->%d', [LOldIters, PBKDF2_ITERATIONS_TARGET]), LIP); - TJSONHelper.SendOK(AResponse, 'Migration complete'); + TJSONHelper.SendError(AResponse, 410, + 'KDF migration disabled — rotate the master password instead'); end; // ===== POST /change-master-password ========================================== @@ -852,9 +688,9 @@ end; // working past the pw change. // - Writes an audit_log entry. // -// The /migrate-kdf endpoint exists for the same "re-encrypt all entries" -// pattern when the master pw stays the same; this endpoint differs by -// rotating the salt + pw too. +// This is now the ONLY correct "re-encrypt all entries under a new key" path +// (the old /migrate-kdf was disabled in §6c-1 for re-encrypting just 1 blob +// of 8). Legacy <600k accounts reach 600k + Argon2id through here. procedure HandleChangeMasterPassword(ARequest: TIdHTTPRequestInfo; AResponse: TIdHTTPResponseInfo; const AParams: TArray); var