Skip to content

Commit 6235312

Browse files
maikypediamaikypedia
committed
Add Improper LDAP Authentication query (CWE-287)
1 parent 2d8318d commit 6235312

File tree

12 files changed

+336
-8
lines changed

12 files changed

+336
-8
lines changed

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

Lines changed: 51 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -212,8 +212,7 @@ module FileSystemWriteAccess {
212212
* Extend this class to refine existing API models. If you want to model new APIs,
213213
* extend `FileSystemPermissionModification::Range` instead.
214214
*/
215-
class FileSystemPermissionModification extends DataFlow::Node instanceof FileSystemPermissionModification::Range
216-
{
215+
class FileSystemPermissionModification extends DataFlow::Node instanceof FileSystemPermissionModification::Range {
217216
/**
218217
* Gets an argument to this permission modification that is interpreted as a
219218
* set of permissions.
@@ -469,8 +468,7 @@ module Http {
469468
}
470469
}
471470

472-
private class RequestInputAccessAsRemoteFlowSource extends RemoteFlowSource::Range instanceof RequestInputAccess
473-
{
471+
private class RequestInputAccessAsRemoteFlowSource extends RemoteFlowSource::Range instanceof RequestInputAccess {
474472
override string getSourceType() { result = this.(RequestInputAccess).getSourceType() }
475473
}
476474

@@ -959,8 +957,7 @@ module Path {
959957
* Extend this class to refine existing API models. If you want to model new APIs,
960958
* extend `CookieSecurityConfigurationSetting::Range` instead.
961959
*/
962-
class CookieSecurityConfigurationSetting extends DataFlow::Node instanceof CookieSecurityConfigurationSetting::Range
963-
{
960+
class CookieSecurityConfigurationSetting extends DataFlow::Node instanceof CookieSecurityConfigurationSetting::Range {
964961
/**
965962
* Gets a description of how this cookie setting may weaken application security.
966963
* This predicate has no results if the setting is considered to be safe.
@@ -1040,8 +1037,7 @@ module Cryptography {
10401037
* Extend this class to refine existing API models. If you want to model new APIs,
10411038
* extend `CryptographicOperation::Range` instead.
10421039
*/
1043-
class CryptographicOperation extends SC::CryptographicOperation instanceof CryptographicOperation::Range
1044-
{
1040+
class CryptographicOperation extends SC::CryptographicOperation instanceof CryptographicOperation::Range {
10451041
/** DEPRECATED: Use `getAlgorithm().isWeak() or getBlockMode().isWeak()` instead */
10461042
deprecated predicate isWeak() { super.isWeak() }
10471043
}
@@ -1195,3 +1191,50 @@ module LdapExecution {
11951191
abstract DataFlow::Node getQuery();
11961192
}
11971193
}
1194+
1195+
/**
1196+
* A data-flow node that collects methods binding a LDAP connection.
1197+
*
1198+
* Extend this class to refine existing API models. If you want to model new APIs,
1199+
* extend `LdapBind::Range` instead.
1200+
*/
1201+
class LdapBind extends DataFlow::Node instanceof LdapBind::Range {
1202+
/** Gets the argument containing the binding host */
1203+
DataFlow::Node getHost() { result = super.getHost() }
1204+
1205+
/** Gets the argument containing the binding expression. */
1206+
DataFlow::Node getPassword() { result = super.getPassword() }
1207+
1208+
/** Holds if the binding process is anonymous. */
1209+
predicate isEmptyPassword() {
1210+
(
1211+
this.getPassword().getConstantValue().isStringlikeValue("")
1212+
or
1213+
this.getPassword().(DataFlow::ExprNode).getExprNode().getConstantValue().getValueType() =
1214+
"nil"
1215+
)
1216+
}
1217+
1218+
/** Holds if the binding process use SSL. */
1219+
predicate useSsl() { super.useSsl() }
1220+
}
1221+
1222+
/** Provides classes for modeling LDAP bind-related APIs. */
1223+
module LdapBind {
1224+
/**
1225+
* A data-flow node that collects methods binding a LDAP connection.
1226+
*
1227+
* Extend this class to model new APIs. If you want to refine existing API models,
1228+
* extend `LdapBind` instead.
1229+
*/
1230+
abstract class Range extends DataFlow::Node {
1231+
/** Gets the argument containing the binding host. */
1232+
abstract DataFlow::Node getHost();
1233+
1234+
/** Gets the argument containing the binding expression. */
1235+
abstract DataFlow::Node getPassword();
1236+
1237+
/** Holds if the binding process use SSL. */
1238+
abstract predicate useSsl();
1239+
}
1240+
}

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,24 @@ module NetLdap {
3030
/** A call that establishes a LDAP Connection */
3131
private class NetLdapConnection extends DataFlow::CallNode {
3232
NetLdapConnection() { this in [ldap().getAnInstantiation(), ldap().getAMethodCall(["open"])] }
33+
34+
predicate useSsl() {
35+
this.getKeywordArgument("encryption").getConstantValue().isStringlikeValue("simple_tls")
36+
or
37+
this.getAMethodCall("encryption")
38+
.getArgument(0)
39+
.getConstantValue()
40+
.isStringlikeValue(":simple_tls")
41+
or
42+
this.getAMethodCall("encryption")
43+
.getKeywordArgument("method")
44+
.getConstantValue()
45+
.isStringlikeValue("simple_tls")
46+
}
47+
48+
DataFlow::Node getAuthValue(string arg) {
49+
result = this.getKeywordArgument("auth").(DataFlow::CallNode).getKeywordArgument(arg)
50+
}
3351
}
3452

3553
/** A call that constructs a LDAP query */
@@ -45,4 +63,27 @@ module NetLdap {
4563

4664
override DataFlow::Node getQuery() { result = this.getKeywordArgument(_) }
4765
}
66+
67+
/** A call considered as a LDAP bind. */
68+
private class NetLdapBind extends LdapBind::Range, DataFlow::CallNode {
69+
NetLdapConnection l;
70+
71+
NetLdapBind() { this = l.getAMethodCall("bind") }
72+
73+
override DataFlow::Node getHost() {
74+
(
75+
result = l.getKeywordArgument("encryption")
76+
or
77+
result = l.getAMethodCall("encryption").getArgument(0)
78+
) and
79+
result.getConstantValue().isStringlikeValue(":simple_tls")
80+
}
81+
82+
override DataFlow::Node getPassword() {
83+
result = l.getAuthValue("password") or
84+
result = l.getAMethodCall("auth").getArgument(1)
85+
}
86+
87+
override predicate useSsl() { l.useSsl() }
88+
}
4889
}

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
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+
}

‎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-29-05-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)