[PM-23229] Add extra validation to kdf changes + authentication data + unlock data (#6121)

* Added MasterPasswordUnlock to UserDecryptionOptions as part of identity response

* Implement support for authentication data and unlock data in kdf change

* Extract to kdf command and add tests

* Fix namespace

* Delete empty file

* Fix build

* Clean up tests

* Fix tests

* Add comments

* Cleanup

* Cleanup

* Cleanup

* Clean-up and fix build

* Address feedback; force new parameters on KDF change request

* Clean-up and add tests

* Re-add logger

* Update logger to interface

* Clean up, remove Kdf Request Model

* Remove kdf request model tests

* Fix types in test

* Address feedback to rename request model and re-add tests

* Fix namespace

* Move comments

* Rename InnerKdfRequestModel to KdfRequestModel

---------

Co-authored-by: Maciej Zieniuk <mzieniuk@bitwarden.com>

src/Api/Auth/Controllers/AccountsController.cs

@@ -16,6 +16,7 @@ using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces;
 using Bit.Core.Auth.UserFeatures.UserMasterPassword.Interfaces;
 using Bit.Core.Enums;
 using Bit.Core.Exceptions;
+using Bit.Core.KeyManagement.Kdf;
 using Bit.Core.Models.Api.Response;
 using Bit.Core.Repositories;
 using Bit.Core.Services;
@@ -39,7 +40,7 @@ public class AccountsController : Controller
     private readonly ITwoFactorIsEnabledQuery _twoFactorIsEnabledQuery;
     private readonly IFeatureService _featureService;
     private readonly ITwoFactorEmailService _twoFactorEmailService;
-
+    private readonly IChangeKdfCommand _changeKdfCommand;
 
     public AccountsController(
         IOrganizationService organizationService,
@@ -51,7 +52,8 @@ public class AccountsController : Controller
         ITdeOffboardingPasswordCommand tdeOffboardingPasswordCommand,
         ITwoFactorIsEnabledQuery twoFactorIsEnabledQuery,
         IFeatureService featureService,
-        ITwoFactorEmailService twoFactorEmailService
+        ITwoFactorEmailService twoFactorEmailService,
+        IChangeKdfCommand changeKdfCommand
         )
     {
         _organizationService = organizationService;
@@ -64,7 +66,7 @@ public class AccountsController : Controller
         _twoFactorIsEnabledQuery = twoFactorIsEnabledQuery;
         _featureService = featureService;
         _twoFactorEmailService = twoFactorEmailService;
-
+        _changeKdfCommand = changeKdfCommand;
     }
 
 
@@ -256,7 +258,7 @@ public class AccountsController : Controller
     }
 
     [HttpPost("kdf")]
-    public async Task PostKdf([FromBody] KdfRequestModel model)
+    public async Task PostKdf([FromBody] PasswordRequestModel model)
     {
         var user = await _userService.GetUserByPrincipalAsync(User);
         if (user == null)
@@ -264,8 +266,12 @@ public class AccountsController : Controller
             throw new UnauthorizedAccessException();
         }
 
-        var result = await _userService.ChangeKdfAsync(user, model.MasterPasswordHash,
-            model.NewMasterPasswordHash, model.Key, model.Kdf.Value, model.KdfIterations.Value, model.KdfMemory, model.KdfParallelism);
+        if (model.AuthenticationData == null || model.UnlockData == null)
+        {
+            throw new BadRequestException("AuthenticationData and UnlockData must be provided.");
+        }
+
+        var result = await _changeKdfCommand.ChangeKdfAsync(user, model.MasterPasswordHash, model.AuthenticationData.ToData(), model.UnlockData.ToData());
         if (result.Succeeded)
         {
             return;

src/Api/Auth/Models/Request/Accounts/KdfRequestModel.cs

@@ -1,25 +0,0 @@
-using System.ComponentModel.DataAnnotations;
-using Bit.Core.Enums;
-using Bit.Core.Utilities;
-
-namespace Bit.Api.Auth.Models.Request.Accounts;
-
-public class KdfRequestModel : PasswordRequestModel, IValidatableObject
-{
-    [Required]
-    public KdfType? Kdf { get; set; }
-    [Required]
-    public int? KdfIterations { get; set; }
-    public int? KdfMemory { get; set; }
-    public int? KdfParallelism { get; set; }
-
-    public override IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
-    {
-        if (Kdf.HasValue && KdfIterations.HasValue)
-        {
-            return KdfSettingsValidator.Validate(Kdf.Value, KdfIterations.Value, KdfMemory, KdfParallelism);
-        }
-
-        return Enumerable.Empty<ValidationResult>();
-    }
-}

src/Api/Auth/Models/Request/Accounts/MasterPasswordUnlockDataAndAuthenticationModel.cs

@@ -7,7 +7,7 @@ using Bit.Core.Utilities;
 
 namespace Bit.Api.Auth.Models.Request.Accounts;
 
-public class MasterPasswordUnlockDataModel : IValidatableObject
+public class MasterPasswordUnlockAndAuthenticationDataModel : IValidatableObject
 {
     public required KdfType KdfType { get; set; }
     public required int KdfIterations { get; set; }
@@ -45,9 +45,9 @@ public class MasterPasswordUnlockDataModel : IValidatableObject
         }
     }
 
