Merge pull request #2967 from mRemoteNG/copilot/fix-command-injection-vulnerability

Fix command injection vulnerabilities in Process.Start calls

mRemoteNG/Connection/Protocol/AnyDesk/ProtocolAnyDesk.cs

@@ -56,6 +56,9 @@ namespace mRemoteNG.Connection.Protocol.AnyDesk
                     return false;
                 }
 
+                // Validate the executable path to prevent command injection
+                PathValidator.ValidateExecutablePathOrThrow(anydeskPath, nameof(anydeskPath));
+
                 // Validate connection info
                 if (string.IsNullOrEmpty(_connectionInfo.Hostname))
                 {
@@ -296,7 +299,7 @@ namespace mRemoteNG.Connection.Protocol.AnyDesk
                     {
                         FileName = anydeskPath,
                         Arguments = arguments,
-                        UseShellExecute = true
+                        UseShellExecute = false // Changed to false for better security
                     },
                     EnableRaisingEvents = true
                 };

mRemoteNG/Connection/Protocol/IntegratedProgram.cs

@@ -67,13 +67,21 @@ namespace mRemoteNG.Connection.Protocol
                 }
 
                 ExternalToolArgumentParser argParser = new(_externalTool.ConnectionInfo);
+                string parsedFileName = argParser.ParseArguments(_externalTool.FileName);
+                string parsedArguments = argParser.ParseArguments(_externalTool.Arguments);
+
+                // Validate the executable path to prevent command injection
+                PathValidator.ValidateExecutablePathOrThrow(parsedFileName, nameof(_externalTool.FileName));
+
                 _process = new Process
                 {
                     StartInfo =
                     {
-                        UseShellExecute = true,
-                        FileName = argParser.ParseArguments(_externalTool.FileName),
-                        Arguments = argParser.ParseArguments(_externalTool.Arguments)
+                        // Use UseShellExecute = false for better security
+                        // Only use true if we need runas for elevation (which IntegratedProgram doesn't use)
+                        UseShellExecute = false,
+                        FileName = parsedFileName,
+                        Arguments = parsedArguments
                     },
                     EnableRaisingEvents = true
                 };

mRemoteNG/Connection/Protocol/PuttyBase.cs

