fix: migrate PIN hashing from SHA-256 to Argon2id (#54) #55
No reviewers
Labels
No labels
source:analyste
source:defenseur
source:human
source:medic
status:approved
status:blocked
status:in-progress
status:needs-fix
status:ready
status:review
status:triage
type:bug
type:feature
type:infra
type:refactor
type:schema
type:security
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: maximus/Simpl-Resultat#55
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/simpl-resultat-54-argon2id-pin"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Fixes #54 — CWE-916: Use of Password Hash With Insufficient Computational Effort
hash_pin: Now uses Argon2id (m=64MiB, t=3, p=1, 32-byte output) instead of SHA-256. Output format:argon2id:<salt_hex>:<hash_hex>verify_pin: Detects format byargon2id:prefix. New Argon2id hashes verified with Argon2id; legacy<salt>:<hash>format still verified with SHA-256 for backward compatibilityexport_import_commands.rs(derive_key)Security impact
A 4-digit PIN has ~10k combinations. With SHA-256, brute-force takes milliseconds. With Argon2id (64MiB memory, 3 iterations), each attempt takes ~100ms, making brute-force significantly slower.
Test plan
cargo build)argon2id:prefixReviewer automatique — needs-fix
Deux exigences explicites de l'issue ne sont pas implémentées : (1) le re-hachage automatique des PINs legacy SHA-256 vers Argon2id lors d'une vérification réussie, et (2) les tests unitaires. Sans le re-hachage, les PINs SHA-256 restent vulnérables indéfiniment. La comparaison de hash non constant-time est un problème secondaire.
Suggestions de simplification
Problemes detectes
computed_hex == expected_hashutilise une comparaison string non constant-time. Même si Argon2id rend le brute-force coûteux par tentative, la bonne pratique pour du code crypto est d'utiliser une comparaison constant-time (ex:subtle::ConstantTimeEqou comparer les bytes directement avec une boucle XOR). Sévérité moyenne car le coût d'Argon2id atténue le risque.Reviewer automatique — needs-fix
Les corrections du round 1 (re-hachage Argon2id, tests, comparaison constant-time) sont bien implementees. Cependant, une erreur lors de la persistance du rehash bloque le switchProfile — le PIN correct est verifie mais l'utilisateur reste coince. Probleme de robustesse a corriger.
Suggestions de simplification
argon2_hashdupliquent la logique dederive_keydansexport_import_commands.rs. Envisager d'extraire ces constantes et la fonction dans un modulesrc/crypto.rspartage pour eviter la derive entre les deux usages. Non bloquant mais reduit le risque de desynchronisation future des parametres.Problemes detectes
updateProfileechoue (ex: disque plein, erreur I/O),switchProfilen'est jamais appele malgre un PIN correct. Le rehash est un upgrade opportuniste — son echec ne doit pas bloquer l'utilisateur. Wrapper dans un try/catch:try { await updateProfile(...) } catch { /* log silently */ }puis toujours appelerswitchProfile.updateProfilenon protege par try/catch. Si le rehash echoue,switchProfilen'est jamais appele. Meme fix: wrapper leupdateProfiledans un try/catch et continuer.Reviewer automatique — needs-fix
Le backend est solide (Argon2id, constant-time, rehash best-effort via .ok()). Cependant, le probleme signale au round 2 n'est que partiellement corrige : cote Rust le rehash est best-effort, mais cote frontend un echec de updateProfile() bloque toujours switchProfile(). Les deux handlers doivent wrapper updateProfile dans un try/catch.
Problemes detectes
await updateProfile(...)throw (erreur disque, JSON corrompu, etc.),switchProfilene sera jamais appele — le PIN est correct mais l'utilisateur reste bloque. C'est exactement le probleme du round 2. Le rehash est une optimisation best-effort, il ne doit jamais bloquer le login. Wrapper dans try/catch :try { await updateProfile(...) } catch { /* best-effort, ignore */ }await updateProfile(pinProfileId, { pin_hash: rehashed })sans try/catch. Si ca throw, switchProfile n'est jamais appele. Ajouter un try/catch identique.Verdict: APPROVE
Summary: Clean migration from SHA-256 to Argon2id for PIN hashing with transparent backward compatibility and automatic rehash of legacy PINs on verification. Complete unit tests, documentation updated, no API break.
Strengths
export_import_commands.rs(internal consistency).subtle::ConstantTimeEqapplied to both Argon2id and legacy SHA-256 paths — free hardening of the existing code.argon2id:prefix is robust and cannot collide with the legacy<salt_hex>:<hash_hex>format.CHANGELOG.mdandCHANGELOG.fr.mdboth updated.docs/architecture.mdupdated to reflect the new format.updateProfilecontext type extended cleanly to acceptpin_hash, no frontend breakage.Non-blocking suggestions
try/catchinProfileSwitcher.tsxandProfileSelectionPage.tsxmeans that ifsaveProfilesrepeatedly fails (locked file, I/O error), the legacy PIN will be re-hashed on every login (~100 ms extra each time) with no signal. Consider aconsole.warnin dev mode to help diagnose.test_verify_invalid_formatdoes not explicitly cover a malformed hex payload in the Argon2id path (e.g.,argon2id:zzzz:zzzz).hex_decodewill error correctly, but an explicit assertion would pin the behavior.Checklist
.envfilesLGTM. Ship it.
c301b25450toe5be6f5a56Review: APPROVE
Summary: Clean migration from SHA-256 to Argon2id with transparent backward compatibility and automatic rehashing of legacy PINs on successful verification. Solid test coverage (8 unit tests), constant-time comparisons via
subtle::ConstantTimeEq, documentation and changelogs updated.Strengths
export_import_commands.rs(m=64MiB, t=3, p=1)argon2id:prefix is safe — legacy salt is hex, cannot collide with the prefixct_eq(fixes a latent timing leak on top of the main fix)hash_pin(pin).ok()never blocks a legitimate loginupdateProfile({pin_hash})wrapped in try/catch so a DB write failure doesn't block the profile switchpin_hashcolumn already accepts arbitrary stringsdocs/architecture.mdall updatedSuggestions (non-blocking)
src/components/profile/ProfileSwitcher.tsx:45andsrc/pages/ProfileSelectionPage.tsx:32swallow the error with an emptycatch {}. Aconsole.warnwould help diagnose rehash persistence failures in production without changing the UX.ARGON2_M_COST/T_COST/P_COST/OUTPUT_LENare now defined in bothprofile_commands.rsandexport_import_commands.rs. Consider extracting them into a sharedcryptomodule so the parameters stay in sync if tuned later.Nothing blocking — good to merge.