From 3d59f5522dc0fd60879451a3352fd3d287f341fd Mon Sep 17 00:00:00 2001 From: Jordan Aasen <166539328+jaasen-livefront@users.noreply.github.com> Date: Wed, 16 Apr 2025 10:33:00 -0700 Subject: [PATCH] [PM-19357] - [Defect] Unauthorised access allows limited access user to change custom hidden field of Items (#5572) * prevent hidden password users from modifying hidden fields * add tests * fix serialization issues * DRY up code * return newly created cipher * add sshKey data type * fix tests --- .../Vault/Controllers/CiphersController.cs | 7 +-- .../Services/Implementations/CipherService.cs | 53 +++++++++++++++--- .../Vault/Services/CipherServiceTests.cs | 56 ++++++++++++++++++- 3 files changed, 100 insertions(+), 16 deletions(-) diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index a9646acd1c..3bdb6c4bf0 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -177,12 +177,7 @@ public class CiphersController : Controller } await _cipherService.SaveDetailsAsync(cipher, user.Id, model.Cipher.LastKnownRevisionDate, model.CollectionIds, cipher.OrganizationId.HasValue); - var response = new CipherResponseModel( - cipher, - user, - await _applicationCacheService.GetOrganizationAbilitiesAsync(), - _globalSettings); - return response; + return await Get(cipher.Id); } [HttpPost("admin")] diff --git a/src/Core/Vault/Services/Implementations/CipherService.cs b/src/Core/Vault/Services/Implementations/CipherService.cs index 989fbf43b8..745d90b741 100644 --- a/src/Core/Vault/Services/Implementations/CipherService.cs +++ b/src/Core/Vault/Services/Implementations/CipherService.cs @@ -1003,7 +1003,7 @@ public class CipherService : ICipherService private async Task ValidateViewPasswordUserAsync(Cipher cipher) { - if (cipher.Type != CipherType.Login || cipher.Data == null || !cipher.OrganizationId.HasValue) + if (cipher.Data == null || !cipher.OrganizationId.HasValue) { return; } @@ -1014,21 +1014,58 @@ public class CipherService : ICipherService // Check if user is a "hidden password" user if (!cipherPermissions.TryGetValue(cipher.Id, out var permission) || !(permission.ViewPassword && permission.Edit)) { + var existingCipherData = DeserializeCipherData(existingCipher); + var newCipherData = DeserializeCipherData(cipher); + // "hidden password" users may not add cipher key encryption if (existingCipher.Key == null && cipher.Key != null) { throw new BadRequestException("You do not have permission to add cipher key encryption."); } - // "hidden password" users may not change passwords, TOTP codes, or passkeys, so we need to set them back to the original values - var existingCipherData = JsonSerializer.Deserialize(existingCipher.Data); - var newCipherData = JsonSerializer.Deserialize(cipher.Data); - newCipherData.Fido2Credentials = existingCipherData.Fido2Credentials; - newCipherData.Totp = existingCipherData.Totp; - newCipherData.Password = existingCipherData.Password; - cipher.Data = JsonSerializer.Serialize(newCipherData); + // Keep only non-hidden fileds from the new cipher + var nonHiddenFields = newCipherData.Fields?.Where(f => f.Type != FieldType.Hidden) ?? []; + // Get hidden fields from the existing cipher + var hiddenFields = existingCipherData.Fields?.Where(f => f.Type == FieldType.Hidden) ?? []; + // Replace the hidden fields in new cipher data with the existing ones + newCipherData.Fields = nonHiddenFields.Concat(hiddenFields); + cipher.Data = SerializeCipherData(newCipherData); + if (existingCipherData is CipherLoginData existingLoginData && newCipherData is CipherLoginData newLoginCipherData) + { + // "hidden password" users may not change passwords, TOTP codes, or passkeys, so we need to set them back to the original values + newLoginCipherData.Fido2Credentials = existingLoginData.Fido2Credentials; + newLoginCipherData.Totp = existingLoginData.Totp; + newLoginCipherData.Password = existingLoginData.Password; + cipher.Data = SerializeCipherData(newLoginCipherData); + } } } + private string SerializeCipherData(CipherData data) + { + return data switch + { + CipherLoginData loginData => JsonSerializer.Serialize(loginData), + CipherIdentityData identityData => JsonSerializer.Serialize(identityData), + CipherCardData cardData => JsonSerializer.Serialize(cardData), + CipherSecureNoteData noteData => JsonSerializer.Serialize(noteData), + CipherSSHKeyData sshKeyData => JsonSerializer.Serialize(sshKeyData), + _ => throw new ArgumentException("Unsupported cipher data type.", nameof(data)) + }; + } + + private CipherData DeserializeCipherData(Cipher cipher) + { + return cipher.Type switch + { + CipherType.Login => JsonSerializer.Deserialize(cipher.Data), + CipherType.Identity => JsonSerializer.Deserialize(cipher.Data), + CipherType.Card => JsonSerializer.Deserialize(cipher.Data), + CipherType.SecureNote => JsonSerializer.Deserialize(cipher.Data), + CipherType.SSHKey => JsonSerializer.Deserialize(cipher.Data), + _ => throw new ArgumentException("Unsupported cipher type.", nameof(cipher)) + }; + } + // This method is used to filter ciphers based on the user's permissions to delete them. // It supports both the old and new logic depending on the feature flag. private async Task> FilterCiphersByDeletePermission( diff --git a/test/Core.Test/Vault/Services/CipherServiceTests.cs b/test/Core.Test/Vault/Services/CipherServiceTests.cs index ed07799c93..061d90bcc3 100644 --- a/test/Core.Test/Vault/Services/CipherServiceTests.cs +++ b/test/Core.Test/Vault/Services/CipherServiceTests.cs @@ -1228,7 +1228,8 @@ public class CipherServiceTests bool editPermission, string? key = null, string? totp = null, - CipherLoginFido2CredentialData[]? passkeys = null + CipherLoginFido2CredentialData[]? passkeys = null, + CipherFieldData[]? fields = null ) { var cipherDetails = new CipherDetails @@ -1241,12 +1242,13 @@ public class CipherServiceTests Key = key, }; - var newLoginData = new CipherLoginData { Username = "user", Password = newPassword, Totp = totp, Fido2Credentials = passkeys }; + var newLoginData = new CipherLoginData { Username = "user", Password = newPassword, Totp = totp, Fido2Credentials = passkeys, Fields = fields }; cipherDetails.Data = JsonSerializer.Serialize(newLoginData); var existingCipher = new Cipher { Id = cipherDetails.Id, + Type = CipherType.Login, Data = JsonSerializer.Serialize( new CipherLoginData { @@ -1442,6 +1444,56 @@ public class CipherServiceTests Assert.Equal(passkeys.Length, updatedLoginData.Fido2Credentials.Length); } + [Theory] + [BitAutoData] + public async Task SaveDetailsAsync_HiddenFieldsChangedWithoutPermission(string _, SutProvider sutProvider) + { + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: false, editPermission: false, fields: + [ + new CipherFieldData + { + Name = "FieldName", + Value = "FieldValue", + Type = FieldType.Hidden, + } + ]); + + await deps.SutProvider.Sut.SaveDetailsAsync( + deps.CipherDetails, + deps.CipherDetails.UserId.Value, + deps.CipherDetails.RevisionDate, + null, + true); + + var updatedLoginData = JsonSerializer.Deserialize(deps.CipherDetails.Data); + Assert.Empty(updatedLoginData.Fields); + } + + [Theory] + [BitAutoData] + public async Task SaveDetailsAsync_HiddenFieldsChangedWithPermission(string _, SutProvider sutProvider) + { + var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: true, fields: + [ + new CipherFieldData + { + Name = "FieldName", + Value = "FieldValue", + Type = FieldType.Hidden, + } + ]); + + await deps.SutProvider.Sut.SaveDetailsAsync( + deps.CipherDetails, + deps.CipherDetails.UserId.Value, + deps.CipherDetails.RevisionDate, + null, + true); + + var updatedLoginData = JsonSerializer.Deserialize(deps.CipherDetails.Data); + Assert.Single(updatedLoginData.Fields.ToArray()); + } + [Theory] [BitAutoData] public async Task DeleteAsync_WithPersonalCipherOwner_DeletesCipher(