Zaki/Password-Manager
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
5e5f908045e830b7370678567a523b80879271ca
Password-Manager/CODE_AUDIT.md
T

19 KiB
Raw Blame History

Audit de code — PMServer (Password Manager)

Analyse statique + revue architecture au 2026-07-03. Portée : crypto, auth, HTTP lockdown, sync, frontend, robustesse. Basé sur la lecture du code, pas sur un pentest dynamique.

Légende sévérité : 🔴 critique · 🟠 important · 🟡 moyen · 🔵 mineur / cosmétique

0. Résumé exécutif

Le produit est solide pour un gestionnaire perso offline : chiffrement client-side AES-GCM, zero-knowledge login (le serveur ne voit jamais le master pw), défense en profondeur sur le serveur loopback (token + PID check + CSP stricte), DPAPI pour la persistance device-bound. La majorité des "bugs" trouvés cette session ont été corrigés.

MAJ 2026-07-09 : les 5 axes de durcissement ci-dessous sont tous traités.

  1. 🔴 Verifier = clé → ✅ découplé (-v2 : verifier = SHA256(keyHex+domaine)). §1.1
  2. 🟠 Sync sans concurrence atomique → ✅ ETag/If-Match + 412 re-merge. §2.1
  3. 🟠 Arbitrage tombstone / horloge → ✅ timestamps UTC partout. §2.2
  4. 🟠 PBKDF2 GPU-friendly → ✅ Argon2id (argon2id-v2). §1.2
  5. 🟡 monofichier + zéro test → ✅ 8 modules + 65 tests (gate de build). §3.1/§3.2

1. Sécurité

1.1 ✅ Verifier = matériel de clé — corrigé (schemes -v2)

Fait : les schemes pbkdf2-sha256-v2 et argon2id-v2 transmettent verifier = SHA256(keyHex + "pmserver/auth-verifier/v2") au lieu de keyHex (isDecoupledVerifierAlgo/verifierFromKeyHex, js/app.crypto.js). Le verifier est désormais une fonction one-way de la clé → intercepter /login ne donne plus la clé AES. Adoption register + rotation ; comptes pré-v2 basculent à leur prochaine rotation. La clé AES reste la sortie brute du KDF (entries déchiffrables). Détails CLAUDE.md « Auth-hash schemes ».

Analyse d'origine

deriveKeyAndVerifier() (js/app.js) dérive un seul output PBKDF2 et l'utilise pour DEUX rôles :

  • cryptoKey = les 32 octets bruts importés en AES-GCM (chiffre les entries)
  • verifier = ces mêmes 32 octets en hex, envoyés au serveur à /login

Le serveur stocke SHA-256(verifier), donc une fuite de la DB ne donne pas la clé (préimage SHA-256). MAIS le verifier transitait en clair vers /login = directement la clé du vault si le corps de requête fuit. C'est exactement ce que -v2 corrige (verifier = one-way de la clé).

1.2 🟠 PBKDF2-SHA256 vs Argon2id — corrigé (2026-07-05)

PBKDF2-SHA256 (600k) reste GPU/ASIC-friendly → un master pw faible tombe vite sur matériel dédié si la DB fuit.

