fix: migrate PIN hashing from SHA-256 to Argon2id (#54) #55

Merged
maximus merged 4 commits from fix/simpl-resultat-54-argon2id-pin into main 2026-04-14 12:49:06 +00:00
Owner

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 by argon2id: prefix. New Argon2id hashes verified with Argon2id; legacy <salt>:<hash> format still verified with SHA-256 for backward compatibility
  • Reuses the same Argon2id parameters as export_import_commands.rs (derive_key)
  • No frontend changes needed — same Tauri command signatures
  • No migration needed — existing SHA-256 PINs continue to work; new PINs are hashed with Argon2id

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

  • Build Rust backend (cargo build)
  • Create a new profile with PIN → verify PIN stored with argon2id: prefix
  • Verify existing profiles with SHA-256 PINs still unlock correctly
  • Set a new PIN on an existing profile → verify new hash uses Argon2id
  • Verify wrong PIN is correctly rejected for both formats
## 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 by `argon2id:` prefix. New Argon2id hashes verified with Argon2id; legacy `<salt>:<hash>` format still verified with SHA-256 for backward compatibility - Reuses the same Argon2id parameters as `export_import_commands.rs` (`derive_key`) - No frontend changes needed — same Tauri command signatures - No migration needed — existing SHA-256 PINs continue to work; new PINs are hashed with Argon2id ## 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 - [ ] Build Rust backend (`cargo build`) - [ ] Create a new profile with PIN → verify PIN stored with `argon2id:` prefix - [ ] Verify existing profiles with SHA-256 PINs still unlock correctly - [ ] Set a new PIN on an existing profile → verify new hash uses Argon2id - [ ] Verify wrong PIN is correctly rejected for both formats
maximus added 1 commit 2026-04-09 04:03:07 +00:00
Replace SHA-256 with Argon2id (m=64MiB, t=3, p=1) for PIN hashing.
Existing SHA-256 hashes are verified transparently via format detection
(argon2id: prefix). New PINs are always hashed with Argon2id.

Addresses CWE-916: Use of Password Hash With Insufficient Computational Effort.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
Owner

Reviewer 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

  • src-tauri/src/commands/profile_commands.rs : Les constantes Argon2 (ARGON2_M_COST, T_COST, P_COST, OUTPUT_LEN, SALT_LEN) et la fonction argon2_hash sont dupliquées depuis export_import_commands.rs (le commentaire le confirme). Extraire dans un module partagé (ex: src/crypto.rs) pour éviter la dérive entre les deux copies.

Problemes detectes

  • src-tauri/src/commands/profile_commands.rs:145 [high] MIGRATION MANQUANTE : l'issue exige explicitement 're-hasher en Argon2id à la prochaine vérification réussie'. Ici, verify_pin détecte le format legacy et vérifie le PIN, mais ne re-hashe jamais vers Argon2id. Les PINs SHA-256 resteront en SHA-256 pour toujours, ce qui annule l'objectif de la migration. verify_pin doit retourner un signal (ex: Result<(bool, Option)>) ou un struct indiquant qu'un nouveau hash Argon2id doit être persisté par l'appelant.
  • src-tauri/src/commands/profile_commands.rs:0 [high] TESTS MANQUANTS : l'issue demande explicitement des tests unitaires couvrant 3 cas — ancien format reconnu, nouveau format pour les nouveaux PINs, migration transparente. Aucun test n'est ajouté dans cette PR. C'est bloquant pour du code de sécurité.
  • src-tauri/src/commands/profile_commands.rs:139 [medium] TIMING ATTACK : computed_hex == expected_hash utilise 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::ConstantTimeEq ou 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 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 - **src-tauri/src/commands/profile_commands.rs** : Les constantes Argon2 (ARGON2_M_COST, T_COST, P_COST, OUTPUT_LEN, SALT_LEN) et la fonction argon2_hash sont dupliquées depuis export_import_commands.rs (le commentaire le confirme). Extraire dans un module partagé (ex: src/crypto.rs) pour éviter la dérive entre les deux copies. ### Problemes detectes - **src-tauri/src/commands/profile_commands.rs:145** [high] MIGRATION MANQUANTE : l'issue exige explicitement 're-hasher en Argon2id à la prochaine vérification réussie'. Ici, verify_pin détecte le format legacy et vérifie le PIN, mais ne re-hashe jamais vers Argon2id. Les PINs SHA-256 resteront en SHA-256 pour toujours, ce qui annule l'objectif de la migration. verify_pin doit retourner un signal (ex: Result<(bool, Option<String>)>) ou un struct indiquant qu'un nouveau hash Argon2id doit être persisté par l'appelant. - **src-tauri/src/commands/profile_commands.rs:0** [high] TESTS MANQUANTS : l'issue demande explicitement des tests unitaires couvrant 3 cas — ancien format reconnu, nouveau format pour les nouveaux PINs, migration transparente. Aucun test n'est ajouté dans cette PR. C'est bloquant pour du code de sécurité. - **src-tauri/src/commands/profile_commands.rs:139** [medium] TIMING ATTACK : `computed_hex == expected_hash` utilise 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::ConstantTimeEq` ou comparer les bytes directement avec une boucle XOR). Sévérité moyenne car le coût d'Argon2id atténue le risque.
maximus added 1 commit 2026-04-09 06:05:17 +00:00
- Add automatic re-hashing of legacy SHA-256 PINs to Argon2id on
  successful verification, returning new hash to frontend for persistence
- Use constant-time comparison (subtle::ConstantTimeEq) for both
  Argon2id and legacy SHA-256 hash verification
- Add unit tests for hash_pin, verify_pin (Argon2id and legacy paths),
  re-hashing flow, error cases, and hex encoding roundtrip
- Update frontend to handle VerifyPinResult struct and save rehashed
  PIN hash via profile update

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
Owner

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

  • src-tauri/src/commands/profile_commands.rs : Les constantes Argon2 (ARGON2_M_COST, ARGON2_T_COST, etc.) et la fonction argon2_hash dupliquent la logique de derive_key dans export_import_commands.rs. Envisager d'extraire ces constantes et la fonction dans un module src/crypto.rs partage pour eviter la derive entre les deux usages. Non bloquant mais reduit le risque de desynchronisation future des parametres.

Problemes detectes

  • src/components/profile/ProfileSwitcher.tsx:39 [medium] Si updateProfile echoue (ex: disque plein, erreur I/O), switchProfile n'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 appeler switchProfile.
  • src/pages/ProfileSelectionPage.tsx:28 [medium] Meme probleme que ProfileSwitcher.tsx: updateProfile non protege par try/catch. Si le rehash echoue, switchProfile n'est jamais appele. Meme fix: wrapper le updateProfile dans un try/catch et continuer.
## 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 - **src-tauri/src/commands/profile_commands.rs** : Les constantes Argon2 (ARGON2_M_COST, ARGON2_T_COST, etc.) et la fonction `argon2_hash` dupliquent la logique de `derive_key` dans `export_import_commands.rs`. Envisager d'extraire ces constantes et la fonction dans un module `src/crypto.rs` partage pour eviter la derive entre les deux usages. Non bloquant mais reduit le risque de desynchronisation future des parametres. ### Problemes detectes - **src/components/profile/ProfileSwitcher.tsx:39** [medium] Si `updateProfile` echoue (ex: disque plein, erreur I/O), `switchProfile` n'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 appeler `switchProfile`. - **src/pages/ProfileSelectionPage.tsx:28** [medium] Meme probleme que ProfileSwitcher.tsx: `updateProfile` non protege par try/catch. Si le rehash echoue, `switchProfile` n'est jamais appele. Meme fix: wrapper le `updateProfile` dans un try/catch et continuer.
maximus added 1 commit 2026-04-09 10:02:21 +00:00
Replace hash_pin(pin)? with hash_pin(pin).ok() so that a rehash
failure does not propagate as an error. The user can now switch
profiles even if the Argon2id re-hashing step fails — the PIN
is still correctly verified, and the legacy hash remains until
the next successful login.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
Owner

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

  • src/components/profile/ProfileSwitcher.tsx:39 [high] Si await updateProfile(...) throw (erreur disque, JSON corrompu, etc.), switchProfile ne 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 */ }
  • src/pages/ProfileSelectionPage.tsx:29 [high] Meme probleme que ProfileSwitcher.tsx : await updateProfile(pinProfileId, { pin_hash: rehashed }) sans try/catch. Si ca throw, switchProfile n'est jamais appele. Ajouter un try/catch identique.
