Merge pull request #13313 from maikypedia/maikypedia/ldap-improper-auth

Ruby: Add Improper LDAP Authentication query (CWE-287)

ruby/ql/lib/codeql/ruby/Concepts.qll

@@ -1250,3 +1250,40 @@ module LdapExecution {
     abstract DataFlow::Node getQuery();
   }
 }
+
+/**
+ * A data-flow node that collects methods binding a LDAP connection.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `LdapBind::Range` instead.
+ */
+class LdapBind extends DataFlow::Node instanceof LdapBind::Range {
+  /** Gets the argument containing the binding host */
+  DataFlow::Node getHost() { result = super.getHost() }
+
+  /** Gets the argument containing the binding expression. */
+  DataFlow::Node getPassword() { result = super.getPassword() }
+
+  /** Holds if the binding process use SSL. */
+  predicate usesSsl() { super.usesSsl() }
+}
+
+/** Provides classes for modeling LDAP bind-related APIs. */
+module LdapBind {
+  /**
+   * A data-flow node that collects methods binding a LDAP connection.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `LdapBind` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /** Gets the argument containing the binding host. */
+    abstract DataFlow::Node getHost();
+
+    /** Gets the argument containing the binding expression. */
+    abstract DataFlow::Node getPassword();
+
+    /** Holds if the binding process use SSL. */
+    abstract predicate usesSsl();
+  }
+}

ruby/ql/lib/codeql/ruby/frameworks/Ldap.qll

@@ -43,6 +43,17 @@ module NetLdap {
   /** A call that establishes a LDAP Connection */
   private class NetLdapConnection extends DataFlow::CallNode {
     NetLdapConnection() { this in [ldap().getAnInstantiation(), ldap().getAMethodCall("open")] }
+
+    predicate usesSsl() {
+      getValue(this, "encryption").getConstantValue().isStringlikeValue("simple_tls")
+    }
+
+    DataFlow::Node getAuthValue(string arg) {
+      result =
+        this.getKeywordArgument("auth")
+            .(DataFlow::HashLiteralNode)
+            .getElementFromKey(any(Ast::ConstantValue cv | cv.isStringlikeValue(arg)))
+    }
   }
 
   /** A call that constructs a LDAP query */
@@ -67,4 +78,29 @@ module NetLdap {
 
     override DataFlow::Node getQuery() { result = this.getKeywordArgument(_) }
   }
+
+  /** A call considered as a LDAP bind. */
+  private class NetLdapBind extends LdapBind::Range, DataFlow::CallNode {
+    private NetLdapConnection l;
+
+    NetLdapBind() { this = l.getAMethodCall("bind") }
+
+    override DataFlow::Node getHost() { result = getValue(l, "host") }
+
+    override DataFlow::Node getPassword() {
+      result = l.getAuthValue("password") or
+      result = l.getAMethodCall("auth").getArgument(1)
+    }
+
+    override predicate usesSsl() { l.usesSsl() }
+  }
+
+  /** LDAP Attribute value */
+  DataFlow::Node getValue(NetLdapConnection l, string attr) {
+    result =
+      [
+        l.getKeywordArgument(attr), l.getAMethodCall(attr).getArgument(0),
+        l.getAMethodCall(attr).getKeywordArgument(attr)
+      ]
+  }
 }

ruby/ql/lib/codeql/ruby/security/ImproperLdapAuthCustomizations.qll

@@ -0,0 +1,49 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * improper LDAP authentication, as well as extension points for adding your own
+ */
+
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+private import codeql.ruby.dataflow.BarrierGuards
+private import codeql.ruby.dataflow.RemoteFlowSources
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * improper LDAP authentication,  as well as extension points for adding your own
+ */
+module ImproperLdapAuth {
+  /** A data flow source for improper LDAP authentication vulnerabilities */
+  abstract class Source extends DataFlow::Node { }
+
+  /** A data flow sink for improper LDAP authentication vulnerabilities */
+  abstract class Sink extends DataFlow::Node { }
+
+  /** A sanitizer for improper LDAP authentication vulnerabilities. */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  private class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  /**
+   * An LDAP query execution considered as a flow sink.
+   */
+  private class LdapBindAsSink extends Sink {
+    LdapBindAsSink() { this = any(LdapBind l).getPassword() }
+  }
+
+  /**
+   * A comparison with a constant string, considered as a sanitizer-guard.
+   */
+  private class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+
+  /**
+   * An inclusion check against an array of constant strings, considered as a
+   * sanitizer-guard.
+   */
+  private class StringConstArrayInclusionCallAsSanitizer extends Sanitizer,
+    StringConstArrayInclusionCallBarrier
+  { }
+}

