From a3c5741164f62c6ee9dd2b14637c900210d78f45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Tue, 17 Jun 2025 11:07:26 -0400 Subject: [PATCH] [PM-22610] validate file within max length; log deletion of invalid uploads (#5960) --- .../Commands/NonAnonymousSendCommand.cs | 47 +++++++++++---- .../Services/AzureSendFileStorageService.cs | 13 ++-- .../Interfaces/ISendStorageService.cs | 19 +++--- .../Services/LocalSendStorageService.cs | 12 ++-- .../NoopSendFileStorageService.cs | 4 +- .../Services/NonAnonymousSendCommandTests.cs | 60 ++++++++++++++----- 6 files changed, 99 insertions(+), 56 deletions(-) diff --git a/src/Core/Tools/SendFeatures/Commands/NonAnonymousSendCommand.cs b/src/Core/Tools/SendFeatures/Commands/NonAnonymousSendCommand.cs index 87b4e581ca..804200a05f 100644 --- a/src/Core/Tools/SendFeatures/Commands/NonAnonymousSendCommand.cs +++ b/src/Core/Tools/SendFeatures/Commands/NonAnonymousSendCommand.cs @@ -8,6 +8,7 @@ using Bit.Core.Tools.Repositories; using Bit.Core.Tools.SendFeatures.Commands.Interfaces; using Bit.Core.Tools.Services; using Bit.Core.Utilities; +using Microsoft.Extensions.Logging; namespace Bit.Core.Tools.SendFeatures.Commands; @@ -18,19 +19,22 @@ public class NonAnonymousSendCommand : INonAnonymousSendCommand private readonly IPushNotificationService _pushNotificationService; private readonly ISendValidationService _sendValidationService; private readonly ISendCoreHelperService _sendCoreHelperService; + private readonly ILogger _logger; public NonAnonymousSendCommand(ISendRepository sendRepository, ISendFileStorageService sendFileStorageService, IPushNotificationService pushNotificationService, ISendAuthorizationService sendAuthorizationService, ISendValidationService sendValidationService, - ISendCoreHelperService sendCoreHelperService) + ISendCoreHelperService sendCoreHelperService, + ILogger logger) { _sendRepository = sendRepository; _sendFileStorageService = sendFileStorageService; _pushNotificationService = pushNotificationService; _sendValidationService = sendValidationService; _sendCoreHelperService = sendCoreHelperService; + _logger = logger; } public async Task SaveSendAsync(Send send) @@ -63,6 +67,11 @@ public class NonAnonymousSendCommand : INonAnonymousSendCommand throw new BadRequestException("No file data."); } + if (fileLength > SendFileSettingHelper.MAX_FILE_SIZE) + { + throw new BadRequestException($"Max file size is {SendFileSettingHelper.MAX_FILE_SIZE_READABLE}."); + } + var storageBytesRemaining = await _sendValidationService.StorageRemainingForSendAsync(send); if (storageBytesRemaining < fileLength) @@ -77,13 +86,17 @@ public class NonAnonymousSendCommand : INonAnonymousSendCommand data.Id = fileId; data.Size = fileLength; data.Validated = false; - send.Data = JsonSerializer.Serialize(data, - JsonHelpers.IgnoreWritingNull); + send.Data = JsonSerializer.Serialize(data, JsonHelpers.IgnoreWritingNull); await SaveSendAsync(send); return await _sendFileStorageService.GetSendFileUploadUrlAsync(send, fileId); } catch { + _logger.LogWarning( + "Deleted file from {SendId} because an error occurred when creating the upload URL.", + send.Id + ); + // Clean up since this is not transactional await _sendFileStorageService.DeleteFileAsync(send, fileId); throw; @@ -135,23 +148,31 @@ public class NonAnonymousSendCommand : INonAnonymousSendCommand { var fileData = JsonSerializer.Deserialize(send.Data); - var (valid, realSize) = await _sendFileStorageService.ValidateFileAsync(send, fileData.Id, fileData.Size, SendFileSettingHelper.FILE_SIZE_LEEWAY); + var minimum = fileData.Size - SendFileSettingHelper.FILE_SIZE_LEEWAY; + var maximum = Math.Min( + fileData.Size + SendFileSettingHelper.FILE_SIZE_LEEWAY, + SendFileSettingHelper.MAX_FILE_SIZE + ); + var (valid, size) = await _sendFileStorageService.ValidateFileAsync(send, fileData.Id, minimum, maximum); - if (!valid || realSize > SendFileSettingHelper.FILE_SIZE_LEEWAY) + // protect file service from upload hijacking by deleting invalid sends + if (!valid) { - // File reported differs in size from that promised. Must be a rogue client. Delete Send + _logger.LogWarning( + "Deleted {SendId} because its reported size {Size} was outside the expected range ({Minimum} - {Maximum}).", + send.Id, + size, + minimum, + maximum + ); await DeleteSendAsync(send); return false; } - // Update Send data if necessary - if (realSize != fileData.Size) - { - fileData.Size = realSize.Value; - } + // replace expected size with validated size + fileData.Size = size; fileData.Validated = true; - send.Data = JsonSerializer.Serialize(fileData, - JsonHelpers.IgnoreWritingNull); + send.Data = JsonSerializer.Serialize(fileData, JsonHelpers.IgnoreWritingNull); await SaveSendAsync(send); return valid; diff --git a/src/Core/Tools/SendFeatures/Services/AzureSendFileStorageService.cs b/src/Core/Tools/SendFeatures/Services/AzureSendFileStorageService.cs index 09f3be29e7..ee54ffd6b6 100644 --- a/src/Core/Tools/SendFeatures/Services/AzureSendFileStorageService.cs +++ b/src/Core/Tools/SendFeatures/Services/AzureSendFileStorageService.cs @@ -88,7 +88,7 @@ public class AzureSendFileStorageService : ISendFileStorageService return sasUri.ToString(); } - public async Task<(bool, long?)> ValidateFileAsync(Send send, string fileId, long expectedFileSize, long leeway) + public async Task<(bool, long)> ValidateFileAsync(Send send, string fileId, long minimum, long maximum) { await InitAsync(); @@ -116,17 +116,14 @@ public class AzureSendFileStorageService : ISendFileStorageService await blobClient.SetHttpHeadersAsync(headers); var length = blobProperties.Value.ContentLength; - if (length < expectedFileSize - leeway || length > expectedFileSize + leeway) - { - return (false, length); - } + var valid = minimum <= length || length <= maximum; - return (true, length); + return (valid, length); } catch (Exception ex) { - _logger.LogError(ex, "Unhandled error in ValidateFileAsync"); - return (false, null); + _logger.LogError(ex, $"A storage operation failed in {nameof(ValidateFileAsync)}"); + return (false, -1); } } diff --git a/src/Core/Tools/SendFeatures/Services/Interfaces/ISendStorageService.cs b/src/Core/Tools/SendFeatures/Services/Interfaces/ISendStorageService.cs index 29bc0c6a6a..8712d07d48 100644 --- a/src/Core/Tools/SendFeatures/Services/Interfaces/ISendStorageService.cs +++ b/src/Core/Tools/SendFeatures/Services/Interfaces/ISendStorageService.cs @@ -56,16 +56,13 @@ public interface ISendFileStorageService /// /// used to help validate file /// File id to identify which file to validate - /// Expected file size of the file - /// - /// Send file size tolerance in bytes. When an uploaded file's `expectedFileSize` - /// is outside of the leeway, the storage operation fails. - /// - /// - /// ❌ Fill this in with an explanation of the error thrown when `leeway` is incorrect - /// - /// Task object for async operations with Tuple of boolean that determines if file was valid and long that - /// the actual file size of the file. + /// The minimum allowed length of the stored file in bytes. + /// The maximuim allowed length of the stored file in bytes + /// + /// A task that completes when validation is finished. The first element of the tuple is + /// when validation succeeded, and false otherwise. The second element + /// of the tuple contains the observed file length in bytes. If an error occurs during validation, + /// this returns `-1`. /// - Task<(bool, long?)> ValidateFileAsync(Send send, string fileId, long expectedFileSize, long leeway); + Task<(bool valid, long length)> ValidateFileAsync(Send send, string fileId, long minimum, long maximum); } diff --git a/src/Core/Tools/SendFeatures/Services/LocalSendStorageService.cs b/src/Core/Tools/SendFeatures/Services/LocalSendStorageService.cs index c205028d9e..a6b3fb0faf 100644 --- a/src/Core/Tools/SendFeatures/Services/LocalSendStorageService.cs +++ b/src/Core/Tools/SendFeatures/Services/LocalSendStorageService.cs @@ -85,9 +85,9 @@ public class LocalSendStorageService : ISendFileStorageService public Task GetSendFileUploadUrlAsync(Send send, string fileId) => Task.FromResult($"/sends/{send.Id}/file/{fileId}"); - public Task<(bool, long?)> ValidateFileAsync(Send send, string fileId, long expectedFileSize, long leeway) + public Task<(bool, long)> ValidateFileAsync(Send send, string fileId, long minimum, long maximum) { - long? length = null; + long length = -1; var path = FilePath(send, fileId); if (!File.Exists(path)) { @@ -95,11 +95,7 @@ public class LocalSendStorageService : ISendFileStorageService } length = new FileInfo(path).Length; - if (expectedFileSize < length - leeway || expectedFileSize > length + leeway) - { - return Task.FromResult((false, length)); - } - - return Task.FromResult((true, length)); + var valid = minimum < length || length < maximum; + return Task.FromResult((valid, length)); } } diff --git a/src/Core/Tools/Services/NoopImplementations/NoopSendFileStorageService.cs b/src/Core/Tools/Services/NoopImplementations/NoopSendFileStorageService.cs index 4fb841e7a3..16c20e521e 100644 --- a/src/Core/Tools/Services/NoopImplementations/NoopSendFileStorageService.cs +++ b/src/Core/Tools/Services/NoopImplementations/NoopSendFileStorageService.cs @@ -37,8 +37,8 @@ public class NoopSendFileStorageService : ISendFileStorageService return Task.FromResult((string)null); } - public Task<(bool, long?)> ValidateFileAsync(Send send, string fileId, long expectedFileSize, long leeway) + public Task<(bool, long)> ValidateFileAsync(Send send, string fileId, long minimum, long maximum) { - return Task.FromResult((false, default(long?))); + return Task.FromResult((false, -1L)); } } diff --git a/test/Core.Test/Tools/Services/NonAnonymousSendCommandTests.cs b/test/Core.Test/Tools/Services/NonAnonymousSendCommandTests.cs index 674cca7d5f..1ad6a08516 100644 --- a/test/Core.Test/Tools/Services/NonAnonymousSendCommandTests.cs +++ b/test/Core.Test/Tools/Services/NonAnonymousSendCommandTests.cs @@ -10,10 +10,10 @@ using Bit.Core.Tools.Entities; using Bit.Core.Tools.Enums; using Bit.Core.Tools.Models.Data; using Bit.Core.Tools.Repositories; -using Bit.Core.Tools.SendFeatures; using Bit.Core.Tools.SendFeatures.Commands; using Bit.Core.Tools.Services; using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.Extensions.Logging; using NSubstitute; using NSubstitute.ExceptionExtensions; using Xunit; @@ -35,6 +35,8 @@ public class NonAnonymousSendCommandTests private readonly ISendCoreHelperService _sendCoreHelperService; private readonly NonAnonymousSendCommand _nonAnonymousSendCommand; + private readonly ILogger _logger; + public NonAnonymousSendCommandTests() { _sendRepository = Substitute.For(); @@ -45,6 +47,7 @@ public class NonAnonymousSendCommandTests _sendValidationService = Substitute.For(); _currentContext = Substitute.For(); _sendCoreHelperService = Substitute.For(); + _logger = Substitute.For>(); _nonAnonymousSendCommand = new NonAnonymousSendCommand( _sendRepository, @@ -52,7 +55,8 @@ public class NonAnonymousSendCommandTests _pushNotificationService, _sendAuthorizationService, _sendValidationService, - _sendCoreHelperService + _sendCoreHelperService, + _logger ); } @@ -652,11 +656,11 @@ public class NonAnonymousSendCommandTests UserId = userId }; var fileData = new SendFileData(); - var fileLength = 15L * 1024L * 1024L * 1024L; // 15GB + var fileLength = 15L * 1024L * 1024L; // 15 MB - // Configure validation service to return large but insufficient storage (10GB for self-hosted non-premium) + // Configure validation service to return insufficient storage _sendValidationService.StorageRemainingForSendAsync(send) - .Returns(10L * 1024L * 1024L * 1024L); // 10GB remaining (self-hosted default) + .Returns(10L * 1024L * 1024L); // 10 MB remaining // Act & Assert var exception = await Assert.ThrowsAsync(() => @@ -687,11 +691,40 @@ public class NonAnonymousSendCommandTests UserId = userId }; var fileData = new SendFileData(); - var fileLength = 2L * 1024L * 1024L * 1024L; // 2GB + var fileLength = 2L * 1024L * 1024L * 1024L; // 2MB - // Configure validation service to return 1GB storage (cloud non-premium default) + // Act & Assert + var exception = await Assert.ThrowsAsync(() => + _nonAnonymousSendCommand.SaveFileSendAsync(send, fileData, fileLength)); + + Assert.Contains("Max file size is ", exception.Message); + + // Verify no further methods were called + await _sendValidationService.DidNotReceive().StorageRemainingForSendAsync(Arg.Any()); + await _sendRepository.DidNotReceive().CreateAsync(Arg.Any()); + await _sendRepository.DidNotReceive().UpsertAsync(Arg.Any()); + await _sendFileStorageService.DidNotReceive().GetSendFileUploadUrlAsync(Arg.Any(), Arg.Any()); + await _pushNotificationService.DidNotReceive().PushSyncSendCreateAsync(Arg.Any()); + await _pushNotificationService.DidNotReceive().PushSyncSendUpdateAsync(Arg.Any()); + } + + [Fact] + public async Task SaveFileSendAsync_UserCanAccessPremium_IsNotPremium_IsNotSelfHosted_NotEnoughSpace_ThrowsBadRequest() + { + // Arrange + var userId = Guid.NewGuid(); + var send = new Send + { + Id = Guid.NewGuid(), + Type = SendType.File, + UserId = userId + }; + var fileData = new SendFileData(); + var fileLength = 2L * 1024L * 1024L; // 2MB + + // Configure validation service to return 1 MB storage remaining _sendValidationService.StorageRemainingForSendAsync(send) - .Returns(1L * 1024L * 1024L * 1024L); // 1GB remaining (cloud default) + .Returns(1L * 1024L * 1024L); // Act & Assert var exception = await Assert.ThrowsAsync(() => @@ -756,7 +789,7 @@ public class NonAnonymousSendCommandTests UserId = null }; var fileData = new SendFileData(); - var fileLength = 2L * 1024L * 1024L * 1024L; // 2GB + var fileLength = 2L * 1024L * 1024L; // 2 MB // Configure validation service to throw BadRequest when checking storage for org without storage _sendValidationService.StorageRemainingForSendAsync(send) @@ -792,11 +825,10 @@ public class NonAnonymousSendCommandTests UserId = null }; var fileData = new SendFileData(); - var fileLength = 2L * 1024L * 1024L * 1024L; // 2GB + var fileLength = 2L * 1024L * 1024L; // 2 MB - // Configure validation service to return 1GB storage (org's max storage limit) _sendValidationService.StorageRemainingForSendAsync(send) - .Returns(1L * 1024L * 1024L * 1024L); // 1GB remaining + .Returns(1L * 1024L * 1024L); // 1 MB remaining // Act & Assert var exception = await Assert.ThrowsAsync(() => @@ -980,7 +1012,7 @@ public class NonAnonymousSendCommandTests }; // Setup validation to succeed - _sendFileStorageService.ValidateFileAsync(send, sendFileData.Id, sendFileData.Size, SendFileSettingHelper.FILE_SIZE_LEEWAY).Returns((true, sendFileData.Size)); + _sendFileStorageService.ValidateFileAsync(send, sendFileData.Id, Arg.Any(), Arg.Any()).Returns((true, sendFileData.Size)); // Act await _nonAnonymousSendCommand.UploadFileToExistingSendAsync(stream, send); @@ -1014,7 +1046,7 @@ public class NonAnonymousSendCommandTests Data = JsonSerializer.Serialize(sendFileData) }; - _sendFileStorageService.ValidateFileAsync(send, sendFileData.Id, sendFileData.Size, SendFileSettingHelper.FILE_SIZE_LEEWAY).Returns((true, sendFileData.Size)); + _sendFileStorageService.ValidateFileAsync(send, sendFileData.Id, Arg.Any(), Arg.Any()).Returns((true, sendFileData.Size)); // Act await _nonAnonymousSendCommand.UploadFileToExistingSendAsync(stream, send);