Skip to content

Commit

Permalink
V9: Fix for migration of non-default configurated users/members (#11684)
Browse files Browse the repository at this point in the history
* #11366
Fallback to try login using super legacy HMACSHA1 even when the algorithm is stated as being HMACSHA256. The issue is that v8 saves HMACSHA256 on the user, but when configured to use legacy encoding it actually uses HMACSHA1

* Support migration of members with:
UseLegacyEncoding+Clear
UseLegacyEncoding+Encrypted (Requires machine key)
UseLegacyEncoding+Hashed

* Fixes unit tests

* Avoid exceptions + unit tests

* Save unknown algorithm if we dont know it, instead of persisting a wrong algorithm.

* Added setting to enable clear text password rehashes.

* Removed support for migration of clear text passwords

* Fixed unit test
  • Loading branch information
bergmania committed Dec 2, 2021
1 parent fce9731 commit 915f1cb
Show file tree
Hide file tree
Showing 12 changed files with 226 additions and 67 deletions.
1 change: 1 addition & 0 deletions src/JsonSchema/AppSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public class CmsDefinition
public BasicAuthSettings BasicAuth { get; set; }

public PackageMigrationSettings PackageMigration { get; set; }
public LegacyPasswordMigrationSettings LegacyPasswordMigration { get; set; }
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright (c) Umbraco.
// See LICENSE for more details.

using System.ComponentModel;

namespace Umbraco.Cms.Core.Configuration.Models
{
/// <summary>
/// Typed configuration options for legacy machine key settings used for migration of members from a v8 solution.
/// </summary>
[UmbracoOptions(Constants.Configuration.ConfigLegacyPasswordMigration)]
public class LegacyPasswordMigrationSettings
{
private const string StaticDecryptionKey = "";

/// <summary>
/// Gets the decryption algorithm.
/// </summary>
/// <remarks>
/// Currently only AES is supported. This should include all machine keys generated by Umbraco.
/// </remarks>
public string MachineKeyDecryption => "AES";

/// <summary>
/// Gets or sets the decryption hex-formatted string key found in legacy web.config machineKey configuration-element.
/// </summary>
[DefaultValue(StaticDecryptionKey)]
public string MachineKeyDecryptionKey { get; set; } = StaticDecryptionKey;
}
}
1 change: 1 addition & 0 deletions src/Umbraco.Core/Constants-Configuration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public static class Configuration
public const string ConfigHostingDebug = ConfigHostingPrefix + "Debug";
public const string ConfigCustomErrorsMode = ConfigCustomErrorsPrefix + "Mode";
public const string ConfigActiveDirectory = ConfigPrefix + "ActiveDirectory";
public const string ConfigLegacyPasswordMigration = ConfigPrefix + "LegacyPasswordMigration";
public const string ConfigContent = ConfigPrefix + "Content";
public const string ConfigCoreDebug = ConfigCorePrefix + "Debug";
public const string ConfigExceptionFilter = ConfigPrefix + "ExceptionFilter";
Expand Down
1 change: 1 addition & 0 deletions src/Umbraco.Core/Constants-Security.cs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ public static class Security
public const string AspNetCoreV2PasswordHashAlgorithmName = "PBKDF2.ASPNETCORE.V2";
public const string AspNetUmbraco8PasswordHashAlgorithmName = "HMACSHA256";
public const string AspNetUmbraco4PasswordHashAlgorithmName = "HMACSHA1";
public const string UnknownPasswordConfigJson = "{\"hashAlgorithm\":\"Unknown\"}";
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public static IUmbracoBuilder AddConfiguration(this IUmbracoBuilder builder)
.AddUmbracoOptions<RichTextEditorSettings>()
.AddUmbracoOptions<BasicAuthSettings>()
.AddUmbracoOptions<RuntimeMinificationSettings>()
.AddUmbracoOptions<LegacyPasswordMigrationSettings>()
.AddUmbracoOptions<PackageMigrationSettings>();

return builder;
Expand Down
30 changes: 27 additions & 3 deletions src/Umbraco.Core/Security/LegacyPasswordSecurity.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,33 @@ public bool VerifyPassword(string algorithm, string password, string dbPassword)
if (dbPassword.StartsWith(Constants.Security.EmptyPasswordPrefix))
return false;

var storedHashedPass = ParseStoredHashPassword(algorithm, dbPassword, out var salt);
var hashed = HashPassword(algorithm, password, salt);
return storedHashedPass == hashed;
try
{
var storedHashedPass = ParseStoredHashPassword(algorithm, dbPassword, out var salt);
var hashed = HashPassword(algorithm, password, salt);
return storedHashedPass == hashed;
}
catch (ArgumentOutOfRangeException)
{
//This can happen if the length of the password is wrong and a salt cannot be extracted.
return false;
}

}

