Skip to content

Commit 9ab6eab

Browse files
maikypediamaikypedia
committed
add filterTaintStep, qhelp file and test files
1 parent 026d94c commit 9ab6eab

File tree

6 files changed

+125
-52
lines changed

6 files changed

+125
-52
lines changed

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

Lines changed: 13 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,6 @@ private import codeql.ruby.dataflow.FlowSummary
88
private import codeql.ruby.Concepts
99
private import codeql.ruby.CFG
1010
private import codeql.ruby.AST
11-
private import codeql.ruby.Concepts
12-
private import codeql.ruby.controlflow.CfgNodes
13-
private import codeql.ruby.DataFlow
14-
private import codeql.ruby.dataflow.internal.DataFlowDispatch
15-
private import codeql.ruby.dataflow.internal.DataFlowPrivate
16-
private import codeql.ruby.ApiGraphs
17-
private import codeql.ruby.frameworks.Stdlib
18-
private import codeql.ruby.frameworks.Core
1911

2012
/**
2113
* Provides modeling for `net-ldap` a ruby library for LDAP.
@@ -27,53 +19,32 @@ module NetLdap {
2719
private class LdapSummary extends SummarizedCallable {
2820
LdapSummary() { this = "Net::LDAP.new" }
2921

30-
override MethodCall getACall() { result = any(LdapConnection l).asExpr().getExpr() }
22+
override MethodCall getACall() { result = any(NetLdapConnection l).asExpr().getExpr() }
3123

3224
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
3325
input = "Argument[0]" and output = "ReturnValue" and preservesValue = false
3426
}
3527
}
3628

37-
private class LdapConnection extends DataFlow::CallNode {
38-
LdapConnection() {
39-
this = API::getTopLevelMember("Net").getMember("LDAP").getAnInstantiation() or
40-
this = API::getTopLevelMember("Net").getMember("LDAP").getAMethodCall(["open"])
41-
}
42-
}
29+
/** Net LDAP Api Nodde */
30+
private API::Node ldap() { result = API::getTopLevelMember("Net").getMember("LDAP") }
4331

44-
/** A call to ` Net::LDAP::Filter.eq`, considered as a LDAP construction. */
45-
private class NetLdapConstruction extends LdapConstruction::Range, DataFlow::CallNode {
46-
DataFlow::Node query;
32+
/** A call that establishes a LDAP Connection */
33+
private class NetLdapConnection extends DataFlow::CallNode {
34+
NetLdapConnection() { this in [ldap().getAnInstantiation(), ldap().getAMethodCall(["open"])] }
35+
}
4736

48-
NetLdapConstruction() {
49-
this =
50-
API::getTopLevelMember("Net").getMember("LDAP").getMember("Filter").getAMethodCall(["eq"]) and
51-
query = this.getArgument([0, 1])
52-
}
37+
/** A call that constructs a LDAP query */
38+
private class NetLdapFilter extends LdapConstruction::Range, DataFlow::CallNode {
39+
NetLdapFilter() { this = any(ldap().getMember("Filter").getAMethodCall(["eq"])) }
5340

54-
override DataFlow::Node getQuery() { result = query }
41+
override DataFlow::Node getQuery() { result = this.getArgument([0, 1]) }
5542
}
5643

5744
/** A call considered as a LDAP execution. */
5845
private class NetLdapExecution extends LdapExecution::Range, DataFlow::CallNode {
59-
DataFlow::Node query;
60-
61-
NetLdapExecution() {
62-
// detecta cuando la query es una string pej
63-
// ldap.search(base: "ou=#{name},dc=example,dc=com"
64-
exists(LdapConnection ldapConnection |
65-
this = ldapConnection.getAMethodCall("search") and
66-
query = this.getKeywordArgument(_)
67-
)
68-
or
69-
// ignora esta parte
70-
exists(LdapConnection ldapConnection, NetLdapConstruction ldapConstruction |
71-
this = ldapConnection.getAMethodCall("search") and
72-
ldapConstruction = this.getKeywordArgument(_) and
73-
query = ldapConstruction
74-
)
75-
}
46+
NetLdapExecution() { this = any(NetLdapConnection l).getAMethodCall("search") }
7647

77-
override DataFlow::Node getQuery() { result = query }
48+
override DataFlow::Node getQuery() { result = this.getKeywordArgument(_) }
7849
}
7950
}

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

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ private import codeql.ruby.DataFlow
88
private import codeql.ruby.dataflow.BarrierGuards
99
private import codeql.ruby.dataflow.RemoteFlowSources
1010
private import codeql.ruby.ApiGraphs
11-
private import codeql.ruby.dataflow.FlowSummary
12-
private import codeql.ruby.CFG
1311

