From 41010984ba2c809a7727643b3a7042f7df8c7cd8 Mon Sep 17 00:00:00 2001 From: Giuseppe Imperato Date: Sat, 16 May 2026 16:07:13 +0200 Subject: [PATCH] =?UTF-8?q?fix(security):=20cluster=202=20=E2=80=94=20DEK?= =?UTF-8?q?=20lifecycle,=20Argon2id=20alignment,=20Backup=20KDF=20upgrade?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/PassKey.Core/Services/BackupService.cs | 55 +++++++++++++++++-- src/PassKey.Core/Services/CryptoService.cs | 7 ++- .../Services/VaultStateService.cs | 39 ++++++++++--- .../ViewModels/LoginViewModel.cs | 6 +- src/PassKey.Tests/BackupServiceTests.cs | 43 ++++++++++++++- src/PassKey.Tests/CryptoServiceTests.cs | 34 ++++++++++++ 6 files changed, 168 insertions(+), 16 deletions(-) diff --git a/src/PassKey.Core/Services/BackupService.cs b/src/PassKey.Core/Services/BackupService.cs index 8fc3fa3..9c84457 100644 --- a/src/PassKey.Core/Services/BackupService.cs +++ b/src/PassKey.Core/Services/BackupService.cs @@ -8,7 +8,13 @@ namespace PassKey.Core.Services; public sealed class BackupService : IBackupService { private static readonly byte[] Magic = [0x50, 0x4B, 0x42, 0x4B]; // "PKBK" - private const byte Version = 0x01; + + /// Legacy backup format derived with PBKDF2-SHA256 (600k iterations). + private const byte VersionPbkdf2 = 0x01; + + /// Current backup format derived with Argon2id (OWASP 2023 parameters). + private const byte VersionArgon2Id = 0x02; + private const int HeaderSize = 5; // magic(4) + version(1) private readonly ICryptoService _crypto; @@ -18,6 +24,12 @@ public BackupService(ICryptoService crypto) _crypto = crypto ?? throw new ArgumentNullException(nameof(crypto)); } + /// + /// Serialises and encrypts the supplied to a self-describing + /// .pkbak blob. New backups always use Argon2id (version byte 0x02); the + /// path still accepts legacy 0x01 (PBKDF2) blobs for + /// backward compatibility with v1.x backups produced before this change. + /// public byte[] CreateBackupBlob(Vault vault, ReadOnlySpan backupPassword) { ArgumentNullException.ThrowIfNull(vault); @@ -26,13 +38,23 @@ public byte[] CreateBackupBlob(Vault vault, ReadOnlySpan backupPassword) try { var salt = _crypto.GenerateRandomBytes(CryptoConstants.SaltSizeBytes); - using var key = _crypto.DeriveKeyFromPassword(backupPassword, salt, CryptoConstants.DefaultKdfIterations); + // Argon2id for new backups — restores symmetry with the main vault KEK, + // which has been Argon2id-derived since v1.0.x. + // IMPORTANT: pass 0 as `iterations` so CryptoService falls back to the + // Argon2-tuned default (CryptoConstants.Argon2TimeCost = 3). Forwarding + // CryptoConstants.DefaultKdfIterations (600,000 — PBKDF2-shaped) to Argon2id + // would cost ~600,000 × 64 MB memory passes and never complete. + using var key = _crypto.DeriveKeyFromPassword( + backupPassword, + salt, + iterations: 0, + CryptoConstants.KdfAlgorithmArgon2Id); var encryptedPayload = _crypto.Encrypt(jsonBytes, key.ReadOnlySpan); var blob = new byte[HeaderSize + CryptoConstants.SaltSizeBytes + encryptedPayload.Length]; Magic.CopyTo(blob, 0); - blob[4] = Version; + blob[4] = VersionArgon2Id; salt.CopyTo(blob.AsSpan(HeaderSize)); encryptedPayload.CopyTo(blob.AsSpan(HeaderSize + CryptoConstants.SaltSizeBytes)); @@ -44,6 +66,11 @@ public byte[] CreateBackupBlob(Vault vault, ReadOnlySpan backupPassword) } } + /// + /// Restores a vault from a .pkbak blob, dispatching to the appropriate KDF + /// based on the version byte: (legacy v1.x backups, + /// PBKDF2-SHA256) or (current, Argon2id). + /// public Vault RestoreFromBlob(ReadOnlySpan blob, ReadOnlySpan backupPassword) { var minSize = HeaderSize + CryptoConstants.SaltSizeBytes @@ -55,13 +82,29 @@ public Vault RestoreFromBlob(ReadOnlySpan blob, ReadOnlySpan backupP if (!blob[..4].SequenceEqual(Magic)) throw new InvalidDataException("Not a valid PassKey backup file (bad magic)."); - if (blob[4] != Version) - throw new NotSupportedException($"Unsupported backup version: {blob[4]}."); + var version = blob[4]; + var kdfAlgorithm = version switch + { + VersionPbkdf2 => CryptoConstants.KdfAlgorithmPbkdf2, + VersionArgon2Id => CryptoConstants.KdfAlgorithmArgon2Id, + _ => throw new NotSupportedException($"Unsupported backup version: {version}."), + }; var salt = blob.Slice(HeaderSize, CryptoConstants.SaltSizeBytes).ToArray(); var payload = blob[(HeaderSize + CryptoConstants.SaltSizeBytes)..]; - using var key = _crypto.DeriveKeyFromPassword(backupPassword, salt, CryptoConstants.DefaultKdfIterations); + // Iterations is only meaningful for PBKDF2 (legacy v0x01 backups); for Argon2id + // CryptoService ignores positive non-default iterations only when the value is 0, + // so we pass 0 for the Argon2id path. For PBKDF2 we honour the historic count. + var iterations = kdfAlgorithm == CryptoConstants.KdfAlgorithmArgon2Id + ? 0 + : CryptoConstants.DefaultKdfIterations; + + using var key = _crypto.DeriveKeyFromPassword( + backupPassword, + salt, + iterations, + kdfAlgorithm); var jsonBytes = _crypto.Decrypt(payload, key.ReadOnlySpan); try { diff --git a/src/PassKey.Core/Services/CryptoService.cs b/src/PassKey.Core/Services/CryptoService.cs index 4bbcf01..dd90e0e 100644 --- a/src/PassKey.Core/Services/CryptoService.cs +++ b/src/PassKey.Core/Services/CryptoService.cs @@ -22,12 +22,17 @@ public PinnedSecureBuffer DeriveKeyFromPassword(ReadOnlySpan password, byt if (kdfAlgorithm == CryptoConstants.KdfAlgorithmArgon2Id) { + // Honour an explicit caller-supplied iteration count when positive; fall back + // to the OWASP-tuned default in CryptoConstants for the (legitimate) callers + // that pass 0 — e.g. unit tests or backward-compat code paths that don't know + // about Argon2-specific tunings. This keeps the Argon2id path symmetric with + // the PBKDF2 branch below, which already threads the parameter through. using var argon2 = new Argon2id(passwordBytes) { Salt = salt, DegreeOfParallelism = CryptoConstants.Argon2Parallelism, MemorySize = CryptoConstants.Argon2MemoryCostKiB, - Iterations = CryptoConstants.Argon2TimeCost + Iterations = iterations > 0 ? iterations : CryptoConstants.Argon2TimeCost, }; var derived = argon2.GetBytes(CryptoConstants.KeySizeBytes); try { derived.CopyTo(buffer.Span); } diff --git a/src/PassKey.Desktop/Services/VaultStateService.cs b/src/PassKey.Desktop/Services/VaultStateService.cs index 0215219..2088d60 100644 --- a/src/PassKey.Desktop/Services/VaultStateService.cs +++ b/src/PassKey.Desktop/Services/VaultStateService.cs @@ -71,28 +71,53 @@ public async Task InitializeAsync(ReadOnlyMemory masterPassword) /// /// The master password to verify. /// True if the password is correct and the vault is now unlocked; false otherwise. + /// + /// DEK lifecycle. The derived is held in a local + /// variable until has succeeded. Only then is it + /// promoted to the long-lived field and + /// assigned. If decryption throws (corrupt blob, GCM tag failure, unexpected DEK length, + /// etc.), the local buffer is zeroed and disposed in the catch/finally path, + /// so plaintext key material is never retained in memory after a failed unlock. + /// public async Task UnlockAsync(ReadOnlyMemory masterPassword) { var metadata = await _repository.LoadMetadataAsync(); if (metadata is null) return false; + PinnedSecureBuffer? candidateDek; try { - _dek = _vaultService.UnlockVault(masterPassword.Span, metadata); + candidateDek = _vaultService.UnlockVault(masterPassword.Span, metadata); } - catch + catch (Exception ex) { + // Wrong password / unsupported KDF / corrupted metadata — nothing to clean up, + // the constructor of the failed buffer (if any) is the implementation's + // responsibility. Surface the exception type/message to a Debug listener so + // diagnostics aren't lost during development; we still return false so the + // user-facing path stays generic ("incorrect master password"). + System.Diagnostics.Debug.WriteLine($"[VaultStateService] UnlockVault failed: {ex.GetType().Name}: {ex.Message}"); return false; } - var encryptedBlob = await _repository.LoadEncryptedVaultAsync(); - if (encryptedBlob is null) + try { - CurrentVault = new Vault(); + var encryptedBlob = await _repository.LoadEncryptedVaultAsync(); + Vault decryptedVault = encryptedBlob is null + ? new Vault() + : _vaultService.DecryptVault(encryptedBlob, candidateDek.ReadOnlySpan); + + // Promote only after decryption succeeds — the previous implementation assigned + // _dek before this line, leaving the plaintext key in memory if decryption threw. + _dek = candidateDek; + CurrentVault = decryptedVault; + candidateDek = null; // ownership transferred to _dek; skip the cleanup below } - else + finally { - CurrentVault = _vaultService.DecryptVault(encryptedBlob, _dek.ReadOnlySpan); + // If we never promoted (decrypt failed or threw) zero and release the candidate + // immediately so the DEK does not outlive a failed unlock attempt. + candidateDek?.Dispose(); } VaultUnlocked?.Invoke(); diff --git a/src/PassKey.Desktop/ViewModels/LoginViewModel.cs b/src/PassKey.Desktop/ViewModels/LoginViewModel.cs index af6fe3f..c96a44b 100644 --- a/src/PassKey.Desktop/ViewModels/LoginViewModel.cs +++ b/src/PassKey.Desktop/ViewModels/LoginViewModel.cs @@ -91,8 +91,12 @@ private async Task LoginAsync(string password) ErrorMessage = "IncorrectPassword"; // Localization key } } - catch + catch (Exception ex) { + // Generic catch — UI shows a localized "UnlockFailed" key without leaking the + // underlying error to the user. Forward the original exception to Debug so it's + // still visible to developers during local diagnostics. + System.Diagnostics.Debug.WriteLine($"[LoginViewModel] Login failed: {ex.GetType().Name}: {ex.Message}"); HasError = true; ErrorMessage = "UnlockFailed"; // Localization key } diff --git a/src/PassKey.Tests/BackupServiceTests.cs b/src/PassKey.Tests/BackupServiceTests.cs index db37f03..86c0a08 100644 --- a/src/PassKey.Tests/BackupServiceTests.cs +++ b/src/PassKey.Tests/BackupServiceTests.cs @@ -42,7 +42,48 @@ public void CreateBackupBlob_HasCorrectMagicHeader() Assert.Equal(0x4B, blob[1]); // 'K' Assert.Equal(0x42, blob[2]); // 'B' Assert.Equal(0x4B, blob[3]); // 'K' - Assert.Equal(0x01, blob[4]); // version + // PassKey 2.0 backups are written with version 0x02 (Argon2id KDF). The 0x01 + // version (PBKDF2-SHA256) is still accepted on restore for v1.x compatibility. + Assert.Equal(0x02, blob[4]); + } + + [Fact] + public void CreateBackupBlob_UsesArgon2id_VersionByteIs2() + { + // Regression guard: ensure new backups never silently regress to the legacy + // PBKDF2 format. The version byte at offset 4 must be exactly 0x02. + var vault = new Vault(); + var blob = _backup.CreateBackupBlob(vault, "AnyP@ssword1!".AsSpan()); + Assert.Equal(0x02, blob[4]); + } + + [Fact] + public void RestoreFromBlob_LegacyV1Pbkdf2Format_StillRestores() + { + // Hand-craft a legacy v1.x backup blob — derive key with PBKDF2, write 0x01 in + // the version byte — and verify that PassKey 2.0 can still restore it. This + // guarantees that .pkbak files produced by previous releases remain usable. + var password = "Legacy@Backup1!".AsSpan(); + var vault = CreateSampleVault(); + var jsonBytes = System.Text.Json.JsonSerializer.SerializeToUtf8Bytes(vault, VaultJsonContext.Default.Vault); + var salt = _crypto.GenerateRandomBytes(PassKey.Core.Constants.CryptoConstants.SaltSizeBytes); + using var key = _crypto.DeriveKeyFromPassword( + password, + salt, + PassKey.Core.Constants.CryptoConstants.DefaultKdfIterations, + PassKey.Core.Constants.CryptoConstants.KdfAlgorithmPbkdf2); + var encrypted = _crypto.Encrypt(jsonBytes, key.ReadOnlySpan); + + var legacyBlob = new byte[5 + PassKey.Core.Constants.CryptoConstants.SaltSizeBytes + encrypted.Length]; + legacyBlob[0] = 0x50; legacyBlob[1] = 0x4B; legacyBlob[2] = 0x42; legacyBlob[3] = 0x4B; + legacyBlob[4] = 0x01; // legacy version byte + salt.CopyTo(legacyBlob.AsSpan(5)); + encrypted.CopyTo(legacyBlob.AsSpan(5 + PassKey.Core.Constants.CryptoConstants.SaltSizeBytes)); + + var restored = _backup.RestoreFromBlob(legacyBlob, password); + + Assert.Single(restored.Passwords); + Assert.Equal("GitHub", restored.Passwords[0].Title); } [Fact] diff --git a/src/PassKey.Tests/CryptoServiceTests.cs b/src/PassKey.Tests/CryptoServiceTests.cs index 0e8e90e..214ec09 100644 --- a/src/PassKey.Tests/CryptoServiceTests.cs +++ b/src/PassKey.Tests/CryptoServiceTests.cs @@ -144,4 +144,38 @@ public void PinnedSecureBuffer_ZerosOnDispose() // After dispose, accessing Span should throw Assert.Throws(() => buffer.Span.ToArray()); } + + [Fact] + public void DeriveKeyFromPassword_Argon2id_RespectsIterationsParam() + { + // The Argon2id branch must honour an explicit `iterations` value when positive, + // not silently fall back to CryptoConstants.Argon2TimeCost as the v1.x code did. + // Verified indirectly: a derivation with iterations=1 must NOT equal a derivation + // with the OWASP-tuned default (Argon2TimeCost > 1). + var salt = _crypto.GenerateRandomBytes(CryptoConstants.SaltSizeBytes); + var password = "Argon2-Iterations-Test".AsSpan(); + + using var keyExplicit1 = _crypto.DeriveKeyFromPassword(password, salt, iterations: 1, CryptoConstants.KdfAlgorithmArgon2Id); + using var keyDefault = _crypto.DeriveKeyFromPassword(password, salt, iterations: 0, CryptoConstants.KdfAlgorithmArgon2Id); + + // Different iteration counts → different key material (same password+salt). + // If the iterations parameter were ignored, the two keys would be identical. + Assert.False(keyExplicit1.Span.SequenceEqual(keyDefault.Span), + "Argon2id derivation must use the supplied iterations count, not the default."); + } + + [Fact] + public void DeriveKeyFromPassword_Argon2id_ZeroIterations_FallsBackToDefault() + { + // Passing iterations=0 documents the contract that the default (Argon2TimeCost) + // is used. Two derivations with iterations=0 must produce identical keys. + var salt = _crypto.GenerateRandomBytes(CryptoConstants.SaltSizeBytes); + var password = "Argon2-Default-Fallback".AsSpan(); + + using var keyA = _crypto.DeriveKeyFromPassword(password, salt, iterations: 0, CryptoConstants.KdfAlgorithmArgon2Id); + using var keyB = _crypto.DeriveKeyFromPassword(password, salt, iterations: 0, CryptoConstants.KdfAlgorithmArgon2Id); + + Assert.True(keyA.Span.SequenceEqual(keyB.Span), + "Two Argon2id derivations with the same password+salt and iterations=0 must produce the same key."); + } }