@@ -81,6 +81,9 @@ namespace mRemoteNG.Connection.Protocol
             {
                 _isPuttyNg = PuttyTypeDetector.GetPuttyType() == PuttyTypeDetector.PuttyType.PuttyNg;
 
+                // Validate PuttyPath to prevent command injection
+                PathValidator.ValidateExecutablePathOrThrow(PuttyPath, nameof(PuttyPath));
+
                 PuttyProcess = new Process
                 {
                     StartInfo =

mRemoteNG/Tools/ExternalTool.cs

@@ -154,15 +154,96 @@ namespace mRemoteNG.Tools
         private void SetProcessProperties(Process process, ConnectionInfo startConnectionInfo)
         {
             ExternalToolArgumentParser argParser = new(startConnectionInfo);
-            process.StartInfo.UseShellExecute = true;
-            process.StartInfo.FileName = argParser.ParseArguments(FileName);
-            var parsedArgs = argParser.ParseArguments(Arguments).Split(' ');
-            foreach (var arg in parsedArgs)
+            string parsedFileName = argParser.ParseArguments(FileName);
+            
+            // Validate the executable path to prevent command injection
+            PathValidator.ValidateExecutablePathOrThrow(parsedFileName, nameof(FileName));
+            
+            // When RunElevated is true, we must use UseShellExecute = true for the "runas" verb
+            // When false, we use UseShellExecute = false for better security with ArgumentList
+            process.StartInfo.UseShellExecute = RunElevated;
+            process.StartInfo.FileName = parsedFileName;
+            
+            if (RunElevated)
             {
-                process.StartInfo.ArgumentList.Add(arg);
+                // With UseShellExecute = true, we must use Arguments property, not ArgumentList
+                // The argument parser already handles escaping properly
+                process.StartInfo.Arguments = argParser.ParseArguments(Arguments);
+                process.StartInfo.Verb = "runas";
             }
-            if (WorkingDir != "") process.StartInfo.WorkingDirectory = argParser.ParseArguments(WorkingDir);
-            if (RunElevated) process.StartInfo.Verb = "runas";
+            else
+            {
+                // With UseShellExecute = false, use ArgumentList for better security
+                // Parse arguments using CommandLineArguments for proper splitting
+                var cmdLineArgs = new Cmdline.CommandLineArguments { EscapeForShell = false };
+                string parsedArguments = argParser.ParseArguments(Arguments);
+                
+                // Split arguments respecting quotes
+                var argumentParts = SplitCommandLineArguments(parsedArguments);
+                foreach (var arg in argumentParts)
+                {
+                    if (!string.IsNullOrWhiteSpace(arg))
+                    {
+                        process.StartInfo.ArgumentList.Add(arg);
+                    }
+                }
+            }
+            
+            if (WorkingDir != "") 
+            {
+                string parsedWorkingDir = argParser.ParseArguments(WorkingDir);
+                PathValidator.ValidatePathOrThrow(parsedWorkingDir, nameof(WorkingDir));
+                process.StartInfo.WorkingDirectory = parsedWorkingDir;
+            }
+        }
+        
+        /// <summary>
+        /// Splits command line arguments respecting quotes
+        /// </summary>
+        private static List<string> SplitCommandLineArguments(string arguments)
+        {
+            List<string> result = new();
+            if (string.IsNullOrWhiteSpace(arguments))
+                return result;
+            
+            bool inQuotes = false;
+            int startIndex = 0;
+            
+            for (int i = 0; i < arguments.Length; i++)
+            {
+                char c = arguments[i];
+                
+                if (c == '"')
+                {
+                    inQuotes = !inQuotes;
+                }
+                else if (c == ' ' && !inQuotes)
+                {
+                    if (i > startIndex)
+                    {
+                        string arg = arguments.Substring(startIndex, i - startIndex).Trim();
+                        // Remove surrounding quotes if present
+                        if (arg.StartsWith("\"") && arg.EndsWith("\"") && arg.Length > 1)
+                            arg = arg.Substring(1, arg.Length - 2);
+                        if (!string.IsNullOrWhiteSpace(arg))
+                            result.Add(arg);
+                    }
+                    startIndex = i + 1;
+                }
+            }
+            
+            // Add the last argument
+            if (startIndex < arguments.Length)
+            {
+                string arg = arguments.Substring(startIndex).Trim();
+                // Remove surrounding quotes if present
+                if (arg.StartsWith("\"") && arg.EndsWith("\"") && arg.Length > 1)
+                    arg = arg.Substring(1, arg.Length - 2);
+                if (!string.IsNullOrWhiteSpace(arg))
+                    result.Add(arg);
+            }
+            
+            return result;
         }
 
         private void StartIntegrated()

mRemoteNG/Tools/PathValidator.cs

@@ -1,5 +1,6 @@
 using System;
 using System.IO;
+using System.Linq;
 using System.Runtime.Versioning;
 
 namespace mRemoteNG.Tools
@@ -42,5 +43,61 @@ namespace mRemoteNG.Tools
             if (!IsValidPath(path))
                 throw new ArgumentException("Invalid file path: path traversal sequences are not allowed", parameterName);
         }
+
+        /// <summary>
+        /// Validates that a file path is safe to execute and doesn't contain command injection characters
+        /// </summary>
+        /// <param name="filePath">The file path to validate</param>
+        /// <returns>True if the path is safe to execute, false otherwise</returns>
+        public static bool IsValidExecutablePath(string filePath)
+        {
+            if (string.IsNullOrWhiteSpace(filePath))
+                return false;
+
+            // First check basic path validation
+            if (!IsValidPath(filePath))
+                return false;
+
+            // Check for shell metacharacters that could be used for command injection
+            // These characters are dangerous when UseShellExecute is true
+            char[] dangerousChars = ['&', '|', ';', '<', '>', '(', ')', '^', '\n', '\r'];
+            if (filePath.Any(c => dangerousChars.Contains(c)))
+                return false;
+
+            // Check for multiple consecutive quotes which could be used to break out of quoting
+            if (filePath.Contains("\"\"") || filePath.Contains("''"))
+                return false;
+
+            try
+            {
+                // Validate that the path doesn't contain invalid path characters
+                string fileName = Path.GetFileName(filePath);
+                if (string.IsNullOrWhiteSpace(fileName))
+                    return false;
+
+                // Check if path contains invalid characters
+                char[] invalidChars = Path.GetInvalidPathChars();
+                if (filePath.Any(c => invalidChars.Contains(c)))
+                    return false;
+            }
+            catch (ArgumentException)
+            {
+                return false;
+            }
+
+            return true;
+        }
+
+        /// <summary>
+        /// Validates an executable file path and throws an exception if invalid
+        /// </summary>
+        /// <param name="filePath">The file path to validate</param>
+        /// <param name="parameterName">The name of the parameter being validated</param>
+        /// <exception cref="ArgumentException">Thrown when the path is not safe to execute</exception>
+        public static void ValidateExecutablePathOrThrow(string filePath, string parameterName = "filePath")
+        {
+            if (!IsValidExecutablePath(filePath))
+                throw new ArgumentException("Invalid executable path: path contains potentially dangerous characters or sequences", parameterName);
+        }
     }
 }

