[PM-39454] feat(km): reject unlock data whose user key id does not match the stored one

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

@@ -18,11 +18,11 @@
     private readonly IPushNotificationService _pushService;
     private readonly IMasterPasswordService _masterPasswordService;
     private readonly IdentityErrorDescriber _identityErrorDescriber;
     private readonly IFeatureService _featureService;
 
     public ChangeKdfCommand(IUserService userService, IPushNotificationService pushService,
         IMasterPasswordService masterPasswordService, IdentityErrorDescriber describer,
         IFeatureService featureService)
     {
         _userService = userService;
         _pushService = pushService;
@@ -47,6 +47,11 @@
         authenticationData.ValidateSaltUnchangedForUser(user);
         unlockData.ValidateSaltUnchangedForUser(user);
 
+        // A KDF change re-wraps the existing user key, it does not replace it, so its key id must not
+        // change. Also checked in the MasterPasswordService via
+        // UpdateExistingKdfConfigurationData.ValidateDataForUser.
+        unlockData.ValidateUserKeyIdUnchangedForUser(user);
+
         // Currently KDF settings are not saved separately for authentication and unlock and must therefore be equal
         if (!authenticationData.Kdf.Equals(unlockData.Kdf))
         {

src/Core/KeyManagement/UserKey/Models/Data/BaseRotateUserAccountKeysData.cs

@@ -1,6 +1,7 @@
 using Bit.Core.Auth.Entities;
 using Bit.Core.Auth.Models.Data;
 using Bit.Core.Entities;
+using Bit.Core.Exceptions;
 using Bit.Core.KeyManagement.Models.Data;
 using Bit.Core.Tools.Entities;
 using Bit.Core.Vault.Entities;
@@ -28,4 +29,23 @@ public class BaseRotateUserAccountKeysData
     /// authoritative key id of the request: it is the value persisted as the account's key id.
     /// </summary>
     public KeyId? UserKeyId { get; set; }
+
+    /// <summary>
+    /// Validates the provided key id against the key id of this request. This should be used to verify
+    /// that the items directly encrypted by the user-key (cipher-keys, private-key, signature-key) are
+    /// encrypted by the correct key.
+    /// </summary>
+    public void ValidateContainedKeyIdMatches(KeyId? containedKeyId)
+    {
+        if (containedKeyId == null)
+        {
+            return;
+        }
+
+        if (containedKeyId != UserKeyId)
+        {
+            throw new BadRequestException(
+                "The user key id contained in the unlock data must match the user key id of the key rotation.");
+        }
+    }
 }

src/Core/KeyManagement/UserKey/Models/Data/MasterPasswordRotateUserAccountKeysData.cs

@@ -19,5 +19,6 @@ public void ValidateForUser(User user)
 
         MasterPasswordUnlockData.ValidateSaltUnchangedForUser(user);
         MasterPasswordUnlockData.Kdf.ValidateUnchangedForUser(user);
+        BaseData.ValidateContainedKeyIdMatches(MasterPasswordUnlockData.ContainedKeyId());
     }
 }

src/Core/KeyManagement/UserKey/Models/Data/PasswordChangeAndRotateUserAccountKeysData.cs

@@ -27,5 +27,7 @@ public void ValidateForUser(User user)
         {
             throw new InvalidOperationException("The provided master password unlock data is not valid for this user.");
         }
+
+        BaseData.ValidateContainedKeyIdMatches(MasterPasswordUnlockData.ContainedKeyId());
     }
 }

test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs

@@ -764,6 +764,23 @@ public async Task RotateUserKeysAsync_V2Rotation_Success(RotateUserKeysRequestMo
             signatureKeyPair.VerifyingKey);
     }
 
+    [Theory]
+    [BitAutoData]
+    public async Task RotateUserKeysAsync_MasterPasswordUnlockKeyIdMismatch_BadRequest(
+        RotateUserKeysRequestModel request)
+    {
+        var user = await SetupUserForKeyRotationAsync(_mockEncryptedType7String, true);
+        SetupMasterPasswordRotateUserAccount(request, user);
+        request.UserKeyId = "0123456789abcdef0123456789abcdef";
+
+        var response = await _client.PostAsJsonAsync("/accounts/key-management/rotate-user-keys", request);
+
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+        var userNewState = await _userRepository.GetByEmailAsync(_ownerEmail);
+        Assert.NotNull(userNewState);
+        Assert.Equal(user.UserKeyId, userNewState.UserKeyId);
+    }
+
     [Theory]
     [BitAutoData]
     public async Task RotateUserKeysAsync_V1ToV2Rotation_Success(RotateUserKeysRequestModel request)

