diff --git a/CODE_AUDIT.md b/CODE_AUDIT.md index 6e2de11..6faeff9 100644 --- a/CODE_AUDIT.md +++ b/CODE_AUDIT.md @@ -16,19 +16,29 @@ 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. -Les points qui méritent attention avant de considérer le produit "durci" : +**MAJ 2026-07-09 : les 5 axes de durcissement ci-dessous sont tous traités.** -1. 🔴 **Le verifier envoyé au serveur EST la clé de chiffrement** (en hex). -2. 🟠 **Sync WebDAV sans concurrence atomique** → lost update possible. -3. 🟠 **Arbitrage tombstone sensible au décalage d'horloge** entre devices. -4. 🟠 **PBKDF2-SHA256** (GPU-friendly) au lieu d'Argon2id. -5. 🟡 **`app.js` monofichier ~14k lignes** + aucun test automatisé. +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é (couplage clé/authentifiant) +### 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](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](js/app.js)) dérive **un seul** output PBKDF2 et l'utilise pour DEUX rôles : @@ -37,19 +47,10 @@ PBKDF2 et l'utilise pour DEUX rôles : - `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` transite en clair vers `/login` (loopback HTTP). Toute - fuite du **corps de requête** (log, proxy de debug, extension, futur - bug XSS contournant la CSP) expose **directement la clé du vault**. -- Conceptuellement, l'authentifiant et le secret de chiffrement ne - devraient jamais être le même matériel. - -**Recommandation** : dériver le verifier dans un **domaine séparé** — -p.ex. `verifier = HKDF(keyBytes, info="auth")` ou une 2ᵉ dérivation -PBKDF2 avec un `info`/salt distinct. Ainsi le verifier transmis n'est -pas la clé. Migration possible sans re-chiffrer les entries (seul le -verifier stocké côté serveur change). +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)** @@ -104,14 +105,11 @@ 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 🟡 Snapshot de sync = tout le vault en clair sous le sync password +### 1.4 ✅ Sync password faible — corrigé (2026-07-09) -`buildSyncSnapshot()` déchiffre chaque entry puis re-chiffre le tout sous -`syncEncPwd`. Si l'utilisateur choisit un sync password faible, le fichier -WebDAV distant devient le maillon faible (le master pw fort ne protège -plus rien sur le remote). **Recommandation** : imposer une politique de -force minimale sur le sync password (déjà ≥ 6 chars — trop peu ; viser -≥ 12 + zxcvbn). +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) @@ -131,16 +129,13 @@ force minimale sur le sync password (déjà ≥ 6 chars — trop peu ; viser ## 2. Bugs / risques latents (non encore observés) -### 2.1 🟠 Sync : lost update (pas de concurrence atomique) +### 2.1 ✅ Sync : lost update — corrigé (ETag/If-Match) -`runSyncNow()` fait `GET` → merge → `PUT`. Deux devices qui syncent -**en même temps** sur le même fichier WebDAV : le 2ᵉ `PUT` écrase le 1ᵉʳ -sans détecter le conflit → perte de la fenêtre de merge de l'un des deux. - -**Recommandation** : concurrence optimiste via **ETag / If-Match** WebDAV. -Récupérer l'ETag au `GET`, l'envoyer en `If-Match` au `PUT` ; si 412 -Precondition Failed → re-pull + re-merge + re-push. La plupart des serveurs -WebDAV (Nextcloud, Apache mod_dav) supportent les ETags. +**Fait** : concurrence optimiste implémentée. `runSyncNow` capture l'ETag au +`GET`, l'envoie en `If-Match` au `PUT` ([js/app.sync.js](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)** @@ -261,7 +256,7 @@ réécriture des call-sites, risque quasi nul vs conversion en modules ES). 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 → **62 tests**. +- 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 + @@ -275,7 +270,7 @@ 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/` (35 tests, Node `node:test`, zéro dépendance, +**Fait** : suite `js/tests/` (65 tests, Node `node:test`, zéro dépendance, ~1.7 s) — cf. [js/tests/README.md](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, @@ -326,29 +321,33 @@ cf. la checklist "Entry payload" de CLAUDE.md). | Feature | Valeur | Effort | Note | |---|---|---|---| -| ~~**Argon2id** (KDF memory-hard)~~ ✅ | 🔴 Haute | ~~Moyen~~ | Fait : `argon2id-v2`, JS pur (noble), zéro Delphi. Cf. §1.2 | -| **ETag/If-Match sur sync** | 🟠 Haute | Faible | Évite les lost updates multi-device | +| ~~**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` — déverrouillage biométrique | -| **Tests automatisés** | 🟠 Haute | Moyen | Filet de sécurité contre les régressions | +| **Windows Hello (biométrie)** | 🟠 Moyenne | Moyen | `Windows.Security.Credentials` | | **Import KeePass XML / 1PUX** | 🟡 Moyenne | Moyen | Complète l'écosystème d'import | -| **Password strength par entry** | 🟡 Moyenne | Faible | zxcvbn dans le slideover + vault health | -| **Verifier découplé de la clé** | 🔴 Haute | Faible | Cf. §1.1 — fix de sécurité prioritaire | -| **i18n (FR/EN propre)** | 🔵 Basse | Moyen | Actuellement FR/EN mélangés dans l'UI | -| **Timestamps UTC + Lamport** | 🟠 Moyenne | Moyen | Fiabilise l'arbitrage sync (§2.2) | +| **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) -1. **`node --check` dans `BuildAssets.cmd`** — 5 min, évite les JS cassés en prod. -2. **Découpler verifier ↔ clé** (§1.1) — fix sécu prioritaire, effort faible. -3. **ETag/If-Match sur sync** (§2.1) — évite la perte de données multi-device. -4. ~~**Timestamps UTC partout** (§2.2)~~ — ✅ fait (`NowUTCStr`, going-forward ; rows existantes self-heal). -5. ~~**Tests unitaires crypto + merge** (§3.2)~~ — ✅ fait (35 tests, `js/tests/`, gate de build). -6. ~~**Argon2id** (§1.2)~~ — ✅ fait (`argon2id-v2`, JS pur, adoption register+rotation). -7. Découpage `app.js` en modules (§3.1) — maintenabilité long terme. +**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). --- @@ -359,9 +358,11 @@ Pour un usage **personnel, local, mono-utilisateur**, le produit est token, PID check, CSP, zero-knowledge, AES-GCM) sont bien pensées et au-dessus de la moyenne des projets perso. -Les axes de durcissement (verifier découplé, Argon2id, sync atomique, -timestamps UTC) deviennent importants **dès qu'on vise le multi-device -sérieux ou un modèle de menace où la DB / le fichier de sync peut fuiter**. -Aucun de ces points n'est un trou béant immédiat en usage loopback solo, -mais §1.1 (verifier=clé) est celui que je corrigerais en premier car il -est peu coûteux et supprime un couplage dangereux. +**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 ».