ruby/ql/lib/codeql/ruby/security/ImproperLdapAuthQuery.qll

@@ -0,0 +1,21 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * improper LDAP authentication, as well as extension points for adding your own
+ */
+
+private import codeql.ruby.DataFlow
+private import codeql.ruby.TaintTracking
+private import ImproperLdapAuthCustomizations::ImproperLdapAuth
+
+/**
+ * A taint-tracking configuration for detecting improper LDAP authentication vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "ImproperLdapAuth" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+}

ruby/ql/src/change-notes/2023-05-29-improper-ldap-auth-query.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new experimental query, `rb/improper-ldap-auth`, to detect cases where user input is used during LDAP authentication without proper validation or sanitization, potentially leading to authentication bypass.

ruby/ql/src/experimental/ldap-improper-auth/ImproperLdapAuth.qhelp

@@ -0,0 +1,39 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+If an LDAP connection uses user-supplied data as password, anonymous bind could be caused using an empty password 
+to result in a successful authentication.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Don't use user-supplied data as password while establishing an LDAP connection.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the following Rails example, an <code>ActionController</code> class
+has a <code>ldap_handler</code> method to handle requests.
+</p>
+
+<p>
+In the first example, the code builds a LDAP query whose authentication depends on user supplied data.
+</p>
+
+<sample src="examples/LdapAuthenticationBad.rb" />
+
+<p>In the second example, the authentication is established using a default password.</p>
+
+<sample src="examples/LdapAuthenticationGood.rb" />
+</example>
+
+<references>
+<li>MITRE: <a href="https://cwe.mitre.org/data/definitions/287.html">CWE-287: Improper Authentication</a>.</li>
+</references>
+</qhelp>

ruby/ql/src/experimental/ldap-improper-auth/ImproperLdapAuth.ql

@@ -0,0 +1,20 @@
+/**
+ * @name Improper LDAP Authentication
+ * @description A user-controlled query carries no authentication
+ * @kind path-problem
+ * @problem.severity warning
+ * @id rb/improper-ldap-auth
+ * @tags security
+ *       experimental
+ *       external/cwe/cwe-287
+ */
+
+import codeql.ruby.DataFlow
+import codeql.ruby.security.ImproperLdapAuthQuery
+import codeql.ruby.Concepts
+import DataFlow::PathGraph
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This LDAP authencation depends on a $@.", source.getNode(),
+  "user-provided value"

ruby/ql/src/experimental/ldap-improper-auth/examples/LdapAuthenticationBad.rb

@@ -0,0 +1,16 @@
+class FooController < ActionController::Base
+  def some_request_handler
+    pass = params[:pass]
+    ldap = Net::LDAP.new(
+        host: 'ldap.example.com',
+        port: 636,
+        encryption: :simple_tls,
+        auth: {
+            method: :simple,
+            username: 'uid=admin,dc=example,dc=com',
+            password: pass
+        }
+    )
+    ldap.bind
+  end
+end

ruby/ql/src/experimental/ldap-improper-auth/examples/LdapAuthenticationGood.rb

@@ -0,0 +1,16 @@
+class FooController < ActionController::Base
+  def some_request_handler
+    pass = params[:pass]
+    ldap = Net::LDAP.new(
+        host: 'ldap.example.com',
+        port: 636,
+        encryption: :simple_tls,
+        auth: {
+            method: :simple,
+            username: 'uid=admin,dc=example,dc=com',
+            password: '$uper$password123'
+        }
+    )
+    ldap.bind
+  end
+end

ruby/ql/test/query-tests/experimental/ImproperLdapAuth/ImproperLdapAuth.expected