## 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 - **src/components/profile/ProfileSwitcher.tsx:39** [high] Si `await updateProfile(...)` throw (erreur disque, JSON corrompu, etc.), `switchProfile` ne 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 */ }` - **src/pages/ProfileSelectionPage.tsx:29** [high] Meme probleme que ProfileSwitcher.tsx : `await updateProfile(pinProfileId, { pin_hash: rehashed })` sans try/catch. Si ca throw, switchProfile n'est jamais appele. Ajouter un try/catch identique.
maximus added 1 commit 2026-04-09 12:01:13 +00:00
Both handlePinSuccess handlers (ProfileSwitcher and ProfileSelectionPage)
now catch updateProfile errors so that a failed rehash persistence does
not block switchProfile.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
Owner

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

  1. Argon2id parameters aligned with export_import_commands.rs (internal consistency).
  2. Bonus beyond the issue scope: automatic best-effort rehash of legacy SHA-256 PINs on successful verification — excellent UX, legacy PINs get upgraded silently over time.
  3. Constant-time comparison via subtle::ConstantTimeEq applied to both Argon2id and legacy SHA-256 paths — free hardening of the existing code.
  4. Format detection via argon2id: prefix is robust and cannot collide with the legacy <salt_hex>:<hash_hex> format.
  5. Seven unit tests covering format, salt uniqueness, correct/wrong verify for both Argon2id and legacy, rehash behavior, invalid format, hex roundtrip.
  6. CHANGELOG.md and CHANGELOG.fr.md both updated.
  7. docs/architecture.md updated to reflect the new format.
  8. updateProfile context type extended cleanly to accept pin_hash, no frontend breakage.