Fait : nouveau scheme argon2id-v2 (Argon2id memory-hard + verifier décplé). Détails dans le CLAUDE.md « Auth-hash schemes ».

  • JS pur (js/argon2.js, bundle @noble/hashes@2.2.0), pas WASM : la CSP script-src 'self' n'accorde pas wasm-unsafe-eval. Vérifié contre le vecteur RFC 9106 §5.3 (test unitaire).
  • Zéro Argon2 côté Delphi (l'audit se trompait sur ce point) : le serveur ne dérive jamais la clé, il stocke/échoie juste les params (argon2_m/t/p) et SHA256-wrappe le verifier comme tout -v2.
  • Params OWASP m=19 MiB, t=2, p=1 (~0.65 s/unlock), stockés par compte.
  • Adoption : register + change-master-pw (nouveau défaut). Comptes existants restent PBKDF2 jusqu'à rotation. Passer à Argon2id re-chiffre tout le vault (la clé dérivée change) — c'est le flow rotation existant.
  • Tests : 8 tests crypto Argon2 (vecteur RFC, branche KDF, contrat de params register↔login, sensibilité aux params). 42/42.

Validé runtime : un compte ayant tourné sa master pw affiche hash_algo=argon2id-v2 (m=19456, t=2, p=1) et se reconnecte/déchiffre.

✅ Async (2026-07-05) : deriveKeyBytes utilise argon2idAsync (bundle re-vendé pour l'exposer) — cède la main à l'event loop pour que le spinner s'anime au lieu de figer ~0.65 s. Parité sync/async testée sur le vecteur RFC 9106. Plus rien d'ouvert sur §1.2.

1.3 🟡 Métadonnées en clair

Documenté mais à rappeler pour un futur modèle de menace :

  • entry_attachments : filename, mime, size_bytes non chiffrés
  • users.avatar_b64 : image non chiffrée (cosmétique, assumé)
  • vault_entries : username, site, title, tags chiffrés (2026-07-09) ; folder, kind, template encore en clair.

username + site + title + tags chiffrés au repos (✅ 2026-07-09) : colonnes <f>_enc/<f>_iv (AES-GCM sous la clé du vault). Clé de l'approche : recherche/tri sont côté client → on déchiffre au loadEntries en mémoire, donc AES-GCM plein (IV aléatoire), pas de searchable-encryption. Choke-point withEncryptedMeta sur tous les writes ; decryptEntryMeta au load ; migration migrateMetadataAtRest (live + corbeille) ; rotation re-chiffre les 4. Validation serveur « Site required » retirée + ?q= neutralisé (LIKE inutile sur ciphertext). Détails CLAUDE.md « Entry payload ». Validé runtime le 2026-07-09 : après rebuild + unlock du compte test, la base montre 0 en clair sur les 4 champs (username/site/title/tags) pour ce compte, _enc remplies, affichage/ recherche/favicons OK. La migration est par utilisateur (tourne au unlock sur les entries du compte connecté) — un compte non connecté garde son clair jusqu'à sa prochaine connexion (comportement normal, pas une régression). Résiduel : folder, kind, template, métadonnées d'attachments, nombre de lignes, timestamps.

1.4 ✅ Sync password faible — corrigé (2026-07-09)

Le snapshot WebDAV n'est protégé que par le sync password. Fait : syncSetEncPwdFlow exige maintenant ≥12 chars ET computeStrength ≥ 50 (réutilise computeStrength, pas de dép zxcvbn). Le maillon faible est fermé.

1.5 🔵 Points positifs (à conserver)

  • ✅ AES-GCM 256 (AEAD, intégrité incluse) — pas de mode ECB/CBC nu
  • ✅ IV aléatoire par blob (crypto.getRandomValues(12)) — pas de réutilisation
  • ✅ Zero-knowledge : le master pw plaintext ne quitte jamais le client
  • ✅ CSP stricte (default-src 'self'; img-src 'self' data:) — bloque l'exfiltration même en cas d'injection
  • ✅ Serveur loopback + token + PID-on-socket check (defense in depth)
  • ✅ DevTools / menu contextuel Edge désactivés
  • ✅ Rendu via textContent / el() (pas d'innerHTML sur données user) → surface XSS minimale
  • ✅ Clipboard sécurisé (exclusion clipboard history + auto-clear 30s)
  • ✅ Recovery code à usage limité (5), consommé, supprimé après rotation

2. Bugs / risques latents (non encore observés)

2.1 ✅ Sync : lost update — corrigé (ETag/If-Match)

Fait : concurrence optimiste implémentée. runSyncNow capture l'ETag au GET, l'envoie en If-Match au PUT (js/app.sync.js) ; un 412 → re-pull + re-merge + re-push (borné à 3). Serveur sans ETag → etag vide → pas d'If-Match → fallback last-write-wins. Détails CLAUDE.md « Sync ».

2.2 🟠 Arbitrage tombstone sensible à l'horloge — corrigé (2026-07-04)

applyRemoteSnapshot() compare entry.updated_at > tombstone.deleted_at pour décider résurrection vs suppression.

Bug réel trouvé (pire que décrit) : deux sources d'horloge coexistaient sur un même device. created_at/updated_at des entries étaient écrits via Delphi FormatDateTime(..., Now) = heure locale, tandis que deleted_at (soft-delete + tombstones) venait de SQLite CURRENT_TIMESTAMP / datetime('now') = UTC. L'arbitrage comparait donc un updated_at local à un deleted_at UTC → décalage = l'offset UTC de la machine, même en solo mono-device (pas seulement entre devices décalés).

Fix : helper NowUTC / NowUTCStr dans PM.Database, substitué à FormatDateTime(..., Now) à tous les sites d'écriture d'entries/attachments (PM.Handler.Entries.pas create/update/bulk, PM.Handler.Attachments.pas). Tout est désormais UTC (les paths SQLite l'étaient déjà). Côté JS aucun changement nécessaire : buildSyncSnapshot utilise toISOString() (UTC) et l'arbitrage compare deux valeurs du même fuseau → Date.parse applique le même offset local aux deux, ordre relatif préservé.

Rows existantes : décision « laisser se soigner » — les anciennes lignes gardent leur updated_at local jusqu'à leur prochain edit (qui le réécrit en UTC). Exposition étroite pour un vault solo ; pas de migration destructive (un shift par l'offset courant serait irréversible et approximatif : DST / changement de fuseau historique).

Reste (optionnel) : compteur logique (Lamport) pour les cas d'égalité 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.

2.3 🟡 catch (_) {} silencieux en cascade

Nombreux catch (_) {} dans applyRemoteSnapshot, buildSyncSnapshot, restauration d'attachments, création de folders. Une erreur réseau/serveur transitoire est avalée → l'entry/folder/attachment est silencieusement sauté. Le garde-fou "failed count → abort push" atténue la perte côté entries, mais pas côté attachments ni folders.

Recommandation : logger chaque catch (au moins console.warn + compteur), et étendre le garde-fou "n échecs → warn utilisateur" aux attachments.

2.4 🟡 Pas de pagination serveur sur GET /entries

Tout le vault est chargé + déchiffré à chaque unlock. Sur un gros vault (5k-10k entries), buildSyncSnapshot (déchiffre chaque entry + fetch attachments un par un) et computeHealthCache deviennent lents (O(n) séquentiel, await en boucle). Acceptable pour usage perso (< 500 entries), problématique au-delà.

Recommandation : batcher les déchiffrements (Promise.all par lots), et si besoin paginer côté serveur pour la vue grille.

2.5 ✅ 500 au lieu de 401 sur session expirée — corrigé (2026-07-09)

Authenticate écrit 401 puis raise ESessionRejected → le try/except global de PM.HTTPServer réécrit un 500. Pré-existant, tous les handlers concernés. Cosmétique (le client voit un non-2xx) mais brouille le debug.

Recommandation : attraper ESessionRejected spécifiquement dans le dispatcher et ne pas réécrire la réponse déjà envoyée.

2.6 🔵 CSV re-import crée des doublons

Documenté/assumé (les lignes CSV sans uuid mint un nouvel uuid à chaque import). Le dedup uuid ne couvre que le JSON. Acceptable si documenté à l'utilisateur.

2.7 🔵 navigator.clipboard fallback échoue silencieusement si caché

Quand le Bridge Delphi est absent, le fallback navigator.clipboard. writeText échoue si le document n'a pas le focus (WebView caché). Cas rare (le Bridge est quasi toujours actif en prod) mais le catch (_) {} masque l'échec → l'utilisateur croit avoir copié.

3. Qualité de code / maintenabilité

3.1 🟡 app.js monofichier ~14 000 lignes

Tout le frontend en un seul fichier. Difficile à naviguer, à tester, et un edit malencontreux casse tout le parse (déjà arrivé cette session : une déclaration de fonction supprimée → app entièrement morte, découvert seulement au runtime).

En cours (2026-07-05) : découpage incrémental en fichiers classic-script chargés dans l'ordre via <script> séparés — pas de bundler ES/esbuild (les classic scripts partagent l'environnement lexical global, donc zéro réécriture des call-sites, risque quasi nul vs conversion en modules ES).

  • ✅ js/app.crypto.js extrait (KDF, verifier, AES-GCM) — byte-for-byte identique, 42 tests verts, pas de const dupliqué. Chargé AVANT app.js.
  • ✅ js/app.sync.js extrait (WebDAV, snapshot, merge applyRemoteSnapshot) — byte-for-byte identique, couvert par les tests merge, chargé APRÈS app.js (effet de bord top-level Bridge.onWebdavResult).
  • ✅ js/app.import.js extrait (export container + parsing CSV/JSON + doImport/doExport) — byte-for-byte identique, couvert par les 14 tests CSV, chargé AVANT app.js (pures déclarations). Pattern + règles d'ordre documentés dans CLAUDE.md « Découpage frontend ».
  • ✅ js/app.totp.js extrait (TOTP RFC 6238 + crypto TOTP/custom-fields) — byte-for-byte identique, chargé AVANT app.js. +13 tests TOTP (vecteurs RFC 6238) ajoutés en même temps → extraction et nouvelle couverture.
  • ✅ js/app.backup.js extrait (auto-backup planifié) — byte-for-byte identique, chargé AVANT app.js (pures déclarations).
  • ✅ js/app.favicon.js extrait (fetch/cache favicons) — chargé AVANT app.js. +7 tests faviconHost (extraction hostname → décide quel domaine part vers DDG). A révélé un commentaire faux (IPs « valides » alors qu'elles sont rejetées par la règle TLD [a-z]{2,}) — corrigé.
  • ✅ js/app.health.js extrait (vault health dashboard) — byte-for-byte identique, chargé AVANT app.js (auditCache/auditFilter viennent avec, résolus cross-fichier).
  • ✅ js/app.overlays.js extrait (quick search + cheatsheet + password history) — quicksearch était entrelacé avec ces 2 overlays, donc extraits ensemble en un bloc contigu byte-for-byte. Chargé AVANT app.js.
  • app.js : 11 936 → 9 170 lignes (8 modules sortis, ~2 770 lignes).
  • Suite de tests : 42 → 65 tests.
  • Reste : sections très couplées au state/DOM (slideover, settings, folders, attachments, autofill, PIN/recovery/quick-unlock) — rendement/risque faible, à faire au fil de l'eau. Le socle §3.1 (pattern + modules à risque isolés + filet de tests) est en place.
  • ✅ node --check en pré-étape de BuildAssets.ps1 : déjà fait (cf. §3.2).

3.2 🟡 Aucun test automatisé — partiellement adressé (2026-07-04)

Toute la validation était manuelle (TEST_PLAN.md, TEST_REGRESSION.md). Les zones à haut risque de régression (crypto round-trip, merge de sync, arbitrage tombstone) sont exactement celles qui bénéficient d'un filet unitaire.

Fait : suite js/tests/ (65 tests, Node node:test, zéro dépendance, ~1.7 s) — cf. js/tests/README.md. Harness node:vm qui charge app.js (monofichier) avec globals navigateur stubbés :

  • crypto.test.js — deriveKeyAndVerifier (clé AES == PBKDF2 brut, cross-checké vs pbkdf2Sync Node), découplage verifier legacy vs -v2, encryptPwd/decryptPwd round-trip, unicité IV, tamper/mauvaise clé → [ERROR].
  • csv.test.js — parseCSV, findColumn, parseEntriesFromCSV (Bitwarden/ KeePass, classification note vs login).
  • merge.test.js — applyRemoteSnapshot : add/update/skip (LWW), tombstone delete, arbitrage résurrection (2 branches NaN), veto tombstone local, merge additif de folders. Seule api() est stubbée (fake server mémoire) ; loadEntries/encryptImportEntry tournent en vrai.

Intégré comme gate de build dans BuildAssets.ps1 (après node --check, bypass PM_SKIP_TESTS=1).

Reste à faire :

  • isSoDirty (couplé DOM/soState — nécessite plus de stubbing).
  • Tests Delphi (DUnitX) sur les handlers critiques (bulk-import + tombstone purge, rotation master pw).

3.3 🔵 Duplication de constantes cross-langage

600000, formats de timestamp, noms de colonnes, shapes JSON sont dupliqués entre JS et Delphi. Un changement d'un côté sans l'autre = bug silencieux (déjà vu avec les champs oubliés dans la rotation/duplicate, cf. la checklist "Entry payload" de CLAUDE.md).

4. Améliorations UX (rapides)

  • 🔵 Force du sync password : afficher un indicateur zxcvbn au réglage, refuser < 12 chars.
  • 🔵 Indicateur de force du master pw à l'enregistrement (zxcvbn).
  • 🔵 Feedback d'échec sync partiel : "3 attachments non synchronisés" au lieu d'un silence.
  • 🔵 Combobox custom fields : navigation clavier (flèches ↑↓ + Enter) dans le menu déroulant, pas seulement souris.
  • 🔵 Auto-lock : afficher le temps restant avant lock dans un coin.
  • 🔵 Avatar : l'inclure dans le sync snapshot + auto-backup — ✅ fait : avatar_b64 dans buildSyncSnapshot + payload auto-backup ; restauré additivement dans applyRemoteSnapshot (adopté seulement si le device n'a pas d'avatar, jamais écrasé). +2 tests merge.

5. Fonctionnalités futures (par valeur / effort)

Feature Valeur Effort Note
Argon2id ✅ 🔴 — Fait : argon2id-v2, JS pur (noble). §1.2
ETag/If-Match sur sync ✅ 🟠 — Fait. §2.1
Verifier découplé de la clé ✅ 🔴 — Fait : -v2. §1.1
Tests automatisés ✅ 🟠 — Fait : 65 tests, gate de build. §3.2
Password strength par entry ✅ 🟡 — Fait : barre slideover + vault health (computeStrength, pas zxcvbn)
Timestamps UTC ✅ (Lamport optionnel) 🟠 — Fait. §2.2
Extension navigateur 🟠 Haute Élevé Autofill in-page, feature #1 demandée
Windows Hello (biométrie) 🟠 Moyenne Moyen Windows.Security.Credentials
Import KeePass XML / 1PUX 🟡 Moyenne Moyen Complète l'écosystème d'import
i18n (FR/EN propre) 🔵 Basse Moyen FR/EN mélangés dans l'UI
Emergency access / partage 🔵 Basse Élevé Hors scope "perso offline"

6. Plan d'action recommandé (ordre)

Tout le plan initial est fait :

  1. node --check dans BuildAssets ✅ (+ suite de tests en gate).
  2. Découpler verifier ↔ clé ✅ (-v2, §1.1).
  3. ETag/If-Match sur sync ✅ (§2.1).
  4. Timestamps UTC ✅ (§2.2).
  5. Tests unitaires crypto + merge ✅ (65 tests, §3.2).
  6. Argon2id ✅ (§1.2).
  7. Découpage app.js ✅ socle (8 modules, §3.1) — reste sections UI couplées, au fil de l'eau.

Bonus fait hors plan : métadonnées chiffrées au repos (§1.3), sync pw fort (§1.4), 401 session (§2.5), avatar sync (§4).

7. Verdict

Pour un usage personnel, local, mono-utilisateur, le produit est sûr et fonctionnel aujourd'hui. Les défenses en profondeur (loopback, token, PID check, CSP, zero-knowledge, AES-GCM) sont bien pensées et au-dessus de la moyenne des projets perso.

MAJ 2026-07-09 : tous les axes de durcissement identifiés sont traités — verifier découplé (§1.1), Argon2id (§1.2), métadonnées chiffrées au repos (§1.3), sync password fort (§1.4), sync atomique ETag (§2.1), timestamps UTC (§2.2), tests + découpage (§3). Reste uniquement du confort/cosmétique (§2.3/2.4/2.6/2.7, §3.3) et des features hors durcissement (§5 : extension 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 ».

Reference in New Issue View Git Blame Copy Permalink