From e5c272b5d2c7166adb25a990d04616ede96c2c8c Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 7 Oct 2020 15:20:43 +1100 Subject: [PATCH] Adds support for the super old password format so we can handle upgrades --- src/Umbraco.Core/Constants-Security.cs | 1 + .../Security/LegacyPasswordSecurity.cs | 86 +++++++++++++++---- .../Security/PasswordSecurityTests.cs | 19 ++-- .../UmbracoServiceMembershipProviderTests.cs | 4 +- .../Security/BackOfficePasswordHasher.cs | 15 ++-- .../Security/BackOfficeOwinUserManager.cs | 8 +- .../Providers/UmbracoMembershipProvider.cs | 8 +- .../Security/UserAwarePasswordHasher.cs | 40 --------- src/Umbraco.Web/Umbraco.Web.csproj | 1 - 9 files changed, 96 insertions(+), 86 deletions(-) delete mode 100644 src/Umbraco.Web/Security/UserAwarePasswordHasher.cs diff --git a/src/Umbraco.Core/Constants-Security.cs b/src/Umbraco.Core/Constants-Security.cs index fda4c23dc0..9bcdedca70 100644 --- a/src/Umbraco.Core/Constants-Security.cs +++ b/src/Umbraco.Core/Constants-Security.cs @@ -62,6 +62,7 @@ public const string AspNetCoreV3PasswordHashAlgorithmName = "PBKDF2.ASPNETCORE.V3"; public const string AspNetCoreV2PasswordHashAlgorithmName = "PBKDF2.ASPNETCORE.V2"; public const string AspNetUmbraco8PasswordHashAlgorithmName = "HMACSHA256"; + public const string AspNetUmbraco4PasswordHashAlgorithmName = "HMACSHA1"; } } } diff --git a/src/Umbraco.Core/Security/LegacyPasswordSecurity.cs b/src/Umbraco.Core/Security/LegacyPasswordSecurity.cs index 9b14c3ccba..dd28afbba0 100644 --- a/src/Umbraco.Core/Security/LegacyPasswordSecurity.cs +++ b/src/Umbraco.Core/Security/LegacyPasswordSecurity.cs @@ -11,9 +11,6 @@ namespace Umbraco.Core.Security /// public class LegacyPasswordSecurity { - // TODO: This class no longer has the logic available to verify the old old old password format, we should - // include this ability so that upgrades for very old versions/data can work and then auto-migrate to the new password format. - private readonly IPasswordConfiguration _passwordConfiguration; private readonly PasswordGenerator _generator; @@ -34,13 +31,14 @@ namespace Umbraco.Core.Security /// /// /// + // TODO: Do we need this method? We shouldn't be using this class to create new password hashes for storage public string HashPasswordForStorage(string password) { if (string.IsNullOrWhiteSpace(password)) throw new ArgumentException("password cannot be empty", nameof(password)); string salt; - var hashed = HashNewPassword(password, out salt); + var hashed = HashNewPassword(_passwordConfiguration.HashAlgorithmType, password, out salt); return FormatPasswordForStorage(hashed, salt); } @@ -51,6 +49,7 @@ namespace Umbraco.Core.Security /// /// /// + // TODO: Do we need this method? We shouldn't be using this class to create new password hashes for storage public string FormatPasswordForStorage(string hashedPassword, string salt) { return salt + hashedPassword; @@ -59,18 +58,24 @@ namespace Umbraco.Core.Security /// /// Hashes a password with a given salt /// + /// The hashing algorithm for the password. /// /// /// - public string HashPassword(string pass, string salt) + public string HashPassword(string algorithmType, string pass, string salt) { + if (IsLegacySHA1Algorithm(algorithmType)) + { + return HashLegacySHA1Password(pass); + } + //This is the correct way to implement this (as per the sql membership provider) var bytes = Encoding.Unicode.GetBytes(pass); var saltBytes = Convert.FromBase64String(salt); byte[] inArray; - var hashAlgorithm = GetCurrentHashAlgorithm(); + var hashAlgorithm = GetHashAlgorithm(algorithmType); var algorithm = hashAlgorithm as KeyedHashAlgorithm; if (algorithm != null) { @@ -116,43 +121,55 @@ namespace Umbraco.Core.Security /// /// Verifies if the password matches the expected hash+salt of the stored password string /// + /// The hashing algorithm for the stored password. /// The password. /// The value of the password stored in a data store. /// - public bool VerifyPassword(string password, string dbPassword) + public bool VerifyPassword(string algorithm, string password, string dbPassword) { if (string.IsNullOrWhiteSpace(dbPassword)) throw new ArgumentException("Value cannot be null or whitespace.", nameof(dbPassword)); if (dbPassword.StartsWith(Constants.Security.EmptyPasswordPrefix)) return false; - var storedHashedPass = ParseStoredHashPassword(dbPassword, out var salt); - var hashed = HashPassword(password, salt); + var storedHashedPass = ParseStoredHashPassword(algorithm, dbPassword, out var salt); + var hashed = HashPassword(algorithm, password, salt); return storedHashedPass == hashed; } /// /// Create a new password hash and a new salt /// + /// The hashing algorithm for the password. /// /// /// - public string HashNewPassword(string newPassword, out string salt) + // TODO: Do we need this method? We shouldn't be using this class to create new password hashes for storage + public string HashNewPassword(string algorithm, string newPassword, out string salt) { salt = GenerateSalt(); - return HashPassword(newPassword, salt); + return HashPassword(algorithm, newPassword, salt); } /// /// Parses out the hashed password and the salt from the stored password string value /// + /// The hashing algorithm for the stored password. /// /// returns the salt /// - public string ParseStoredHashPassword(string storedString, out string salt) + public string ParseStoredHashPassword(string algorithm, string storedString, out string salt) { if (string.IsNullOrWhiteSpace(storedString)) throw new ArgumentException("Value cannot be null or whitespace.", nameof(storedString)); + // This is for the <= v4 hashing algorithm for which there was no salt + if (IsLegacySHA1Algorithm(algorithm)) + { + salt = string.Empty; + return storedString; + } + + var saltLen = GenerateSalt(); salt = storedString.Substring(0, saltLen.Length); return storedString.Substring(saltLen.Length); @@ -168,28 +185,61 @@ namespace Umbraco.Core.Security /// /// Return the hash algorithm to use based on the /// + /// The hashing algorithm name. /// /// - private HashAlgorithm GetCurrentHashAlgorithm() + private HashAlgorithm GetHashAlgorithm(string algorithm) { - if (_passwordConfiguration.HashAlgorithmType.IsNullOrWhiteSpace()) + if (algorithm.IsNullOrWhiteSpace()) throw new InvalidOperationException("No hash algorithm type specified"); - var alg = HashAlgorithm.Create(_passwordConfiguration.HashAlgorithmType); + var alg = HashAlgorithm.Create(algorithm); if (alg == null) - throw new InvalidOperationException($"The hash algorithm specified {_passwordConfiguration.HashAlgorithmType} cannot be resolved"); + throw new InvalidOperationException($"The hash algorithm specified {algorithm} cannot be resolved"); return alg; } public bool SupportHashAlgorithm(string algorithm) { - if (algorithm.InvariantEquals(typeof(HMACSHA256).Name)) + // This is for the v6-v8 hashing algorithm + if (algorithm.InvariantEquals(Constants.Security.AspNetUmbraco8PasswordHashAlgorithmName)) + return true; + + // This is for the <= v4 hashing algorithm + if (IsLegacySHA1Algorithm(algorithm)) return true; - // TODO: Need to add the old old old format in here too which was just HMACSHA1 IIRC but had a custom key implementation as the password itself return false; } + private bool IsLegacySHA1Algorithm(string algorithm) => algorithm.InvariantEquals(Constants.Security.AspNetUmbraco4PasswordHashAlgorithmName); + + /// + /// Hashes the password with the old v4 algorithm + /// + /// The password. + /// The encoded password. + private string HashLegacySHA1Password(string password) + { + var hashAlgorithm = GetLegacySHA1Algorithm(password); + var hash = Convert.ToBase64String(hashAlgorithm.ComputeHash(Encoding.Unicode.GetBytes(password))); + return hash; + } + + /// + /// Returns the old v4 algorithm and settings + /// + /// + /// + private HashAlgorithm GetLegacySHA1Algorithm(string password) + { + return new HMACSHA1 + { + //the legacy salt was actually the password :( + Key = Encoding.Unicode.GetBytes(password) + }; + } + } } diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Security/PasswordSecurityTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Security/PasswordSecurityTests.cs index 0ef6063910..4b5c084b01 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/Security/PasswordSecurityTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/Security/PasswordSecurityTests.cs @@ -14,14 +14,15 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Security [Test] public void Check_Password_Hashed_Non_KeyedHashAlgorithm() { - var passwordSecurity = new LegacyPasswordSecurity(Mock.Of(x => x.HashAlgorithmType == "SHA256")); + var passwordConfiguration = Mock.Of(x => x.HashAlgorithmType == "SHA256"); + var passwordSecurity = new LegacyPasswordSecurity(passwordConfiguration); string salt; var pass = "ThisIsAHashedPassword"; - var hashed = passwordSecurity.HashNewPassword(pass, out salt); + var hashed = passwordSecurity.HashNewPassword(passwordConfiguration.HashAlgorithmType, pass, out salt); var storedPassword = passwordSecurity.FormatPasswordForStorage(hashed, salt); - var result = passwordSecurity.VerifyPassword("ThisIsAHashedPassword", storedPassword); + var result = passwordSecurity.VerifyPassword(passwordConfiguration.HashAlgorithmType, "ThisIsAHashedPassword", storedPassword); Assert.IsTrue(result); } @@ -29,14 +30,15 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Security [Test] public void Check_Password_Hashed_KeyedHashAlgorithm() { - var passwordSecurity = new LegacyPasswordSecurity(Mock.Of(x => x.HashAlgorithmType == Constants.Security.AspNetUmbraco8PasswordHashAlgorithmName)); + var passwordConfiguration = Mock.Of(x => x.HashAlgorithmType == Constants.Security.AspNetUmbraco8PasswordHashAlgorithmName); + var passwordSecurity = new LegacyPasswordSecurity(passwordConfiguration); string salt; var pass = "ThisIsAHashedPassword"; - var hashed = passwordSecurity.HashNewPassword(pass, out salt); + var hashed = passwordSecurity.HashNewPassword(passwordConfiguration.HashAlgorithmType, pass, out salt); var storedPassword = passwordSecurity.FormatPasswordForStorage(hashed, salt); - var result = passwordSecurity.VerifyPassword("ThisIsAHashedPassword", storedPassword); + var result = passwordSecurity.VerifyPassword(passwordConfiguration.HashAlgorithmType, "ThisIsAHashedPassword", storedPassword); Assert.IsTrue(result); } @@ -57,13 +59,14 @@ namespace Umbraco.Tests.UnitTests.Umbraco.Core.Security [Test] public void Get_Stored_Password_Hashed() { - var passwordSecurity = new LegacyPasswordSecurity(Mock.Of(x => x.HashAlgorithmType == Constants.Security.AspNetUmbraco8PasswordHashAlgorithmName)); + var passwordConfiguration = Mock.Of(x => x.HashAlgorithmType == Constants.Security.AspNetUmbraco8PasswordHashAlgorithmName); + var passwordSecurity = new LegacyPasswordSecurity(passwordConfiguration); var salt = LegacyPasswordSecurity.GenerateSalt(); var stored = salt + "ThisIsAHashedPassword"; string initSalt; - var result = passwordSecurity.ParseStoredHashPassword(stored, out initSalt); + var result = passwordSecurity.ParseStoredHashPassword(passwordConfiguration.HashAlgorithmType, stored, out initSalt); Assert.AreEqual("ThisIsAHashedPassword", result); } diff --git a/src/Umbraco.Tests/Membership/UmbracoServiceMembershipProviderTests.cs b/src/Umbraco.Tests/Membership/UmbracoServiceMembershipProviderTests.cs index 4f2cb22b4a..e54eb97c67 100644 --- a/src/Umbraco.Tests/Membership/UmbracoServiceMembershipProviderTests.cs +++ b/src/Umbraco.Tests/Membership/UmbracoServiceMembershipProviderTests.cs @@ -106,8 +106,8 @@ namespace Umbraco.Tests.Membership Assert.AreNotEqual("test", createdMember.RawPasswordValue); string salt; - var storedPassword = provider.PasswordSecurity.ParseStoredHashPassword(createdMember.RawPasswordValue, out salt); - var hashedPassword = provider.PasswordSecurity.HashPassword("testtest$1", salt); + var storedPassword = provider.PasswordSecurity.ParseStoredHashPassword(Constants.Security.AspNetUmbraco8PasswordHashAlgorithmName, createdMember.RawPasswordValue, out salt); + var hashedPassword = provider.PasswordSecurity.HashPassword(Constants.Security.AspNetUmbraco8PasswordHashAlgorithmName, "testtest$1", salt); Assert.AreEqual(hashedPassword, storedPassword); } diff --git a/src/Umbraco.Web.BackOffice/Security/BackOfficePasswordHasher.cs b/src/Umbraco.Web.BackOffice/Security/BackOfficePasswordHasher.cs index df3bc2935b..4ec3947263 100644 --- a/src/Umbraco.Web.BackOffice/Security/BackOfficePasswordHasher.cs +++ b/src/Umbraco.Web.BackOffice/Security/BackOfficePasswordHasher.cs @@ -13,12 +13,12 @@ namespace Umbraco.Web.BackOffice.Security /// public class BackOfficePasswordHasher : PasswordHasher { - private readonly LegacyPasswordSecurity _passwordSecurity; + private readonly LegacyPasswordSecurity _legacyPasswordSecurity; private readonly IJsonSerializer _jsonSerializer; public BackOfficePasswordHasher(LegacyPasswordSecurity passwordSecurity, IJsonSerializer jsonSerializer) { - _passwordSecurity = passwordSecurity; + _legacyPasswordSecurity = passwordSecurity; _jsonSerializer = jsonSerializer; } @@ -27,9 +27,12 @@ namespace Umbraco.Web.BackOffice.Security if (!user.PasswordConfig.IsNullOrWhiteSpace()) { // check if the (legacy) password security supports this hash algorith and if so then use it + // TODO: I don't think we really want to do this because we want to migrate all old password formats + // to the new format, not keep using it. AFAIK this block should be removed along with the code within the + // LegacyPasswordSecurity except the part that just validates old ones. var deserialized = _jsonSerializer.Deserialize(user.PasswordConfig); - if (_passwordSecurity.SupportHashAlgorithm(deserialized.HashAlgorithm)) - return _passwordSecurity.HashPasswordForStorage(password); + if (_legacyPasswordSecurity.SupportHashAlgorithm(deserialized.HashAlgorithm)) + return _legacyPasswordSecurity.HashPasswordForStorage(password); // We will explicitly detect names here since this allows us to future proof these checks. // The default is PBKDF2.ASPNETCORE.V3: @@ -58,9 +61,9 @@ namespace Umbraco.Web.BackOffice.Security { // check if the (legacy) password security supports this hash algorith and if so then use it var deserialized = _jsonSerializer.Deserialize(user.PasswordConfig); - if (_passwordSecurity.SupportHashAlgorithm(deserialized.HashAlgorithm)) + if (_legacyPasswordSecurity.SupportHashAlgorithm(deserialized.HashAlgorithm)) { - var result = _passwordSecurity.VerifyPassword(providedPassword, hashedPassword); + var result = _legacyPasswordSecurity.VerifyPassword(deserialized.HashAlgorithm, providedPassword, hashedPassword); return result ? PasswordVerificationResult.Success : PasswordVerificationResult.Failed; diff --git a/src/Umbraco.Web/Security/BackOfficeOwinUserManager.cs b/src/Umbraco.Web/Security/BackOfficeOwinUserManager.cs index 65d7836d42..2f5e858687 100644 --- a/src/Umbraco.Web/Security/BackOfficeOwinUserManager.cs +++ b/src/Umbraco.Web/Security/BackOfficeOwinUserManager.cs @@ -7,15 +7,14 @@ using Microsoft.Extensions.Options; using Microsoft.Owin.Security.DataProtection; using Umbraco.Core; using Umbraco.Core.BackOffice; -using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.Models; using Umbraco.Core.Mapping; -using Umbraco.Core.Security; using Umbraco.Core.Services; using Umbraco.Net; namespace Umbraco.Web.Security { + // TODO: Most of this is already migrated to netcore, there's probably not much more to go and then we can complete remove it public class BackOfficeOwinUserManager : BackOfficeUserManager { public const string OwinMarkerKey = "Umbraco.Web.Security.Identity.BackOfficeUserManagerMarker"; @@ -118,11 +117,6 @@ namespace Umbraco.Web.Security #endregion - protected override IPasswordHasher GetDefaultPasswordHasher(IPasswordConfiguration passwordConfiguration) - { - return new UserAwarePasswordHasher(new LegacyPasswordSecurity(passwordConfiguration)); - } - protected void InitUserManager(BackOfficeOwinUserManager manager, IDataProtectionProvider dataProtectionProvider) { // use a custom hasher based on our membership provider diff --git a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs index d059113e0a..87f5864cdf 100644 --- a/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs +++ b/src/Umbraco.Web/Security/Providers/UmbracoMembershipProvider.cs @@ -83,7 +83,7 @@ namespace Umbraco.Web.Security.Providers if (m == null) return false; string salt; - var encodedPassword = PasswordSecurity.HashNewPassword(newPassword, out salt); + var encodedPassword = PasswordSecurity.HashNewPassword(Membership.HashAlgorithmType, newPassword, out salt); m.RawPasswordValue = PasswordSecurity.FormatPasswordForStorage(encodedPassword, salt); m.LastPasswordChangeDate = DateTime.Now; @@ -143,7 +143,7 @@ namespace Umbraco.Web.Security.Providers } string salt; - var encodedPassword = PasswordSecurity.HashNewPassword(password, out salt); + var encodedPassword = PasswordSecurity.HashNewPassword(Membership.HashAlgorithmType, password, out salt); var member = MemberService.CreateWithIdentity( username, @@ -406,7 +406,7 @@ namespace Umbraco.Web.Security.Providers } string salt; - var encodedPassword = PasswordSecurity.HashNewPassword(generatedPassword, out salt); + var encodedPassword = PasswordSecurity.HashNewPassword(Membership.HashAlgorithmType, generatedPassword, out salt); m.RawPasswordValue = PasswordSecurity.FormatPasswordForStorage(encodedPassword, salt); m.LastPasswordChangeDate = DateTime.Now; MemberService.Save(m); @@ -519,7 +519,7 @@ namespace Umbraco.Web.Security.Providers }; } - var authenticated = PasswordSecurity.VerifyPassword(password, member.RawPasswordValue); + var authenticated = PasswordSecurity.VerifyPassword(Membership.HashAlgorithmType, password, member.RawPasswordValue); var requiresFullSave = false; diff --git a/src/Umbraco.Web/Security/UserAwarePasswordHasher.cs b/src/Umbraco.Web/Security/UserAwarePasswordHasher.cs deleted file mode 100644 index 040d79be02..0000000000 --- a/src/Umbraco.Web/Security/UserAwarePasswordHasher.cs +++ /dev/null @@ -1,40 +0,0 @@ -using Microsoft.AspNetCore.Identity; -using Umbraco.Core.BackOffice; -using Umbraco.Core.Security; - -namespace Umbraco.Web.Security -{ - public class UserAwarePasswordHasher : IPasswordHasher - where T : BackOfficeIdentityUser - { - private readonly LegacyPasswordSecurity _passwordSecurity; - - public UserAwarePasswordHasher(LegacyPasswordSecurity passwordSecurity) - { - _passwordSecurity = passwordSecurity; - } - - public string HashPassword(string password) - { - return _passwordSecurity.HashPasswordForStorage(password); - } - - public string HashPassword(T user, string password) - { - // TODO: Implement the logic for this, we need to lookup the password format for the user and hash accordingly: http://issues.umbraco.org/issue/U4-10089 - //NOTE: For now this just falls back to the hashing we are currently using - - return HashPassword(password); - } - - public PasswordVerificationResult VerifyHashedPassword(T user, string hashedPassword, string providedPassword) - { - // TODO: Implement the logic for this, we need to lookup the password format for the user and hash accordingly: http://issues.umbraco.org/issue/U4-10089 - //NOTE: For now this just falls back to the hashing we are currently using - - return _passwordSecurity.VerifyPassword(providedPassword, hashedPassword) - ? PasswordVerificationResult.Success - : PasswordVerificationResult.Failed; - } - } -} diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index af9f9f76f6..c015e0edee 100644 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -194,7 +194,6 @@ -