Non-blocking suggestions

  1. Observability of failed rehash persistence. The silent try/catch in ProfileSwitcher.tsx and ProfileSelectionPage.tsx means that if saveProfiles repeatedly 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 a console.warn in dev mode to help diagnose.
  2. Follow-up: proactive legacy migration. Once stabilized, consider a startup task that rehashes all legacy PINs (requires the user's cleartext PIN, so it can only happen on verify — same trigger as now). Alternatively, plan a future major version that drops the SHA-256 path entirely. Worth tracking as a follow-up issue.
  3. Test coverage edge case. test_verify_invalid_format does not explicitly cover a malformed hex payload in the Argon2id path (e.g., argon2id:zzzz:zzzz). hex_decode will error correctly, but an explicit assertion would pin the behavior.

Checklist

  • No secrets, tokens, credentials in the diff
  • No SQL injection, XSS, command injection
  • Input validation at system boundaries (hex length check, format check, part count check)
  • No new .env files
  • Logic matches stated intent (hash_pin → Argon2id, verify_pin handles both formats)
  • Edge cases handled (odd hex length, wrong part count, wrong Argon2id subparts)
  • No regressions: legacy SHA-256 PINs continue to unlock
  • Error handling appropriate
  • No unnecessary complexity
  • No dead code / debug artifacts
  • i18n: no user-facing strings added, N/A
  • No SQL migration modified
  • CHANGELOG updated in both languages

LGTM. Ship it.

## 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 1. Argon2id parameters aligned with `export_import_commands.rs` (internal consistency). 2. Bonus beyond the issue scope: automatic best-effort rehash of legacy SHA-256 PINs on successful verification — excellent UX, legacy PINs get upgraded silently over time. 3. Constant-time comparison via `subtle::ConstantTimeEq` applied to **both** Argon2id and legacy SHA-256 paths — free hardening of the existing code. 4. Format detection via `argon2id:` prefix is robust and cannot collide with the legacy `<salt_hex>:<hash_hex>` format. 5. Seven unit tests covering format, salt uniqueness, correct/wrong verify for both Argon2id and legacy, rehash behavior, invalid format, hex roundtrip. 6. `CHANGELOG.md` and `CHANGELOG.fr.md` both updated. 7. `docs/architecture.md` updated to reflect the new format. 8. `updateProfile` context type extended cleanly to accept `pin_hash`, no frontend breakage. ### Non-blocking suggestions 1. **Observability of failed rehash persistence.** The silent `try/catch` in `ProfileSwitcher.tsx` and `ProfileSelectionPage.tsx` means that if `saveProfiles` repeatedly 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 a `console.warn` in dev mode to help diagnose. 2. **Follow-up: proactive legacy migration.** Once stabilized, consider a startup task that rehashes all legacy PINs (requires the user's cleartext PIN, so it can only happen on verify — same trigger as now). Alternatively, plan a future major version that drops the SHA-256 path entirely. Worth tracking as a follow-up issue. 3. **Test coverage edge case.** `test_verify_invalid_format` does not explicitly cover a malformed hex payload in the Argon2id path (e.g., `argon2id:zzzz:zzzz`). `hex_decode` will error correctly, but an explicit assertion would pin the behavior. ### Checklist - [x] No secrets, tokens, credentials in the diff - [x] No SQL injection, XSS, command injection - [x] Input validation at system boundaries (hex length check, format check, part count check) - [x] No new `.env` files - [x] Logic matches stated intent (hash_pin → Argon2id, verify_pin handles both formats) - [x] Edge cases handled (odd hex length, wrong part count, wrong Argon2id subparts) - [x] No regressions: legacy SHA-256 PINs continue to unlock - [x] Error handling appropriate - [x] No unnecessary complexity - [x] No dead code / debug artifacts - [x] i18n: no user-facing strings added, N/A - [x] No SQL migration modified - [x] CHANGELOG updated in both languages LGTM. Ship it.
maximus force-pushed fix/simpl-resultat-54-argon2id-pin from c301b25450 to e5be6f5a56 2026-04-09 19:46:37 +00:00 Compare
Author
Owner

Review: 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

  • Argon2id parameters consistent with export_import_commands.rs (m=64MiB, t=3, p=1)
  • Format disambiguation via argon2id: prefix is safe — legacy salt is hex, cannot collide with the prefix
  • Legacy SHA-256 verification now also uses ct_eq (fixes a latent timing leak on top of the main fix)
  • Rehash is best-effort: hash_pin(pin).ok() never blocks a legitimate login
  • Frontend persists the rehash via updateProfile({pin_hash}) wrapped in try/catch so a DB write failure doesn't block the profile switch
  • No SQL migration needed — pin_hash column already accepts arbitrary strings
  • CHANGELOG.md, CHANGELOG.fr.md, and docs/architecture.md all updated

Suggestions (non-blocking)

  1. Silent catch on rehash persistencesrc/components/profile/ProfileSwitcher.tsx:45 and src/pages/ProfileSelectionPage.tsx:32 swallow the error with an empty catch {}. A console.warn would help diagnose rehash persistence failures in production without changing the UX.
  2. Duplicated Argon2 constantsARGON2_M_COST/T_COST/P_COST/OUTPUT_LEN are now defined in both profile_commands.rs and export_import_commands.rs. Consider extracting them into a shared crypto module so the parameters stay in sync if tuned later.

Nothing blocking — good to merge.

## Review: 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 - Argon2id parameters consistent with `export_import_commands.rs` (m=64MiB, t=3, p=1) - Format disambiguation via `argon2id:` prefix is safe — legacy salt is hex, cannot collide with the prefix - Legacy SHA-256 verification now also uses `ct_eq` (fixes a latent timing leak on top of the main fix) - Rehash is best-effort: `hash_pin(pin).ok()` never blocks a legitimate login - Frontend persists the rehash via `updateProfile({pin_hash})` wrapped in try/catch so a DB write failure doesn't block the profile switch - No SQL migration needed — `pin_hash` column already accepts arbitrary strings - CHANGELOG.md, CHANGELOG.fr.md, and `docs/architecture.md` all updated ### Suggestions (non-blocking) 1. **Silent catch on rehash persistence** — `src/components/profile/ProfileSwitcher.tsx:45` and `src/pages/ProfileSelectionPage.tsx:32` swallow the error with an empty `catch {}`. A `console.warn` would help diagnose rehash persistence failures in production without changing the UX. 2. **Duplicated Argon2 constants** — `ARGON2_M_COST`/`T_COST`/`P_COST`/`OUTPUT_LEN` are now defined in both `profile_commands.rs` and `export_import_commands.rs`. Consider extracting them into a shared `crypto` module so the parameters stay in sync if tuned later. Nothing blocking — good to merge.
maximus merged commit ba5257791f into main 2026-04-14 12:49:06 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: maximus/Simpl-Resultat#55
No description provided.