diff --git a/mRemoteNG/Connection/Protocol/AnyDesk/ProtocolAnyDesk.cs b/mRemoteNG/Connection/Protocol/AnyDesk/ProtocolAnyDesk.cs index 8a7a00a66..d3f2e1fa6 100644 --- a/mRemoteNG/Connection/Protocol/AnyDesk/ProtocolAnyDesk.cs +++ b/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 }; diff --git a/mRemoteNG/Connection/Protocol/IntegratedProgram.cs b/mRemoteNG/Connection/Protocol/IntegratedProgram.cs index d6c8f0c0e..c350b2623 100644 --- a/mRemoteNG/Connection/Protocol/IntegratedProgram.cs +++ b/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 }; diff --git a/mRemoteNG/Connection/Protocol/PuttyBase.cs b/mRemoteNG/Connection/Protocol/PuttyBase.cs index 13d33e662..bcce8d03b 100644 --- a/mRemoteNG/Connection/Protocol/PuttyBase.cs +++ b/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 = diff --git a/mRemoteNG/Tools/ExternalTool.cs b/mRemoteNG/Tools/ExternalTool.cs index d9297e36c..09ebaf001 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 5c440ae5d..0df155c45 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 ecfa6921f..87fbc4ffa 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) diff --git a/mRemoteNGTests/Tools/ExternalToolTests.cs b/mRemoteNGTests/Tools/ExternalToolTests.cs index 7334c0243..2ae5fff76 100644 --- a/mRemoteNGTests/Tools/ExternalToolTests.cs +++ b/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(() => + setProcessPropertiesMethod?.Invoke(externalTool, new object[] { process, connectionInfo })); + Assert.That(ex.InnerException, Is.TypeOf()); + } + + [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); + } } } diff --git a/mRemoteNGTests/Tools/PathValidatorTests.cs b/mRemoteNGTests/Tools/PathValidatorTests.cs index 7874a268f..8afdf6dff 100644 --- a/mRemoteNGTests/Tools/PathValidatorTests.cs +++ b/mRemoteNGTests/Tools/PathValidatorTests.cs @@ -88,4 +88,161 @@ public class PathValidatorTests var exception = Assert.Throws(() => 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(() => PathValidator.ValidateExecutablePathOrThrow(maliciousPath)); + Assert.That(exception.Message, Does.Contain("dangerous characters")); + } + + [Test] + public void ValidateExecutablePathOrThrow_PathWithTraversal_ThrowsArgumentException() + { + string maliciousPath = @"..\notepad.exe"; + Assert.Throws(() => PathValidator.ValidateExecutablePathOrThrow(maliciousPath)); + } + + [Test] + public void ValidateExecutablePathOrThrow_WithCustomParameterName_IncludesParameterName() + { + string maliciousPath = @"notepad.exe & calc.exe"; + var exception = Assert.Throws(() => PathValidator.ValidateExecutablePathOrThrow(maliciousPath, "executablePath")); + Assert.That(exception.ParamName, Is.EqualTo("executablePath")); + } + + #endregion }