From b89ffb7c18210d7b7415a1483cf84ecb866ebd22 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 5 Jul 2026 17:01:51 +0200 Subject: [PATCH 1/3] 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 8b4559b0030e4c78012399770146bfa0d87e53c8 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 5 Jul 2026 17:16:36 +0200 Subject: [PATCH 2/3] Stop storing passwords reversibly and fail fast on weak JWT secrets Audit #2 (crypto) - eliminate reversible/deterministic 3DES "encryption" used as a password verifier; hash instead: - Installer now creates the default administrator with a PBKDF2 hash (via IEncryptionService) instead of the legacy TripleDES "Encrypted" format. - API users (UserApi) are stored as PBKDF2 hashes on create/update; login verifies PBKDF2 for new records and falls back to the legacy TripleDES check for existing ones (re-saving the user migrates it). Removes the reversible password + per-user key stored side by side. - EncryptText/DecryptText are documented as legacy verification-only; the algorithm is left unchanged so existing values still verify. Audit #3 (JWT secrets) - add ApiSecurityStartup that validates BackendAPI and FrontendAPI signing keys at startup: outside Development it aborts startup when an enabled API has a missing, placeholder or <32-char secret (a weak key allows forging valid tokens); in Development it only warns. appsettings comments updated to require setting the secrets via env/secret store. Co-Authored-By: Claude Opus 4.8 --- .../Common/Security/IEncryptionService.cs | 7 +- .../Infrastructure/ApiSecurityStartup.cs | 72 +++++++++++++++++++ .../Validators/Common/LoginValidator.cs | 7 +- .../Grand.Module.Installer.csproj | 3 + .../Services/InstallationService.cs | 33 ++------- .../Controllers/ApiUserController.cs | 4 +- src/Web/Grand.Web/App_Data/appsettings.json | 6 +- 7 files changed, 100 insertions(+), 32 deletions(-) create mode 100644 src/Modules/Grand.Module.Api/Infrastructure/ApiSecurityStartup.cs 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 b437ba3cb..40fb4b118 100644 --- a/src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs +++ b/src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs @@ -51,7 +51,10 @@ bool VerifyPassword(string enteredPassword, PasswordFormat passwordFormat, strin bool PasswordHashNeedsUpgrade(PasswordFormat passwordFormat, string storedPassword); /// - /// Encrypt text + /// Encrypt text (legacy TripleDES). Retained only to verify pre-existing reversibly-encrypted values + /// (legacy customer "Encrypted" password format and legacy API users). Do NOT use for new data or for + /// protecting secrets at rest - hash passwords with and use ASP.NET Core Data + /// Protection for reversible secrets. /// /// Text to encrypt /// Encryption private key @@ -59,7 +62,7 @@ bool VerifyPassword(string enteredPassword, PasswordFormat passwordFormat, strin string EncryptText(string plainText, string privateKey); /// - /// Decrypt text + /// Decrypt text (legacy TripleDES). Legacy counterpart of ; see its remarks. /// /// Text to decrypt /// Encryption private key diff --git a/src/Modules/Grand.Module.Api/Infrastructure/ApiSecurityStartup.cs b/src/Modules/Grand.Module.Api/Infrastructure/ApiSecurityStartup.cs new file mode 100644 index 000000000..6436707b9 --- /dev/null +++ b/src/Modules/Grand.Module.Api/Infrastructure/ApiSecurityStartup.cs @@ -0,0 +1,72 @@ +using Grand.Infrastructure; +using Grand.Infrastructure.Configuration; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace Grand.Module.Api.Infrastructure; + +/// +/// Fails fast at startup when an enabled API is configured with a missing, placeholder or too-short JWT signing +/// key. A weak/known key lets anyone forge valid tokens, so outside Development this aborts startup instead of +/// silently running with a forgeable secret. +/// +public class ApiSecurityStartup : IStartupApplication +{ + //shipped placeholder in appsettings.json - must never be used to sign real tokens + private const string PlaceholderSecret = "your private secret key to use api"; + + //HS256 needs a >= 256-bit key; the signing key is the raw ASCII bytes of the secret, so require >= 32 chars + private const int MinSecretLength = 32; + + public void Configure(WebApplication application, IWebHostEnvironment webHostEnvironment) + { + var logger = application.Services.GetRequiredService() + .CreateLogger(nameof(ApiSecurityStartup)); + + var backend = new BackendAPIConfig(); + application.Configuration.GetSection("BackendAPI").Bind(backend); + Validate("BackendAPI", backend.Enabled, backend.SecretKey, webHostEnvironment, logger); + + var frontend = new FrontendAPIConfig(); + application.Configuration.GetSection("FrontendAPI").Bind(frontend); + Validate("FrontendAPI", frontend.Enabled, frontend.SecretKey, webHostEnvironment, logger); + } + + private static void Validate(string section, bool enabled, string secret, IWebHostEnvironment env, ILogger logger) + { + if (!enabled) + return; + + if (IsStrong(secret)) + return; + + var message = + $"{section}.SecretKey is missing, still set to the placeholder, or shorter than {MinSecretLength} " + + "characters. Configure a strong, unique secret (e.g. via the environment variable " + + $"{section}__SecretKey or a secret store) - a weak key allows forging valid API tokens."; + + //in Development we only warn so local setups keep working; in every other environment we abort startup + if (env.IsDevelopment()) + logger.LogWarning("{Message}", message); + else + throw new InvalidOperationException(message); + } + + private static bool IsStrong(string secret) + { + return !string.IsNullOrWhiteSpace(secret) + && secret.Length >= MinSecretLength + && !string.Equals(secret, PlaceholderSecret, StringComparison.Ordinal); + } + + public void ConfigureServices(IServiceCollection services, IConfiguration configuration) + { + } + + public int Priority => 0; + public bool BeforeConfigure => true; +} diff --git a/src/Modules/Grand.Module.Api/Validators/Common/LoginValidator.cs b/src/Modules/Grand.Module.Api/Validators/Common/LoginValidator.cs index 68d4b2440..171e7d772 100644 --- a/src/Modules/Grand.Module.Api/Validators/Common/LoginValidator.cs +++ b/src/Modules/Grand.Module.Api/Validators/Common/LoginValidator.cs @@ -30,7 +30,12 @@ public LoginValidator( { var base64EncodedBytes = Convert.FromBase64String(x.Password); var password = Encoding.UTF8.GetString(base64EncodedBytes); - if (userapi.Password == encryptionService.EncryptText(password, userapi.PrivateKey)) + //modern records are PBKDF2-hashed; legacy records are reversibly encrypted with the per-user PrivateKey + var isValid = string.IsNullOrEmpty(userapi.PrivateKey) + ? encryptionService.VerifyPassword(password, PasswordFormat.Hashed, userapi.Password, + string.Empty, HashedPasswordFormat.SHA1) + : userapi.Password == encryptionService.EncryptText(password, userapi.PrivateKey); + if (isValid) return true; } } diff --git a/src/Modules/Grand.Module.Installer/Grand.Module.Installer.csproj b/src/Modules/Grand.Module.Installer/Grand.Module.Installer.csproj index 0c2aa8bfd..c23d05e9e 100644 --- a/src/Modules/Grand.Module.Installer/Grand.Module.Installer.csproj +++ b/src/Modules/Grand.Module.Installer/Grand.Module.Installer.csproj @@ -6,6 +6,9 @@ true + + False + False diff --git a/src/Modules/Grand.Module.Installer/Services/InstallationService.cs b/src/Modules/Grand.Module.Installer/Services/InstallationService.cs index e7e0efa51..d82cc4dfd 100644 --- a/src/Modules/Grand.Module.Installer/Services/InstallationService.cs +++ b/src/Modules/Grand.Module.Installer/Services/InstallationService.cs @@ -1,3 +1,4 @@ +using Grand.Business.Core.Interfaces.Common.Security; using Grand.Data; using Grand.Domain; using Grand.Domain.Admin; @@ -436,33 +437,13 @@ protected virtual Task InstallMenuAdminSiteMap() protected virtual async Task HashDefaultCustomerPassword(string defaultUserEmail, string defaultUserPassword) { - var passwordSalt = CommonHelper.GenerateRandomDigitCode(24); - - var tDes = TripleDES.Create(); - - tDes.Key = new ASCIIEncoding().GetBytes(passwordSalt); - tDes.IV = new ASCIIEncoding().GetBytes(passwordSalt[^8..]); - - var encryptedBinary = EncryptTextToMemory(defaultUserPassword, tDes.Key, tDes.IV); - var password = Convert.ToBase64String(encryptedBinary); - var customer = _customerRepository.Table.FirstOrDefault(x=>x.Email == defaultUserEmail); - customer!.Password = password; - customer!.PasswordSalt = passwordSalt; - customer!.PasswordFormatId = PasswordFormat.Encrypted; + //create the default administrator with a modern PBKDF2 hash (salt/parameters embedded in the value) + var encryptionService = _serviceProvider.GetRequiredService(); + var customer = _customerRepository.Table.FirstOrDefault(x => x.Email == defaultUserEmail); + customer!.Password = encryptionService.HashPassword(defaultUserPassword); + customer!.PasswordSalt = string.Empty; + customer!.PasswordFormatId = PasswordFormat.Hashed; await _customerRepository.UpdateAsync(customer); - - } - private static byte[] EncryptTextToMemory(string data, byte[] key, byte[] iv) - { - using var ms = new MemoryStream(); - using (var cs = new CryptoStream(ms, TripleDES.Create().CreateEncryptor(key, iv), CryptoStreamMode.Write)) - { - var toEncrypt = new UnicodeEncoding().GetBytes(data); - cs.Write(toEncrypt, 0, toEncrypt.Length); - cs.FlushFinalBlock(); - } - - return ms.ToArray(); } private async Task CreateIndexes(IDatabaseContext dbContext, DataSettings dataSettings) diff --git a/src/Web/Grand.Web.Admin/Controllers/ApiUserController.cs b/src/Web/Grand.Web.Admin/Controllers/ApiUserController.cs index e6c235c95..af031cc0e 100644 --- a/src/Web/Grand.Web.Admin/Controllers/ApiUserController.cs +++ b/src/Web/Grand.Web.Admin/Controllers/ApiUserController.cs @@ -24,8 +24,8 @@ public ApiUserController(IUserApiService userApiService, IEncryptionService encr protected (string hashpassword, string privatekey) HashPassword(string password) { - var pk = CommonHelper.GenerateRandomDigitCode(24); - return (_encryptionService.EncryptText(password, pk), pk); + //store a one-way PBKDF2 hash instead of reversible encryption; PrivateKey is no longer used for new records + return (_encryptionService.HashPassword(password), string.Empty); } public IActionResult Index() diff --git a/src/Web/Grand.Web/App_Data/appsettings.json b/src/Web/Grand.Web/App_Data/appsettings.json index af0e5803f..43b956c35 100644 --- a/src/Web/Grand.Web/App_Data/appsettings.json +++ b/src/Web/Grand.Web/App_Data/appsettings.json @@ -198,7 +198,9 @@ "FrontendAPI": { "Enabled": true, "JsonContentType": false, - //when is enabled, use ContentType = application/json to read json from body, default is form-data + //when is enabled, use ContentType = application/json to read json from body, default is form-data + //REQUIRED: replace with a strong, unique secret (>= 32 chars) kept out of source control (env var FrontendAPI__SecretKey or a secret store). + //Outside Development the app refuses to start while this is empty/placeholder/too short - a weak key lets anyone forge API tokens. "SecretKey": "your private secret key to use api", "ValidateIssuer": false, "ValidIssuer": "", @@ -214,6 +216,8 @@ //locally: https://localhost:44350/scalar/v1 "BackendAPI": { "Enabled": true, + //REQUIRED: replace with a strong, unique secret (>= 32 chars) kept out of source control (env var BackendAPI__SecretKey or a secret store). + //Outside Development the app refuses to start while this is empty/placeholder/too short - a weak key lets anyone forge admin API tokens. "SecretKey": "your private secret key to use api", "ValidateIssuer": false, "ValidIssuer": "", From 0ac70b33ee268567769dd47a8d0dfe14bc4d6354 Mon Sep 17 00:00:00 2001 From: KrzysztofPajak Date: Sun, 5 Jul 2026 18:40:43 +0200 Subject: [PATCH 3/3] Detect API user password format from the stored hash, not PrivateKey Review feedback: choosing the PBKDF2 vs legacy path from an empty PrivateKey is fragile - a PBKDF2 record that still carries a PrivateKey (imported data or a future update that only rewrites Password) would take the legacy path and always fail to authenticate. Add IEncryptionService.IsHashedPassword to detect the modern self-describing hash from the stored value, and use it as the discriminator. The legacy path now also verifies in constant time and is guarded against a missing key. Co-Authored-By: Claude Opus 4.8 --- .../Services/Security/EncryptionService.cs | 5 +++++ .../Interfaces/Common/Security/IEncryptionService.cs | 7 +++++++ .../Validators/Common/LoginValidator.cs | 8 +++++--- .../Services/Security/EncryptionServiceTests.cs | 10 ++++++++++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs b/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs index 908816040..704ff8b79 100644 --- a/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs +++ b/src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs @@ -159,6 +159,11 @@ public virtual bool PasswordHashNeedsUpgrade(PasswordFormat passwordFormat, stri return parts.Length != 5 || !int.TryParse(parts[2], out var iterations) || iterations < Iterations; } + public virtual bool IsHashedPassword(string storedPassword) + { + return IsPbkdf2Hash(storedPassword); + } + private static bool IsPbkdf2Hash(string storedPassword) { return storedPassword != null && storedPassword.StartsWith(Pbkdf2Prefix + "$", StringComparison.Ordinal); 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 40fb4b118..a5888d3d0 100644 --- a/src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs +++ b/src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs @@ -50,6 +50,13 @@ bool VerifyPassword(string enteredPassword, PasswordFormat passwordFormat, strin /// bool PasswordHashNeedsUpgrade(PasswordFormat passwordFormat, string storedPassword); + /// + /// Returns true when the stored value is the modern self-describing (PBKDF2) hash produced by + /// . Use it to pick the verification path for records whose password format is + /// not tracked in a separate field (e.g. API users), instead of inferring it from other columns. + /// + bool IsHashedPassword(string storedPassword); + /// /// Encrypt text (legacy TripleDES). Retained only to verify pre-existing reversibly-encrypted values /// (legacy customer "Encrypted" password format and legacy API users). Do NOT use for new data or for diff --git a/src/Modules/Grand.Module.Api/Validators/Common/LoginValidator.cs b/src/Modules/Grand.Module.Api/Validators/Common/LoginValidator.cs index 171e7d772..08ea7710c 100644 --- a/src/Modules/Grand.Module.Api/Validators/Common/LoginValidator.cs +++ b/src/Modules/Grand.Module.Api/Validators/Common/LoginValidator.cs @@ -30,11 +30,13 @@ public LoginValidator( { var base64EncodedBytes = Convert.FromBase64String(x.Password); var password = Encoding.UTF8.GetString(base64EncodedBytes); - //modern records are PBKDF2-hashed; legacy records are reversibly encrypted with the per-user PrivateKey - var isValid = string.IsNullOrEmpty(userapi.PrivateKey) + //detect the format from the stored password itself: modern records are PBKDF2 hashes, legacy + //records are reversibly encrypted with the per-user PrivateKey (verified in constant time) + var isValid = encryptionService.IsHashedPassword(userapi.Password) ? encryptionService.VerifyPassword(password, PasswordFormat.Hashed, userapi.Password, string.Empty, HashedPasswordFormat.SHA1) - : userapi.Password == encryptionService.EncryptText(password, userapi.PrivateKey); + : !string.IsNullOrEmpty(userapi.PrivateKey) && encryptionService.VerifyPassword(password, + PasswordFormat.Encrypted, userapi.Password, userapi.PrivateKey, HashedPasswordFormat.SHA1); if (isValid) return true; } 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 0ce7b6cf5..ac1ddd91e 100644 --- a/src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs +++ b/src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs @@ -145,6 +145,16 @@ public void PasswordHashNeedsUpgrade_TrueWhenEmbeddedIterationsBelowConfigured() Assert.IsFalse(stronger.PasswordHashNeedsUpgrade(PasswordFormat.Hashed, stronger.HashPassword("password"))); } + [TestMethod] + public void IsHashedPassword_TrueOnlyForPbkdf2Format() + { + Assert.IsTrue(_encryptionService.IsHashedPassword(_encryptionService.HashPassword("password"))); + Assert.IsFalse(_encryptionService.IsHashedPassword( + _encryptionService.CreatePasswordHash("password", _encryptionService.CreateSaltKey(16)))); + Assert.IsFalse(_encryptionService.IsHashedPassword("some-reversibly-encrypted-value")); + Assert.IsFalse(_encryptionService.IsHashedPassword(null)); + } + [TestMethod] public void VerifyPassword_Pbkdf2_FailsClosedOnMalformedHash() {