Merge pull request #6781 from erik-krogh/ldap

JS: Move LDAP injection out of experimental

Author: erik-krogh

javascript/change-notes/2021-10-01-ldap.md

@@ -0,0 +1,4 @@
+lgtm,codescanning
+* The `js/sql-injection` now recognizes the `ldapjs` library as a sink.
+  Affected packages are
+    [ldapjs](https://www.npmjs.com/package/ldapjs)

javascript/ql/lib/javascript.qll

@@ -99,6 +99,7 @@ import semmle.javascript.frameworks.History
 import semmle.javascript.frameworks.Immutable
 import semmle.javascript.frameworks.Knex
 import semmle.javascript.frameworks.LazyCache
+import semmle.javascript.frameworks.LdapJS
 import semmle.javascript.frameworks.LodashUnderscore
 import semmle.javascript.frameworks.Logging
 import semmle.javascript.frameworks.HttpFrameworks

javascript/ql/lib/semmle/javascript/frameworks/LdapJS.qll

@@ -0,0 +1,71 @@
+/**
+ * Provides classes for working with [LDAPjs](https://www.npmjs.com/package/ldapjs)
+ */
+
+import javascript
+
+/**
+ * A module providing sinks and sanitizers for LDAP injection.
+ */
+module LdapJS {
+  /** Gets a reference to the ldapjs library. */
+  API::Node ldapjs() { result = API::moduleImport("ldapjs") }
+
+  /** Gets an LDAPjs client. */
+  private API::Node ldapClient() { result = ldapjs().getMember("createClient").getReturn() }
+
+  /** A call to a LDAPjs Client API method. */
+  class ClientCall extends API::CallNode {
+    string methodName;
+
+    ClientCall() {
+      methodName = ["add", "bind", "compare", "del", "modify", "modifyDN", "search"] and
+      this = ldapClient().getMember(methodName).getACall()
+    }
+
+    /** Gets the name of the LDAPjs Client API method. */
+    string getMethodName() { result = methodName }
+  }
+
+  /** A reference to a LDAPjs client `search` options. */
+  class SearchOptions extends API::Node {
+    ClientCall call;
+
+    SearchOptions() { call.getMethodName() = "search" and this = call.getParameter(1) }
+  }
+
+  /** A creation of an LDAPjs filter, or object containing a filter, that doesn't sanitizes the input. */
+  abstract class TaintPreservingLdapFilterStep extends DataFlow::Node {
+    /** The input that creates (part of) an LDAPjs filter. */
+    abstract DataFlow::Node getInput();
+
+    /** The resulting LDAPjs filter. */
+    abstract DataFlow::Node getOutput();
+  }
+
+  /** A call to the LDAPjs utility method "parseFilter". */
+  private class ParseFilter extends TaintPreservingLdapFilterStep, API::CallNode {
+    ParseFilter() { this = ldapjs().getMember("parseFilter").getACall() }
+
+    override DataFlow::Node getInput() { result = this.getArgument(0) }
+
+    override DataFlow::Node getOutput() { result = this }
+  }
+
+  /**
+   * A filter used in call to "search" on an LDAPjs client.
+   * We model that as a step from the ".filter" write to the options object itself.
+   */
+  private class SearchFilter extends TaintPreservingLdapFilterStep {
+    SearchOptions options;
+
+    SearchFilter() {
+      options = ldapClient().getMember("search").getACall().getParameter(1) and
+      this = options.getARhs()
+    }
+
+    override DataFlow::Node getInput() { result = options.getMember("filter").getARhs() }
+
+    override DataFlow::Node getOutput() { result = this }
+  }
+}

javascript/ql/lib/semmle/javascript/security/dataflow/SqlInjectionCustomizations.qll

@@ -41,4 +41,35 @@ module SqlInjection {
   class GraphqlInjectionSink extends Sink {
     GraphqlInjectionSink() { this instanceof GraphQL::GraphQLString }
   }
+
+  /**
+   * An LDAPjs sink.
+   */
+  class LdapJSSink extends Sink {
+    LdapJSSink() {
+      // A distinguished name (DN) used in a call to the client API.
+      this = any(LdapJS::ClientCall call).getArgument(0)
+      or
+      // A search options object, which contains a filter and a baseDN.
+      this = any(LdapJS::SearchOptions opt).getARhs()
+      or
+      // A call to "parseDN", which parses a DN from a string.
+      this = LdapJS::ldapjs().getMember("parseDN").getACall().getArgument(0)
+    }
+  }
+
+  import semmle.javascript.security.IncompleteBlacklistSanitizer as IncompleteBlacklistSanitizer
+
+  /**
+   * A chain of replace calls that replaces all unsafe chars for ldap injection.
+   * For simplicity it's used as a sanitizer for all of `js/sql-injection`.
+   */
+  class LdapStringSanitizer extends Sanitizer,
+    IncompleteBlacklistSanitizer::StringReplaceCallSequence {
+    LdapStringSanitizer() {
+      forall(string char | char = ["*", "(", ")", "\\", "/"] |
+        this.getAMember().getAReplacedString() = char
+      )
+    }
+  }
 }

javascript/ql/lib/semmle/javascript/security/dataflow/SqlInjectionQuery.qll

@@ -24,4 +24,11 @@ class Configuration extends TaintTracking::Configuration {
     super.isSanitizer(node) or
     node instanceof Sanitizer
   }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(LdapJS::TaintPreservingLdapFilterStep filter |
+      pred = filter.getInput() and
+      succ = filter.getOutput()
+    )
+  }
 }

javascript/ql/src/Security/CWE-089/SqlInjection.ql

@@ -9,6 +9,8 @@
  * @id js/sql-injection
  * @tags security
  *       external/cwe/cwe-089
+ *       external/cwe/cwe-090
+ *       external/cwe/cwe-943
  */
 
 import javascript