1412
/**
1513
* Provides default sources, sinks and sanitizers for detecting
@@ -29,12 +27,18 @@ module LdapInjection {
2927
* Additional taint steps for "LDAP Injection" vulnerabilities.
3028
*/
3129
predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
32-
exists(API::Node n, API::Node n2 |
33-
n = API::getTopLevelMember("Net").getMember("LDAP").getMember("Filter")
34-
|
35-
n2 = API::getTopLevelMember("Net").getMember("LDAP") and
36-
nodeTo = n2.getAMethodCall(["new"]).getAMethodCall(["search"]) and
37-
nodeFrom = n.getAMethodCall(["eq"]).getArgument([0, 1])
30+
filterTaintStep(nodeFrom, nodeTo)
31+
}
32+
33+
private predicate filterTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
34+
exists(DataFlow::CallNode filterCall |
35+
(
36+
filterCall =
37+
API::getTopLevelMember("Net").getMember("LDAP").getMember("Filter").getAMethodCall(["eq"]) or
38+
filterCall.getMethodName() = ["[]"]
39+
) and
40+
n1 = filterCall.getArgument([0, 1]) and
41+
n2 = filterCall
3842
)
3943
}
4044

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@
55

66
private import codeql.ruby.DataFlow
77
private import codeql.ruby.TaintTracking
8-
import LdapInjectionCustomizations
9-
import LdapInjectionCustomizations::LdapInjection
8+
private import LdapInjectionCustomizations
9+
private import LdapInjectionCustomizations::LdapInjection
1010

1111
/**
1212
* A taint-tracking configuration for detecting LDAP Injections vulnerabilities.

‎ruby/ql/src/experimental/ldap-injection/LdapInjection.qhelp

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
If an LDAP query or DN is built using string concatenation or string formatting, and the
9+
components of the concatenation include user input without any proper sanitization, a user
10+
is likely to be able to run malicious LDAP queries.
11+
</p>
12+
</overview>
13+
14+
<recommendation>
15+
<p>
16+
If user input must be included in an LDAP query or DN, it should be escaped to
17+
avoid a malicious user providing special characters that change the meaning
18+
of the query.
19+
</p>
20+
</recommendation>
21+
22+
<example>
23+
<p>
24+
In the following Rails example, an <code>ActionController</code> class
25+
has a <code>ldap_handler</code> method to handle requests.
26+
</p>
27+
28+
<p>
29+
The user and dc is specified using a parameter, <code>user_name</code> and <code>dc</code> provided by
30+
the client which it then uses to build a LDAP query and DN. This value is accessible using the <code>params</code> method.
31+
</p>
32+
33+
<p>The first example uses the unsanitized user input directly
34+
in the search filter and DN for the LDAP query.
35+
A malicious user could provide special characters to change the meaning of these
36+
components, and search for a completely different set of values.</p>
37+
38+
<sample src="examples/LdapInjectionBad.rb" />
39+
40+
<p>In the second example, the input provided by the user is sanitized before it is included in the search filter or DN.
41+
This ensures the meaning of the query cannot be changed by a malicious user.</p>
42+
43+
<sample src="examples/LdapInjectionBad.rb" />
44+
</example>
45+
46+
<references>
47+
<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/LDAP_Injection_Prevention_Cheat_Sheet.html">LDAP Injection Prevention Cheat Sheet</a>.</li>
48+
<li>OWASP: <a href="https://owasp.org/www-community/attacks/LDAP_Injection">LDAP Injection</a>.</li>
49+
</references>
50+
</qhelp>

‎ruby/ql/src/experimental/ldap-injection/examples/LdapInjectionBad.rb

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
require 'net/ldap'
2+
3+
class BadLdapController < ActionController::Base
4+
def ldap_handler
5+
name = params[:user_name]
6+
dc = params[:dc]
7+
ldap = Net::LDAP.new(
8+
host: 'ldap.example.com',
9+
port: 636,
10+
encryption: :simple_tls,
11+
auth: {
12+
method: :simple,
13+
username: 'uid=admin,dc=example,dc=com',
14+
password: 'adminpassword'
15+
}
16+
)
17+
filter = Net::LDAP::Filter.eq('foo', name)
18+
attrs = [name]
19+
result = ldap.search(base: "ou=people,dc=#{dc},dc=com", filter: filter, attributes: attrs)
20+
end
21+
end

‎ruby/ql/src/experimental/ldap-injection/examples/LdapInjectionGood.rb

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
require 'net/ldap'
2+
3+
class GoodLdapController < ActionController::Base
4+
def ldap_handler
5+
name = params[:user_name]
6+
ldap = Net::LDAP.new(
7+
host: 'ldap.example.com',
8+
port: 636,
9+
encryption: :simple_tls,
10+
auth: {
11+
method: :simple,
12+
username: 'uid=admin,dc=example,dc=com',
13+
password: 'adminpassword'
14+
}
15+
)
16+
17+
name = if ["admin", "guest"].include? name
18+
name
19+
else
20+
name = "none"
21+
end
22+
23+
filter = Net::LDAP::Filter.eq('foo', name)
24+
attrs = ['dn']
25+
result = ldap.search(base: 'ou=people,dc=example,dc=com', filter: filter, attributes: attrs)
26+
end
27+
end

0 commit comments

Comments
 (0)