/// <summary>
/// Verify a legacy hashed password (HMACSHA1)
/// </summary>
public bool VerifyLegacyHashedPassword(string password, string dbPassword)
{
var hashAlgorith = new HMACSHA1
{
//the legacy salt was actually the password :(
Key = Encoding.Unicode.GetBytes(password)
};
var hashed = Convert.ToBase64String(hashAlgorith.ComputeHash(Encoding.Unicode.GetBytes(password)));

return dbPassword == hashed;
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,9 @@ protected override void PersistUpdatedItem(IMember entity)
{
memberDto.PasswordConfig = DefaultPasswordConfigJson;
changedCols.Add("passwordConfig");
}else if (memberDto.PasswordConfig == Constants.Security.UnknownPasswordConfigJson)
{
changedCols.Add("passwordConfig");
}

// do NOT update the password if it has not changed or if it is null or empty
Expand Down
116 changes: 110 additions & 6 deletions src/Umbraco.Infrastructure/Security/MemberPasswordHasher.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
using System;
using System.IO;
using System.Linq;
using System.Security.Cryptography;
using System.Text;
using Microsoft.AspNetCore.Identity;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Models.Membership;
using Umbraco.Cms.Core.Security;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Serialization;
using Umbraco.Cms.Web.Common.DependencyInjection;
using Umbraco.Extensions;

namespace Umbraco.Cms.Core.Security
Expand All @@ -16,9 +22,27 @@ namespace Umbraco.Cms.Core.Security
/// </remarks>
public class MemberPasswordHasher : UmbracoPasswordHasher<MemberIdentityUser>
{
private readonly IOptions<LegacyPasswordMigrationSettings> _legacyMachineKeySettings;
private readonly ILogger<MemberPasswordHasher> _logger;

[Obsolete("Use ctor with all params")]
public MemberPasswordHasher(LegacyPasswordSecurity legacyPasswordHasher, IJsonSerializer jsonSerializer)
: this(legacyPasswordHasher,
jsonSerializer,
StaticServiceProvider.Instance.GetRequiredService<IOptions<LegacyPasswordMigrationSettings>>(),
StaticServiceProvider.Instance.GetRequiredService<ILogger<MemberPasswordHasher>>())
{
}

public MemberPasswordHasher(
LegacyPasswordSecurity legacyPasswordHasher,
IJsonSerializer jsonSerializer,
IOptions<LegacyPasswordMigrationSettings> legacyMachineKeySettings,
ILogger<MemberPasswordHasher> logger)
: base(legacyPasswordHasher, jsonSerializer)
{
_legacyMachineKeySettings = legacyMachineKeySettings;
_logger = logger;
}

/// <summary>
Expand All @@ -36,10 +60,21 @@ public override PasswordVerificationResult VerifyHashedPassword(MemberIdentityUs
throw new ArgumentNullException(nameof(user));
}

var isPasswordAlgorithmKnown = user.PasswordConfig.IsNullOrWhiteSpace() == false &&
user.PasswordConfig != Constants.Security.UnknownPasswordConfigJson;
// if there's password config use the base implementation
if (!user.PasswordConfig.IsNullOrWhiteSpace())
if (isPasswordAlgorithmKnown)
{
return base.VerifyHashedPassword(user, hashedPassword, providedPassword);
var result = base.VerifyHashedPassword(user, hashedPassword, providedPassword);
if (result != PasswordVerificationResult.Failed)
{
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))
{
return PasswordVerificationResult.SuccessRehashNeeded;
}

// Else we need to detect what the password is. This will be the case
Expand All @@ -66,7 +101,16 @@ public override PasswordVerificationResult VerifyHashedPassword(MemberIdentityUs
return base.VerifyHashedPassword(user, hashedPassword, providedPassword);
}

throw new InvalidOperationException("unable to determine member password hashing algorith");
if (isPasswordAlgorithmKnown)
{
_logger.LogError("Unable to determine member password hashing algorithm");
}
else
{
_logger.LogDebug("Unable to determine member password hashing algorithm, but this can happen when member enters a wrong password, before it has be rehashed");
}

return PasswordVerificationResult.Failed;
}

var isValid = LegacyPasswordSecurity.VerifyPassword(
Expand All @@ -76,5 +120,65 @@ public override PasswordVerificationResult VerifyHashedPassword(MemberIdentityUs

return isValid ? PasswordVerificationResult.SuccessRehashNeeded : PasswordVerificationResult.Failed;
}

private bool IsSuccessfulLegacyPassword(string hashedPassword, string providedPassword)
{
if (!string.IsNullOrEmpty(_legacyMachineKeySettings.Value.MachineKeyDecryptionKey))
{
try
{
var decryptedPassword = DecryptLegacyPassword(hashedPassword, _legacyMachineKeySettings.Value.MachineKeyDecryption, _legacyMachineKeySettings.Value.MachineKeyDecryptionKey);
return decryptedPassword == providedPassword;
}
catch (Exception ex)
{
_logger.LogError(ex, "Could not decrypt password even that a DecryptionKey is provided. This means the DecryptionKey is wrong.");
return false;
}
}

var result = LegacyPasswordSecurity.VerifyPassword(Constants.Security.AspNetUmbraco8PasswordHashAlgorithmName, providedPassword, hashedPassword);
return result || LegacyPasswordSecurity.VerifyPassword(Constants.Security.AspNetUmbraco4PasswordHashAlgorithmName, providedPassword, hashedPassword);
}

private static string DecryptLegacyPassword(string encryptedPassword, string algorithmName, string decryptionKey)
{
SymmetricAlgorithm algorithm;
switch (algorithmName)
{
case "AES":
algorithm = new AesCryptoServiceProvider()
{
Key = StringToByteArray(decryptionKey),
IV = new byte[16]
};
break;
default:
throw new NotSupportedException($"The algorithm ({algorithmName}) is not supported");
}

using (algorithm)
{
return DecryptLegacyPassword(encryptedPassword, algorithm);
}
}

private static string DecryptLegacyPassword(string encryptedPassword, SymmetricAlgorithm algorithm)
{
using var memoryStream = new MemoryStream();
ICryptoTransform cryptoTransform = algorithm.CreateDecryptor();
var cryptoStream = new CryptoStream((Stream)memoryStream, cryptoTransform, CryptoStreamMode.Write);
var buf = Convert.FromBase64String(encryptedPassword);
cryptoStream.Write(buf, 0, 32);
cryptoStream.FlushFinalBlock();

return Encoding.Unicode.GetString(memoryStream.ToArray());
}

private static byte[] StringToByteArray(string hex) =>
Enumerable.Range(0, hex.Length)
.Where(x => x % 2 == 0)
.Select(x => Convert.ToByte(hex.Substring(x, 2), 16))
.ToArray();
}
}
6 changes: 6 additions & 0 deletions src/Umbraco.Infrastructure/Security/MemberUserStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,12 @@ private MemberDataChangeType UpdateMemberProperties(IMember member, MemberIdenti
member.PasswordConfiguration = identityUser.PasswordConfig;
}