mRemoteNG/Tools/ProcessController.cs

@@ -16,6 +16,9 @@ namespace mRemoteNG.Tools
 
         public bool Start(string fileName, CommandLineArguments arguments = null)
         {
+            // Validate the executable path to prevent command injection
+            PathValidator.ValidateExecutablePathOrThrow(fileName, nameof(fileName));
+            
             Process.StartInfo.UseShellExecute = false;
             Process.StartInfo.FileName = fileName;
             if (arguments != null)

mRemoteNGTests/Tools/ExternalToolTests.cs

@@ -111,5 +111,136 @@ namespace mRemoteNGTests.Tools
             Assert.That(arguments, Does.Contain("TestPass"));
             Assert.That(arguments, Does.Contain("123"));
         }
+
+        [Test]
+        public void ArgumentsWithSpaces_AreParsedCorrectly()
+        {
+            // Arrange
+            var connectionInfo = new ConnectionInfo
+            {
+                Hostname = "test host",
+                Username = "user name"
+            };
+
+            var externalTool = new ExternalTool
+            {
+                DisplayName = "Test Tool",
+                FileName = "app.exe",
+                Arguments = "--host \"%HOSTNAME%\" --user \"%USERNAME%\""
+            };
+
+            // Act
+            var process = new Process();
+            var setProcessPropertiesMethod = typeof(ExternalTool).GetMethod(
+                "SetProcessProperties",
+                BindingFlags.NonPublic | BindingFlags.Instance
+            );
+            setProcessPropertiesMethod?.Invoke(externalTool, new object[] { process, connectionInfo });
+
+            // Assert - When not elevated, arguments should be in ArgumentList
+            if (!externalTool.RunElevated)
+            {
+                Assert.That(process.StartInfo.ArgumentList.Count, Is.GreaterThan(0));
+                // Arguments with spaces should be preserved in ArgumentList
+                Assert.That(process.StartInfo.ArgumentList, Does.Contain("test host").Or.Contain("--host"));
+            }
+        }
+
+        [Test]
+        public void ValidExecutablePath_DoesNotThrow()
+        {
+            // Arrange
+            var connectionInfo = new ConnectionInfo();
+            var externalTool = new ExternalTool
+            {
+                DisplayName = "Test Tool",
+                FileName = "notepad.exe",
+                Arguments = ""
+            };
+
+            // Act & Assert
+            var process = new Process();
+            var setProcessPropertiesMethod = typeof(ExternalTool).GetMethod(
+                "SetProcessProperties",
+                BindingFlags.NonPublic | BindingFlags.Instance
+            );
+            
+            Assert.DoesNotThrow(() => setProcessPropertiesMethod?.Invoke(externalTool, new object[] { process, connectionInfo }));
+        }
+
+        [Test]
+        public void InvalidExecutablePath_ThrowsArgumentException()
+        {
+            // Arrange
+            var connectionInfo = new ConnectionInfo();
+            var externalTool = new ExternalTool
+            {
+                DisplayName = "Test Tool",
+                FileName = "notepad.exe & calc.exe", // Command injection attempt
+                Arguments = ""
+            };
+
+            // Act & Assert
+            var process = new Process();
+            var setProcessPropertiesMethod = typeof(ExternalTool).GetMethod(
+                "SetProcessProperties",
+                BindingFlags.NonPublic | BindingFlags.Instance
+            );
+
+            var ex = Assert.Throws<TargetInvocationException>(() => 
+                setProcessPropertiesMethod?.Invoke(externalTool, new object[] { process, connectionInfo }));
+            Assert.That(ex.InnerException, Is.TypeOf<ArgumentException>());
+        }
+
+        [Test]
+        public void RunElevated_UsesShellExecuteTrue()
+        {
+            // Arrange
+            var connectionInfo = new ConnectionInfo();
+            var externalTool = new ExternalTool
+            {
+                DisplayName = "Test Tool",
+                FileName = "notepad.exe",
+                Arguments = "--test",
+                RunElevated = true
+            };
+
+            // Act
+            var process = new Process();
+            var setProcessPropertiesMethod = typeof(ExternalTool).GetMethod(
+                "SetProcessProperties",
+                BindingFlags.NonPublic | BindingFlags.Instance
+            );
+            setProcessPropertiesMethod?.Invoke(externalTool, new object[] { process, connectionInfo });
+
+            // Assert
+            Assert.That(process.StartInfo.UseShellExecute, Is.True);
+            Assert.That(process.StartInfo.Verb, Is.EqualTo("runas"));
+        }
+
+        [Test]
+        public void RunNotElevated_UsesShellExecuteFalse()
+        {
+            // Arrange
+            var connectionInfo = new ConnectionInfo();
+            var externalTool = new ExternalTool
+            {
+                DisplayName = "Test Tool",
+                FileName = "notepad.exe",
+                Arguments = "--test",
+                RunElevated = false
+            };
+
+            // Act
+            var process = new Process();
+            var setProcessPropertiesMethod = typeof(ExternalTool).GetMethod(
+                "SetProcessProperties",
+                BindingFlags.NonPublic | BindingFlags.Instance
+            );
+            setProcessPropertiesMethod?.Invoke(externalTool, new object[] { process, connectionInfo });
+
+            // Assert
+            Assert.That(process.StartInfo.UseShellExecute, Is.False);
+        }
     }
 }

