diff --git a/src/Umbraco.Core/Security/LegacyPasswordSecurity.cs b/src/Umbraco.Core/Security/LegacyPasswordSecurity.cs index 35528a48ca..b8c7596b2d 100644 --- a/src/Umbraco.Core/Security/LegacyPasswordSecurity.cs +++ b/src/Umbraco.Core/Security/LegacyPasswordSecurity.cs @@ -8,12 +8,17 @@ namespace Umbraco.Cms.Core.Security { /// - /// Handles password hashing and formatting for legacy hashing algorithms + /// Handles password hashing and formatting for legacy hashing algorithms. /// + /// + /// Should probably be internal. + /// public class LegacyPasswordSecurity { + // TODO: Remove v11 // Used for tests [EditorBrowsable(EditorBrowsableState.Never)] + [Obsolete("We shouldn't be altering our public API to make test code easier, removing v11")] public string HashPasswordForStorage(string algorithmType, string password) { if (string.IsNullOrWhiteSpace(password)) @@ -24,13 +29,15 @@ namespace Umbraco.Cms.Core.Security return FormatPasswordForStorage(algorithmType, hashed, salt); } + // TODO: Remove v11 // Used for tests [EditorBrowsable(EditorBrowsableState.Never)] + [Obsolete("We shouldn't be altering our public API to make test code easier, removing v11")] public string FormatPasswordForStorage(string algorithmType, string hashedPassword, string salt) { - if (IsLegacySHA1Algorithm(algorithmType)) + if (!SupportHashAlgorithm(algorithmType)) { - return hashedPassword; + throw new InvalidOperationException($"{algorithmType} is not supported"); } return salt + hashedPassword; @@ -45,10 +52,15 @@ namespace Umbraco.Cms.Core.Security /// 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 (string.IsNullOrWhiteSpace(dbPassword)) + { + throw new ArgumentException("Value cannot be null or whitespace.", nameof(dbPassword)); + } if (dbPassword.StartsWith(Constants.Security.EmptyPasswordPrefix)) + { return false; + } try { @@ -61,7 +73,6 @@ namespace Umbraco.Cms.Core.Security //This can happen if the length of the password is wrong and a salt cannot be extracted. return false; } - } /// @@ -69,12 +80,13 @@ namespace Umbraco.Cms.Core.Security /// public bool VerifyLegacyHashedPassword(string password, string dbPassword) { - var hashAlgorith = new HMACSHA1 + var hashAlgorithm = new HMACSHA1 { //the legacy salt was actually the password :( Key = Encoding.Unicode.GetBytes(password) }; - var hashed = Convert.ToBase64String(hashAlgorith.ComputeHash(Encoding.Unicode.GetBytes(password))); + + var hashed = Convert.ToBase64String(hashAlgorithm.ComputeHash(Encoding.Unicode.GetBytes(password))); return dbPassword == hashed; } @@ -87,6 +99,8 @@ namespace Umbraco.Cms.Core.Security /// /// // TODO: Do we need this method? We shouldn't be using this class to create new password hashes for storage + // TODO: Remove v11 + [Obsolete("We shouldn't be altering our public API to make test code easier, removing v11")] public string HashNewPassword(string algorithm, string newPassword, out string salt) { salt = GenerateSalt(); @@ -102,15 +116,15 @@ namespace Umbraco.Cms.Core.Security /// 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)) + if (string.IsNullOrWhiteSpace(storedString)) { - salt = string.Empty; - return storedString; + throw new ArgumentException("Value cannot be null or whitespace.", nameof(storedString)); } + if (!SupportHashAlgorithm(algorithm)) + { + throw new InvalidOperationException($"{algorithm} is not supported"); + } var saltLen = GenerateSalt(); salt = storedString.Substring(0, saltLen.Length); @@ -133,12 +147,12 @@ namespace Umbraco.Cms.Core.Security /// private string HashPassword(string algorithmType, string pass, string salt) { - if (IsLegacySHA1Algorithm(algorithmType)) + if (!SupportHashAlgorithm(algorithmType)) { - return HashLegacySHA1Password(pass); + throw new InvalidOperationException($"{algorithmType} is not supported"); } - //This is the correct way to implement this (as per the sql membership provider) + // 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); @@ -219,42 +233,7 @@ namespace Umbraco.Cms.Core.Security return true; } - // This is for the <= v4 hashing algorithm - if (IsLegacySHA1Algorithm(algorithm)) - { - return true; - } - 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) - { - using 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.Infrastructure/Security/MemberPasswordHasher.cs b/src/Umbraco.Infrastructure/Security/MemberPasswordHasher.cs index e470bf0a6c..02aef30217 100644 --- a/src/Umbraco.Infrastructure/Security/MemberPasswordHasher.cs +++ b/src/Umbraco.Infrastructure/Security/MemberPasswordHasher.cs @@ -71,6 +71,7 @@ namespace Umbraco.Cms.Core.Security return result; } } + // We need to check for clear text passwords from members as the first thing. This was possible in v8 :( else if (IsSuccessfulLegacyPassword(hashedPassword, providedPassword)) { @@ -138,7 +139,7 @@ namespace Umbraco.Cms.Core.Security } var result = LegacyPasswordSecurity.VerifyPassword(Constants.Security.AspNetUmbraco8PasswordHashAlgorithmName, providedPassword, hashedPassword); - return result || LegacyPasswordSecurity.VerifyPassword(Constants.Security.AspNetUmbraco4PasswordHashAlgorithmName, providedPassword, hashedPassword); + return result || LegacyPasswordSecurity.VerifyLegacyHashedPassword(providedPassword, hashedPassword); } private static string DecryptLegacyPassword(string encryptedPassword, string algorithmName, string decryptionKey) diff --git a/src/Umbraco.Infrastructure/Security/UmbracoPasswordHasher.cs b/src/Umbraco.Infrastructure/Security/UmbracoPasswordHasher.cs index da08bc8713..2847f13dc4 100644 --- a/src/Umbraco.Infrastructure/Security/UmbracoPasswordHasher.cs +++ b/src/Umbraco.Infrastructure/Security/UmbracoPasswordHasher.cs @@ -1,3 +1,4 @@ +using System; using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Options; using Umbraco.Cms.Core.Models.Membership; @@ -10,7 +11,6 @@ namespace Umbraco.Cms.Core.Security where TUser: UmbracoIdentityUser { private readonly IJsonSerializer _jsonSerializer; - private readonly PasswordHasher _aspnetV2PasswordHasher = new PasswordHasher(new V2PasswordHasherOptions()); public UmbracoPasswordHasher(LegacyPasswordSecurity legacyPasswordSecurity, IJsonSerializer jsonSerializer) { @@ -43,57 +43,64 @@ namespace Umbraco.Cms.Core.Security { if (user is null) { - throw new System.ArgumentNullException(nameof(user)); + throw new ArgumentNullException(nameof(user)); } - if (!user.PasswordConfig.IsNullOrWhiteSpace()) + try { - // check if the (legacy) password security supports this hash algorith and if so then use it - var deserialized = _jsonSerializer.Deserialize(user.PasswordConfig); - if (LegacyPasswordSecurity.SupportHashAlgorithm(deserialized.HashAlgorithm)) + // Best case and most likely scenario, a modern hash supported by ASP.Net identity. + PasswordVerificationResult upstreamResult = base.VerifyHashedPassword(user, hashedPassword, providedPassword); + if (upstreamResult != PasswordVerificationResult.Failed) { - var result = LegacyPasswordSecurity.VerifyPassword(deserialized.HashAlgorithm, providedPassword, hashedPassword); - - //We need to special handle this case, apparently v8 still saves the user algorithm as {"hashAlgorithm":"HMACSHA256"}, when using legacy encoding and hasinging. - if (result == false) - { - result = LegacyPasswordSecurity.VerifyLegacyHashedPassword(providedPassword, hashedPassword); - } - - return result - ? PasswordVerificationResult.SuccessRehashNeeded - : PasswordVerificationResult.Failed; - } - - // We will explicitly detect names here - // The default is PBKDF2.ASPNETCORE.V3: - // PBKDF2 with HMAC-SHA256, 128-bit salt, 256-bit subkey, 10000 iterations. - // The underlying class only lets us change 2 things which is the version: options.CompatibilityMode and the iteration count - // The PBKDF2.ASPNETCORE.V2 settings are: - // PBKDF2 with HMAC-SHA1, 128-bit salt, 256-bit subkey, 1000 iterations. - - switch (deserialized.HashAlgorithm) - { - case Constants.Security.AspNetCoreV3PasswordHashAlgorithmName: - return base.VerifyHashedPassword(user, hashedPassword, providedPassword); - case Constants.Security.AspNetCoreV2PasswordHashAlgorithmName: - var legacyResult = _aspnetV2PasswordHasher.VerifyHashedPassword(user, hashedPassword, providedPassword); - if (legacyResult == PasswordVerificationResult.Success) - return PasswordVerificationResult.SuccessRehashNeeded; - return legacyResult; + return upstreamResult; } } - - // else go the default (v3) - return base.VerifyHashedPassword(user, hashedPassword, providedPassword); - } - - private class V2PasswordHasherOptions : IOptions - { - public PasswordHasherOptions Value => new PasswordHasherOptions + catch (FormatException) { - CompatibilityMode = PasswordHasherCompatibilityMode.IdentityV2 - }; + // hash wasn't a valid base64 encoded string, MS concat the salt bytes and hash bytes and base 64 encode both together. + // We however historically base 64 encoded the salt bytes and hash bytes separately then concat the strings so we got 2 sets of padding. + // both salt bytes and hash bytes lengths were not evenly divisible by 3 hence 2 sets of padding. + + // We could check upfront with TryFromBase64String, but not whilst we target netstandard 2.0 + // so might as well just deal with the exception. + } + + // At this point we either have a legacy password or a bad attempt. + + // Check the supported worst case scenario, a "useLegacyEncoding" password - HMACSHA1 but with password used as key so not unique for users sharing same password + // This was the standard for v4. + // Do this first because with useLegacyEncoding the algorithm stored in the database is irrelevant. + if (LegacyPasswordSecurity.VerifyLegacyHashedPassword(providedPassword, hashedPassword)) + { + return PasswordVerificationResult.SuccessRehashNeeded; + } + + // For users we expect to know the historic algorithm. + // NOTE: MemberPasswordHasher subclasses this class to deal with the fact that PasswordConfig wasn't stored. + if (user.PasswordConfig.IsNullOrWhiteSpace()) + { + return PasswordVerificationResult.Failed; + } + + PersistedPasswordSettings deserialized; + try + { + deserialized = _jsonSerializer.Deserialize(user.PasswordConfig); + } + catch + { + return PasswordVerificationResult.Failed; + } + + if (!LegacyPasswordSecurity.SupportHashAlgorithm(deserialized.HashAlgorithm)) + { + return PasswordVerificationResult.Failed; + } + + // Last chance must be HMACSHA256 or SHA1 + return LegacyPasswordSecurity.VerifyPassword(deserialized.HashAlgorithm, providedPassword, hashedPassword) + ? PasswordVerificationResult.SuccessRehashNeeded + : PasswordVerificationResult.Failed; } } } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/LegacyPasswordSecurityTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/LegacyPasswordSecurityTests.cs index 273823eec3..12c5f50d30 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/LegacyPasswordSecurityTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Security/LegacyPasswordSecurityTests.cs @@ -1,10 +1,14 @@ // Copyright (c) Umbraco. // See LICENSE for more details. +using System; +using System.Security.Cryptography; +using System.Text; using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Configuration; using Umbraco.Cms.Core.Security; +using Umbraco.Extensions; using Constants = Umbraco.Cms.Core.Constants; namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Security @@ -15,7 +19,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Security [Test] public void Check_Password_Hashed_Non_KeyedHashAlgorithm() { - IPasswordConfiguration passwordConfiguration = Mock.Of(x => x.HashAlgorithmType == "SHA256"); + IPasswordConfiguration passwordConfiguration = Mock.Of(x => x.HashAlgorithmType == "SHA1"); var passwordSecurity = new LegacyPasswordSecurity(); var pass = "ThisIsAHashedPassword"; @@ -45,14 +49,12 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Security [Test] public void Check_Password_Legacy_v4_SHA1() { - IPasswordConfiguration passwordConfiguration = Mock.Of(x => x.HashAlgorithmType == Constants.Security.AspNetUmbraco4PasswordHashAlgorithmName); - var passwordSecurity = new LegacyPasswordSecurity(); + const string clearText = "ThisIsAHashedPassword"; + var clearTextUnicodeBytes = Encoding.Unicode.GetBytes(clearText); + using var algorithm = new HMACSHA1(clearTextUnicodeBytes); + var dbPassword = Convert.ToBase64String(algorithm.ComputeHash(clearTextUnicodeBytes)); - var pass = "ThisIsAHashedPassword"; - var hashed = passwordSecurity.HashNewPassword(passwordConfiguration.HashAlgorithmType, pass, out string salt); - var storedPassword = passwordSecurity.FormatPasswordForStorage(passwordConfiguration.HashAlgorithmType, hashed, salt); - - var result = passwordSecurity.VerifyPassword(passwordConfiguration.HashAlgorithmType, "ThisIsAHashedPassword", storedPassword); + var result = new LegacyPasswordSecurity().VerifyLegacyHashedPassword(clearText, dbPassword); Assert.IsTrue(result); } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/UmbracoPasswordHasherTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/UmbracoPasswordHasherTests.cs index aa6bb4156b..db4ec3392f 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/UmbracoPasswordHasherTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/UmbracoPasswordHasherTests.cs @@ -1,5 +1,9 @@ +using System; +using System.Security.Cryptography; +using System.Text; using AutoFixture.NUnit3; using Microsoft.AspNetCore.Identity; +using Microsoft.Extensions.Options; using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Models.Membership; @@ -15,9 +19,8 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security // Technically MD5, HMACSHA384 & HMACSHA512 were also possible but opt in as opposed to historic defaults. [Test] [InlineAutoMoqData("HMACSHA256", "Umbraco9Rocks!", "uB/pLEhhe1W7EtWMv/pSgg==1y8+aso9+h3AKRtJXlVYeg2TZKJUr64hccj82ZZ7Ksk=")] // Actually HMACSHA256 - [InlineAutoMoqData("HMACSHA256", "Umbraco9Rocks!", "t0U8atXTX/efNCtTafukwZeIpr8=")] // v4 site legacy password, with incorrect algorithm specified in database actually HMACSHA1 with password used as key. [InlineAutoMoqData("SHA1", "Umbraco9Rocks!", "6tZGfG9NTxJJYp19Fac9og==zzRggqANxhb+CbD/VabEt8cIde8=")] // When SHA1 is set on machine key. - public void VerifyHashedPassword_WithValidLegacyPasswordHash_ReturnsSuccessRehashNeeded( + public void VerifyHashedPassword_ValidHashWithoutLegacyEncoding_ReturnsSuccessRehashNeeded( string algorithm, string providedPassword, string hashedPassword, @@ -34,10 +37,13 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security Assert.AreEqual(PasswordVerificationResult.SuccessRehashNeeded, result); } - [Test] - [InlineAutoMoqData("PBKDF2.ASPNETCORE.V3", "Umbraco9Rocks!", "AQAAAAEAACcQAAAAEDCrYcnIhHKr38yuchsDu6AFqqmLNvRooKObV25GC1LC1tLY+gWGU4xNug0lc17PHA==")] - public void VerifyHashedPassword_WithValidModernPasswordHash_ReturnsSuccess( + [InlineAutoMoqData("HMACSHA1", "Umbraco9Rocks!", "t0U8atXTX/efNCtTafukwZeIpr8=")] + [InlineAutoMoqData("HMACSHA256", "Umbraco9Rocks!", "t0U8atXTX/efNCtTafukwZeIpr8=")] + [InlineAutoMoqData("FOOBARBAZQUX", "Umbraco9Rocks!", "t0U8atXTX/efNCtTafukwZeIpr8=")] + [InlineAutoMoqData("", "Umbraco9Rocks!", "t0U8atXTX/efNCtTafukwZeIpr8=")] + [InlineAutoMoqData(null, "Umbraco9Rocks!", "t0U8atXTX/efNCtTafukwZeIpr8=")] + public void VerifyHashedPassword_ValidHashWithLegacyEncoding_ReturnsSuccessRehashNeeded( string algorithm, string providedPassword, string hashedPassword, @@ -51,7 +57,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security var result = sut.VerifyHashedPassword(aUser, hashedPassword, providedPassword); - Assert.AreEqual(PasswordVerificationResult.Success, result); + Assert.AreEqual(PasswordVerificationResult.SuccessRehashNeeded, result); } [Test] @@ -73,6 +79,46 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Security Assert.AreEqual(PasswordVerificationResult.Failed, result); } + [Test] + [AutoMoqData] + public void VerifyHashedPassword_WithIdentityV1OrV2StyleHash_ReturnsSuccessRehashNeeded( + TestUserStub aUser, + UmbracoPasswordHasher sut) + { + var options = Options.Create(new PasswordHasherOptions + { + CompatibilityMode = PasswordHasherCompatibilityMode.IdentityV2 + }); + + var upstreamHasher = new PasswordHasher(options); + + const string password = "Umbraco9Rocks!"; + var identityV1Or2StyleHash = upstreamHasher.HashPassword(aUser, password); + var result = sut.VerifyHashedPassword(aUser, identityV1Or2StyleHash, password); + + Assert.AreEqual(PasswordVerificationResult.SuccessRehashNeeded, result); + } + + [Test] + [AutoMoqData] + public void VerifyHashedPassword_WithIdentityV3StyleHash_ReturnsSuccess( + TestUserStub aUser, + UmbracoPasswordHasher sut) + { + var options = Options.Create(new PasswordHasherOptions + { + CompatibilityMode = PasswordHasherCompatibilityMode.IdentityV3 + }); + + var upstreamHasher = new PasswordHasher(options); + + const string password = "Umbraco9Rocks!"; + var identityV1Or2StyleHash = upstreamHasher.HashPassword(aUser, password); + var result = sut.VerifyHashedPassword(aUser, identityV1Or2StyleHash, password); + + Assert.AreEqual(PasswordVerificationResult.Success, result); + } + public class TestUserStub : UmbracoIdentityUser { public TestUserStub() => PasswordConfig = "not null or empty";