@@ -0,0 +1,20 @@
+edges
+| ImproperLdapAuth.rb:5:5:5:8 | pass | ImproperLdapAuth.rb:15:23:15:26 | pass |
+| ImproperLdapAuth.rb:5:12:5:17 | call to params | ImproperLdapAuth.rb:5:12:5:24 | ...[...] |
+| ImproperLdapAuth.rb:5:12:5:24 | ...[...] | ImproperLdapAuth.rb:5:5:5:8 | pass |
+| ImproperLdapAuth.rb:24:5:24:8 | pass | ImproperLdapAuth.rb:31:24:31:27 | pass |
+| ImproperLdapAuth.rb:24:12:24:17 | call to params | ImproperLdapAuth.rb:24:12:24:24 | ...[...] |
+| ImproperLdapAuth.rb:24:12:24:24 | ...[...] | ImproperLdapAuth.rb:24:5:24:8 | pass |
+nodes
+| ImproperLdapAuth.rb:5:5:5:8 | pass | semmle.label | pass |
+| ImproperLdapAuth.rb:5:12:5:17 | call to params | semmle.label | call to params |
+| ImproperLdapAuth.rb:5:12:5:24 | ...[...] | semmle.label | ...[...] |
+| ImproperLdapAuth.rb:15:23:15:26 | pass | semmle.label | pass |
+| ImproperLdapAuth.rb:24:5:24:8 | pass | semmle.label | pass |
+| ImproperLdapAuth.rb:24:12:24:17 | call to params | semmle.label | call to params |
+| ImproperLdapAuth.rb:24:12:24:24 | ...[...] | semmle.label | ...[...] |
+| ImproperLdapAuth.rb:31:24:31:27 | pass | semmle.label | pass |
+subpaths
+#select
+| ImproperLdapAuth.rb:15:23:15:26 | pass | ImproperLdapAuth.rb:5:12:5:17 | call to params | ImproperLdapAuth.rb:15:23:15:26 | pass | This LDAP authencation depends on a $@. | ImproperLdapAuth.rb:5:12:5:17 | call to params | user-provided value |
+| ImproperLdapAuth.rb:31:24:31:27 | pass | ImproperLdapAuth.rb:24:12:24:17 | call to params | ImproperLdapAuth.rb:31:24:31:27 | pass | This LDAP authencation depends on a $@. | ImproperLdapAuth.rb:24:12:24:17 | call to params | user-provided value |

ruby/ql/test/query-tests/experimental/ImproperLdapAuth/ImproperLdapAuth.qlref

@@ -0,0 +1 @@
+experimental/ldap-improper-auth/ImproperLdapAuth.ql

ruby/ql/test/query-tests/experimental/ImproperLdapAuth/ImproperLdapAuth.rb

@@ -0,0 +1,59 @@
+class FooController < ActionController::Base
+  def some_request_handler
+    # A string tainted by user input is used directly as password
+    # (i.e a remote flow source)
+    pass = params[:pass]
+
+    # BAD: user input is not sanitized
+    ldap = Net::LDAP.new(
+        host: 'ldap.example.com',
+        port: 636,
+        encryption: :simple_tls,
+        auth: {
+            method: :simple,
+            username: 'uid=admin,dc=example,dc=com',
+            password: pass
+        }
+    )
+    ldap.bind
+  end
+
+  def some_request_handler
+    # A string tainted by user input is used directly as password
+    # (i.e a remote flow source)
+    pass = params[:pass]
+
+    # BAD: user input is not sanitized
+    ldap = Net::LDAP.new
+    ldap.host = your_server_ip_address
+    ldap.encryption(:method => :simple_tls)
+    ldap.port = 639
+    ldap.auth "admin", pass
+    ldap.bind
+  end
+end
+
+class BarController < ApplicationController
+  def safe_paths
+    pass = params[:pass]
+
+    # GOOD: barrier guard prevents taint flow
+    if password.nil? || password.empty?
+      # protect against passwordless auth from ldap server
+      pass = "$uper$secure123"
+    else
+      pass
+    end
+
+    ldap = Net::LDAP.new(
+        host: 'ldap.example.com',
+        port: 636,
+        encryption: :simple_tls,
+        auth: {
+            method: :simple,
+            username: 'uid=admin,dc=example,dc=com',
+            password: pass
+        }
+    )
+  end
+end