-    public MasterPasswordUnlockData ToUnlockData()
+    public MasterPasswordUnlockAndAuthenticationData ToUnlockData()
     {
-        var data = new MasterPasswordUnlockData
+        var data = new MasterPasswordUnlockAndAuthenticationData
         {
             KdfType = KdfType,
             KdfIterations = KdfIterations,

src/Api/Auth/Models/Request/Accounts/PasswordRequestModel.cs

@@ -1,7 +1,7 @@
-// FIXME: Update this file to be null safe and then delete the line below
-#nullable disable
+#nullable enable
 
 using System.ComponentModel.DataAnnotations;
+using Bit.Api.KeyManagement.Models.Requests;
 
 namespace Bit.Api.Auth.Models.Request.Accounts;
 
@@ -9,9 +9,13 @@ public class PasswordRequestModel : SecretVerificationRequestModel
 {
     [Required]
     [StringLength(300)]
-    public string NewMasterPasswordHash { get; set; }
+    public required string NewMasterPasswordHash { get; set; }
     [StringLength(50)]
-    public string MasterPasswordHint { get; set; }
+    public string? MasterPasswordHint { get; set; }
     [Required]
-    public string Key { get; set; }
+    public required string Key { get; set; }
+
+    // Note: These will eventually become required, but not all consumers are moved over yet.
+    public MasterPasswordAuthenticationDataRequestModel? AuthenticationData { get; set; }
+    public MasterPasswordUnlockDataRequestModel? UnlockData { get; set; }
 }

src/Api/KeyManagement/Models/Requests/KdfRequestModel.cs

@@ -0,0 +1,26 @@
+using System.ComponentModel.DataAnnotations;
+using Bit.Core.Enums;
+using Bit.Core.KeyManagement.Models.Data;
+
+namespace Bit.Api.KeyManagement.Models.Requests;
+
+public class KdfRequestModel
+{
+    [Required]
+    public required KdfType KdfType { get; init; }
+    [Required]
+    public required int Iterations { get; init; }
+    public int? Memory { get; init; }
+    public int? Parallelism { get; init; }
+
+    public KdfSettings ToData()
+    {
+        return new KdfSettings
+        {
+            KdfType = KdfType,
+            Iterations = Iterations,
+            Memory = Memory,
+            Parallelism = Parallelism
+        };
+    }
+}

src/Api/KeyManagement/Models/Requests/MasterPasswordAuthenticationDataRequestModel.cs

@@ -0,0 +1,21 @@
+using System.ComponentModel.DataAnnotations;
+using Bit.Core.KeyManagement.Models.Data;
+
+namespace Bit.Api.KeyManagement.Models.Requests;
+
+public class MasterPasswordAuthenticationDataRequestModel
+{
+    public required KdfRequestModel Kdf { get; init; }
+    public required string MasterPasswordAuthenticationHash { get; init; }
+    [StringLength(256)] public required string Salt { get; init; }
+
+    public MasterPasswordAuthenticationData ToData()
+    {
+        return new MasterPasswordAuthenticationData
+        {
+            Kdf = Kdf.ToData(),
+            MasterPasswordAuthenticationHash = MasterPasswordAuthenticationHash,
+            Salt = Salt
+        };
+    }
+}

src/Api/KeyManagement/Models/Requests/MasterPasswordUnlockDataRequestModel.cs

@@ -0,0 +1,22 @@
+using System.ComponentModel.DataAnnotations;
+using Bit.Core.KeyManagement.Models.Data;
+using Bit.Core.Utilities;
+
+namespace Bit.Api.KeyManagement.Models.Requests;
+
+public class MasterPasswordUnlockDataRequestModel
+{
+    public required KdfRequestModel Kdf { get; init; }
+    [EncryptedString] public required string MasterKeyWrappedUserKey { get; init; }
+    [StringLength(256)] public required string Salt { get; init; }
+
+    public MasterPasswordUnlockData ToData()
+    {
+        return new MasterPasswordUnlockData
+        {
+            Kdf = Kdf.ToData(),
+            MasterKeyWrappedUserKey = MasterKeyWrappedUserKey,
+            Salt = Salt
+        };
+    }
+}

src/Api/KeyManagement/Models/Requests/UnlockDataRequestModel.cs

@@ -10,7 +10,7 @@ namespace Bit.Api.KeyManagement.Models.Requests;
 public class UnlockDataRequestModel
 {
     // All methods to get to the userkey
-    public required MasterPasswordUnlockDataModel MasterPasswordUnlockData { get; set; }
+    public required MasterPasswordUnlockAndAuthenticationDataModel MasterPasswordUnlockData { get; set; }
     public required IEnumerable<EmergencyAccessWithIdRequestModel> EmergencyAccessUnlockData { get; set; }
     public required IEnumerable<ResetPasswordWithOrgIdRequestModel> OrganizationAccountRecoveryUnlockData { get; set; }
     public required IEnumerable<WebAuthnLoginRotateKeyRequestModel> PasskeyUnlockData { get; set; }

src/Core/Entities/User.cs

@@ -78,6 +78,11 @@ public class User : ITableObject<Guid>, IStorableSubscriber, IRevisable, ITwoFac
     public DateTime? LastEmailChangeDate { get; set; }
     public bool VerifyDevices { get; set; } = true;
 
+    public string GetMasterPasswordSalt()
+    {
+        return Email.ToLowerInvariant().Trim();
+    }
+
     public void SetNewId()
     {
         Id = CoreHelpers.GenerateComb();

src/Core/KeyManagement/Kdf/IChangeKdfCommand.cs

@@ -0,0 +1,15 @@
+using Bit.Core.Entities;
+using Bit.Core.KeyManagement.Models.Data;
+using Microsoft.AspNetCore.Identity;
+
+namespace Bit.Core.KeyManagement.Kdf;
+
+/// <summary>
+/// Command to change the Key Derivation Function (KDF) settings for a user. This includes 
+/// changing the masterpassword authentication hash, and the masterkey encrypted userkey.
+/// The salt must not change during the KDF change.
+/// </summary>
+public interface IChangeKdfCommand
+{
+    public Task<IdentityResult> ChangeKdfAsync(User user, string masterPasswordAuthenticationHash, MasterPasswordAuthenticationData authenticationData, MasterPasswordUnlockData unlockData);
+}

src/Core/KeyManagement/Kdf/Implementations/ChangeKdfCommand.cs

@@ -0,0 +1,94 @@
+using Bit.Core.Entities;
+using Bit.Core.Exceptions;
+using Bit.Core.KeyManagement.Models.Data;
+using Bit.Core.Platform.Push;
+using Bit.Core.Repositories;
+using Bit.Core.Services;
+using Bit.Core.Utilities;
+using Microsoft.AspNetCore.Identity;
+using Microsoft.Extensions.Logging;
+
+namespace Bit.Core.KeyManagement.Kdf.Implementations;
+
+/// <inheritdoc />
+public class ChangeKdfCommand : IChangeKdfCommand
+{
+    private readonly IUserService _userService;
+    private readonly IPushNotificationService _pushService;
+    private readonly IUserRepository _userRepository;
+    private readonly IdentityErrorDescriber _identityErrorDescriber;
+    private readonly ILogger<ChangeKdfCommand> _logger;
+
+    public ChangeKdfCommand(IUserService userService, IPushNotificationService pushService, IUserRepository userRepository, IdentityErrorDescriber describer, ILogger<ChangeKdfCommand> logger)
+    {
+        _userService = userService;
+        _pushService = pushService;
+        _userRepository = userRepository;
+        _identityErrorDescriber = describer;
+        _logger = logger;
+    }
+
+    public async Task<IdentityResult> ChangeKdfAsync(User user, string masterPasswordAuthenticationHash, MasterPasswordAuthenticationData authenticationData, MasterPasswordUnlockData unlockData)
+    {
+        ArgumentNullException.ThrowIfNull(user);
+        if (!await _userService.CheckPasswordAsync(user, masterPasswordAuthenticationHash))
+        {
+            return IdentityResult.Failed(_identityErrorDescriber.PasswordMismatch());
+        }
+
+        // Validate to prevent user account from becoming un-decryptable from invalid parameters
+        // 
+        // Prevent a de-synced salt value from creating an un-decryptable unlock method 
+        authenticationData.ValidateSaltUnchangedForUser(user);
+        unlockData.ValidateSaltUnchangedForUser(user);
+
+        // Currently KDF settings are not saved separately for authentication and unlock and must therefore be equal
+        if (!authenticationData.Kdf.Equals(unlockData.Kdf))
+        {
+            throw new BadRequestException("KDF settings must be equal for authentication and unlock.");
+        }
+        var validationErrors = KdfSettingsValidator.Validate(unlockData.Kdf);
+        if (validationErrors.Any())
+        {
+            throw new BadRequestException("KDF settings are invalid.");
+        }
+
+        // Update the user with the new KDF settings
+        // This updates the authentication data and unlock data for the user separately. Currently these still
+        // use shared values for KDF settings and salt.
+        // The authentication hash, and the unlock data each are dependent on:
+        // - The master password (entered by the user every time)
+        // - The KDF settings (iterations, memory, parallelism)
+        // - The salt
+        // These combinations - (password, authentication hash, KDF settings, salt) and (password, unlock data, KDF settings, salt)
+        // must remain consistent to unlock correctly.
+
+        // Authentication
+        // Note: This mutates the user but does not yet save it to DB. That is done atomically, later.
+        // This entire operation MUST be atomic to prevent a user from being locked out of their account.
+        // Salt is ensured to be the same as unlock data, and the value stored in the account and not updated.
+        // KDF is ensured to be the same as unlock data above and updated below.
+        var result = await _userService.UpdatePasswordHash(user, authenticationData.MasterPasswordAuthenticationHash);
+        if (!result.Succeeded)
+        {
+            _logger.LogWarning("Change KDF failed for user {userId}.", user.Id);
+            return result;
+        }
+
+        // Salt is ensured to be the same as authentication data, and the value stored in the account, and is not updated.
+        // Kdf - These will be seperated in the future, but for now are ensured to be the same as authentication data above.
+        user.Key = unlockData.MasterKeyWrappedUserKey;
+        user.Kdf = unlockData.Kdf.KdfType;
+        user.KdfIterations = unlockData.Kdf.Iterations;
+        user.KdfMemory = unlockData.Kdf.Memory;
+        user.KdfParallelism = unlockData.Kdf.Parallelism;
+
+        var now = DateTime.UtcNow;
+        user.RevisionDate = user.AccountRevisionDate = now;
+        user.LastKdfChangeDate = now;
+
+        await _userRepository.ReplaceAsync(user);
+        await _pushService.PushLogOutAsync(user.Id);
+        return IdentityResult.Success;
+    }
+}

src/Core/KeyManagement/KeyManagementServiceCollectionExtensions.cs

@@ -1,5 +1,7 @@
 using Bit.Core.KeyManagement.Commands;
 using Bit.Core.KeyManagement.Commands.Interfaces;
+using Bit.Core.KeyManagement.Kdf;
+using Bit.Core.KeyManagement.Kdf.Implementations;
 using Microsoft.Extensions.DependencyInjection;
 
 namespace Bit.Core.KeyManagement;
@@ -15,5 +17,6 @@ public static class KeyManagementServiceCollectionExtensions
     private static void AddKeyManagementCommands(this IServiceCollection services)
     {
         services.AddScoped<IRegenerateUserAsymmetricKeysCommand, RegenerateUserAsymmetricKeysCommand>();
+        services.AddScoped<IChangeKdfCommand, ChangeKdfCommand>();
     }
 }

src/Core/KeyManagement/Models/Data/KdfSettings.cs

@@ -0,0 +1,38 @@
+using Bit.Core.Entities;
+using Bit.Core.Enums;
+
+namespace Bit.Core.KeyManagement.Models.Data;
+
+public class KdfSettings
+{
+    public required KdfType KdfType { get; init; }
+    public required int Iterations { get; init; }
+    public int? Memory { get; init; }
+    public int? Parallelism { get; init; }
+
+    public void ValidateUnchangedForUser(User user)
+    {
+        if (user.Kdf != KdfType || user.KdfIterations != Iterations || user.KdfMemory != Memory || user.KdfParallelism != Parallelism)
+        {
+            throw new ArgumentException("Invalid KDF settings.");
+        }
+    }
+
+    public override bool Equals(object? obj)
+    {
+        if (obj is not KdfSettings other)
+        {
+            return false;
+        }
+
+        return KdfType == other.KdfType &&
+               Iterations == other.Iterations &&
+               Memory == other.Memory &&
+               Parallelism == other.Parallelism;
+    }
+
+    public override int GetHashCode()
+    {
+        return HashCode.Combine(KdfType, Iterations, Memory, Parallelism);
+    }
+}

src/Core/KeyManagement/Models/Data/MasterPasswordAuthenticationData.cs

@@ -0,0 +1,18 @@
+using Bit.Core.Entities;
+
+namespace Bit.Core.KeyManagement.Models.Data;
+
+public class MasterPasswordAuthenticationData
+{
+    public required KdfSettings Kdf { get; init; }
+    public required string MasterPasswordAuthenticationHash { get; init; }
+    public required string Salt { get; init; }
+
+    public void ValidateSaltUnchangedForUser(User user)
+    {
+        if (user.GetMasterPasswordSalt() != Salt)
+        {
+            throw new ArgumentException("Invalid master password salt.");
+        }
+    }
+}

src/Core/KeyManagement/Models/Data/MasterPasswordUnlockAndAuthenticationData.cs

@@ -0,0 +1,34 @@
+#nullable enable
+using Bit.Core.Entities;
+using Bit.Core.Enums;
+
+namespace Bit.Core.KeyManagement.Models.Data;
+
+public class MasterPasswordUnlockAndAuthenticationData
+{
+    public KdfType KdfType { get; set; }
+    public int KdfIterations { get; set; }
+    public int? KdfMemory { get; set; }
+    public int? KdfParallelism { get; set; }
+
+    public required string Email { get; set; }
+    public required string MasterKeyAuthenticationHash { get; set; }
+    public required string MasterKeyEncryptedUserKey { get; set; }
+    public string? MasterPasswordHint { get; set; }
+
+    public bool ValidateForUser(User user)
+    {
+        if (KdfType != user.Kdf || KdfMemory != user.KdfMemory || KdfParallelism != user.KdfParallelism || KdfIterations != user.KdfIterations)
+        {
+            return false;
+        }
+        else if (Email != user.Email)
+        {
+            return false;
+        }
+        else
+        {
+            return true;
+        }
+    }
+}

src/Core/KeyManagement/Models/Data/MasterPasswordUnlockData.cs

@@ -1,34 +1,20 @@
 #nullable enable
+
 using Bit.Core.Entities;
-using Bit.Core.Enums;
 
 namespace Bit.Core.KeyManagement.Models.Data;
 
 public class MasterPasswordUnlockData
 {
-    public KdfType KdfType { get; set; }
-    public int KdfIterations { get; set; }
-    public int? KdfMemory { get; set; }
-    public int? KdfParallelism { get; set; }
+    public required KdfSettings Kdf { get; init; }
+    public required string MasterKeyWrappedUserKey { get; init; }
+    public required string Salt { get; init; }
 
-    public required string Email { get; set; }
-    public required string MasterKeyAuthenticationHash { get; set; }
-    public required string MasterKeyEncryptedUserKey { get; set; }
-    public string? MasterPasswordHint { get; set; }
-
-    public bool ValidateForUser(User user)
+    public void ValidateSaltUnchangedForUser(User user)
     {
-        if (KdfType != user.Kdf || KdfMemory != user.KdfMemory || KdfParallelism != user.KdfParallelism || KdfIterations != user.KdfIterations)
+        if (user.GetMasterPasswordSalt() != Salt)
         {
-            return false;
-        }
-        else if (Email != user.Email)
-        {
-            return false;
-        }
-        else
-        {
-            return true;
+            throw new ArgumentException("Invalid master password salt.");
         }
     }
 }

src/Core/KeyManagement/Models/Data/RotateUserAccountKeysData.cs

@@ -19,7 +19,7 @@ public class RotateUserAccountKeysData
     public string AccountPublicKey { get; set; }
 
     // All methods to get to the userkey
-    public MasterPasswordUnlockData MasterPasswordUnlockData { get; set; }
+    public MasterPasswordUnlockAndAuthenticationData MasterPasswordUnlockData { get; set; }
     public IEnumerable<EmergencyAccess> EmergencyAccesses { get; set; }
     public IReadOnlyList<OrganizationUser> OrganizationUsers { get; set; }
     public IEnumerable<WebAuthnLoginRotateKeyData> WebAuthnKeys { get; set; }

src/Core/Services/IUserService.cs

@@ -38,8 +38,6 @@ public interface IUserService
     Task<IdentityResult> ConvertToKeyConnectorAsync(User user);
     Task<IdentityResult> AdminResetPasswordAsync(OrganizationUserType type, Guid orgId, Guid id, string newMasterPassword, string key);
     Task<IdentityResult> UpdateTempPasswordAsync(User user, string newMasterPassword, string key, string hint);
-    Task<IdentityResult> ChangeKdfAsync(User user, string masterPassword, string newMasterPassword, string key,
-        KdfType kdf, int kdfIterations, int? kdfMemory, int? kdfParallelism);
     Task<IdentityResult> RefreshSecurityStampAsync(User user, string masterPasswordHash);
     Task UpdateTwoFactorProviderAsync(User user, TwoFactorProviderType type, bool setEnabled = true, bool logEvent = true);
     Task DisableTwoFactorProviderAsync(User user, TwoFactorProviderType type);

src/Core/Services/Implementations/UserService.cs

@@ -777,39 +777,6 @@ public class UserService : UserManager<User>, IUserService
         return IdentityResult.Success;
     }
 
-    public async Task<IdentityResult> ChangeKdfAsync(User user, string masterPassword, string newMasterPassword,
-        string key, KdfType kdf, int kdfIterations, int? kdfMemory, int? kdfParallelism)
-    {
-        if (user == null)
-        {
-            throw new ArgumentNullException(nameof(user));
-        }
-
-        if (await CheckPasswordAsync(user, masterPassword))
-        {
-            var result = await UpdatePasswordHash(user, newMasterPassword);
-            if (!result.Succeeded)
-            {
-                return result;
-            }
-
-            var now = DateTime.UtcNow;
-            user.RevisionDate = user.AccountRevisionDate = now;
-            user.LastKdfChangeDate = now;
-            user.Key = key;
-            user.Kdf = kdf;
-            user.KdfIterations = kdfIterations;
-            user.KdfMemory = kdfMemory;
-            user.KdfParallelism = kdfParallelism;
-            await _userRepository.ReplaceAsync(user);
-            await _pushService.PushLogOutAsync(user.Id);
-            return IdentityResult.Success;
-        }
-
-        Logger.LogWarning("Change KDF failed for user {userId}.", user.Id);
-        return IdentityResult.Failed(_identityErrorDescriber.PasswordMismatch());
-    }
-
     public async Task<IdentityResult> RefreshSecurityStampAsync(User user, string secret)
     {
         if (user == null)

src/Core/Utilities/KdfSettingsValidator.cs

@@ -1,5 +1,6 @@
 using System.ComponentModel.DataAnnotations;
 using Bit.Core.Enums;
+using Bit.Core.KeyManagement.Models.Data;
 
 namespace Bit.Core.Utilities;
 
@@ -34,4 +35,9 @@ public static class KdfSettingsValidator
                 break;
         }
     }
+
+    public static IEnumerable<ValidationResult> Validate(KdfSettings settings)
+    {
+        return Validate(settings.KdfType, settings.Iterations, settings.Memory, settings.Parallelism);
+    }
 }