Add path validation to prevent path traversal attacks

Co-authored-by: Kvarkas <3611964+Kvarkas@users.noreply.github.com>

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)

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);
 

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<string>
+    public class FileDataProvider : IDataProvider<string>
     {
+        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;
             }

mRemoteNG/Tools/PathValidator.cs

@@ -0,0 +1,46 @@
+using System;
+using System.IO;
+using System.Runtime.Versioning;
+
+namespace mRemoteNG.Tools
+{
+    /// <summary>
+    /// Provides path validation to prevent path traversal attacks
+    /// </summary>
+    [SupportedOSPlatform("windows")]
+    public static class PathValidator
+    {
+        /// <summary>
+        /// Validates that a file path does not contain path traversal sequences
+        /// </summary>
+        /// <param name="path">The path to validate</param>
+        /// <returns>True if the path is safe, false otherwise</returns>
+        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;
+        }
+
+        /// <summary>
+        /// Validates a file path and throws an exception if invalid
+        /// </summary>
+        /// <param name="path">The path to validate</param>
+        /// <param name="parameterName">The name of the parameter being validated</param>
+        /// <exception cref="ArgumentException">Thrown when the path contains path traversal sequences</exception>
+        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);
+        }
+    }
+}

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<ArgumentException>(() => _fileBackupCreator.CreateBackupFile(maliciousPath));
+    }
+
+    [Test]
+    public void CreateBackupFile_WithForwardSlashTraversal_ThrowsArgumentException()
+    {
+        string maliciousPath = @"../../../etc/passwd";
+        Assert.Throws<ArgumentException>(() => _fileBackupCreator.CreateBackupFile(maliciousPath));
+    }
 }

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<ArgumentException>(() => _fileBackupPruner.PruneBackupFiles(maliciousPath, 5));
+    }
+
+    [Test]
+    public void PruneBackupFiles_WithForwardSlashTraversal_ThrowsArgumentException()
+    {
+        string maliciousPath = @"../../../etc/passwd";
+        Assert.Throws<ArgumentException>(() => _fileBackupPruner.PruneBackupFiles(maliciousPath, 5));
+    }
+
+    [Test]
+    public void PruneBackupFiles_WithValidPath_DoesNotThrow()
+    {
+        // Create the test file
+        File.WriteAllText(_testFilePath, "test");
+        
+        Assert.DoesNotThrow(() => _fileBackupPruner.PruneBackupFiles(_testFilePath, 5));
+    }
+}

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<ArgumentException>(() => new FileDataProvider(maliciousPath));
+    }
+
+    [Test]
+    public void FilePath_SetWithPathTraversal_ThrowsArgumentException()
+    {
+        string maliciousPath = @"..\..\..\Windows\System32\config.xml";
+        Assert.Throws<ArgumentException>(() => _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);
+    }
 }

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<ArgumentException>(() => PathValidator.ValidatePathOrThrow(maliciousPath));
+        Assert.That(exception.Message, Does.Contain("path traversal"));
+    }
+
+    [Test]
+    public void ValidatePathOrThrow_WithNullPath_ThrowsArgumentException()
+    {
+        Assert.Throws<ArgumentException>(() => PathValidator.ValidatePathOrThrow(null));
+    }
+
+    [Test]
+    public void ValidatePathOrThrow_WithCustomParameterName_IncludesParameterName()
+    {
+        string maliciousPath = @"..\..\..\test.xml";
+        var exception = Assert.Throws<ArgumentException>(() => PathValidator.ValidatePathOrThrow(maliciousPath, "customParam"));
+        Assert.That(exception.ParamName, Is.EqualTo("customParam"));
+    }
+}