From a94e58c83eb7baed681e4ce99926ec5ff2adcc15 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 22 Oct 2025 08:16:25 +0000 Subject: [PATCH] Add path validation to prevent command injection in Process.Start calls Co-authored-by: Kvarkas <3611964+Kvarkas@users.noreply.github.com> --- mRemoteNG/Tools/ExternalTool.cs | 95 ++++++++++++++++++++++++++-- mRemoteNG/Tools/PathValidator.cs | 57 +++++++++++++++++ mRemoteNG/Tools/ProcessController.cs | 3 + 3 files changed, 148 insertions(+), 7 deletions(-) diff --git a/mRemoteNG/Tools/ExternalTool.cs b/mRemoteNG/Tools/ExternalTool.cs index d9297e36..09ebaf00 100644 --- a/mRemoteNG/Tools/ExternalTool.cs +++ b/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; + } + } + + /// + /// Splits command line arguments respecting quotes + /// + private static List SplitCommandLineArguments(string arguments) + { + List 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() diff --git a/mRemoteNG/Tools/PathValidator.cs b/mRemoteNG/Tools/PathValidator.cs index 5c440ae5..0df155c4 100644 --- a/mRemoteNG/Tools/PathValidator.cs +++ b/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); } + + /// + /// Validates that a file path is safe to execute and doesn't contain command injection characters + /// + /// The file path to validate + /// True if the path is safe to execute, false otherwise + 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; + } + + /// + /// Validates an executable file path and throws an exception if invalid + /// + /// The file path to validate + /// The name of the parameter being validated + /// Thrown when the path is not safe to execute + 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); + } } } diff --git a/mRemoteNG/Tools/ProcessController.cs b/mRemoteNG/Tools/ProcessController.cs index ecfa6921..87fbc4ff 100644 --- a/mRemoteNG/Tools/ProcessController.cs +++ b/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)