diff --git a/mRemoteNG/Config/DataProviders/FileBackupCreator.cs b/mRemoteNG/Config/DataProviders/FileBackupCreator.cs index d2335cb2..41f482da 100644 --- a/mRemoteNG/Config/DataProviders/FileBackupCreator.cs +++ b/mRemoteNG/Config/DataProviders/FileBackupCreator.cs @@ -4,6 +4,7 @@ using System.Runtime.Versioning; using mRemoteNG.App; using mRemoteNG.Messages; using mRemoteNG.Resources.Language; +using mRemoteNG.Tools; namespace mRemoteNG.Config.DataProviders { @@ -17,12 +18,13 @@ namespace mRemoteNG.Config.DataProviders if (WeDontNeedToBackup(fileName)) return; + PathValidator.ValidatePathOrThrow(fileName, nameof(fileName)); + string backupFileName = string.Format(Properties.OptionsBackupPage.Default.BackupFileNameFormat, fileName, DateTime.Now); - if (fileName == null || fileName.Contains("../") || fileName.Contains(@"..\")) - throw new ArgumentException("Invalid file path"); - if (backupFileName == null || backupFileName.Contains("../") || backupFileName.Contains(@"..\")) - throw new ArgumentException("Invalid file path"); + + PathValidator.ValidatePathOrThrow(backupFileName, nameof(backupFileName)); + File.Copy(fileName, backupFileName); } catch (Exception ex) diff --git a/mRemoteNG/Config/DataProviders/FileBackupPruner.cs b/mRemoteNG/Config/DataProviders/FileBackupPruner.cs index 8e6c3283..71fe57ac 100644 --- a/mRemoteNG/Config/DataProviders/FileBackupPruner.cs +++ b/mRemoteNG/Config/DataProviders/FileBackupPruner.cs @@ -1,5 +1,6 @@ using System.IO; using System.Linq; +using mRemoteNG.Tools; namespace mRemoteNG.Config.DataProviders { @@ -7,6 +8,8 @@ namespace mRemoteNG.Config.DataProviders { public void PruneBackupFiles(string filePath, int maxBackupsToKeep) { + PathValidator.ValidatePathOrThrow(filePath, nameof(filePath)); + string fileName = Path.GetFileName(filePath); string directoryName = Path.GetDirectoryName(filePath); diff --git a/mRemoteNG/Config/DataProviders/FileDataProvider.cs b/mRemoteNG/Config/DataProviders/FileDataProvider.cs index 42360edd..55ee4d97 100644 --- a/mRemoteNG/Config/DataProviders/FileDataProvider.cs +++ b/mRemoteNG/Config/DataProviders/FileDataProvider.cs @@ -2,14 +2,31 @@ using System.IO; using System.Runtime.Versioning; using mRemoteNG.App; +using mRemoteNG.Tools; namespace mRemoteNG.Config.DataProviders { [SupportedOSPlatform("windows")] - public class FileDataProvider(string filePath) : IDataProvider + public class FileDataProvider : IDataProvider { + private string _filePath; + [SupportedOSPlatform("windows")] - public string FilePath { get; set; } = filePath; + public string FilePath + { + get => _filePath; + set + { + PathValidator.ValidatePathOrThrow(value, nameof(FilePath)); + _filePath = value; + } + } + + public FileDataProvider(string filePath) + { + PathValidator.ValidatePathOrThrow(filePath, nameof(filePath)); + _filePath = filePath; + } public virtual string Load() { @@ -54,6 +71,7 @@ namespace mRemoteNG.Config.DataProviders { try { + PathValidator.ValidatePathOrThrow(newPath, nameof(newPath)); File.Move(FilePath, newPath); FilePath = newPath; } diff --git a/mRemoteNG/Tools/PathValidator.cs b/mRemoteNG/Tools/PathValidator.cs new file mode 100644 index 00000000..5c440ae5 --- /dev/null +++ b/mRemoteNG/Tools/PathValidator.cs @@ -0,0 +1,46 @@ +using System; +using System.IO; +using System.Runtime.Versioning; + +namespace mRemoteNG.Tools +{ + /// + /// Provides path validation to prevent path traversal attacks + /// + [SupportedOSPlatform("windows")] + public static class PathValidator + { + /// + /// Validates that a file path does not contain path traversal sequences + /// + /// The path to validate + /// True if the path is safe, false otherwise + public static bool IsValidPath(string path) + { + if (string.IsNullOrEmpty(path)) + return false; + + // Check for path traversal sequences + if (path.Contains("../") || path.Contains("..\\")) + return false; + + // Also check for encoded versions of path traversal + if (path.Contains("%2e%2e") || path.Contains("%2E%2E")) + return false; + + return true; + } + + /// + /// Validates a file path and throws an exception if invalid + /// + /// The path to validate + /// The name of the parameter being validated + /// Thrown when the path contains path traversal sequences + public static void ValidatePathOrThrow(string path, string parameterName = "path") + { + if (!IsValidPath(path)) + throw new ArgumentException("Invalid file path: path traversal sequences are not allowed", parameterName); + } + } +} diff --git a/mRemoteNGTests/Config/DataProviders/FileBackupCreatorTests.cs b/mRemoteNGTests/Config/DataProviders/FileBackupCreatorTests.cs index 56a0a1aa..31abc493 100644 --- a/mRemoteNGTests/Config/DataProviders/FileBackupCreatorTests.cs +++ b/mRemoteNGTests/Config/DataProviders/FileBackupCreatorTests.cs @@ -2,6 +2,7 @@ using mRemoteNG.Config.DataProviders; using mRemoteNGTests.TestHelpers; using NUnit.Framework; +using System; namespace mRemoteNGTests.Config.DataProviders; @@ -46,4 +47,18 @@ public class FileBackupCreatorTests var backupFileExists = File.Exists(_testFilePathBackup); Assert.That(backupFileExists, Is.False); } + + [Test] + public void CreateBackupFile_WithPathTraversal_ThrowsArgumentException() + { + string maliciousPath = @"..\..\..\Windows\System32\config.xml"; + Assert.Throws(() => _fileBackupCreator.CreateBackupFile(maliciousPath)); + } + + [Test] + public void CreateBackupFile_WithForwardSlashTraversal_ThrowsArgumentException() + { + string maliciousPath = @"../../../etc/passwd"; + Assert.Throws(() => _fileBackupCreator.CreateBackupFile(maliciousPath)); + } } \ No newline at end of file diff --git a/mRemoteNGTests/Config/DataProviders/FileBackupPrunerTests.cs b/mRemoteNGTests/Config/DataProviders/FileBackupPrunerTests.cs new file mode 100644 index 00000000..d5f275e1 --- /dev/null +++ b/mRemoteNGTests/Config/DataProviders/FileBackupPrunerTests.cs @@ -0,0 +1,52 @@ +using System; +using System.IO; +using mRemoteNG.Config.DataProviders; +using mRemoteNGTests.TestHelpers; +using NUnit.Framework; + +namespace mRemoteNGTests.Config.DataProviders; + +public class FileBackupPrunerTests +{ + private FileBackupPruner _fileBackupPruner; + private string _testFilePath; + private string _testFileDirectory; + + [SetUp] + public void Setup() + { + _testFilePath = FileTestHelpers.NewTempFilePath(); + _testFileDirectory = Path.GetDirectoryName(_testFilePath); + _fileBackupPruner = new FileBackupPruner(); + } + + [TearDown] + public void Teardown() + { + if (Directory.Exists(_testFileDirectory)) + Directory.Delete(_testFileDirectory, true); + } + + [Test] + public void PruneBackupFiles_WithPathTraversal_ThrowsArgumentException() + { + string maliciousPath = @"..\..\..\Windows\System32\config.xml"; + Assert.Throws(() => _fileBackupPruner.PruneBackupFiles(maliciousPath, 5)); + } + + [Test] + public void PruneBackupFiles_WithForwardSlashTraversal_ThrowsArgumentException() + { + string maliciousPath = @"../../../etc/passwd"; + Assert.Throws(() => _fileBackupPruner.PruneBackupFiles(maliciousPath, 5)); + } + + [Test] + public void PruneBackupFiles_WithValidPath_DoesNotThrow() + { + // Create the test file + File.WriteAllText(_testFilePath, "test"); + + Assert.DoesNotThrow(() => _fileBackupPruner.PruneBackupFiles(_testFilePath, 5)); + } +} diff --git a/mRemoteNGTests/Config/DataProviders/FileDataProviderTests.cs b/mRemoteNGTests/Config/DataProviders/FileDataProviderTests.cs index 9301a0f5..e9aea5d5 100644 --- a/mRemoteNGTests/Config/DataProviders/FileDataProviderTests.cs +++ b/mRemoteNGTests/Config/DataProviders/FileDataProviderTests.cs @@ -56,4 +56,32 @@ public class FileDataProviderTests _dataProvider.Save(""); Assert.That(File.Exists(fileThatShouldExist), Is.True); } + + [Test] + public void Constructor_WithPathTraversal_ThrowsArgumentException() + { + string maliciousPath = @"C:\Users\..\..\..\Windows\System32\config.xml"; + Assert.Throws(() => new FileDataProvider(maliciousPath)); + } + + [Test] + public void FilePath_SetWithPathTraversal_ThrowsArgumentException() + { + string maliciousPath = @"..\..\..\Windows\System32\config.xml"; + Assert.Throws(() => _dataProvider.FilePath = maliciousPath); + } + + [Test] + public void MoveTo_WithPathTraversal_ThrowsArgumentException() + { + string maliciousPath = @"..\..\..\Windows\System32\config.xml"; + // The method catches the exception internally, so we need to verify it doesn't move the file + _dataProvider.Save("test"); + _dataProvider.MoveTo(maliciousPath); + + // Verify the file wasn't moved to the malicious path + Assert.That(File.Exists(maliciousPath), Is.False); + // Verify the original file still exists + Assert.That(File.Exists(_testFilePath), Is.True); + } } \ No newline at end of file diff --git a/mRemoteNGTests/Tools/PathValidatorTests.cs b/mRemoteNGTests/Tools/PathValidatorTests.cs new file mode 100644 index 00000000..7874a268 --- /dev/null +++ b/mRemoteNGTests/Tools/PathValidatorTests.cs @@ -0,0 +1,91 @@ +using System; +using mRemoteNG.Tools; +using NUnit.Framework; + +namespace mRemoteNGTests.Tools; + +public class PathValidatorTests +{ + [Test] + public void ValidPath_ReturnsTrue() + { + string validPath = @"C:\Users\TestUser\Documents\test.xml"; + Assert.That(PathValidator.IsValidPath(validPath), Is.True); + } + + [Test] + public void PathWithForwardSlashTraversal_ReturnsFalse() + { + string maliciousPath = @"C:\Users\TestUser\Documents\..\..\..\Windows\System32\test.xml"; + Assert.That(PathValidator.IsValidPath(maliciousPath), Is.False); + } + + [Test] + public void PathWithBackslashTraversal_ReturnsFalse() + { + string maliciousPath = @"C:\Users\TestUser\Documents\..\..\test.xml"; + Assert.That(PathValidator.IsValidPath(maliciousPath), Is.False); + } + + [Test] + public void PathWithMixedTraversal_ReturnsFalse() + { + string maliciousPath = @"C:\Users\TestUser\Documents\.././..\test.xml"; + Assert.That(PathValidator.IsValidPath(maliciousPath), Is.False); + } + + [Test] + public void PathWithEncodedTraversal_ReturnsFalse() + { + string maliciousPath = @"C:\Users\TestUser\Documents\%2e%2e\test.xml"; + Assert.That(PathValidator.IsValidPath(maliciousPath), Is.False); + } + + [Test] + public void PathWithUppercaseEncodedTraversal_ReturnsFalse() + { + string maliciousPath = @"C:\Users\TestUser\Documents\%2E%2E\test.xml"; + Assert.That(PathValidator.IsValidPath(maliciousPath), Is.False); + } + + [Test] + public void NullPath_ReturnsFalse() + { + Assert.That(PathValidator.IsValidPath(null), Is.False); + } + + [Test] + public void EmptyPath_ReturnsFalse() + { + Assert.That(PathValidator.IsValidPath(""), Is.False); + } + + [Test] + public void ValidatePathOrThrow_WithValidPath_DoesNotThrow() + { + string validPath = @"C:\Users\TestUser\Documents\test.xml"; + Assert.DoesNotThrow(() => PathValidator.ValidatePathOrThrow(validPath)); + } + + [Test] + public void ValidatePathOrThrow_WithTraversalPath_ThrowsArgumentException() + { + string maliciousPath = @"C:\Users\TestUser\Documents\..\..\..\test.xml"; + var exception = Assert.Throws(() => PathValidator.ValidatePathOrThrow(maliciousPath)); + Assert.That(exception.Message, Does.Contain("path traversal")); + } + + [Test] + public void ValidatePathOrThrow_WithNullPath_ThrowsArgumentException() + { + Assert.Throws(() => PathValidator.ValidatePathOrThrow(null)); + } + + [Test] + public void ValidatePathOrThrow_WithCustomParameterName_IncludesParameterName() + { + string maliciousPath = @"..\..\..\test.xml"; + var exception = Assert.Throws(() => PathValidator.ValidatePathOrThrow(maliciousPath, "customParam")); + Assert.That(exception.ParamName, Is.EqualTo("customParam")); + } +}