diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index 44e3a30632a3..848a9b8cc9e5 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -165,15 +165,8 @@ public async Task Post([FromBody] CipherRequestModel model) { var user = await _userService.GetUserByPrincipalAsync(User); - // Validate the model was encrypted for the posting user - if (model.EncryptedFor != null) - { - if (model.EncryptedFor != user.Id) - { - _logger.LogError("Cipher was not encrypted for the current user. CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", user.Id, model.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + // Validate the model was encrypted by the posting user + ValidateCipherEncryptedByUser(model, user); var cipher = model.ToCipherDetails(user.Id); if (cipher.OrganizationId.HasValue && !await _currentContext.OrganizationUser(cipher.OrganizationId.Value)) @@ -191,15 +184,8 @@ public async Task PostCreate([FromBody] CipherCreateRequest { var user = await _userService.GetUserByPrincipalAsync(User); - // Validate the model was encrypted for the posting user - if (model.Cipher.EncryptedFor != null) - { - if (model.Cipher.EncryptedFor != user.Id) - { - _logger.LogError("Cipher was not encrypted for the current user. CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", user.Id, model.Cipher.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + // Validate the model was encrypted by the posting user + ValidateCipherEncryptedByUser(model.Cipher, user); var cipher = model.Cipher.ToCipherDetails(user.Id); if (cipher.OrganizationId.HasValue && !await _currentContext.OrganizationUser(cipher.OrganizationId.Value)) @@ -225,14 +211,7 @@ public async Task PostAdmin([FromBody] CipherCreateRequ var userId = _userService.GetProperUserId(User).Value; // Validate the model was encrypted for the posting user - if (model.Cipher.EncryptedFor != null) - { - if (model.Cipher.EncryptedFor != userId) - { - _logger.LogError("Cipher was not encrypted for the current user. CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", userId, model.Cipher.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + ValidateCipherEncryptedForUser(model.Cipher, userId); await _cipherService.SaveAsync(cipher, userId, model.Cipher.LastKnownRevisionDate, model.CollectionIds, true, false); @@ -250,15 +229,8 @@ public async Task Put(Guid id, [FromBody] CipherRequestMode throw new NotFoundException(); } - // Validate the model was encrypted for the posting user - if (model.EncryptedFor != null) - { - if (model.EncryptedFor != user.Id) - { - _logger.LogError("Cipher was not encrypted for the current user. CipherId: {CipherId}, CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", id, user.Id, model.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + // Validate the model was encrypted by the posting user + ValidateCipherEncryptedByUser(model, user, id); ValidateClientVersionForFido2CredentialSupport(cipher); @@ -291,14 +263,7 @@ public async Task PutAdmin(Guid id, [FromBody] CipherRe var cipher = await _cipherRepository.GetOrganizationDetailsByIdAsync(id); // Validate the model was encrypted for the posting user - if (model.EncryptedFor != null) - { - if (model.EncryptedFor != userId) - { - _logger.LogError("Cipher was not encrypted for the current user. CipherId: {CipherId}, CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", id, userId, model.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + ValidateCipherEncryptedForUser(model, userId, id); ValidateClientVersionForFido2CredentialSupport(cipher); @@ -732,15 +697,8 @@ public async Task PutShare(Guid id, [FromBody] CipherShareR throw new NotFoundException(); } - // Validate the model was encrypted for the posting user - if (model.Cipher.EncryptedFor != null) - { - if (model.Cipher.EncryptedFor != user.Id) - { - _logger.LogError("Cipher was not encrypted for the current user. CipherId: {CipherId} CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", id, user.Id, model.Cipher.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + // Validate the model was encrypted by the posting user + ValidateCipherEncryptedByUser(model.Cipher, user, id); ValidateClientVersionForFido2CredentialSupport(cipher); @@ -1237,14 +1195,10 @@ public async Task> PutShareMany([From var ciphers = await _cipherRepository.GetManyByUserIdAsync(userId, withOrganizations: false); var ciphersDict = ciphers.ToDictionary(c => c.Id); - // Validate the model was encrypted for the posting user + // Validate the models were encrypted for the posting user foreach (var cipher in model.Ciphers) { - if (cipher.EncryptedFor.HasValue && cipher.EncryptedFor.Value != userId) - { - _logger.LogError("Cipher was not encrypted for the current user. CipherId: {CipherId}, CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", cipher.Id, userId, cipher.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } + ValidateCipherEncryptedForUser(cipher, userId, cipher.Id); } var shareCiphers = new List<(CipherDetails, DateTime?)>(); @@ -1661,6 +1615,58 @@ private void ValidateClientVersionForFido2CredentialSupport(Cipher cipher) } } + /// + /// Validates that the cipher in was encrypted by the acting user. + /// + /// Deprecated in favor of , which identifies the key + /// rather than the user. Only checked when the client sends the field. + /// + /// + private void ValidateCipherEncryptedForUser(CipherRequestModel model, Guid userId, Guid? cipherId = null) + { +#pragma warning disable CS0618 // EncryptedFor is deprecated, but is still honored for clients that send it. + var encryptedFor = model.EncryptedFor; +#pragma warning restore CS0618 + + if (encryptedFor != null && encryptedFor != userId) + { + _logger.LogError( + "Cipher was not encrypted for the current user. CipherId: {CipherId}, CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", + cipherId, userId, encryptedFor); + throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); + } + } + + /// + /// Validates that the cipher in was encrypted by the acting user, with that + /// user's current user key. + /// + /// The key id is only compared when both sides are present: the client may predate the field, and the + /// user may not have a key id recorded yet. This mirrors + /// . + /// + /// + private void ValidateCipherEncryptedByUser(CipherRequestModel model, User user, Guid? cipherId = null) + { + ValidateCipherEncryptedForUser(model, user.Id, cipherId); + + var currentUserKeyId = user.GetUserKeyId(); + var encryptedByKeyId = model.GetEncryptedByKeyId(); + if (currentUserKeyId is null || encryptedByKeyId is null) + { + // Either the user has no key id recorded yet, or the client predates the field; nothing to compare. + return; + } + + if (!currentUserKeyId.Equals(encryptedByKeyId)) + { + _logger.LogError( + "Cipher was not encrypted with the current user key. CipherId: {CipherId}, CurrentUser: {CurrentUserId}, EncryptedByKeyId: {EncryptedByKeyId}", + cipherId, user.Id, encryptedByKeyId); + throw new BadRequestException("Cipher was not encrypted with the current user key. Please try again."); + } + } + private async Task GetByIdAsyncAdmin(Guid cipherId) { return await _cipherRepository.GetOrganizationDetailsByIdAsync(cipherId); diff --git a/src/Api/Vault/Models/Request/CipherRequestModel.cs b/src/Api/Vault/Models/Request/CipherRequestModel.cs index 9c419de4cb38..49b0b2767694 100644 --- a/src/Api/Vault/Models/Request/CipherRequestModel.cs +++ b/src/Api/Vault/Models/Request/CipherRequestModel.cs @@ -3,6 +3,7 @@ using System.ComponentModel.DataAnnotations; using System.Text.Json; +using Bit.Core.KeyManagement.Models.Data; using Bit.Core.Utilities; using Bit.Core.Vault.Entities; using Bit.Core.Vault.Enums; @@ -15,7 +16,16 @@ public class CipherRequestModel /// /// The Id of the user that encrypted the cipher. It should always represent a UserId. /// + [Obsolete("Use EncryptedByKeyId instead, which identifies the key the cipher was encrypted with.")] public Guid? EncryptedFor { get; set; } + + /// + /// Hex-encoded key id of the user key the client held when it encrypted this cipher. Absent for + /// clients that predate the field. When present, it must match the acting user's current user key id. + /// + [KeyId] + public string EncryptedByKeyId { get; set; } + public CipherType Type { get; set; } [StringLength(36)] @@ -67,6 +77,12 @@ public class CipherRequestModel public DateTime? LastKnownRevisionDate { get; set; } = null; public DateTime? ArchivedDate { get; set; } + /// + /// The key the client encrypted this cipher with, or null when it did not supply one. + /// + public KeyId GetEncryptedByKeyId() => + KeyId.FromHexEncodedString(string.IsNullOrEmpty(EncryptedByKeyId) ? null : EncryptedByKeyId); + public CipherDetails ToCipherDetails(Guid userId, bool allowOrgIdSet = true) { var hasOrgId = !string.IsNullOrWhiteSpace(OrganizationId); diff --git a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs index 926180400dd7..0c1d13a87bf8 100644 --- a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs +++ b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs @@ -1809,13 +1809,17 @@ public async Task PutShareMany_OrganizationUserFalse_ThrowsNotFound( await Assert.ThrowsAsync(() => sut.Sut.PutShareMany(model)); } [Theory, BitAutoData] - public async Task PutShareMany_CipherNotOwned_ThrowsNotFoundException( + public async Task PutShareMany_CipherNotOwned_ThrowsBadRequestException( Guid organizationId, Guid userId, CipherWithIdRequestModel request, SutProvider sutProvider) { - request.EncryptedFor = userId; + // The controller reads the organization off the first cipher, so it has to match the stub below. + request.OrganizationId = organizationId.ToString(); +#pragma warning disable CS0618 + request.EncryptedFor = null; +#pragma warning restore CS0618 var model = new CipherBulkShareRequestModel { Ciphers = new[] { request }, @@ -1832,19 +1836,23 @@ public async Task PutShareMany_CipherNotOwned_ThrowsNotFoundException( .GetManyByUserIdAsync(userId, withOrganizations: false) .Returns(Task.FromResult((ICollection)new List())); - await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.PutShareMany(model) ); } [Theory, BitAutoData] - public async Task PutShareMany_EncryptedForWrongUser_ThrowsNotFoundException( + public async Task PutShareMany_EncryptedForWrongUser_ThrowsBadRequestException( Guid organizationId, Guid userId, CipherWithIdRequestModel request, SutProvider sutProvider) { + // The controller reads the organization off the first cipher, so it has to match the stub below. + request.OrganizationId = organizationId.ToString(); +#pragma warning disable CS0618 // Deliberately exercising the deprecated field. request.EncryptedFor = Guid.NewGuid(); // not equal to userId +#pragma warning restore CS0618 var model = new CipherBulkShareRequestModel { Ciphers = new[] { request }, @@ -1863,7 +1871,7 @@ public async Task PutShareMany_EncryptedForWrongUser_ThrowsNotFoundException( .GetManyByUserIdAsync(userId, withOrganizations: false) .Returns(Task.FromResult((ICollection)(new[] { existing }))); - await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.PutShareMany(model) ); } @@ -2019,8 +2027,7 @@ public async Task PutShare_WithNullFolderAndFalseFavorite_UpdatesFieldsCorrectly Name = "SharedCipher", Data = JsonSerializer.Serialize(new { Username = "test", Password = "test" }), FolderId = null, - Favorite = false, - EncryptedFor = userId + Favorite = false }, CollectionIds = [Guid.NewGuid().ToString()] }; @@ -2092,8 +2099,7 @@ public async Task PutShare_WithFolderAndFavoriteSet_AddsUserSpecificFields( Name = "SharedCipher", Data = JsonSerializer.Serialize(new { Username = "test", Password = "test" }), FolderId = folderId.ToString(), - Favorite = true, - EncryptedFor = userId + Favorite = true }, CollectionIds = [Guid.NewGuid().ToString()] }; @@ -2168,8 +2174,7 @@ public async Task PutShare_UpdateExistingFolderAndFavorite_UpdatesUserSpecificFi Name = "SharedCipher", Data = JsonSerializer.Serialize(new { Username = "test", Password = "test" }), FolderId = newFolderId.ToString(), // Update to new folder - Favorite = true, // Add favorite - EncryptedFor = userId + Favorite = true // Add favorite }, CollectionIds = [Guid.NewGuid().ToString()] }; @@ -2539,4 +2544,197 @@ await sutProvider.GetDependency().Received(1) ApiHelpers.EventGridKey = previousEventGridKey; } } + + /// + /// A well-formed key id that is never the one hands out, so it always + /// mismatches the user key id on an AutoFixture-generated . + /// + private const string MismatchedKeyId = "ffffffffffffffffffffffffffffffff"; + + private static CipherRequestModel SecureNoteRequestModel(string encryptedByKeyId) => new() + { + Type = CipherType.SecureNote, + Name = "test", + Data = "{}", + EncryptedByKeyId = encryptedByKeyId + }; + + [Theory, BitAutoData] + public async Task Post_EncryptedByKeyIdMatchesUserKeyId_SavesCipher( + User user, + SutProvider sutProvider) + { + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + + await sutProvider.Sut.Post(SecureNoteRequestModel(KeyIdBuilder.HexEncodedKeyId)); + + await sutProvider.GetDependency().Received(1) + .SaveDetailsAsync(Arg.Any(), user.Id, Arg.Any(), Arg.Any>(), Arg.Any()); + } + + [Theory, BitAutoData] + public async Task Post_EncryptedByKeyIdDoesNotMatchUserKeyId_ThrowsBadRequestException( + User user, + SutProvider sutProvider) + { + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.Post(SecureNoteRequestModel(MismatchedKeyId))); + Assert.Contains("current user key", exception.Message); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .SaveDetailsAsync(default, default, default, default, default); + } + + [Theory, BitAutoData] + public async Task Post_EncryptedByKeyIdNotSent_SavesCipher( + User user, + SutProvider sutProvider) + { + // A client that predates the field sends nothing, and must keep working. + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + + await sutProvider.Sut.Post(SecureNoteRequestModel(null)); + + await sutProvider.GetDependency().Received(1) + .SaveDetailsAsync(Arg.Any(), user.Id, Arg.Any(), Arg.Any>(), Arg.Any()); + } + + [Theory, BitAutoData] + public async Task Post_UserHasNoKeyId_DoesNotValidateEncryptedByKeyId( + User user, + SutProvider sutProvider) + { + // Nothing to compare against until the user's key id has been backfilled. + user.UserKeyId = null; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + + await sutProvider.Sut.Post(SecureNoteRequestModel(MismatchedKeyId)); + + await sutProvider.GetDependency().Received(1) + .SaveDetailsAsync(Arg.Any(), user.Id, Arg.Any(), Arg.Any>(), Arg.Any()); + } + + [Theory, BitAutoData] + public async Task PostCreate_EncryptedByKeyIdDoesNotMatchUserKeyId_ThrowsBadRequestException( + User user, + SutProvider sutProvider) + { + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + + var model = new CipherCreateRequestModel + { + Cipher = SecureNoteRequestModel(MismatchedKeyId), + CollectionIds = [] + }; + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.PostCreate(model)); + Assert.Contains("current user key", exception.Message); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .SaveDetailsAsync(default, default, default, default, default); + } + + [Theory, BitAutoData] + public async Task Put_EncryptedByKeyIdDoesNotMatchUserKeyId_ThrowsBadRequestException( + User user, + Guid cipherId, + SutProvider sutProvider) + { + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + // The cipher-not-found check runs before validation, so the cipher has to exist. + sutProvider.GetDependency() + .GetByIdAsync(cipherId, user.Id) + .Returns(new CipherDetails + { + Id = cipherId, + UserId = user.Id, + Type = CipherType.SecureNote, + Data = "{}" + }); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.Put(cipherId, SecureNoteRequestModel(MismatchedKeyId))); + Assert.Contains("current user key", exception.Message); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .SaveDetailsAsync(default, default, default, default, default); + } + + [Theory, BitAutoData] + public async Task PutShare_EncryptedByKeyIdDoesNotMatchUserKeyId_ThrowsBadRequestException( + User user, + Guid cipherId, + Guid organizationId, + SutProvider sutProvider) + { + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + // Ownership and organization membership are checked before validation. + sutProvider.GetDependency() + .GetByIdAsync(cipherId) + .Returns(new Cipher + { + Id = cipherId, + UserId = user.Id, + Type = CipherType.Login, + Data = "{}" + }); + sutProvider.GetDependency() + .OrganizationUser(organizationId) + .Returns(true); + + var cipherModel = SecureNoteRequestModel(MismatchedKeyId); + cipherModel.OrganizationId = organizationId.ToString(); + var model = new CipherShareRequestModel + { + Cipher = cipherModel, + CollectionIds = [Guid.NewGuid().ToString()] + }; + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.PutShare(cipherId, model)); + Assert.Contains("current user key", exception.Message); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .ShareAsync(default, default, default, default, default, default); + } + + [Theory, BitAutoData] + public async Task PutAdmin_EncryptedForWrongUser_ThrowsBadRequestExceptionBeforeAuthorizationCheck( + Guid userId, + Guid cipherId, + SutProvider sutProvider) + { + sutProvider.GetDependency() + .GetProperUserId(default) + .ReturnsForAnyArgs(userId); + + var model = SecureNoteRequestModel(null); +#pragma warning disable CS0618 + model.EncryptedFor = Guid.NewGuid(); // not equal to userId +#pragma warning restore CS0618 + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.PutAdmin(cipherId, model)); + Assert.Contains("encrypted for the current user", exception.Message); + } }