Review LDAP filters composed from strings

Rule ND3137 Security

Matches methods that assign DirectorySearcher.Filter, construct a DirectorySearcher, a SearchRequest or a DirectoryEntry from a string, or call LdapConnection.Search(), while also assembling a string in the same method (String.Concat(), String.Format(), interpolation, StringBuilder).

LDAP filters have their own syntax: a value containing )(uid=) or \) closes the intended clause and opens another, which turns a login lookup into a query that matches every user, or bypasses the authentication altogether. This is LDAP injection, CWE-90 (OWASP Top 10 A03 Injection).

Flagged Code
static SearchResult FindUser(DirectoryEntry root, string login) {
   var searcher = new DirectorySearcher(root);
   searcher.Filter = "(&(objectClass=user)(sAMAccountName=" + login + "))";   // flagged
   return searcher.FindOne();
}
Fixed Code
static SearchResult FindUser(DirectoryEntry root, string login) {
   var searcher = new DirectorySearcher(root);
   searcher.Filter = LdapFilter.ForUser(login);   // escaped and composed in one reviewed helper
   return searcher.FindOne();
}
static class LdapFilter {
   public static string ForUser(string login) =>
      "(&(objectClass=user)(sAMAccountName=" + Escape(login) + "))";
   static string Escape(string v) =>            // RFC 4515: escape \ * ( ) and NUL
      v.Replace(@"\", @"\5c").Replace("*", @"\2a").Replace("(", @"\28")
       .Replace(")", @"\29").Replace("\0", @"\00");
}

How to fix

Escape every dynamic value inserted in a filter as RFC 4515 requires (the characters backslash, parenthesis, star and NUL become \5c, \28, \29, \2a, \00), and escape values inserted in a distinguished name as RFC 4514 requires. Validate the value against the expected shape first (a login is alphanumeric, an email has one @).

Keep the escaping and the composition in one dedicated helper, as in the fixed code: the method that assigns the filter no longer assembles strings itself, and the rule cannot tell an escaped fragment from a raw one.

CQLinq source

This rule is written in CQLinq, C# LINQ over the NDepend code model. It ships with every new NDepend project: you can read it, adapt it, or use it as a starting point for your own custom rules.

// <Name>Review LDAP filters composed from strings</Name>
// <Id>ND3137:ReviewLdapFiltersComposedFromStrings</Id>
warnif count > 0

// Third-party members are part of the code model only when the application uses them.
// Members that take an LDAP filter or search base as a string.
let ldapFilterMembers = ThirdParty.Methods.Where(m =>
   (m.ParentType.FullName == "System.DirectoryServices.DirectorySearcher" &&
    (m.SimpleName == "set_Filter" || (m.IsConstructor && m.Name.Contains("String")))) ||
   (m.ParentType.FullName == "System.DirectoryServices.Protocols.SearchRequest" &&
    (m.SimpleName == "set_Filter" || (m.IsConstructor && m.Name.Contains("String")))) ||
   (m.ParentType.FullName == "System.DirectoryServices.DirectoryEntry" && m.IsConstructor && m.Name.Contains("String")) ||
   (m.ParentType.FullName == "Novell.Directory.Ldap.LdapConnection" && m.SimpleName.StartsWith("Search")))
   .ToHashSetEx()
where ldapFilterMembers.Any()

// Concatenation, formatting, interpolation or a builder: the filter is assembled in the method.
let stringBuildingMembers = ThirdParty.Methods.Where(m =>
   (m.ParentType.FullName == "System.String" && m.SimpleNameEqualsAny("Concat", "Format", "Join", "Replace", "Insert")) ||
    m.ParentType.FullNameEqualsAny("System.Text.StringBuilder", "System.Runtime.CompilerServices.DefaultInterpolatedStringHandler"))
   .ToHashSetEx()

from m in Application.Methods.UsingAny(ldapFilterMembers)
where m.MethodsCalled.Intersect(stringBuildingMembers).Any()

select new {
  m,
  ldapFilterMembersUsed = m.MethodsCalled.Intersect(ldapFilterMembers).ToArray(),
  Debt = 15.ToMinutes().ToDebt(),
  Severity = Severity.High
}

//<Expl>
// The method *{0}* assembles a string and passes it to *{1}* as an LDAP
// filter or path. If any fragment derives from user input, characters
// such as *)(|* rewrite the filter: use an escaping helper on every
// dynamic value.
//</Expl>

// <Desc>
// Matches methods that assign *DirectorySearcher.Filter*, construct a
// *DirectorySearcher*, a *SearchRequest* or a *DirectoryEntry* from a
// string, or call *LdapConnection.Search()*, while also assembling a
// string in the same method (*String.Concat()*, *String.Format()*,
// interpolation, *StringBuilder*).
//
// LDAP filters have their own syntax: a value containing *)(uid=*)* or
// *\*)* closes the intended clause and opens another, which turns a
// login lookup into a query that matches every user, or bypasses the
// authentication altogether. This is **LDAP injection**, CWE-90 (OWASP
// Top 10 A03 Injection).
//
// **Flagged Code**
// ```csharp
// static SearchResult FindUser(DirectoryEntry root, string login) {
//    var searcher = new DirectorySearcher(root);
//    searcher.Filter = "(&(objectClass=user)(sAMAccountName=" + login + "))";   // flagged
//    return searcher.FindOne();
// }
// ```
//
// **Fixed Code**
// ```csharp
// static SearchResult FindUser(DirectoryEntry root, string login) {
//    var searcher = new DirectorySearcher(root);
//    searcher.Filter = LdapFilter.ForUser(login);   // escaped and composed in one reviewed helper
//    return searcher.FindOne();
// }
// static class LdapFilter {
//    public static string ForUser(string login) =>
//       "(&(objectClass=user)(sAMAccountName=" + Escape(login) + "))";
//    static string Escape(string v) =>            // RFC 4515: escape \ * ( ) and NUL
//       v.Replace(@"\", @"\5c").Replace("*", @"\2a").Replace("(", @"\28")
//        .Replace(")", @"\29").Replace("\0", @"\00");
// }
// ```
// </Desc>

// <HowToFix>
// Escape every dynamic value inserted in a filter as RFC 4515 requires
// (the characters backslash, parenthesis, star and NUL become *\5c*,
// *\28*, *\29*, *\2a*, *\00*), and escape values inserted in a
// distinguished name as RFC 4514 requires. Validate the value against
// the expected shape first (a login is alphanumeric, an email has one
// *@*).
//
// Keep the escaping and the composition in one dedicated helper, as in
// the fixed code: the method that assigns the filter no longer
// assembles strings itself, and the rule cannot tell an escaped fragment
// from a raw one.
// </HowToFix>

Open in the interactive Rules Explorer · All default rules

Security: all 39 items