From 63e100c23311dec65158b4bd4327f909db9de88f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 29 Dec 2025 09:59:26 +0000 Subject: [PATCH 1/3] Initial plan From 97f5daa13dd78c72dc6c8d7d6c1177a97330632b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 29 Dec 2025 10:04:43 +0000 Subject: [PATCH 2/3] Add LDAP injection protection with sanitization utility Co-authored-by: Kvarkas <3611964+Kvarkas@users.noreply.github.com> --- .../ActiveDirectoryDeserializer.cs | 42 +++- mRemoteNG/Security/LdapPathSanitizer.cs | 166 +++++++++++++ mRemoteNG/Tools/ADhelper.cs | 49 +++- .../Security/LdapPathSanitizerTests.cs | 231 ++++++++++++++++++ 4 files changed, 484 insertions(+), 4 deletions(-) create mode 100644 mRemoteNG/Security/LdapPathSanitizer.cs create mode 100644 mRemoteNGTests/Security/LdapPathSanitizerTests.cs diff --git a/mRemoteNG/Config/Serializers/MiscSerializers/ActiveDirectoryDeserializer.cs b/mRemoteNG/Config/Serializers/MiscSerializers/ActiveDirectoryDeserializer.cs index f9ec3e7b..c1580517 100644 --- a/mRemoteNG/Config/Serializers/MiscSerializers/ActiveDirectoryDeserializer.cs +++ b/mRemoteNG/Config/Serializers/MiscSerializers/ActiveDirectoryDeserializer.cs @@ -10,6 +10,7 @@ using mRemoteNG.Tools; using mRemoteNG.Tree; using mRemoteNG.Tree.Root; using mRemoteNG.Resources.Language; +using mRemoteNG.Security; using System.Runtime.Versioning; namespace mRemoteNG.Config.Serializers.MiscSerializers @@ -17,9 +18,48 @@ namespace mRemoteNG.Config.Serializers.MiscSerializers [SupportedOSPlatform("windows")] public class ActiveDirectoryDeserializer(string ldapPath, bool importSubOu) { - private readonly string _ldapPath = ldapPath.ThrowIfNullOrEmpty(nameof(ldapPath)); + private readonly string _ldapPath = SanitizeLdapPath(ldapPath.ThrowIfNullOrEmpty(nameof(ldapPath))); private readonly bool _importSubOu = importSubOu; + private static string SanitizeLdapPath(string ldapPath) + { + // Validate the LDAP path format + if (!LdapPathSanitizer.IsValidDistinguishedNameFormat(ldapPath)) + { + throw new ArgumentException("Invalid LDAP path format", nameof(ldapPath)); + } + + // For LDAP paths (URIs like LDAP://...), we need to sanitize the DN portion + // If it starts with LDAP:// or LDAPS://, extract and sanitize the DN part + if (ldapPath.StartsWith("LDAP://", StringComparison.OrdinalIgnoreCase) || + ldapPath.StartsWith("LDAPS://", StringComparison.OrdinalIgnoreCase)) + { + int schemeEndIndex = ldapPath.IndexOf("://", StringComparison.OrdinalIgnoreCase) + 3; + if (schemeEndIndex < ldapPath.Length) + { + // Find the server/domain part (before the first /) + int pathStartIndex = ldapPath.IndexOf('/', schemeEndIndex); + if (pathStartIndex > 0) + { + string scheme = ldapPath.Substring(0, schemeEndIndex); + string serverPart = ldapPath.Substring(schemeEndIndex, pathStartIndex - schemeEndIndex); + string dnPart = ldapPath.Substring(pathStartIndex + 1); + + // Sanitize the DN part + string sanitizedDn = LdapPathSanitizer.SanitizeDistinguishedName(dnPart); + return scheme + serverPart + "/" + sanitizedDn; + } + } + // If no DN part found, return the path as-is (just the server) + return ldapPath; + } + else + { + // For plain DN strings, sanitize directly + return LdapPathSanitizer.SanitizeDistinguishedName(ldapPath); + } + } + public ConnectionTreeModel Deserialize() { ConnectionTreeModel connectionTreeModel = new(); diff --git a/mRemoteNG/Security/LdapPathSanitizer.cs b/mRemoteNG/Security/LdapPathSanitizer.cs new file mode 100644 index 00000000..58ab6f19 --- /dev/null +++ b/mRemoteNG/Security/LdapPathSanitizer.cs @@ -0,0 +1,166 @@ +using System; +using System.Text; + +namespace mRemoteNG.Security +{ + /// + /// Provides methods to sanitize LDAP distinguished names and filter values to prevent LDAP injection attacks. + /// Based on OWASP recommendations for LDAP injection prevention. + /// + public static class LdapPathSanitizer + { + /// + /// Sanitizes an LDAP distinguished name (DN) by escaping special characters. + /// This should be used for DN values like those passed to DirectoryEntry constructor. + /// + /// The DN to sanitize + /// A sanitized DN string safe from LDAP injection + public static string SanitizeDistinguishedName(string distinguishedName) + { + if (string.IsNullOrEmpty(distinguishedName)) + return distinguishedName; + + // Escape special characters in DN according to RFC 4514 + // Special characters that need escaping: , \ # + < > ; " = + // and leading/trailing spaces + StringBuilder result = new StringBuilder(); + + for (int i = 0; i < distinguishedName.Length; i++) + { + char c = distinguishedName[i]; + + // Escape special characters + switch (c) + { + case '\\': + result.Append("\\\\"); + break; + case ',': + result.Append("\\,"); + break; + case '+': + result.Append("\\+"); + break; + case '"': + result.Append("\\\""); + break; + case '<': + result.Append("\\<"); + break; + case '>': + result.Append("\\>"); + break; + case ';': + result.Append("\\;"); + break; + case '=': + result.Append("\\="); + break; + case '#': + // # needs to be escaped only if it's the first character + if (i == 0) + result.Append("\\#"); + else + result.Append(c); + break; + case ' ': + // Leading and trailing spaces need to be escaped + if (i == 0 || i == distinguishedName.Length - 1) + result.Append("\\ "); + else + result.Append(c); + break; + default: + // Check for control characters (0x00-0x1F and 0x7F) + if (c < 0x20 || c == 0x7F) + { + result.Append("\\"); + result.Append(((int)c).ToString("X2")); + } + else + { + result.Append(c); + } + break; + } + } + + return result.ToString(); + } + + /// + /// Sanitizes an LDAP filter value by escaping special characters. + /// This should be used for filter values in LDAP search filters. + /// + /// The filter value to sanitize + /// A sanitized filter value safe from LDAP injection + public static string SanitizeFilterValue(string filterValue) + { + if (string.IsNullOrEmpty(filterValue)) + return filterValue; + + // Escape special characters in filter according to RFC 4515 + // Special characters that need escaping: * ( ) \ NUL + StringBuilder result = new StringBuilder(); + + foreach (char c in filterValue) + { + switch (c) + { + case '\\': + result.Append("\\5c"); + break; + case '*': + result.Append("\\2a"); + break; + case '(': + result.Append("\\28"); + break; + case ')': + result.Append("\\29"); + break; + case '\0': + result.Append("\\00"); + break; + default: + // Check for other control characters + if (c < 0x20 || c == 0x7F) + { + result.Append("\\"); + result.Append(((int)c).ToString("x2")); + } + else + { + result.Append(c); + } + break; + } + } + + return result.ToString(); + } + + /// + /// Validates that a distinguished name appears to be in valid LDAP DN format. + /// This is a basic check to ensure the DN structure is reasonable. + /// + /// The DN to validate + /// True if the DN appears valid, false otherwise + public static bool IsValidDistinguishedNameFormat(string distinguishedName) + { + if (string.IsNullOrWhiteSpace(distinguishedName)) + return false; + + // Basic validation: should start with LDAP:// or contain = for attribute=value pairs + // This is a simple heuristic check, not a full RFC validation + if (distinguishedName.StartsWith("LDAP://", StringComparison.OrdinalIgnoreCase) || + distinguishedName.StartsWith("LDAPS://", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + // Check if it looks like a DN (contains attribute=value pairs) + return distinguishedName.Contains("="); + } + } +} diff --git a/mRemoteNG/Tools/ADhelper.cs b/mRemoteNG/Tools/ADhelper.cs index 6e4d6a3e..47766290 100644 --- a/mRemoteNG/Tools/ADhelper.cs +++ b/mRemoteNG/Tools/ADhelper.cs @@ -3,6 +3,7 @@ using System.Collections; using System.DirectoryServices; using System.Runtime.InteropServices; using System.Runtime.Versioning; +using mRemoteNG.Security; namespace mRemoteNG.Tools { @@ -17,9 +18,13 @@ namespace mRemoteNG.Tools public void GetChildEntries(string adPath = "") { - _dEntry = adPath.Length <= 0 - ? Domain.Length <= 0 ? new DirectoryEntry() : new DirectoryEntry("LDAP://" + Domain) - : new DirectoryEntry(adPath); + // Sanitize inputs to prevent LDAP injection + string sanitizedDomain = string.IsNullOrEmpty(Domain) ? string.Empty : LdapPathSanitizer.SanitizeDistinguishedName(Domain); + string sanitizedAdPath = string.IsNullOrEmpty(adPath) ? string.Empty : SanitizeLdapPath(adPath); + + _dEntry = sanitizedAdPath.Length <= 0 + ? sanitizedDomain.Length <= 0 ? new DirectoryEntry() : new DirectoryEntry("LDAP://" + sanitizedDomain) + : new DirectoryEntry(sanitizedAdPath); try { foreach (DirectoryEntry child in _dEntry.Children) @@ -31,5 +36,43 @@ namespace mRemoteNG.Tools throw new Exception("Could not find AD Server", ex); } } + + private static string SanitizeLdapPath(string ldapPath) + { + // Validate the LDAP path format + if (!LdapPathSanitizer.IsValidDistinguishedNameFormat(ldapPath)) + { + throw new ArgumentException("Invalid LDAP path format", nameof(ldapPath)); + } + + // For LDAP paths (URIs like LDAP://...), we need to sanitize the DN portion + if (ldapPath.StartsWith("LDAP://", StringComparison.OrdinalIgnoreCase) || + ldapPath.StartsWith("LDAPS://", StringComparison.OrdinalIgnoreCase)) + { + int schemeEndIndex = ldapPath.IndexOf("://", StringComparison.OrdinalIgnoreCase) + 3; + if (schemeEndIndex < ldapPath.Length) + { + // Find the server/domain part (before the first /) + int pathStartIndex = ldapPath.IndexOf('/', schemeEndIndex); + if (pathStartIndex > 0) + { + string scheme = ldapPath.Substring(0, schemeEndIndex); + string serverPart = ldapPath.Substring(schemeEndIndex, pathStartIndex - schemeEndIndex); + string dnPart = ldapPath.Substring(pathStartIndex + 1); + + // Sanitize the DN part + string sanitizedDn = LdapPathSanitizer.SanitizeDistinguishedName(dnPart); + return scheme + serverPart + "/" + sanitizedDn; + } + } + // If no DN part found, return the path as-is (just the server) + return ldapPath; + } + else + { + // For plain DN strings, sanitize directly + return LdapPathSanitizer.SanitizeDistinguishedName(ldapPath); + } + } } } \ No newline at end of file diff --git a/mRemoteNGTests/Security/LdapPathSanitizerTests.cs b/mRemoteNGTests/Security/LdapPathSanitizerTests.cs new file mode 100644 index 00000000..18df868e --- /dev/null +++ b/mRemoteNGTests/Security/LdapPathSanitizerTests.cs @@ -0,0 +1,231 @@ +using mRemoteNG.Security; +using NUnit.Framework; + +namespace mRemoteNGTests.Security +{ + [TestFixture] + public class LdapPathSanitizerTests + { + [Test] + public void SanitizeDistinguishedName_EscapesBackslash() + { + string input = "CN=Test\\User"; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("CN\\=Test\\\\User")); + } + + [Test] + public void SanitizeDistinguishedName_EscapesComma() + { + string input = "CN=User,Test"; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("CN\\=User\\,Test")); + } + + [Test] + public void SanitizeDistinguishedName_EscapesPlus() + { + string input = "CN=User+Admin"; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("CN\\=User\\+Admin")); + } + + [Test] + public void SanitizeDistinguishedName_EscapesQuotes() + { + string input = "CN=\"Test User\""; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("CN\\=\\\"Test User\\\"")); + } + + [Test] + public void SanitizeDistinguishedName_EscapesLessThan() + { + string input = "CN="; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("CN\\=\\")); + } + + [Test] + public void SanitizeDistinguishedName_EscapesSemicolon() + { + string input = "CN=User;Admin"; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("CN\\=User\\;Admin")); + } + + [Test] + public void SanitizeDistinguishedName_EscapesLeadingHash() + { + string input = "#Test"; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("\\#Test")); + } + + [Test] + public void SanitizeDistinguishedName_DoesNotEscapeMiddleHash() + { + string input = "Test#User"; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("Test#User")); + } + + [Test] + public void SanitizeDistinguishedName_EscapesLeadingSpace() + { + string input = " Test"; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("\\ Test")); + } + + [Test] + public void SanitizeDistinguishedName_EscapesTrailingSpace() + { + string input = "Test "; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("Test\\ ")); + } + + [Test] + public void SanitizeDistinguishedName_DoesNotEscapeMiddleSpace() + { + string input = "Test User"; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + Assert.That(result, Is.EqualTo("Test User")); + } + + [Test] + public void SanitizeDistinguishedName_ReturnsEmptyForNull() + { + string result = LdapPathSanitizer.SanitizeDistinguishedName(null); + Assert.That(result, Is.Null); + } + + [Test] + public void SanitizeDistinguishedName_ReturnsEmptyForEmpty() + { + string result = LdapPathSanitizer.SanitizeDistinguishedName(string.Empty); + Assert.That(result, Is.EqualTo(string.Empty)); + } + + [Test] + public void SanitizeFilterValue_EscapesAsterisk() + { + string input = "user*"; + string result = LdapPathSanitizer.SanitizeFilterValue(input); + Assert.That(result, Is.EqualTo("user\\2a")); + } + + [Test] + public void SanitizeFilterValue_EscapesParentheses() + { + string input = "(admin)"; + string result = LdapPathSanitizer.SanitizeFilterValue(input); + Assert.That(result, Is.EqualTo("\\28admin\\29")); + } + + [Test] + public void SanitizeFilterValue_EscapesBackslash() + { + string input = "user\\admin"; + string result = LdapPathSanitizer.SanitizeFilterValue(input); + Assert.That(result, Is.EqualTo("user\\5cadmin")); + } + + [Test] + public void SanitizeFilterValue_EscapesNullCharacter() + { + string input = "user\0admin"; + string result = LdapPathSanitizer.SanitizeFilterValue(input); + Assert.That(result, Is.EqualTo("user\\00admin")); + } + + [Test] + public void SanitizeFilterValue_ReturnsNullForNull() + { + string result = LdapPathSanitizer.SanitizeFilterValue(null); + Assert.That(result, Is.Null); + } + + [Test] + public void SanitizeFilterValue_ReturnsEmptyForEmpty() + { + string result = LdapPathSanitizer.SanitizeFilterValue(string.Empty); + Assert.That(result, Is.EqualTo(string.Empty)); + } + + [Test] + public void IsValidDistinguishedNameFormat_ReturnsTrueForLdapUri() + { + string input = "LDAP://dc=example,dc=com"; + bool result = LdapPathSanitizer.IsValidDistinguishedNameFormat(input); + Assert.That(result, Is.True); + } + + [Test] + public void IsValidDistinguishedNameFormat_ReturnsTrueForLdapsUri() + { + string input = "LDAPS://dc=example,dc=com"; + bool result = LdapPathSanitizer.IsValidDistinguishedNameFormat(input); + Assert.That(result, Is.True); + } + + [Test] + public void IsValidDistinguishedNameFormat_ReturnsTrueForDnWithEquals() + { + string input = "CN=User,OU=Users,DC=example,DC=com"; + bool result = LdapPathSanitizer.IsValidDistinguishedNameFormat(input); + Assert.That(result, Is.True); + } + + [Test] + public void IsValidDistinguishedNameFormat_ReturnsFalseForNull() + { + bool result = LdapPathSanitizer.IsValidDistinguishedNameFormat(null); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidDistinguishedNameFormat_ReturnsFalseForEmpty() + { + bool result = LdapPathSanitizer.IsValidDistinguishedNameFormat(string.Empty); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidDistinguishedNameFormat_ReturnsFalseForWhitespace() + { + bool result = LdapPathSanitizer.IsValidDistinguishedNameFormat(" "); + Assert.That(result, Is.False); + } + + [Test] + public void IsValidDistinguishedNameFormat_ReturnsFalseForPlainString() + { + bool result = LdapPathSanitizer.IsValidDistinguishedNameFormat("plainstring"); + Assert.That(result, Is.False); + } + + [Test] + public void SanitizeDistinguishedName_PreventsSQLInjectionAttempt() + { + // Test common LDAP injection patterns + string input = "*)(objectClass=*))(|(cn=*"; + string result = LdapPathSanitizer.SanitizeDistinguishedName(input); + // Verify special characters are escaped + Assert.That(result, Does.Contain("\\=")); + } + + [Test] + public void SanitizeFilterValue_PreventsSQLInjectionAttempt() + { + // Test common LDAP injection patterns + string input = "*)(uid=*))(|(uid=*"; + string result = LdapPathSanitizer.SanitizeFilterValue(input); + // Verify special characters are escaped + Assert.That(result, Does.Contain("\\2a")); + Assert.That(result, Does.Contain("\\28")); + Assert.That(result, Does.Contain("\\29")); + } + } +} From 1c2b5fadc4e24c3aa1836e52cebf196f5a7edfb3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 29 Dec 2025 10:06:39 +0000 Subject: [PATCH 3/3] Fix code review feedback: consistent hex formatting and test names Co-authored-by: Kvarkas <3611964+Kvarkas@users.noreply.github.com> --- mRemoteNG/Security/LdapPathSanitizer.cs | 2 +- mRemoteNGTests/Security/LdapPathSanitizerTests.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/mRemoteNG/Security/LdapPathSanitizer.cs b/mRemoteNG/Security/LdapPathSanitizer.cs index 58ab6f19..4e107d83 100644 --- a/mRemoteNG/Security/LdapPathSanitizer.cs +++ b/mRemoteNG/Security/LdapPathSanitizer.cs @@ -75,7 +75,7 @@ namespace mRemoteNG.Security if (c < 0x20 || c == 0x7F) { result.Append("\\"); - result.Append(((int)c).ToString("X2")); + result.Append(((int)c).ToString("x2")); } else { diff --git a/mRemoteNGTests/Security/LdapPathSanitizerTests.cs b/mRemoteNGTests/Security/LdapPathSanitizerTests.cs index 18df868e..cce31cc2 100644 --- a/mRemoteNGTests/Security/LdapPathSanitizerTests.cs +++ b/mRemoteNGTests/Security/LdapPathSanitizerTests.cs @@ -207,7 +207,7 @@ namespace mRemoteNGTests.Security } [Test] - public void SanitizeDistinguishedName_PreventsSQLInjectionAttempt() + public void SanitizeDistinguishedName_PreventsLdapInjectionAttempt() { // Test common LDAP injection patterns string input = "*)(objectClass=*))(|(cn=*"; @@ -217,7 +217,7 @@ namespace mRemoteNGTests.Security } [Test] - public void SanitizeFilterValue_PreventsSQLInjectionAttempt() + public void SanitizeFilterValue_PreventsLdapInjectionAttempt() { // Test common LDAP injection patterns string input = "*)(uid=*))(|(uid=*";