Cluster 2 — Security Hardening (DEK lifecycle, Argon2id alignment, Backup KDF) [v2.0]#44
Merged
Conversation
…KDF upgrade Security hardening for PassKey 2.0 (cluster 2 of the 2.0 plan). VaultStateService.UnlockAsync — DEK lifecycle (CVE-class): - Previously the derived PinnedSecureBuffer was assigned to the long-lived _dek field before the encrypted vault blob was decrypted. If DecryptVault threw (corrupt blob, GCM tag failure, unexpected DEK length, etc.) the plaintext key remained pinned in memory until the user manually locked the vault. - The candidate buffer is now held in a local variable and only promoted to _dek after DecryptVault returns successfully. A finally block zeroes and disposes the buffer if promotion didn't happen, so plaintext key material never outlives a failed unlock. CryptoService.DeriveKeyFromPassword — Argon2id iterations parameter: - The Argon2id branch was hardcoding Iterations = CryptoConstants.Argon2TimeCost, ignoring the iterations argument that callers passed. The PBKDF2 branch already honoured the parameter, creating an asymmetry that would silently mask future Argon2id tuning bugs. - Argon2id now uses the supplied iterations when positive and falls back to the OWASP-tuned default only when the caller explicitly passes 0 (documented contract). BackupService — KDF upgrade with backward compatibility: - New .pkbak backups (version byte 0x02) are now derived with Argon2id, restoring symmetry with the main vault KEK which has been Argon2id since v1.0.x. Legacy v1.x backups (version byte 0x01, PBKDF2-SHA256) continue to restore correctly, the version byte at offset 4 is read at restore time to pick the KDF. - IMPORTANT: BackupService passes iterations=0 on the Argon2id path so CryptoService uses Argon2TimeCost (3). Forwarding DefaultKdfIterations (600,000 — PBKDF2-shaped) to Argon2id would cost ~600,000 × 64MB memory passes and effectively never complete. Diagnostics (silent catches): - VaultStateService.UnlockAsync and LoginViewModel.LoginAsync previously swallowed exceptions silently. They now emit Debug.WriteLine with the exception type/message so developers don't lose information during local debugging. The user-facing path remains a generic localized "incorrect password" / "unlock failed" message. AOT audit (no changes): - Verified that OnePuxImporter, BitwardenImporter, CsvImporter, VaultService, BackupService and SettingsService all go through their respective generated JsonSerializerContext (OnePuxJsonContext, BitwardenJsonContext, VaultJsonContext, SettingsJsonContext, IpcJsonContext). No reflective JsonSerializer.Deserialize calls remain in the codebase. Tests: 182 passed (was 178). - CryptoServiceTests +2: DeriveKey_Argon2id_RespectsIterationsParam, DeriveKey_Argon2id_ZeroIterations_FallsBackToDefault - BackupServiceTests +2: CreateBackupBlob_UsesArgon2id_VersionByteIs2, RestoreFromBlob_LegacyV1Pbkdf2Format_StillRestores (plus updated HasCorrectMagicHeader to assert 0x02 instead of 0x01) Verified manually (user T2.GATE smoke test): - Vault unlock + DEK never retained on bad password - Export new backup → format v0x02 - Restore new v0x02 backup → vault preserved - Restore legacy v0x01 backup (created with v1.0.17) → still works - Nuke vault (manual %LOCALAPPDATA%\PassKey wipe) + first-run setup → works
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second cluster of the PassKey 2.0 refactor described in
piano-revisione-correzione-miglioramento.md(private notes file).Summary
`VaultStateService.UnlockAsync` — DEK lifecycle (CVE-class fix)
Previously the derived `PinnedSecureBuffer` was assigned to the long-lived `_dek`
field before the encrypted vault blob was decrypted. If `DecryptVault` threw
(corrupt blob, GCM tag failure, unexpected DEK length, ...), the plaintext key
remained pinned in memory until the user manually locked the vault.
The candidate buffer is now held in a local variable and only promoted to `_dek`
after `DecryptVault` returns successfully. A `finally` block zeroes and disposes
the buffer if promotion didn't happen, so plaintext key material never outlives a
failed unlock attempt.
`CryptoService.DeriveKeyFromPassword` — Argon2id honours `iterations` param
The Argon2id branch was hardcoding `Iterations = CryptoConstants.Argon2TimeCost`,
ignoring the `iterations` argument that callers passed. The PBKDF2 branch already
honoured the parameter, creating an asymmetry that would silently mask future
Argon2id tuning bugs.
Argon2id now uses the supplied iterations when positive and falls back to the
OWASP-tuned default only when the caller explicitly passes `0` (documented contract).
`BackupService` — KDF upgrade with backward compatibility
restoring symmetry with the main vault KEK (Argon2id since v1.0.x).
correctly — the version byte at offset 4 is read at restore time to pick the KDF.
so `CryptoService` uses `Argon2TimeCost` (3). Forwarding `DefaultKdfIterations`
(600,000 — PBKDF2-shaped) to Argon2id would cost ~600,000 × 64 MB memory passes
and effectively never complete.
Silent-catch diagnostics
`VaultStateService.UnlockAsync` and `LoginViewModel.LoginAsync` previously
swallowed exceptions silently. They now emit `Debug.WriteLine` with the exception
type/message so developers don't lose information during local debugging. The
user-facing path remains a generic localized error.
AOT audit (no functional change)
Verified that all `JsonSerializer.Deserialize` / `Serialize` call sites already
go through their respective generated `JsonSerializerContext` — no reflective calls
remain in the codebase.
Test plan
New / updated tests