From 7619ff8516a968d03b30e359ac0be02e2908038d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 16 Aug 2025 10:21:42 +0000 Subject: [PATCH 1/3] Initial plan From 7992959a15e645fd0f3e37051d775cbf24f8142d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 16 Aug 2025 10:40:43 +0000 Subject: [PATCH 2/3] Fix security vulnerabilities in encryption and file merge operations Co-authored-by: paul-fresquet <61119222+paul-fresquet@users.noreply.github.com> --- .../Controls/Encryptions/IMergerDecrypter.cs | 5 +-- .../Communications/Transfers/FileMerger.cs | 28 ++++++++++++++-- .../Services/Encryptions/MergerDecrypter.cs | 33 ++++++++++++++++--- .../Transfers/FileMergerTests.cs | 26 ++++++++++++++- 4 files changed, 81 insertions(+), 11 deletions(-) diff --git a/src/ByteSync.Client/Interfaces/Controls/Encryptions/IMergerDecrypter.cs b/src/ByteSync.Client/Interfaces/Controls/Encryptions/IMergerDecrypter.cs index 396350782..bd033368e 100644 --- a/src/ByteSync.Client/Interfaces/Controls/Encryptions/IMergerDecrypter.cs +++ b/src/ByteSync.Client/Interfaces/Controls/Encryptions/IMergerDecrypter.cs @@ -1,8 +1,9 @@ -using System.Threading.Tasks; +using System; +using System.Threading.Tasks; namespace ByteSync.Interfaces.Controls.Encryptions; -public interface IMergerDecrypter +public interface IMergerDecrypter : IDisposable { Task MergeAndDecrypt(); } \ No newline at end of file diff --git a/src/ByteSync.Client/Services/Communications/Transfers/FileMerger.cs b/src/ByteSync.Client/Services/Communications/Transfers/FileMerger.cs index 0aaf99782..98e861d11 100644 --- a/src/ByteSync.Client/Services/Communications/Transfers/FileMerger.cs +++ b/src/ByteSync.Client/Services/Communications/Transfers/FileMerger.cs @@ -1,5 +1,8 @@ using ByteSync.Interfaces.Controls.Encryptions; using System.Threading; +using System.Threading.Tasks; +using System.Collections.Generic; +using System.Security.Cryptography; using ByteSync.Business.Communications.Downloading; using ByteSync.Interfaces.Controls.Communications; @@ -31,15 +34,34 @@ public async Task MergeAsync(int partToMerge) { foreach (var mergerDecrypter in _mergerDecrypters) { - await mergerDecrypter.MergeAndDecrypt(); + try + { + await mergerDecrypter.MergeAndDecrypt(); + } + finally + { + mergerDecrypter.Dispose(); + } } _downloadTarget.RemoveMemoryStream(partToMerge); } - catch + catch (InvalidOperationException ex) { + // Log security-related exceptions without exposing sensitive details await _errorManager.SetOnErrorAsync(); - throw; + throw new InvalidOperationException("Encryption operation failed", ex); + } + catch (CryptographicException ex) + { + // Handle cryptographic failures securely + await _errorManager.SetOnErrorAsync(); + throw new InvalidOperationException("Cryptographic operation failed", ex); + } + catch (Exception ex) + { + await _errorManager.SetOnErrorAsync(); + throw new InvalidOperationException("Merge operation failed", ex); } } finally diff --git a/src/ByteSync.Client/Services/Encryptions/MergerDecrypter.cs b/src/ByteSync.Client/Services/Encryptions/MergerDecrypter.cs index 461c4c1c9..b9a1c85fe 100644 --- a/src/ByteSync.Client/Services/Encryptions/MergerDecrypter.cs +++ b/src/ByteSync.Client/Services/Encryptions/MergerDecrypter.cs @@ -6,10 +6,11 @@ using ByteSync.Common.Business.SharedFiles; using ByteSync.Interfaces.Controls.Encryptions; using ByteSync.Interfaces.Repositories; +using Microsoft.Extensions.Logging; namespace ByteSync.Services.Encryptions; -public class MergerDecrypter : IMergerDecrypter +public class MergerDecrypter : IMergerDecrypter, IDisposable { private readonly ICloudSessionConnectionRepository _cloudSessionConnectionRepository; private readonly ILogger _logger; @@ -31,11 +32,11 @@ public MergerDecrypter(string localPath, DownloadTarget downloadTarget, Cancella public SharedFileDefinition SharedFileDefinition { get; private set; } = null!; - private Aes Aes { get; set; } = null!; + private Aes? Aes { get; set; } private DownloadTarget DownloadTarget { get; set; } = null!; - private CancellationTokenSource CancellationTokenSource { get; set; } + private CancellationTokenSource CancellationTokenSource { get; set; } = null!; private void Initialize(string finalFile, DownloadTarget downloadTarget, CancellationTokenSource cancellationTokenSource) { @@ -45,7 +46,19 @@ private void Initialize(string finalFile, DownloadTarget downloadTarget, Cancell SharedFileDefinition = downloadTarget.SharedFileDefinition; Aes = Aes.Create(); - Aes.Key = _cloudSessionConnectionRepository.GetAesEncryptionKey()!; + + var encryptionKey = _cloudSessionConnectionRepository.GetAesEncryptionKey(); + if (encryptionKey == null) + { + throw new InvalidOperationException("Encryption key is not available"); + } + + if (SharedFileDefinition.IV == null || SharedFileDefinition.IV.Length == 0) + { + throw new InvalidOperationException("Invalid IV provided"); + } + + Aes.Key = encryptionKey; Aes.IV = SharedFileDefinition.IV; CancellationTokenSource = cancellationTokenSource; @@ -67,10 +80,15 @@ public async Task MergeAndDecrypt() { return; } + + if (Aes == null) + { + throw new InvalidOperationException("AES encryption not initialized"); + } await using var outStream = new FileStream(FinalFile, FileMode.Append); - var cryptoTransform = Aes.CreateDecryptor(Aes.Key, Aes.IV); + using var cryptoTransform = Aes.CreateDecryptor(Aes.Key, Aes.IV); await using var cryptoStream = new CryptoStream(outStream, cryptoTransform, CryptoStreamMode.Write); TotalReadFiles += 1; @@ -81,4 +99,9 @@ public async Task MergeAndDecrypt() memoryStream.Position = 0; await memoryStream.CopyToAsync(cryptoStream, CancellationTokenSource.Token); } + + public void Dispose() + { + Aes?.Dispose(); + } } \ No newline at end of file diff --git a/tests/ByteSync.Client.Tests/Services/Communications/Transfers/FileMergerTests.cs b/tests/ByteSync.Client.Tests/Services/Communications/Transfers/FileMergerTests.cs index 88fdfff64..727d73cbd 100644 --- a/tests/ByteSync.Client.Tests/Services/Communications/Transfers/FileMergerTests.cs +++ b/tests/ByteSync.Client.Tests/Services/Communications/Transfers/FileMergerTests.cs @@ -4,6 +4,7 @@ using ByteSync.Interfaces.Controls.Encryptions; using ByteSync.Services.Communications.Transfers; using FluentAssertions; +using System.Security.Cryptography; namespace ByteSync.Tests.Services.Communications.Transfers; @@ -25,6 +26,8 @@ public async Task MergeAsync_CallsAllDecryptersAndRemovesMemoryStreamAndNotifies await fileMerger.MergeAsync(42); decrypter1.Verify(d => d.MergeAndDecrypt(), Times.Once); decrypter2.Verify(d => d.MergeAndDecrypt(), Times.Once); + decrypter1.Verify(d => d.Dispose(), Times.Once); + decrypter2.Verify(d => d.Dispose(), Times.Once); } [Test] @@ -41,8 +44,29 @@ public async Task MergeAsync_OnError_CallsOnErrorAndThrows() downloadTarget, new SemaphoreSlim(1, 1) ); - await FluentActions.Invoking(async () => await fileMerger.MergeAsync(1)).Should().ThrowAsync(); + await FluentActions.Invoking(async () => await fileMerger.MergeAsync(1)).Should().ThrowAsync(); errorManager.Verify(e => e.SetOnErrorAsync(), Times.Once); + decrypter.Verify(d => d.Dispose(), Times.Once); + } + + [Test] + public async Task MergeAsync_OnCryptographicError_CallsOnErrorAndThrowsSecureException() + { + var decrypter = new Mock(); + decrypter.Setup(d => d.MergeAndDecrypt()).ThrowsAsync(new CryptographicException("crypto fail")); + var errorManager = new Mock(); + errorManager.Setup(e => e.SetOnErrorAsync()).Returns(Task.CompletedTask).Verifiable(); + var downloadTarget = new ByteSync.Business.Communications.Downloading.DownloadTarget(null!, null, new HashSet()); + var fileMerger = new FileMerger( + new List { decrypter.Object }, + errorManager.Object, + downloadTarget, + new SemaphoreSlim(1, 1) + ); + var exception = await FluentActions.Invoking(async () => await fileMerger.MergeAsync(1)).Should().ThrowAsync(); + exception.WithMessage("Cryptographic operation failed"); + errorManager.Verify(e => e.SetOnErrorAsync(), Times.Once); + decrypter.Verify(d => d.Dispose(), Times.Once); } } \ No newline at end of file From e257c539f9301037e510e950ddb32a7abd19786b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 16 Aug 2025 10:45:02 +0000 Subject: [PATCH 3/3] Comprehensive security improvements: prevent information disclosure via exception messages Co-authored-by: paul-fresquet <61119222+paul-fresquet@users.noreply.github.com> --- .../Services/Synchronizations/TemporaryFileManager.cs | 4 ++-- .../ViewModels/Home/CreateCloudSessionViewModel.cs | 4 +++- .../ViewModels/Home/JoinCloudSessionViewModel.cs | 4 +++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/ByteSync.Client/Services/Synchronizations/TemporaryFileManager.cs b/src/ByteSync.Client/Services/Synchronizations/TemporaryFileManager.cs index c73f7d294..1cbf1dea1 100644 --- a/src/ByteSync.Client/Services/Synchronizations/TemporaryFileManager.cs +++ b/src/ByteSync.Client/Services/Synchronizations/TemporaryFileManager.cs @@ -98,8 +98,8 @@ public void TryRevertOnError(Exception exception) { try { - _logger.LogWarning("An exception has occurred during validation operations on file {file}: {type} - {message}. Trying to revert operation", - DestinationFullName, exception.GetType().Name, exception.Message); + _logger.LogWarning("An exception has occurred during validation operations on file {file}: {type}. Trying to revert operation", + DestinationFullName, exception.GetType().Name); if (!HasValidationStarted) { // We try to delete the DestinationTemporaryPath file if it exists diff --git a/src/ByteSync.Client/ViewModels/Home/CreateCloudSessionViewModel.cs b/src/ByteSync.Client/ViewModels/Home/CreateCloudSessionViewModel.cs index 49e8ed620..02c3a277f 100644 --- a/src/ByteSync.Client/ViewModels/Home/CreateCloudSessionViewModel.cs +++ b/src/ByteSync.Client/ViewModels/Home/CreateCloudSessionViewModel.cs @@ -8,6 +8,7 @@ using ByteSync.Interfaces.Repositories; using ByteSync.Interfaces.Services.Sessions; using ByteSync.Interfaces.Services.Sessions.Connecting; +using Microsoft.Extensions.Logging; using ReactiveUI; using ReactiveUI.Fody.Helpers; using Unit = System.Reactive.Unit; @@ -122,7 +123,8 @@ private void UpdateErrorMessage(string? errorMessageSource, Exception? exception string errorMessage = _localizationService[ErrorMessageSource!]; if (exception != null) { - errorMessage += $" ({exception.Message})"; + _logger.LogError(exception, "Session creation error occurred"); + // Don't expose exception details to the user for security reasons } ErrorMessage = errorMessage; diff --git a/src/ByteSync.Client/ViewModels/Home/JoinCloudSessionViewModel.cs b/src/ByteSync.Client/ViewModels/Home/JoinCloudSessionViewModel.cs index 187b60313..06b4aa39d 100644 --- a/src/ByteSync.Client/ViewModels/Home/JoinCloudSessionViewModel.cs +++ b/src/ByteSync.Client/ViewModels/Home/JoinCloudSessionViewModel.cs @@ -10,6 +10,7 @@ using ByteSync.Interfaces.Repositories; using ByteSync.Interfaces.Services.Sessions.Connecting; using ByteSync.Interfaces.Services.Sessions.Connecting.Joining; +using Microsoft.Extensions.Logging; using ReactiveUI; using ReactiveUI.Fody.Helpers; @@ -143,7 +144,8 @@ private void UpdateErrorMessage(string? errorMessageSource, Exception? exception string errorMessage = _localizationService[ErrorMessageSource!]; if (exception != null) { - errorMessage += $" ({exception.Message})"; + _logger.LogError(exception, "Session join error occurred"); + // Don't expose exception details to the user for security reasons } ErrorMessage = errorMessage;