mRemoteNGTests/Tools/PathValidatorTests.cs

@@ -88,4 +88,161 @@ public class PathValidatorTests
         var exception = Assert.Throws<ArgumentException>(() => PathValidator.ValidatePathOrThrow(maliciousPath, "customParam"));
         Assert.That(exception.ParamName, Is.EqualTo("customParam"));
     }
+
+    #region IsValidExecutablePath Tests
+
+    [Test]
+    public void IsValidExecutablePath_ValidWindowsPath_ReturnsTrue()
+    {
+        string validPath = @"C:\Windows\System32\notepad.exe";
+        Assert.That(PathValidator.IsValidExecutablePath(validPath), Is.True);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_ValidRelativePath_ReturnsTrue()
+    {
+        string validPath = @"app.exe";
+        Assert.That(PathValidator.IsValidExecutablePath(validPath), Is.True);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithSpaces_ReturnsTrue()
+    {
+        string validPath = @"C:\Program Files\App\app.exe";
+        Assert.That(PathValidator.IsValidExecutablePath(validPath), Is.True);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithCommandInjectionAmpersand_ReturnsFalse()
+    {
+        string maliciousPath = @"notepad.exe & calc.exe";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithCommandInjectionPipe_ReturnsFalse()
+    {
+        string maliciousPath = @"notepad.exe | calc.exe";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithCommandInjectionSemicolon_ReturnsFalse()
+    {
+        string maliciousPath = @"notepad.exe; calc.exe";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithRedirection_ReturnsFalse()
+    {
+        string maliciousPath = @"notepad.exe > output.txt";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithInputRedirection_ReturnsFalse()
+    {
+        string maliciousPath = @"notepad.exe < input.txt";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithParentheses_ReturnsFalse()
+    {
+        string maliciousPath = @"notepad.exe (calc.exe)";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithCaret_ReturnsFalse()
+    {
+        string maliciousPath = @"notepad.exe^calc.exe";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithNewline_ReturnsFalse()
+    {
+        string maliciousPath = "notepad.exe\ncalc.exe";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithCarriageReturn_ReturnsFalse()
+    {
+        string maliciousPath = "notepad.exe\rcalc.exe";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithMultipleQuotes_ReturnsFalse()
+    {
+        string maliciousPath = @"notepad.exe """"";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithMultipleSingleQuotes_ReturnsFalse()
+    {
+        string maliciousPath = @"notepad.exe ''";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_EmptyPath_ReturnsFalse()
+    {
+        Assert.That(PathValidator.IsValidExecutablePath(""), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_NullPath_ReturnsFalse()
+    {
+        Assert.That(PathValidator.IsValidExecutablePath(null), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_WhitespacePath_ReturnsFalse()
+    {
+        Assert.That(PathValidator.IsValidExecutablePath("   "), Is.False);
+    }
+
+    [Test]
+    public void IsValidExecutablePath_PathWithTraversal_ReturnsFalse()
+    {
+        string maliciousPath = @"..\notepad.exe";
+        Assert.That(PathValidator.IsValidExecutablePath(maliciousPath), Is.False);
+    }
+
+    [Test]
+    public void ValidateExecutablePathOrThrow_ValidPath_DoesNotThrow()
+    {
+        string validPath = @"C:\Windows\System32\notepad.exe";
+        Assert.DoesNotThrow(() => PathValidator.ValidateExecutablePathOrThrow(validPath));
+    }
+
+    [Test]
+    public void ValidateExecutablePathOrThrow_PathWithCommandInjection_ThrowsArgumentException()
+    {
+        string maliciousPath = @"notepad.exe & calc.exe";
+        var exception = Assert.Throws<ArgumentException>(() => PathValidator.ValidateExecutablePathOrThrow(maliciousPath));
+        Assert.That(exception.Message, Does.Contain("dangerous characters"));
+    }
+
+    [Test]
+    public void ValidateExecutablePathOrThrow_PathWithTraversal_ThrowsArgumentException()
+    {
+        string maliciousPath = @"..\notepad.exe";
+        Assert.Throws<ArgumentException>(() => PathValidator.ValidateExecutablePathOrThrow(maliciousPath));
+    }
+
+    [Test]
+    public void ValidateExecutablePathOrThrow_WithCustomParameterName_IncludesParameterName()
+    {
+        string maliciousPath = @"notepad.exe & calc.exe";
+        var exception = Assert.Throws<ArgumentException>(() => PathValidator.ValidateExecutablePathOrThrow(maliciousPath, "executablePath"));
+        Assert.That(exception.ParamName, Is.EqualTo("executablePath"));
+    }
+
+    #endregion
 }