Skip to content

Commit 9957e26

Browse files
alexrfordalexrford
authored
Merge pull request github#13313 from maikypedia/maikypedia/ldap-improper-auth
Ruby: Add Improper LDAP Authentication query (CWE-287)
2 parents c5612ae + ae635c6 commit 9957e26

File tree

12 files changed

+318
-0
lines changed

12 files changed

+318
-0
lines changed

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1250,3 +1250,40 @@ module LdapExecution {
12501250
abstract DataFlow::Node getQuery();
12511251
}
12521252
}
1253+
1254+
/**
1255+
* A data-flow node that collects methods binding a LDAP connection.
1256+
*
1257+
* Extend this class to refine existing API models. If you want to model new APIs,
1258+
* extend `LdapBind::Range` instead.
1259+
*/
1260+
class LdapBind extends DataFlow::Node instanceof LdapBind::Range {
1261+
/** Gets the argument containing the binding host */
1262+
DataFlow::Node getHost() { result = super.getHost() }
1263+
1264+
/** Gets the argument containing the binding expression. */
1265+
DataFlow::Node getPassword() { result = super.getPassword() }
1266+
1267+
/** Holds if the binding process use SSL. */
1268+
predicate usesSsl() { super.usesSsl() }
1269+
}
1270+
1271+
/** Provides classes for modeling LDAP bind-related APIs. */
1272+
module LdapBind {
1273+
/**
1274+
* A data-flow node that collects methods binding a LDAP connection.
1275+
*
1276+
* Extend this class to model new APIs. If you want to refine existing API models,
1277+
* extend `LdapBind` instead.
1278+
*/
1279+
abstract class Range extends DataFlow::Node {
1280+
/** Gets the argument containing the binding host. */
1281+
abstract DataFlow::Node getHost();
1282+
1283+
/** Gets the argument containing the binding expression. */
1284+
abstract DataFlow::Node getPassword();
1285+
1286+
/** Holds if the binding process use SSL. */
1287+
abstract predicate usesSsl();
1288+
}
1289+
}

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,17 @@ module NetLdap {
4343
/** A call that establishes a LDAP Connection */
4444
private class NetLdapConnection extends DataFlow::CallNode {
4545
NetLdapConnection() { this in [ldap().getAnInstantiation(), ldap().getAMethodCall("open")] }
46+
47+
predicate usesSsl() {
48+
getValue(this, "encryption").getConstantValue().isStringlikeValue("simple_tls")
49+
}
50+
51+
DataFlow::Node getAuthValue(string arg) {
52+
result =
53+
this.getKeywordArgument("auth")
54+
.(DataFlow::HashLiteralNode)
55+
.getElementFromKey(any(Ast::ConstantValue cv | cv.isStringlikeValue(arg)))
56+
}
4657
}
4758

4859
/** A call that constructs a LDAP query */
@@ -67,4 +78,29 @@ module NetLdap {
6778

6879
override DataFlow::Node getQuery() { result = this.getKeywordArgument(_) }
6980
}
81+
82+
/** A call considered as a LDAP bind. */
83+
private class NetLdapBind extends LdapBind::Range, DataFlow::CallNode {
84+
private NetLdapConnection l;
85+
86+
NetLdapBind() { this = l.getAMethodCall("bind") }
87+
88+
override DataFlow::Node getHost() { result = getValue(l, "host") }
89+
90+
override DataFlow::Node getPassword() {
91+
result = l.getAuthValue("password") or
92+
result = l.getAMethodCall("auth").getArgument(1)
93+
}
94+
95+
override predicate usesSsl() { l.usesSsl() }
96+
}
97+
98+
/** LDAP Attribute value */
99+
DataFlow::Node getValue(NetLdapConnection l, string attr) {
100+
result =
101+
[
102+
l.getKeywordArgument(attr), l.getAMethodCall(attr).getArgument(0),
103+
l.getAMethodCall(attr).getKeywordArgument(attr)
104+
]
105+
}
70106
}

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for detecting
3+
* improper LDAP authentication, as well as extension points for adding your own
4+
*/
5+
6+
private import codeql.ruby.Concepts
7+
private import codeql.ruby.DataFlow
8+
private import codeql.ruby.dataflow.BarrierGuards
9+
private import codeql.ruby.dataflow.RemoteFlowSources
10+
11+
/**
12+
* Provides default sources, sinks and sanitizers for detecting
13+
* improper LDAP authentication, as well as extension points for adding your own
14+
*/
15+
module ImproperLdapAuth {
16+
/** A data flow source for improper LDAP authentication vulnerabilities */
17+
abstract class Source extends DataFlow::Node { }
18+
19+
/** A data flow sink for improper LDAP authentication vulnerabilities */
20+
abstract class Sink extends DataFlow::Node { }
21+
22+
/** A sanitizer for improper LDAP authentication vulnerabilities. */
23+
abstract class Sanitizer extends DataFlow::Node { }
24+
25+
/**
26+
* A source of remote user input, considered as a flow source.
27+
*/
28+
private class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
29+
30+
/**
31+
* An LDAP query execution considered as a flow sink.
32+
*/
33+
private class LdapBindAsSink extends Sink {
34+
LdapBindAsSink() { this = any(LdapBind l).getPassword() }
35+
}
36+
37+
/**
38+
* A comparison with a constant string, considered as a sanitizer-guard.
39+
*/
40+
private class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
41+
42+
/**
43+
* An inclusion check against an array of constant strings, considered as a
44+
* sanitizer-guard.
45+
*/
46+
private class StringConstArrayInclusionCallAsSanitizer extends Sanitizer,
47+
StringConstArrayInclusionCallBarrier
48+
{ }
49+
}

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for detecting
3+
* improper LDAP authentication, as well as extension points for adding your own
4+
*/
5+
6+
private import codeql.ruby.DataFlow
7+
private import codeql.ruby.TaintTracking
8+
private import ImproperLdapAuthCustomizations::ImproperLdapAuth
9+
10+
/**
11+
* A taint-tracking configuration for detecting improper LDAP authentication vulnerabilities.
12+
*/
13+
class Configuration extends TaintTracking::Configuration {
14+
Configuration() { this = "ImproperLdapAuth" }
15+
16+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
17+
18+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
19+
20+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
21+
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* 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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
If an LDAP connection uses user-supplied data as password, anonymous bind could be caused using an empty password
9+
to result in a successful authentication.
10+
</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>
15+
Don't use user-supplied data as password while establishing an LDAP connection.
16+
</p>
17+
</recommendation>
18+
19+
<example>
20+
<p>
21+
In the following Rails example, an <code>ActionController</code> class
22+
has a <code>ldap_handler</code> method to handle requests.
23+
</p>
24+
25+
<p>
26+
In the first example, the code builds a LDAP query whose authentication depends on user supplied data.
27+
</p>
28+
29+
<sample src="examples/LdapAuthenticationBad.rb" />
30+
31+
<p>In the second example, the authentication is established using a default password.</p>
32+
33+
<sample src="examples/LdapAuthenticationGood.rb" />
34+
</example>
35+
36+
<references>
37+
<li>MITRE: <a href="https://cwe.mitre.org/data/definitions/287.html">CWE-287: Improper Authentication</a>.</li>
38+
</references>
39+
</qhelp>

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name Improper LDAP Authentication
3+
* @description A user-controlled query carries no authentication
4+
* @kind path-problem
5+
* @problem.severity warning
6+
* @id rb/improper-ldap-auth
7+
* @tags security
8+
* experimental
9+
* external/cwe/cwe-287
10+
*/
11+
12+
import codeql.ruby.DataFlow
13+
import codeql.ruby.security.ImproperLdapAuthQuery
14+
import codeql.ruby.Concepts
15+
import DataFlow::PathGraph
16+
17+
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
18+
where config.hasFlowPath(source, sink)
19+
select sink.getNode(), source, sink, "This LDAP authencation depends on a $@.", source.getNode(),
20+
"user-provided value"

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
class FooController < ActionController::Base
2+
def some_request_handler
3+
pass = params[:pass]
4+
ldap = Net::LDAP.new(
5+
host: 'ldap.example.com',
6+
port: 636,
7+
encryption: :simple_tls,
8+
auth: {
9+
method: :simple,
10+
username: 'uid=admin,dc=example,dc=com',
11+
password: pass
12+
}
13+
)
14+
ldap.bind
15+
end
16+
end

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
class FooController < ActionController::Base
2+
def some_request_handler
3+
pass = params[:pass]
4+
ldap = Net::LDAP.new(
5+
host: 'ldap.example.com',
6+
port: 636,
7+
encryption: :simple_tls,
8+
auth: {
9+
method: :simple,
10+
username: 'uid=admin,dc=example,dc=com',
11+
password: '$uper$password123'
12+
}
13+
)
14+
ldap.bind
15+
end
16+
end

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
edges
2+
| ImproperLdapAuth.rb:5:5:5:8 | pass | ImproperLdapAuth.rb:15:23:15:26 | pass |
3+
| ImproperLdapAuth.rb:5:12:5:17 | call to params | ImproperLdapAuth.rb:5:12:5:24 | ...[...] |
4+
| ImproperLdapAuth.rb:5:12:5:24 | ...[...] | ImproperLdapAuth.rb:5:5:5:8 | pass |
5+
| ImproperLdapAuth.rb:24:5:24:8 | pass | ImproperLdapAuth.rb:31:24:31:27 | pass |
6+
| ImproperLdapAuth.rb:24:12:24:17 | call to params | ImproperLdapAuth.rb:24:12:24:24 | ...[...] |
7+
| ImproperLdapAuth.rb:24:12:24:24 | ...[...] | ImproperLdapAuth.rb:24:5:24:8 | pass |
8+
nodes
9+
| ImproperLdapAuth.rb:5:5:5:8 | pass | semmle.label | pass |
10+
| ImproperLdapAuth.rb:5:12:5:17 | call to params | semmle.label | call to params |
11+
| ImproperLdapAuth.rb:5:12:5:24 | ...[...] | semmle.label | ...[...] |
12+
| ImproperLdapAuth.rb:15:23:15:26 | pass | semmle.label | pass |
13+
| ImproperLdapAuth.rb:24:5:24:8 | pass | semmle.label | pass |
14+
| ImproperLdapAuth.rb:24:12:24:17 | call to params | semmle.label | call to params |
15+
| ImproperLdapAuth.rb:24:12:24:24 | ...[...] | semmle.label | ...[...] |
16+
| ImproperLdapAuth.rb:31:24:31:27 | pass | semmle.label | pass |
17+
subpaths
18+
#select
19+
| 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 |
20+
| 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 |

0 commit comments

Comments
 (0)