From b89ffb7c18210d7b7415a1483cf84ecb866ebd22 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 5 Jul 2026 17:01:51 +0200 Subject: [PATCH 1/4] Hash customer passwords with PBKDF2 and upgrade legacy hashes transparently Replaces the fast SHA1/SHA-x password hashing with PBKDF2 (HMAC-SHA256, 210k iterations, per-password 16-byte salt) and an optional server-side pepper from configuration. No database schema change. - New/changed passwords are stored as a self-describing value in the existing Customer.Password field: PBKDF2$1$$$, so the algorithm/parameters are known per record and never depend on a global setting. PasswordSalt is left empty (salt is embedded). - Verification is format-aware and constant-time (VerifyPassword): legacy SHA hashes still verify via the existing HashedPasswordFormat; PBKDF2 is detected by prefix. - Legacy hashes are transparently re-hashed to PBKDF2 after a successful login (LoginCustomer), so no password reset or data migration is needed. - Centralizes the previously duplicated hash-compare logic across the customer manager and the login/change-password/delete-account validators. - Adds Security:PasswordHashKey (pepper) and Security:PasswordHashIterations to configuration; both optional (empty pepper = no pepper). Co-Authored-By: Claude Opus 4.8 --- .../Services/Security/EncryptionService.cs | 103 ++++++++++++++++++ .../Common/Security/IEncryptionService.cs | 29 +++++ .../Services/CustomerManagerService.cs | 52 +++++---- .../Configuration/SecurityConfig.cs | 15 +++ .../Security/EncryptionServiceTests.cs | 53 +++++++++ .../Services/CustomerManagerServiceTests.cs | 5 + .../Validators/Common/LoginValidator.cs | 12 +- src/Web/Grand.Web/App_Data/appsettings.json | 8 +- .../Customer/ChangePasswordValidator.cs | 11 +- .../Customer/DeleteAccountValidator.cs | 13 +-- .../Validators/Customer/LoginValidator.cs | 10 +- 11 files changed, 250 insertions(+), 61 deletions(-) diff --git a/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs b/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs index a961063f7..0bf2097c0 100644 --- a/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs +++ b/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs @@ -1,11 +1,27 @@ using Grand.Business.Core.Interfaces.Common.Security; using Grand.Domain.Customers; +using Grand.Infrastructure.Configuration; using System.Security.Cryptography; namespace Grand.Business.Common.Services.Security; public class EncryptionService : IEncryptionService { + private const string Pbkdf2Prefix = "PBKDF2"; + private const int Pbkdf2SaltSize = 16; + private const int Pbkdf2HashSize = 32; + private const int DefaultIterations = 210_000; + + private readonly SecurityConfig _securityConfig; + + public EncryptionService(SecurityConfig securityConfig = null) + { + _securityConfig = securityConfig ?? new SecurityConfig(); + } + + private int Iterations => + _securityConfig.PasswordHashIterations > 0 ? _securityConfig.PasswordHashIterations : DefaultIterations; + /// /// Create salt key /// @@ -93,6 +109,93 @@ public virtual string DecryptText(string cipherText, string encryptionPrivateKey return DecryptTextFromMemory(buffer, tDes.Key, tDes.IV); } + /// + /// Creates a strong, self-describing PBKDF2 (HMAC-SHA256) password hash. + /// Format: PBKDF2$1$<iterations>$<saltBase64>$<hashBase64> + /// + public virtual string HashPassword(string password) + { + ArgumentNullException.ThrowIfNull(password); + + var salt = RandomNumberGenerator.GetBytes(Pbkdf2SaltSize); + var iterations = Iterations; + var hash = Rfc2898DeriveBytes.Pbkdf2( + PepperedPassword(password), salt, iterations, HashAlgorithmName.SHA256, Pbkdf2HashSize); + + return string.Join('$', + Pbkdf2Prefix, "1", iterations.ToString(), Convert.ToBase64String(salt), Convert.ToBase64String(hash)); + } + + public virtual bool VerifyPassword(string enteredPassword, PasswordFormat passwordFormat, string storedPassword, + string storedSalt, HashedPasswordFormat legacyHashedFormat) + { + if (enteredPassword == null || storedPassword == null) + return false; + + switch (passwordFormat) + { + case PasswordFormat.Clear: + return FixedTimeEquals(enteredPassword, storedPassword); + case PasswordFormat.Encrypted: + return FixedTimeEquals(EncryptText(enteredPassword, storedSalt), storedPassword); + case PasswordFormat.Hashed: + return IsPbkdf2Hash(storedPassword) + ? VerifyPbkdf2(enteredPassword, storedPassword) + : FixedTimeEquals(CreatePasswordHash(enteredPassword, storedSalt, legacyHashedFormat), + storedPassword); + default: + return false; + } + } + + public virtual bool PasswordHashNeedsUpgrade(PasswordFormat passwordFormat, string storedPassword) + { + //only a current-strength PBKDF2 hash is considered up to date; everything else is upgraded on next login + return !(passwordFormat == PasswordFormat.Hashed && IsPbkdf2Hash(storedPassword)); + } + + private static bool IsPbkdf2Hash(string storedPassword) + { + return storedPassword != null && storedPassword.StartsWith(Pbkdf2Prefix + "$", StringComparison.Ordinal); + } + + private bool VerifyPbkdf2(string enteredPassword, string storedPassword) + { + //PBKDF2$1$$$ + var parts = storedPassword.Split('$'); + if (parts.Length != 5 || !int.TryParse(parts[2], out var iterations)) + return false; + + byte[] salt; + byte[] expected; + try + { + salt = Convert.FromBase64String(parts[3]); + expected = Convert.FromBase64String(parts[4]); + } + catch (FormatException) + { + return false; + } + + var actual = Rfc2898DeriveBytes.Pbkdf2( + PepperedPassword(enteredPassword), salt, iterations, HashAlgorithmName.SHA256, expected.Length); + return CryptographicOperations.FixedTimeEquals(actual, expected); + } + + private byte[] PepperedPassword(string password) + { + var pepper = _securityConfig.PasswordHashKey; + //a NUL separator prevents ambiguity between password and pepper boundaries + return Encoding.UTF8.GetBytes(string.IsNullOrEmpty(pepper) ? password : password + "\0" + pepper); + } + + private static bool FixedTimeEquals(string a, string b) + { + return CryptographicOperations.FixedTimeEquals( + Encoding.UTF8.GetBytes(a), Encoding.UTF8.GetBytes(b)); + } + #region Utilities private static byte[] EncryptTextToMemory(string data, byte[] key, byte[] iv) diff --git a/src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs b/src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs index 4d96d48fd..b437ba3cb 100644 --- a/src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs +++ b/src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs @@ -21,6 +21,35 @@ public interface IEncryptionService string CreatePasswordHash(string password, string saltKey, HashedPasswordFormat passwordFormat = HashedPasswordFormat.SHA1); + /// + /// Creates a strong, self-describing password hash for the current default algorithm (PBKDF2/HMAC-SHA256). + /// The salt and parameters are embedded in the returned value, so a separate salt field is not needed and + /// verification never depends on a global setting. Store the result in Customer.Password with + /// . + /// + /// Plain-text password + /// Self-describing hash string + string HashPassword(string password); + + /// + /// Verifies an entered password against a stored credential in a format-aware, constant-time way. + /// Handles Clear, Encrypted, the modern PBKDF2 hash and legacy SHA-x hashes transparently. + /// + /// Plain-text password supplied by the user + /// Stored password format (Customer.PasswordFormatId) + /// Stored password/hash (Customer.Password) + /// Stored salt (Customer.PasswordSalt); ignored for PBKDF2 + /// Hash algorithm used by legacy SHA hashes (global setting) + /// True when the password matches + bool VerifyPassword(string enteredPassword, PasswordFormat passwordFormat, string storedPassword, + string storedSalt, HashedPasswordFormat legacyHashedFormat); + + /// + /// Indicates whether a stored credential uses a weak/legacy format (Clear, Encrypted or a non-PBKDF2 hash) + /// and should be transparently re-hashed to the modern format after the next successful authentication. + /// + bool PasswordHashNeedsUpgrade(PasswordFormat passwordFormat, string storedPassword); + /// /// Encrypt text /// diff --git a/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs b/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs index b32096d71..cbbf0e6d5 100644 --- a/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs +++ b/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs @@ -61,14 +61,9 @@ public CustomerManagerService( public virtual bool PasswordMatch(PasswordFormat passwordFormat, string oldPassword, string newPassword, string passwordSalt) { - var newPwd = passwordFormat switch { - PasswordFormat.Clear => newPassword, - PasswordFormat.Encrypted => _encryptionService.EncryptText(newPassword, passwordSalt), - PasswordFormat.Hashed => _encryptionService.CreatePasswordHash(newPassword, passwordSalt, - _customerSettings.HashedPasswordFormat), - _ => throw new Exception("PasswordFormat not supported") - }; - return oldPassword.Equals(newPwd); + //oldPassword is the stored credential, newPassword is the plain-text candidate being checked against it + return _encryptionService.VerifyPassword(newPassword, passwordFormat, oldPassword, passwordSalt, + _customerSettings.HashedPasswordFormat); } @@ -84,17 +79,15 @@ public virtual async Task LoginCustomer(string usernameOrE ? await _customerService.GetCustomerByUsername(usernameOrEmail, storeId) : await _customerService.GetCustomerByEmail(usernameOrEmail, storeId); - var pwd = customer.PasswordFormatId switch { - PasswordFormat.Clear => password, - PasswordFormat.Encrypted => _encryptionService.EncryptText(password, customer.PasswordSalt), - PasswordFormat.Hashed => _encryptionService.CreatePasswordHash(password, customer.PasswordSalt, - _customerSettings.HashedPasswordFormat), - _ => throw new Exception("PasswordFormat not supported") - }; - var isValid = pwd == customer.Password; + var isValid = _encryptionService.VerifyPassword(password, customer.PasswordFormatId, customer.Password, + customer.PasswordSalt, _customerSettings.HashedPasswordFormat); if (!isValid) return CustomerLoginResults.WrongPassword; + //transparently migrate weak/legacy hashes (SHA-x, Clear, Encrypted) to the modern PBKDF2 format + if (_encryptionService.PasswordHashNeedsUpgrade(customer.PasswordFormatId, customer.Password)) + await UpgradePasswordHash(customer, password); + //2fa required if (customer.GetUserFieldFromEntity(SystemCustomerFieldNames.TwoFactorEnabled) && _customerSettings.TwoFactorAuthenticationEnabled) @@ -134,10 +127,9 @@ public virtual async Task RegisterCustomer(RegistrationRequest request) _encryptionService.EncryptText(request.Password, request.Customer.PasswordSalt); break; case PasswordFormat.Hashed: - var saltKey = _encryptionService.CreateSaltKey(5); - request.Customer.PasswordSalt = saltKey; - request.Customer.Password = _encryptionService.CreatePasswordHash(request.Password, saltKey, - _customerSettings.HashedPasswordFormat); + //modern self-describing PBKDF2 hash - salt and parameters are embedded in the value itself + request.Customer.PasswordSalt = string.Empty; + request.Customer.Password = _encryptionService.HashPassword(request.Password); break; } @@ -190,10 +182,9 @@ public virtual async Task ChangePassword(ChangePasswordRequest request, string s break; case PasswordFormat.Hashed: { - var saltKey = _encryptionService.CreateSaltKey(5); - customer.PasswordSalt = saltKey; - customer.Password = _encryptionService.CreatePasswordHash(request.NewPassword, saltKey, - _customerSettings.HashedPasswordFormat); + //modern self-describing PBKDF2 hash - salt and parameters are embedded in the value itself + customer.PasswordSalt = string.Empty; + customer.Password = _encryptionService.HashPassword(request.NewPassword); } break; } @@ -208,5 +199,18 @@ public virtual async Task ChangePassword(ChangePasswordRequest request, string s await _customerService.UpdateUserField(customer, SystemCustomerFieldNames.PasswordToken, Guid.NewGuid().ToString()); } + /// + /// Re-hashes an already-verified plain-text password to the modern PBKDF2 format and persists it. + /// Called right after a successful authentication against a weak/legacy credential, so no password reset is + /// required and no schema change is involved (the value simply moves to the self-describing hash format). + /// + private async Task UpgradePasswordHash(Customer customer, string plainPassword) + { + customer.Password = _encryptionService.HashPassword(plainPassword); + customer.PasswordSalt = string.Empty; + customer.PasswordFormatId = PasswordFormat.Hashed; + await _customerService.UpdateCustomer(customer); + } + #endregion } \ No newline at end of file diff --git a/src/Core/Grand.Infrastructure/Configuration/SecurityConfig.cs b/src/Core/Grand.Infrastructure/Configuration/SecurityConfig.cs index ff29b3f5e..2b2a2b63b 100644 --- a/src/Core/Grand.Infrastructure/Configuration/SecurityConfig.cs +++ b/src/Core/Grand.Infrastructure/Configuration/SecurityConfig.cs @@ -78,4 +78,19 @@ public class SecurityConfig /// using menu configuration. /// public bool AuthorizeAdminMenu { get; set; } + + /// + /// Server-side secret ("pepper") mixed into the PBKDF2 password hash. Optional but recommended: it must be stored + /// outside the database (appsettings/secret store), so a database-only leak is not enough to verify hashes offline. + /// Leave empty to hash without a pepper. + /// IMPORTANT: changing this value invalidates all existing PBKDF2 hashes (affected customers must reset their + /// password); legacy SHA hashes are unaffected. Set it once, before going live. + /// + public string PasswordHashKey { get; set; } + + /// + /// PBKDF2 (HMAC-SHA256) iteration count for newly created/upgraded password hashes. Default 210000 (OWASP 2023). + /// The value is embedded in each stored hash, so raising it later does not break existing hashes. + /// + public int PasswordHashIterations { get; set; } } \ No newline at end of file diff --git a/src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs b/src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs index 05cad3772..f3298ea08 100644 --- a/src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs +++ b/src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs @@ -1,5 +1,6 @@ using Grand.Business.Common.Services.Security; using Grand.Domain.Customers; +using Grand.Infrastructure.Configuration; using Microsoft.VisualStudio.TestTools.UnitTesting; namespace Grand.Business.Common.Tests.Services.Security; @@ -93,4 +94,56 @@ public void DecryptText_InvalidPrivateKeyLength_ThrowException() var toDescrypt = "gdfgdfgt45gfdfg"; Assert.ThrowsExactly(() => _encryptionService.DecryptText(toDescrypt, privateKey)); } + + [TestMethod] + public void HashPassword_ProducesSelfDescribingSaltedHash_ThatVerifies() + { + var hash1 = _encryptionService.HashPassword("password"); + var hash2 = _encryptionService.HashPassword("password"); + + //self-describing PBKDF2 format and a random salt per call + StringAssert.StartsWith(hash1, "PBKDF2$"); + Assert.AreNotEqual(hash1, hash2); + + Assert.IsTrue(_encryptionService.VerifyPassword("password", PasswordFormat.Hashed, hash1, string.Empty, + HashedPasswordFormat.SHA1)); + Assert.IsFalse(_encryptionService.VerifyPassword("wrong", PasswordFormat.Hashed, hash1, string.Empty, + HashedPasswordFormat.SHA1)); + } + + [TestMethod] + public void VerifyPassword_VerifiesLegacyShaHash_WithoutRehashing() + { + var salt = _encryptionService.CreateSaltKey(16); + var legacy = _encryptionService.CreatePasswordHash("password", salt, HashedPasswordFormat.SHA512); + + Assert.IsTrue(_encryptionService.VerifyPassword("password", PasswordFormat.Hashed, legacy, salt, + HashedPasswordFormat.SHA512)); + Assert.IsFalse(_encryptionService.VerifyPassword("password", PasswordFormat.Hashed, legacy, salt, + HashedPasswordFormat.SHA256)); + } + + [TestMethod] + public void PasswordHashNeedsUpgrade_TrueForLegacyOrReversible_FalseForPbkdf2() + { + var pbkdf2 = _encryptionService.HashPassword("password"); + Assert.IsFalse(_encryptionService.PasswordHashNeedsUpgrade(PasswordFormat.Hashed, pbkdf2)); + + Assert.IsTrue(_encryptionService.PasswordHashNeedsUpgrade(PasswordFormat.Hashed, "5FDEFB16C983")); + Assert.IsTrue(_encryptionService.PasswordHashNeedsUpgrade(PasswordFormat.Clear, "password")); + Assert.IsTrue(_encryptionService.PasswordHashNeedsUpgrade(PasswordFormat.Encrypted, "cipher")); + } + + [TestMethod] + public void VerifyPassword_Pbkdf2_IsPepperSensitive() + { + var peppered = new EncryptionService(new SecurityConfig { PasswordHashKey = "server-pepper" }); + var hash = peppered.HashPassword("password"); + + //same pepper verifies, missing/different pepper does not + Assert.IsTrue(peppered.VerifyPassword("password", PasswordFormat.Hashed, hash, string.Empty, + HashedPasswordFormat.SHA1)); + Assert.IsFalse(_encryptionService.VerifyPassword("password", PasswordFormat.Hashed, hash, string.Empty, + HashedPasswordFormat.SHA1)); + } } \ No newline at end of file diff --git a/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs b/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs index 3314e2eab..8164a0832 100644 --- a/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs +++ b/src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs @@ -28,6 +28,11 @@ public void Init() _customerServiceMock = new Mock(); _groupServiceMock = new Mock(); _encryptionServiceMock = new Mock(); + //emulate the real format-aware verification for the Clear-format credentials used by these tests + _encryptionServiceMock.Setup(e => e.VerifyPassword(It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny())) + .Returns((string entered, PasswordFormat _, string stored, string _, HashedPasswordFormat _) => + entered == stored); _mediatorMock = new Mock(); _customerHistoryPasswordServiceMock = new Mock(); _customerSettings = new CustomerSettings { diff --git a/src/Web/Grand.Web.AdminShared/Validators/Common/LoginValidator.cs b/src/Web/Grand.Web.AdminShared/Validators/Common/LoginValidator.cs index 79456f741..404aa782c 100644 --- a/src/Web/Grand.Web.AdminShared/Validators/Common/LoginValidator.cs +++ b/src/Web/Grand.Web.AdminShared/Validators/Common/LoginValidator.cs @@ -71,16 +71,8 @@ public LoginValidator( break; case not null: { - var pwd = customer.PasswordFormatId switch { - PasswordFormat.Clear => x.Password, - PasswordFormat.Encrypted => - encryptionService.EncryptText(x.Password, customer.PasswordSalt), - PasswordFormat.Hashed => encryptionService.CreatePasswordHash(x.Password, - customer.PasswordSalt, - customerSettings.HashedPasswordFormat), - _ => throw new Exception("Password format not supported") - }; - var isValid = pwd == customer.Password; + var isValid = encryptionService.VerifyPassword(x.Password, customer.PasswordFormatId, + customer.Password, customer.PasswordSalt, customerSettings.HashedPasswordFormat); if (!isValid) { context.AddFailure(translationService.GetResource("Account.Login.WrongCredentials")); diff --git a/src/Web/Grand.Web/App_Data/appsettings.json b/src/Web/Grand.Web/App_Data/appsettings.json index ab2136d1c..af0e5803f 100644 --- a/src/Web/Grand.Web/App_Data/appsettings.json +++ b/src/Web/Grand.Web/App_Data/appsettings.json @@ -88,7 +88,13 @@ "CookieSameSite": "Lax", "CookieSameSiteExternalAuth": "None", //Enabling this setting allows for verification of access to a specific controller and action in the admin panel using menu configuration. - "AuthorizeAdminMenu": false + "AuthorizeAdminMenu": false, + //Server-side secret ("pepper") mixed into the PBKDF2 password hash. Keep it OUT of the database (here or in a secret store). + //Optional but recommended. IMPORTANT: changing it invalidates existing PBKDF2 hashes (those customers must reset their password); + //legacy SHA hashes are not affected. Set it once, before going live. Empty = hash without a pepper. + "PasswordHashKey": "", + //PBKDF2 (HMAC-SHA256) iteration count for new/upgraded password hashes. Default 210000 (OWASP). Embedded per-hash, so it can be raised safely later. + "PasswordHashIterations": 210000 }, "Cache": { //Gets or sets a value indicating for default cache time in minutes" diff --git a/src/Web/Grand.Web/Validators/Customer/ChangePasswordValidator.cs b/src/Web/Grand.Web/Validators/Customer/ChangePasswordValidator.cs index 224b26a40..cd6c3ab70 100644 --- a/src/Web/Grand.Web/Validators/Customer/ChangePasswordValidator.cs +++ b/src/Web/Grand.Web/Validators/Customer/ChangePasswordValidator.cs @@ -68,14 +68,9 @@ public ChangePasswordValidator( if (customer is not null) { - var oldPwd = customer.PasswordFormatId switch { - PasswordFormat.Encrypted => encryptionService.EncryptText(x.OldPassword, - customer.PasswordSalt), - PasswordFormat.Hashed => encryptionService.CreatePasswordHash(x.OldPassword, - customer.PasswordSalt, customerSettings.HashedPasswordFormat), - _ => x.OldPassword - }; - if (oldPwd != customer.Password) + var oldPasswordValid = encryptionService.VerifyPassword(x.OldPassword, customer.PasswordFormatId, + customer.Password, customer.PasswordSalt, customerSettings.HashedPasswordFormat); + if (!oldPasswordValid) context.AddFailure( translationService.GetResource("Account.ChangePassword.Errors.OldPasswordDoesntMatch")); } diff --git a/src/Web/Grand.Web/Validators/Customer/DeleteAccountValidator.cs b/src/Web/Grand.Web/Validators/Customer/DeleteAccountValidator.cs index 9c5b65b60..6ec36f0fb 100644 --- a/src/Web/Grand.Web/Validators/Customer/DeleteAccountValidator.cs +++ b/src/Web/Grand.Web/Validators/Customer/DeleteAccountValidator.cs @@ -20,16 +20,9 @@ public DeleteAccountValidator( .WithMessage(translationService.GetResource("Account.DeleteAccount.Fields.Password.Required")); RuleFor(x => x).Custom((x, context) => { - var pwd = contextAccessor.WorkContext.CurrentCustomer.PasswordFormatId switch { - PasswordFormat.Clear => x.Password, - PasswordFormat.Encrypted => encryptionService.EncryptText(x.Password, - contextAccessor.WorkContext.CurrentCustomer.PasswordSalt), - PasswordFormat.Hashed => encryptionService.CreatePasswordHash(x.Password, - contextAccessor.WorkContext.CurrentCustomer.PasswordSalt, - customerSettings.HashedPasswordFormat), - _ => throw new Exception("PasswordFormat not supported") - }; - var isValid = pwd == contextAccessor.WorkContext.CurrentCustomer.Password; + var customer = contextAccessor.WorkContext.CurrentCustomer; + var isValid = encryptionService.VerifyPassword(x.Password, customer.PasswordFormatId, customer.Password, + customer.PasswordSalt, customerSettings.HashedPasswordFormat); if (!isValid) context.AddFailure(translationService.GetResource("Account.Login.WrongCredentials")); }); } diff --git a/src/Web/Grand.Web/Validators/Customer/LoginValidator.cs b/src/Web/Grand.Web/Validators/Customer/LoginValidator.cs index 9520be907..58e7029b2 100644 --- a/src/Web/Grand.Web/Validators/Customer/LoginValidator.cs +++ b/src/Web/Grand.Web/Validators/Customer/LoginValidator.cs @@ -75,14 +75,8 @@ public LoginValidator( break; case not null: { - var pwd = customer.PasswordFormatId switch { - PasswordFormat.Clear => x.Password, - PasswordFormat.Encrypted => encryptionService.EncryptText(x.Password, customer.PasswordSalt), - PasswordFormat.Hashed => encryptionService.CreatePasswordHash(x.Password, customer.PasswordSalt, - customerSettings.HashedPasswordFormat), - _ => throw new Exception("PasswordFormat not supported") - }; - var isValid = pwd == customer.Password; + var isValid = encryptionService.VerifyPassword(x.Password, customer.PasswordFormatId, + customer.Password, customer.PasswordSalt, customerSettings.HashedPasswordFormat); if (!isValid) { context.AddFailure(translationService.GetResource("Account.Login.WrongCredentials")); From b36889b7bdd3896a84c257a206f8deb22c2cdcab Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 5 Jul 2026 17:39:17 +0200 Subject: [PATCH 2/4] Harden PBKDF2 verification: fail closed on malformed hashes, upgrade on weaker cost Addresses review feedback on the password hashing PR: - VerifyPbkdf2 now fails closed (returns false) for corrupted stored hashes - non-positive iteration counts, invalid Base64, empty hash, wrong segment count - instead of letting Rfc2898DeriveBytes throw on the auth path. - PasswordHashNeedsUpgrade now compares the embedded iteration count to the configured cost, so raising PasswordHashIterations re-hashes existing PBKDF2 credentials on next login (matches the documented behavior). - Adds tests for both. Co-Authored-By: Claude Opus 4.8 --- .../Services/Security/EncryptionService.cs | 30 +++++++++++-------- .../Security/EncryptionServiceTests.cs | 27 +++++++++++++++++ 2 files changed, 45 insertions(+), 12 deletions(-) diff --git a/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs b/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs index 0bf2097c0..ed9d584b6 100644 --- a/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs +++ b/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs @@ -150,8 +150,13 @@ public virtual bool VerifyPassword(string enteredPassword, PasswordFormat passwo public virtual bool PasswordHashNeedsUpgrade(PasswordFormat passwordFormat, string storedPassword) { - //only a current-strength PBKDF2 hash is considered up to date; everything else is upgraded on next login - return !(passwordFormat == PasswordFormat.Hashed && IsPbkdf2Hash(storedPassword)); + //up to date only if it is a PBKDF2 hash whose embedded cost meets the currently configured iteration count; + //everything else (Clear/Encrypted, legacy SHA, or a weaker/unparseable PBKDF2 hash) is upgraded on next login + if (passwordFormat != PasswordFormat.Hashed || !IsPbkdf2Hash(storedPassword)) + return true; + + var parts = storedPassword.Split('$'); + return parts.Length != 5 || !int.TryParse(parts[2], out var iterations) || iterations < Iterations; } private static bool IsPbkdf2Hash(string storedPassword) @@ -163,24 +168,25 @@ private bool VerifyPbkdf2(string enteredPassword, string storedPassword) { //PBKDF2$1$$$ var parts = storedPassword.Split('$'); - if (parts.Length != 5 || !int.TryParse(parts[2], out var iterations)) + if (parts.Length != 5 || !int.TryParse(parts[2], out var iterations) || iterations <= 0) return false; - byte[] salt; - byte[] expected; try { - salt = Convert.FromBase64String(parts[3]); - expected = Convert.FromBase64String(parts[4]); + var salt = Convert.FromBase64String(parts[3]); + var expected = Convert.FromBase64String(parts[4]); + if (expected.Length == 0) + return false; + + var actual = Rfc2898DeriveBytes.Pbkdf2( + PepperedPassword(enteredPassword), salt, iterations, HashAlgorithmName.SHA256, expected.Length); + return CryptographicOperations.FixedTimeEquals(actual, expected); } - catch (FormatException) + catch (Exception ex) when (ex is FormatException or ArgumentException) { + //corrupted/malformed stored hash - fail closed instead of throwing on the auth path return false; } - - var actual = Rfc2898DeriveBytes.Pbkdf2( - PepperedPassword(enteredPassword), salt, iterations, HashAlgorithmName.SHA256, expected.Length); - return CryptographicOperations.FixedTimeEquals(actual, expected); } private byte[] PepperedPassword(string password) diff --git a/src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs b/src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs index f3298ea08..0ce7b6cf5 100644 --- a/src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs +++ b/src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs @@ -134,6 +134,33 @@ public void PasswordHashNeedsUpgrade_TrueForLegacyOrReversible_FalseForPbkdf2() Assert.IsTrue(_encryptionService.PasswordHashNeedsUpgrade(PasswordFormat.Encrypted, "cipher")); } + [TestMethod] + public void PasswordHashNeedsUpgrade_TrueWhenEmbeddedIterationsBelowConfigured() + { + //hash created at the default cost, then verified against a service configured with a higher cost + var weakHash = _encryptionService.HashPassword("password"); + var stronger = new EncryptionService(new SecurityConfig { PasswordHashIterations = 400_000 }); + + Assert.IsTrue(stronger.PasswordHashNeedsUpgrade(PasswordFormat.Hashed, weakHash)); + Assert.IsFalse(stronger.PasswordHashNeedsUpgrade(PasswordFormat.Hashed, stronger.HashPassword("password"))); + } + + [TestMethod] + public void VerifyPassword_Pbkdf2_FailsClosedOnMalformedHash() + { + //corrupted stored hashes must not throw on the auth path - they simply do not match + foreach (var malformed in new[] + { + "PBKDF2$1$0$YQ==$YQ==", //zero iterations + "PBKDF2$1$-5$YQ==$YQ==", //negative iterations + "PBKDF2$1$210000$not-base64$YQ==", //invalid salt + "PBKDF2$1$210000$YQ==", //too few segments + "PBKDF2$1$210000$YQ==$" //empty hash + }) + Assert.IsFalse(_encryptionService.VerifyPassword("password", PasswordFormat.Hashed, malformed, + string.Empty, HashedPasswordFormat.SHA1), $"should fail closed for: {malformed}"); + } + [TestMethod] public void VerifyPassword_Pbkdf2_IsPepperSensitive() { From fd9ca18f7083e3aa04851549010a592a4f9155a6 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 5 Jul 2026 17:42:57 +0200 Subject: [PATCH 3/4] Narrow PBKDF2 verify catch to FormatException The only reachable throw inside the block is Convert.FromBase64String (FormatException); iteration count and output length are already guarded above and the remaining Pbkdf2 arguments are constant/non-null, so the broader ArgumentException branch was dead. Narrowing makes the intent (reject a corrupted stored value) explicit without masking argument errors. Co-Authored-By: Claude Opus 4.8 --- .../Services/Security/EncryptionService.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs b/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs index ed9d584b6..908816040 100644 --- a/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs +++ b/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs @@ -182,9 +182,10 @@ private bool VerifyPbkdf2(string enteredPassword, string storedPassword) PepperedPassword(enteredPassword), salt, iterations, HashAlgorithmName.SHA256, expected.Length); return CryptographicOperations.FixedTimeEquals(actual, expected); } - catch (Exception ex) when (ex is FormatException or ArgumentException) + catch (FormatException) { - //corrupted/malformed stored hash - fail closed instead of throwing on the auth path + //salt/hash are not valid Base64 (corrupted stored value) - fail closed on the auth path. + //Other arguments cannot throw here: iterations/output length are guarded above and the rest are constant. return false; } } From 177061a641d3af28148fc01e744429219540207c Mon Sep 17 00:00:00 2001 From: Krzysztof Pajak Date: Sun, 5 Jul 2026 17:48:21 +0200 Subject: [PATCH 4/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../Services/CustomerManagerService.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs b/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs index cbbf0e6d5..20d39143f 100644 --- a/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs +++ b/src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs @@ -86,8 +86,16 @@ public virtual async Task LoginCustomer(string usernameOrE //transparently migrate weak/legacy hashes (SHA-x, Clear, Encrypted) to the modern PBKDF2 format if (_encryptionService.PasswordHashNeedsUpgrade(customer.PasswordFormatId, customer.Password)) - await UpgradePasswordHash(customer, password); - + { + try + { + await UpgradePasswordHash(customer, password); + } + catch + { + //best-effort upgrade only; don't block login if persisting the upgraded hash fails + } + } //2fa required if (customer.GetUserFieldFromEntity(SystemCustomerFieldNames.TwoFactorEnabled) && _customerSettings.TwoFactorAuthenticationEnabled)