test/Core.Test/KeyManagement/Models/Data/MasterPasswordUnlockDataTests.cs

@@ -72,6 +72,7 @@ public void ValidateUserKeyIdUnchangedForUser_Throws_WhenKeyIdsDisagree()
 
         var exception = Assert.Throws<BadRequestException>(
             () => BuildUnlockData(DifferentKeyId).ValidateUserKeyIdUnchangedForUser(user));
+
         Assert.Equal("Invalid user key id.", exception.Message);
     }
 
@@ -94,4 +95,12 @@ public void ValidateUserKeyIdUnchangedForUser_DoesNotMutateTheUser()
 
         Assert.Equal(StoredKeyId, user.UserKeyId);
     }
+
+    [Fact]
+    public void Equals_ComparesKeyIdByValue()
+    {
+        Assert.Equal(BuildUnlockData(StoredKeyId), BuildUnlockData(StoredKeyId));
+        Assert.NotEqual(BuildUnlockData(StoredKeyId), BuildUnlockData(DifferentKeyId));
+        Assert.NotEqual(BuildUnlockData(StoredKeyId), BuildUnlockData(null));
+    }
 }

test/Core.Test/KeyManagement/UserKey/Models/Data/BaseRotateUserAccountKeysDataTests.cs

@@ -0,0 +1,67 @@
+using Bit.Core.Exceptions;
+using Bit.Core.KeyManagement.Models.Data;
+using Bit.Core.KeyManagement.UserKey.Models.Data;
+using Xunit;
+
+namespace Bit.Core.Test.KeyManagement.UserKey.Models.Data;
+
+public class BaseRotateUserAccountKeysDataTests
+{
+    private const string _keyIdA = "0123456789abcdef0123456789abcdef";
+    private const string _keyIdB = "fedcba9876543210fedcba9876543210";
+
+    private static BaseRotateUserAccountKeysData MakeData(string? rotationKeyId) => new()
+    {
+        AccountKeys = new UserAccountKeysData
+        {
+            PublicKeyEncryptionKeyPairData =
+                new PublicKeyEncryptionKeyPairData("mockWrappedPrivateKey", "mockPublicKey")
+        },
+        EmergencyAccesses = [],
+        OrganizationUsers = [],
+        WebAuthnKeys = [],
+        DeviceKeys = [],
+        Ciphers = [],
+        Folders = [],
+        Sends = [],
+        UserKeyId = KeyId.FromHexEncodedString(rotationKeyId)
+    };
+
+    [Fact]
+    public void ValidateContainedKeyIdMatches_WhenKeyIdsMatch_DoesNotThrow()
+    {
+        MakeData(_keyIdA).ValidateContainedKeyIdMatches(KeyId.FromHexEncodedString(_keyIdA));
+    }
+
+    [Fact]
+    public void ValidateContainedKeyIdMatches_WhenKeyIdsDiffer_Throws()
+    {
+        var exception = Assert.Throws<BadRequestException>(() =>
+            MakeData(_keyIdA).ValidateContainedKeyIdMatches(KeyId.FromHexEncodedString(_keyIdB)));
+
+        Assert.Equal("The user key id contained in the unlock data must match the user key id of the key rotation.",
+            exception.Message);
+    }
+
+    [Fact]
+    public void ValidateContainedKeyIdMatches_WhenContainedKeyIdIsAbsent_DoesNotThrow()
+    {
+        // Clients that predate the field carry no key id in the unlock data.
+        MakeData(_keyIdA).ValidateContainedKeyIdMatches(null);
+    }
+
+    [Fact]
+    public void ValidateContainedKeyIdMatches_WhenRotationKeyIdIsAbsent_Throws()
+    {
+        // Only the rotation's own key id is persisted, so a request that reports one solely in its
+        // unlock data would silently leave the server with no key id for the new user key.
+        Assert.Throws<BadRequestException>(() =>
+            MakeData(null).ValidateContainedKeyIdMatches(KeyId.FromHexEncodedString(_keyIdA)));
+    }
+
+    [Fact]
+    public void ValidateContainedKeyIdMatches_WhenNeitherKeyIdIsPresent_DoesNotThrow()
+    {
+        MakeData(null).ValidateContainedKeyIdMatches(null);
+    }
+}

