diff --git a/mRemoteNG/App/Shutdown.cs b/mRemoteNG/App/Shutdown.cs index f577c8f46..c27dd238d 100644 --- a/mRemoteNG/App/Shutdown.cs +++ b/mRemoteNG/App/Shutdown.cs @@ -121,7 +121,11 @@ namespace mRemoteNG.App private static void RunUpdateFile() { if (UpdatePending) + { + // Validate the update file path to prevent command injection + Tools.PathValidator.ValidateExecutablePathOrThrow(_updateFilePath, nameof(_updateFilePath)); Process.Start(new ProcessStartInfo(_updateFilePath) { UseShellExecute = true }); + } } } } \ No newline at end of file diff --git a/mRemoteNG/Connection/Protocol/AnyDesk/ProtocolAnyDesk.cs b/mRemoteNG/Connection/Protocol/AnyDesk/ProtocolAnyDesk.cs index e6df6a9f5..26167c79b 100644 --- a/mRemoteNG/Connection/Protocol/AnyDesk/ProtocolAnyDesk.cs +++ b/mRemoteNG/Connection/Protocol/AnyDesk/ProtocolAnyDesk.cs @@ -211,7 +211,17 @@ namespace mRemoteNG.Connection.Protocol.AnyDesk // Username field is optional and not used in the CLI (reserved for future use) // Password field is piped via stdin when --with-password flag is used string anydeskId = _connectionInfo.Hostname.Trim(); - string arguments = $"{anydeskId}"; + + // Validate AnyDesk ID to prevent command injection + // AnyDesk IDs are numeric or alphanumeric with @ and - characters for aliases + if (!IsValidAnydeskId(anydeskId)) + { + Runtime.MessageCollector?.AddMessage(MessageClass.ErrorMsg, + "Invalid AnyDesk ID format. Only alphanumeric characters, @, -, _, and . are allowed.", true); + return false; + } + + string arguments = anydeskId; // Add --with-password flag if password is provided bool hasPassword = !string.IsNullOrEmpty(_connectionInfo.Password); @@ -242,27 +252,45 @@ namespace mRemoteNG.Connection.Protocol.AnyDesk return false; } } + + private bool IsValidAnydeskId(string anydeskId) + { + if (string.IsNullOrWhiteSpace(anydeskId)) + return false; + + // AnyDesk IDs can be: + // - Pure numeric (e.g., 123456789) + // - Alphanumeric with @ for aliases (e.g., alias@ad) + // - May contain hyphens and underscores in aliases + // Reject any characters that could be used for command injection + foreach (char c in anydeskId) + { + if (!char.IsLetterOrDigit(c) && c != '@' && c != '-' && c != '_' && c != '.') + { + return false; + } + } + + return true; + } private bool StartAnydeskWithPassword(string anydeskPath, string arguments) { try { - // Use PowerShell to pipe the password to AnyDesk - // This is the recommended way according to AnyDesk documentation - string escapedPassword = _connectionInfo.Password.Replace("'", "''"); - string powershellCommand = $"echo '{escapedPassword}' | & '{anydeskPath}' {arguments}"; - + // Start AnyDesk and pipe the password directly to stdin + // This avoids command injection vulnerabilities from using PowerShell _process = new Process { StartInfo = new ProcessStartInfo { - FileName = "powershell.exe", - Arguments = $"-WindowStyle Hidden -Command \"{powershellCommand}\"", + FileName = anydeskPath, + Arguments = arguments, UseShellExecute = false, - CreateNoWindow = true, + CreateNoWindow = false, RedirectStandardOutput = false, RedirectStandardError = false, - RedirectStandardInput = false + RedirectStandardInput = true }, EnableRaisingEvents = true }; @@ -270,8 +298,23 @@ namespace mRemoteNG.Connection.Protocol.AnyDesk _process.Exited += ProcessExited; _process.Start(); + // Write the password to stdin and close the stream + // AnyDesk expects the password on stdin when --with-password is used + try + { + if (_process.StandardInput != null) + { + _process.StandardInput.WriteLine(_connectionInfo.Password); + _process.StandardInput.Close(); + } + } + catch (Exception ex) + { + Runtime.MessageCollector?.AddMessage(MessageClass.WarningMsg, + $"Failed to send password to AnyDesk: {ex.Message}", true); + } + // Wait for the AnyDesk window to appear - // Note: The window belongs to the AnyDesk process, not PowerShell if (!WaitForAnydeskWindow()) { Runtime.MessageCollector?.AddMessage(MessageClass.WarningMsg, diff --git a/mRemoteNG/UI/Forms/FrmAbout.cs b/mRemoteNG/UI/Forms/FrmAbout.cs index 06e8be286..a884c35ed 100644 --- a/mRemoteNG/UI/Forms/FrmAbout.cs +++ b/mRemoteNG/UI/Forms/FrmAbout.cs @@ -1,4 +1,5 @@ -using System.Diagnostics; +using System; +using System.Diagnostics; using System.Windows.Forms; using mRemoteNG.App.Info; using mRemoteNG.Themes; @@ -77,29 +78,74 @@ namespace mRemoteNG.UI.Forms private void OpenUrl(string url) { + // Validate URL format to prevent injection + if (string.IsNullOrWhiteSpace(url)) + return; + + // Basic URL validation - ensure it starts with http:// or https:// + if (!url.StartsWith("http://", StringComparison.OrdinalIgnoreCase) && + !url.StartsWith("https://", StringComparison.OrdinalIgnoreCase)) + { + // Invalid URL format - don't try to open it + return; + } + try { - Process.Start(url); + // Use the standard .NET approach for opening URLs securely + // UseShellExecute=true delegates to the OS default handler + var startInfo = new ProcessStartInfo + { + FileName = url, + UseShellExecute = true + }; + Process.Start(startInfo); } catch { - // hack because of this: https://github.com/dotnet/corefx/issues/10361 - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + // Fallback for older .NET Core versions with bug: https://github.com/dotnet/corefx/issues/10361 + // Use platform-specific URL launchers + try { - url = url.Replace("&", "^&"); - Process.Start(new ProcessStartInfo("cmd", $"/c start {url}") { CreateNoWindow = true }); + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + // Use rundll32 with url.dll as fallback + var startInfo = new ProcessStartInfo + { + FileName = "rundll32.exe", + UseShellExecute = false, + CreateNoWindow = true + }; + startInfo.ArgumentList.Add("url.dll,FileProtocolHandler"); + startInfo.ArgumentList.Add(url); + Process.Start(startInfo); + } + else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + { + var startInfo = new ProcessStartInfo + { + FileName = "xdg-open", + UseShellExecute = false + }; + startInfo.ArgumentList.Add(url); + Process.Start(startInfo); + } + else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) + { + var startInfo = new ProcessStartInfo + { + FileName = "open", + UseShellExecute = false + }; + startInfo.ArgumentList.Add(url); + Process.Start(startInfo); + } } - else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + catch { - Process.Start("xdg-open", url); - } - else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) - { - Process.Start("open", url); - } - else - { - throw; + // Unable to open URL - notify the user + Runtime.MessageCollector?.AddMessage(MessageClass.WarningMsg, + "Unable to open URL in browser. Please open manually: " + url, true); } } } diff --git a/mRemoteNG/UI/Forms/FrmUnhandledException.cs b/mRemoteNG/UI/Forms/FrmUnhandledException.cs index c42e27cd2..8f4310600 100644 --- a/mRemoteNG/UI/Forms/FrmUnhandledException.cs +++ b/mRemoteNG/UI/Forms/FrmUnhandledException.cs @@ -91,7 +91,12 @@ namespace mRemoteNG.UI.Forms private void buttonCreateBug_Click(object sender, EventArgs e) { - Process.Start(GeneralAppInfo.UrlBugs); + var startInfo = new ProcessStartInfo + { + FileName = GeneralAppInfo.UrlBugs, + UseShellExecute = true + }; + Process.Start(startInfo); } } } \ No newline at end of file diff --git a/mRemoteNG/UI/Forms/OptionsPages/NotificationsPage.cs b/mRemoteNG/UI/Forms/OptionsPages/NotificationsPage.cs index a2701cf74..2c1948729 100644 --- a/mRemoteNG/UI/Forms/OptionsPages/NotificationsPage.cs +++ b/mRemoteNG/UI/Forms/OptionsPages/NotificationsPage.cs @@ -7,6 +7,7 @@ using mRemoteNG.App; using mRemoteNG.Config.Settings.Registry; using mRemoteNG.Properties; using mRemoteNG.Resources.Language; +using mRemoteNG.Tools; namespace mRemoteNG.UI.Forms.OptionsPages { @@ -342,8 +343,17 @@ namespace mRemoteNG.UI.Forms.OptionsPages { try { + // Validate path to prevent command injection + PathValidator.ValidatePathOrThrow(path, nameof(path)); + // Open the file using the default application associated with its file type based on the user's preference - Process.Start(path); + // Use ProcessStartInfo with UseShellExecute for better control + var startInfo = new ProcessStartInfo + { + FileName = path, + UseShellExecute = true + }; + Process.Start(startInfo); return true; } catch @@ -362,9 +372,19 @@ namespace mRemoteNG.UI.Forms.OptionsPages { try { + // Validate path to prevent command injection + PathValidator.ValidatePathOrThrow(path, nameof(path)); + // Open it in "Notepad" (Windows default editor). // Usually available on all Windows systems - Process.Start("notepad.exe", path); + // Use ProcessStartInfo with ArgumentList for better security + var startInfo = new ProcessStartInfo + { + FileName = "notepad.exe", + UseShellExecute = false + }; + startInfo.ArgumentList.Add(path); + Process.Start(startInfo); return true; } catch @@ -383,11 +403,22 @@ namespace mRemoteNG.UI.Forms.OptionsPages { try { + // Validate path to prevent command injection + PathValidator.ValidatePathOrThrow(path, nameof(path)); + // when all fails open filelocation to logfile... // Open Windows Explorer to the directory containing the file - Process.Start("explorer.exe", $"/select,\"{path}\""); - return true; - } + // Explorer expects /select,"path" as a single argument + var startInfo = new ProcessStartInfo + { + FileName = "explorer.exe", + UseShellExecute = false + }; + startInfo.ArgumentList.Add("/select,"); + startInfo.ArgumentList.Add(path); + Process.Start(startInfo); + return true; + } catch { // If necessary, the error can be logged here. diff --git a/mRemoteNG/UI/Menu/msMain/HelpMenu.cs b/mRemoteNG/UI/Menu/msMain/HelpMenu.cs index e948b53e4..89adeb9f8 100644 --- a/mRemoteNG/UI/Menu/msMain/HelpMenu.cs +++ b/mRemoteNG/UI/Menu/msMain/HelpMenu.cs @@ -188,19 +188,29 @@ namespace mRemoteNG.UI.Menu } } - private void mMenInfoHelp_Click(object? sender, EventArgs e) => Process.Start("explorer.exe", GeneralAppInfo.UrlDocumentation); + private void mMenInfoHelp_Click(object? sender, EventArgs e) => OpenUrl(GeneralAppInfo.UrlDocumentation); - private void mMenInfoForum_Click(object? sender, EventArgs e) => Process.Start("explorer.exe", GeneralAppInfo.UrlForum); + private void mMenInfoForum_Click(object? sender, EventArgs e) => OpenUrl(GeneralAppInfo.UrlForum); - private void mMenInfoChat_Click(object? sender, EventArgs e) => Process.Start("explorer.exe", GeneralAppInfo.UrlChat); + private void mMenInfoChat_Click(object? sender, EventArgs e) => OpenUrl(GeneralAppInfo.UrlChat); - private void mMenInfoCommunity_Click(object? sender, EventArgs e) => Process.Start("explorer.exe", GeneralAppInfo.UrlCommunity); + private void mMenInfoCommunity_Click(object? sender, EventArgs e) => OpenUrl(GeneralAppInfo.UrlCommunity); - private void mMenInfoBug_Click(object? sender, EventArgs e) => Process.Start("explorer.exe", GeneralAppInfo.UrlBugs); + private void mMenInfoBug_Click(object? sender, EventArgs e) => OpenUrl(GeneralAppInfo.UrlBugs); - private void mMenInfoWebsite_Click(object? sender, EventArgs e) => Process.Start("explorer.exe", GeneralAppInfo.UrlHome); + private void mMenInfoWebsite_Click(object? sender, EventArgs e) => OpenUrl(GeneralAppInfo.UrlHome); - private void mMenInfoDonate_Click(object? sender, EventArgs e) => Process.Start("explorer.exe", GeneralAppInfo.UrlDonate); + private void mMenInfoDonate_Click(object? sender, EventArgs e) => OpenUrl(GeneralAppInfo.UrlDonate); + + private static void OpenUrl(string url) + { + var startInfo = new ProcessStartInfo + { + FileName = url, + UseShellExecute = true + }; + Process.Start(startInfo); + } private void mMenInfoAbout_Click(object? sender, EventArgs e) { diff --git a/mRemoteNG/UI/Window/UpdateWindow.cs b/mRemoteNG/UI/Window/UpdateWindow.cs index d590152d8..935940207 100644 --- a/mRemoteNG/UI/Window/UpdateWindow.cs +++ b/mRemoteNG/UI/Window/UpdateWindow.cs @@ -99,7 +99,12 @@ namespace mRemoteNG.UI.Window return; } - Process.Start(linkUri.ToString()); + var startInfo = new ProcessStartInfo + { + FileName = linkUri.ToString(), + UseShellExecute = true + }; + Process.Start(startInfo); } #endregion diff --git a/mRemoteNGTests/Connection/Protocol/ProtocolAnydeskTests.cs b/mRemoteNGTests/Connection/Protocol/ProtocolAnydeskTests.cs new file mode 100644 index 000000000..0fe849d33 --- /dev/null +++ b/mRemoteNGTests/Connection/Protocol/ProtocolAnydeskTests.cs @@ -0,0 +1,204 @@ +using System; +using System.Reflection; +using mRemoteNG.Connection; +using mRemoteNG.Connection.Protocol.AnyDesk; +using NUnit.Framework; + +namespace mRemoteNGTests.Connection.Protocol; + +public class ProtocolAnyDeskTests +{ + private ProtocolAnyDesk _protocolAnyDesk; + private ConnectionInfo _connectionInfo; + + [SetUp] + public void Setup() + { + _connectionInfo = new ConnectionInfo(); + _protocolAnyDesk = new ProtocolAnyDesk(_connectionInfo); + } + + [TearDown] + public void Teardown() + { + _protocolAnyDesk?.Close(); + _protocolAnyDesk = null; + _connectionInfo = null; + } + + #region IsValidAnydeskId Tests + + [Test] + public void IsValidAnydeskId_NumericId_ReturnsTrue() + { + // Valid numeric AnyDesk ID + bool result = InvokeIsValidAnydeskId("123456789"); + Assert.That(result, Is.True); + } + + [Test] + public void IsValidAnydeskId_AlphanumericWithAt_ReturnsTrue() + { + // Valid alias format: alias@ad + bool result = InvokeIsValidAnydeskId("myalias@ad"); + Assert.That(result, Is.True); + } + + [Test] + public void IsValidAnydeskId_WithHyphen_ReturnsTrue() + { + // Valid ID with hyphen + bool result = InvokeIsValidAnydeskId("my-alias@ad"); + Assert.That(result, Is.True); + } + + [Test] + public void IsValidAnydeskId_WithUnderscore_ReturnsTrue() + { + // Valid ID with underscore + bool result = InvokeIsValidAnydeskId("my_alias@ad"); + Assert.That(result, Is.True); + } + + [Test] + public void IsValidAnydeskId_WithDot_ReturnsTrue() + { + // Valid ID with dot + bool result = InvokeIsValidAnydeskId("alias.name@ad"); + Assert.That(result, Is.True); + } + + [Test] + public void IsValidAnydeskId_WithSemicolon_ReturnsFalse() + { + // Command injection attempt with semicolon + bool result = InvokeIsValidAnydeskId("123456789; calc.exe"); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_WithAmpersand_ReturnsFalse() + { + // Command injection attempt with ampersand + bool result = InvokeIsValidAnydeskId("123456789 & calc.exe"); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_WithPipe_ReturnsFalse() + { + // Command injection attempt with pipe + bool result = InvokeIsValidAnydeskId("123456789 | calc.exe"); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_WithRedirection_ReturnsFalse() + { + // Command injection attempt with redirection + bool result = InvokeIsValidAnydeskId("123456789 > output.txt"); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_WithBacktick_ReturnsFalse() + { + // PowerShell escape character + bool result = InvokeIsValidAnydeskId("123456789`calc"); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_WithDollarSign_ReturnsFalse() + { + // PowerShell variable indicator + bool result = InvokeIsValidAnydeskId("123456789$var"); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_WithParentheses_ReturnsFalse() + { + // Command substitution + bool result = InvokeIsValidAnydeskId("123456789(calc)"); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_WithNewline_ReturnsFalse() + { + // Newline injection + bool result = InvokeIsValidAnydeskId("123456789\ncalc"); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_WithCarriageReturn_ReturnsFalse() + { + // Carriage return injection + bool result = InvokeIsValidAnydeskId("123456789\rcalc"); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_WithQuotes_ReturnsFalse() + { + // Quote escape attempt + bool result = InvokeIsValidAnydeskId("123456789\"calc\""); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_WithSingleQuotes_ReturnsFalse() + { + // Single quote escape attempt + bool result = InvokeIsValidAnydeskId("123456789'calc'"); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_EmptyString_ReturnsFalse() + { + // Empty string + bool result = InvokeIsValidAnydeskId(""); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_Whitespace_ReturnsFalse() + { + // Only whitespace + bool result = InvokeIsValidAnydeskId(" "); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidAnydeskId_Null_ReturnsFalse() + { + // Null string + bool result = InvokeIsValidAnydeskId(null); + Assert.That(result, Is.False); + } + + #endregion + + #region Helper Methods + + /// + /// Uses reflection to invoke the private IsValidAnydeskId method + /// + private bool InvokeIsValidAnydeskId(string anydeskId) + { + var method = typeof(ProtocolAnyDesk).GetMethod("IsValidAnydeskId", + BindingFlags.NonPublic | BindingFlags.Instance); + + if (method == null) + { + throw new Exception("IsValidAnydeskId method not found. The method may have been renamed or removed."); + } + + return (bool)method.Invoke(_protocolAnydesk, new object[] { anydeskId }); + } + + #endregion +}