if (member.PasswordConfiguration != identityUser.PasswordConfig)
{
changeType = MemberDataChangeType.FullSave;
member.PasswordConfiguration = identityUser.PasswordConfig;
}

if (member.SecurityStamp != identityUser.SecurityStamp)
{
changeType = MemberDataChangeType.FullSave;
Expand Down
7 changes: 7 additions & 0 deletions src/Umbraco.Infrastructure/Security/UmbracoPasswordHasher.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ public override PasswordVerificationResult VerifyHashedPassword(TUser user, stri
if (LegacyPasswordSecurity.SupportHashAlgorithm(deserialized.HashAlgorithm))
{
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;
Expand Down
9 changes: 8 additions & 1 deletion src/Umbraco.Infrastructure/Security/UmbracoUserManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public string GeneratePassword()
string password = _passwordGenerator.GeneratePassword();
return password;
}

/// <summary>
/// Used to validate the password without an identity user
/// Validation code is based on the default ValidatePasswordAsync code
Expand Down Expand Up @@ -205,6 +205,8 @@ public override async Task<IdentityResult> ResetAccessFailedCountAsync(TUser use

await lockoutStore.ResetAccessFailedCountAsync(user, CancellationToken.None);

//Ensure the password config is null, so it is set to the default in repository
user.PasswordConfig = null;
return await UpdateAsync(user);
}

Expand Down Expand Up @@ -234,6 +236,11 @@ public override async Task<IdentityResult> AccessFailedAsync(TUser user)
// here we are persisting the value for the back office
}

if (string.IsNullOrEmpty(user.PasswordConfig))
{
//We cant pass null as that would be interpreted as the default algoritm, but due to the failing attempt we dont know.
user.PasswordConfig = Constants.Security.UnknownPasswordConfigJson;
}
IdentityResult result = await UpdateAsync(user);
return result;
}
Expand Down

0 comments on commit 915f1cb

Please sign in to comment.