test/Core.Test/KeyManagement/UserKey/Models/Data/MasterPasswordRotateUserAccountKeysDataTests.cs

@@ -0,0 +1,105 @@
+using Bit.Core.Entities;
+using Bit.Core.Enums;
+using Bit.Core.Exceptions;
+using Bit.Core.KeyManagement.Models.Data;
+using Bit.Core.KeyManagement.UserKey.Models.Data;
+using Bit.Test.Common.AutoFixture.Attributes;
+using Xunit;
+
+namespace Bit.Core.Test.KeyManagement.UserKey.Models.Data;
+
+public class MasterPasswordRotateUserAccountKeysDataTests
+{
+    private const string _keyIdA = "0123456789abcdef0123456789abcdef";
+    private const string _keyIdB = "fedcba9876543210fedcba9876543210";
+    private const string _mockMasterKeyWrappedUserKey = "mockMasterKeyWrappedUserKey";
+
+    private static KdfSettings ValidKdf
+    {
+        get => new() { KdfType = KdfType.PBKDF2_SHA256, Iterations = 600000, Memory = null, Parallelism = null };
+    }
+
+    private static void SetupValidUser(User user)
+    {
+        user.Email = "test@example.com";
+        user.MasterPasswordSalt = null;
+        user.Key = "mockUserKey";
+        user.MasterPassword = "mockMasterPasswordHash";
+        user.Kdf = ValidKdf.KdfType;
+        user.KdfIterations = ValidKdf.Iterations;
+        user.KdfMemory = ValidKdf.Memory;
+        user.KdfParallelism = ValidKdf.Parallelism;
+    }
+
+    private static MasterPasswordRotateUserAccountKeysData CreateModel(string salt, string? unlockUserKeyId,
+        string? rotationUserKeyId) =>
+        new()
+        {
+            MasterPasswordUnlockData = new MasterPasswordUnlockData
+            {
+                Kdf = ValidKdf,
+                MasterKeyWrappedUserKey = _mockMasterKeyWrappedUserKey,
+                Salt = salt,
+                UserKeyId = KeyId.FromHexEncodedString(unlockUserKeyId)
+            },
+            BaseData = new BaseRotateUserAccountKeysData
+            {
+                AccountKeys = new UserAccountKeysData
+                {
+                    PublicKeyEncryptionKeyPairData =
+                        new PublicKeyEncryptionKeyPairData("mockWrappedPrivateKey", "mockPublicKey")
+                },
+                EmergencyAccesses = [],
+                OrganizationUsers = [],
+                WebAuthnKeys = [],
+                DeviceKeys = [],
+                Ciphers = [],
+                Folders = [],
+                Sends = [],
+                UserKeyId = KeyId.FromHexEncodedString(rotationUserKeyId)
+            }
+        };
+
+    [Theory]
+    [BitAutoData]
+    public void ValidateForUser_UnlockKeyIdMatchesRotationKeyId_DoesNotThrow(User user)
+    {
+        SetupValidUser(user);
+        var model = CreateModel(user.Email, _keyIdA, _keyIdA);
+
+        model.ValidateForUser(user);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public void ValidateForUser_NoKeyIds_DoesNotThrow(User user)
+    {
+        // Clients that predate the field send neither key id.
+        SetupValidUser(user);
+        var model = CreateModel(user.Email, null, null);
+
+        model.ValidateForUser(user);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public void ValidateForUser_UnlockKeyIdDiffersFromRotationKeyId_ThrowsBadRequestException(User user)
+    {
+        SetupValidUser(user);
+        var model = CreateModel(user.Email, _keyIdB, _keyIdA);
+
+        Assert.Throws<BadRequestException>(() => model.ValidateForUser(user));
+    }
+
+    [Theory]
+    [BitAutoData]
+    public void ValidateForUser_UnlockKeyIdWithoutRotationKeyId_ThrowsBadRequestException(User user)
+    {
+        // The rotation's key id is the only one persisted, so a request that reports one solely in
+        // its unlock data must not pass as if the key id were tracked.
+        SetupValidUser(user);
+        var model = CreateModel(user.Email, _keyIdA, null);
+
+        Assert.Throws<BadRequestException>(() => model.ValidateForUser(user));
+    }
+}

test/Core.Test/KeyManagement/UserKey/Models/Data/PasswordChangeAndRotateUserAccountKeysDataTests.cs

@@ -1,5 +1,6 @@
 using Bit.Core.Entities;
 using Bit.Core.Enums;
+using Bit.Core.Exceptions;
 using Bit.Core.KeyManagement.Models.Data;
 using Bit.Core.KeyManagement.UserKey.Models.Data;
 using Bit.Test.Common.AutoFixture.Attributes;
@@ -9,6 +10,8 @@ namespace Bit.Core.Test.KeyManagement.UserKey.Models.Data;
 
 public class PasswordChangeAndRotateUserAccountKeysDataTests
 {
+    private const string _keyIdA = "0123456789abcdef0123456789abcdef";
+    private const string _keyIdB = "fedcba9876543210fedcba9876543210";
     private const string _mockOldMasterKeyAuthenticationHash = "hash";
     private const string _mockMasterPasswordAuthenticationHash = "mockAuthenticationHash";
     private const string _mockMasterKeyWrappedUserKey = "mockMasterKeyWrappedUserKey";
@@ -28,7 +31,8 @@ private static void SetupValidUser(User user)
         user.KdfParallelism = ValidKdf.Parallelism;
     }
 
-    private static PasswordChangeAndRotateUserAccountKeysData CreateValidModel(string salt, KdfSettings kdf) =>
+    private static PasswordChangeAndRotateUserAccountKeysData CreateValidModel(string salt, KdfSettings kdf,
+        string? unlockUserKeyId = null, string? rotationUserKeyId = null) =>
         new()
         {
             OldMasterKeyAuthenticationHash = _mockOldMasterKeyAuthenticationHash,
@@ -44,7 +48,8 @@ private static PasswordChangeAndRotateUserAccountKeysData CreateValidModel(strin
                 {
                     Kdf = kdf,
                     MasterKeyWrappedUserKey = _mockMasterKeyWrappedUserKey,
-                    Salt = salt
+                    Salt = salt,
+                    UserKeyId = KeyId.FromHexEncodedString(unlockUserKeyId)
                 },
             BaseData = new BaseRotateUserAccountKeysData
             {
@@ -59,7 +64,8 @@ private static PasswordChangeAndRotateUserAccountKeysData CreateValidModel(strin
                 DeviceKeys = [],
                 Ciphers = [],
                 Folders = [],
-                Sends = []
+                Sends = [],
+                UserKeyId = KeyId.FromHexEncodedString(rotationUserKeyId)
             }
         };
 
@@ -73,6 +79,38 @@ public void ValidateForUser_ValidData_DoesNotThrow(User user)
         model.ValidateForUser(user);
     }
 
+    [Theory]
+    [BitAutoData]
+    public void ValidateForUser_UnlockKeyIdMatchesRotationKeyId_DoesNotThrow(User user)
+    {
+        SetupValidUser(user);
+        var model = CreateValidModel(user.Email, ValidKdf, _keyIdA, _keyIdA);
+
+        model.ValidateForUser(user);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public void ValidateForUser_UnlockKeyIdDiffersFromRotationKeyId_ThrowsBadRequestException(User user)
+    {
+        SetupValidUser(user);
+        var model = CreateValidModel(user.Email, ValidKdf, _keyIdB, _keyIdA);
+
+        Assert.Throws<BadRequestException>(() => model.ValidateForUser(user));
+    }
+
+    [Theory]
+    [BitAutoData]
+    public void ValidateForUser_UnlockKeyIdWithoutRotationKeyId_ThrowsBadRequestException(User user)
+    {
+        // The rotation's key id is the only one persisted, so a request that reports one solely in
+        // its unlock data must not pass as if the key id were tracked.
+        SetupValidUser(user);
+        var model = CreateValidModel(user.Email, ValidKdf, _keyIdA, null);
+
+        Assert.Throws<BadRequestException>(() => model.ValidateForUser(user));
+    }
+
     [Theory]
     [BitAutoData]
     public void ValidateForUser_AuthenticationSaltMismatch_ThrowsInvalidOperationException(User user)