Skip to content

Commit c55b7bc

Browse files
erik-krogherik-krogh
committed
model ldap filters as taint steps
1 parent 9b5ff66 commit c55b7bc

File tree

5 files changed

+99
-67
lines changed

5 files changed

+99
-67
lines changed

‎javascript/ql/lib/semmle/javascript/frameworks/Ldapjs.qll

Lines changed: 50 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -6,37 +6,37 @@ import javascript
66

77
module Ldapjs {
88
/**
9-
* Gets a method name on an LDAPjs client that accepts a DN as the first argument.
9+
* Gets a data flow source node for the ldapjs library.
1010
*/
11-
private string getLdapjsClientDNMethodName() {
12-
result = ["add", "bind", "compare", "del", "modify", "modifyDN", "search"]
13-
}
11+
private API::Node ldapjs() { result = API::moduleImport("ldapjs") } // TODO: createServer, parseDN.
1412

1513
/**
16-
* Gets a data flow source node for an LDAP client.
14+
* Gets an LDAP client.
1715
*/
18-
abstract class LdapClient extends API::Node { }
16+
private API::Node ldapClient() { result = ldapjs().getMember("createClient").getReturn() }
1917

2018
/**
21-
* Gets a data flow source node for the ldapjs library.
19+
* A call to the ldapjs Client API methods.
2220
*/
23-
private API::Node ldapjs() { result = API::moduleImport("ldapjs") }
21+
class LdapjsClientAPICall extends API::CallNode {
22+
string methodName;
2423

25-
/**
26-
* Gets a data flow source node for the ldapjs client.
27-
*/
28-
class LdapjsClient extends LdapClient {
29-
LdapjsClient() { this = ldapjs().getMember("createClient").getReturn() }
24+
LdapjsClientAPICall() {
25+
methodName = ["add", "bind", "compare", "del", "modify", "modifyDN", "search"] and
26+
this = ldapClient().getMember(methodName).getACall()
27+
}
28+
29+
string getMethodName() { result = methodName }
3030
}
3131

3232
/**
3333
* Gets a data flow node for the client `search` options.
3434
*/
3535
class LdapjsSearchOptions extends API::Node {
36-
API::CallNode queryCall;
36+
LdapjsClientAPICall queryCall;
3737

3838
LdapjsSearchOptions() {
39-
queryCall = any(LdapjsClient client).getMember("search").getACall() and
39+
queryCall.getMethodName() = "search" and
4040
this = queryCall.getParameter(1)
4141
}
4242

@@ -47,46 +47,59 @@ module Ldapjs {
4747
}
4848

4949
/**
50-
* A filter used in a `search` operation against the LDAP server.
50+
* A distinguished name (DN) used in a Client API call against the LDAP server.
5151
*/
52-
class LdapjsSearchFilter extends DataFlow::Node {
53-
LdapjsSearchOptions options;
52+
class LdapjsDNArgument extends DataFlow::Node {
53+
LdapjsClientAPICall queryCall;
5454

55-
LdapjsSearchFilter() { this = options.getMember("filter").getARhs() }
55+
LdapjsDNArgument() { this = queryCall.getArgument(0) }
5656

5757
/**
58-
* Gets the LDAP query call that this filter is used in.
58+
* Gets the LDAP query call that this DN is used in.
5959
*/
60-
API::CallNode getQueryCall() { result = options.getQueryCall() }
60+
API::CallNode getQueryCall() { result = queryCall }
6161
}
6262

6363
/**
64-
* A call to the ldapjs Client API methods.
64+
* A creation of an Ldap filter that doesn't sanitizes the input.
6565
*/
66-
class LdapjsClientAPICall extends API::CallNode {
67-
LdapjsClientAPICall() {
68-
this = any(LdapjsClient client).getMember(getLdapjsClientDNMethodName()).getACall()
69-
}
66+
abstract class LdapFilter extends DataFlow::Node {
67+
/**
68+
* The input that creates (part of) an Ldap filter.
69+
*/
70+
abstract DataFlow::Node getInput();
71+
72+
/**
73+
* The resulting Ldap filter.
74+
*/
75+
abstract DataFlow::Node getOutput();
7076
}
7177

7278
/**
73-
* A distinguished name (DN) used in a Client API call against the LDAP server.
79+
* Ldapjs parseFilter method call.
7480
*/
75-
class LdapjsDNArgument extends DataFlow::Node {
76-
LdapjsClientAPICall queryCall;
81+
private class LdapjsParseFilter extends LdapFilter, API::CallNode {
82+
LdapjsParseFilter() { this = ldapjs().getMember("parseFilter").getACall() }
7783

78-
LdapjsDNArgument() { this = queryCall.getArgument(0) }
84+
override DataFlow::Node getInput() { result = this.getArgument(0) }
7985

80-
/**
81-
* Gets the LDAP query call that this DN is used in.
82-
*/
83-
API::CallNode getQueryCall() { result = queryCall }
86+
override DataFlow::Node getOutput() { result = this }
8487
}
8588

8689
/**
87-
* Ldapjs parseFilter method call.
90+
* A filter used in call to "search" on an LDAP client.
91+
* We model that as a step from the ".filter" write to the options object itself.
8892
*/
89-
class LdapjsParseFilter extends DataFlow::CallNode {
90-
LdapjsParseFilter() { this = ldapjs().getMember("parseFilter").getACall() }
93+
class LdapjsSearchFilter extends LdapFilter {
94+
LdapjsSearchOptions options;
95+
96+
LdapjsSearchFilter() {
97+
options = ldapClient().getMember("search").getACall().getParameter(1) and
98+
this = options.getARhs()
99+
}
100+
101+
override DataFlow::Node getInput() { result = options.getMember("filter").getARhs() }
102+
103+
override DataFlow::Node getOutput() { result = this }
91104
}
92105
}

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

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -43,27 +43,14 @@ module SqlInjection {
4343
}
4444

4545
/**
46-
* An LDAP filter for an API call that executes an operation against the LDAP server.
46+
* An LDAPjs sink.
4747
*/
48-
class LdapjsSearchFilterAsSink extends Sink {
49-
// TODO: As taint-step?
50-
/*
51-
* override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
52-
* exists(LdapjsParseFilter filter |
53-
* pred = filter.getArgument(0) and
54-
* succ = filter
55-
* )
56-
* }
57-
*/
58-
59-
LdapjsSearchFilterAsSink() { this instanceof Ldapjs::LdapjsSearchFilter }
60-
}
61-
62-
/**
63-
* An LDAP DN argument for an API call that executes an operation against the LDAP server.
64-
*/
65-
class LdapjsDNArgumentAsSink extends Sink {
66-
LdapjsDNArgumentAsSink() { this instanceof Ldapjs::LdapjsDNArgument }
48+
class LdapJSSink extends Sink {
49+
LdapJSSink() {
50+
this instanceof Ldapjs::LdapjsDNArgument
51+
or
52+
this = any(Ldapjs::LdapjsSearchOptions opt).getARhs()
53+
}
6754
}
6855

6956
/**

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,11 @@ class Configuration extends TaintTracking::Configuration {
2424
super.isSanitizer(node) or
2525
node instanceof Sanitizer
2626
}
27+
28+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
29+
exists(Ldapjs::LdapFilter filter |
30+
pred = filter.getInput() and
31+
succ = filter.getOutput()
32+
)
33+
}
2734
}

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -77,13 +77,23 @@ nodes
7777
| ldap.js:22:18:22:24 | q.query |
7878
| ldap.js:22:18:22:33 | q.query.username |
7979
| ldap.js:25:13:25:57 | `(\|(nam ... ame}))` |
80-
| ldap.js:25:13:25:57 | `(\|(nam ... ame}))` |
8180
| ldap.js:25:24:25:31 | username |
8281
| ldap.js:25:46:25:53 | username |
83-
| ldap.js:32:15:32:59 | `(\|(nam ... ame}))` |
82+
| ldap.js:28:30:28:34 | opts1 |
83+
| ldap.js:28:30:28:34 | opts1 |
84+
| ldap.js:32:5:32:61 | { filte ... e}))` } |
85+
| ldap.js:32:5:32:61 | { filte ... e}))` } |
8486
| ldap.js:32:15:32:59 | `(\|(nam ... ame}))` |
8587
| ldap.js:32:26:32:33 | username |
8688
| ldap.js:32:48:32:55 | username |
89+
| ldap.js:63:9:65:3 | parsedFilter |
90+
| ldap.js:63:24:65:3 | ldap.pa ... ))`\\n ) |
91+
| ldap.js:64:5:64:49 | `(\|(nam ... ame}))` |
92+
| ldap.js:64:16:64:23 | username |
93+
| ldap.js:64:38:64:45 | username |
94+
| ldap.js:66:30:66:53 | { filte ... ilter } |
95+
| ldap.js:66:30:66:53 | { filte ... ilter } |
96+
| ldap.js:66:40:66:51 | parsedFilter |
8797
| marsdb-flow-to.js:10:9:10:18 | query |
8898
| marsdb-flow-to.js:10:17:10:18 | {} |
8999
| marsdb-flow-to.js:11:17:11:24 | req.body |
@@ -468,17 +478,26 @@ edges
468478
| ldap.js:22:7:22:33 | username | ldap.js:25:46:25:53 | username |
469479
| ldap.js:22:7:22:33 | username | ldap.js:32:26:32:33 | username |
470480
| ldap.js:22:7:22:33 | username | ldap.js:32:48:32:55 | username |
481+
| ldap.js:22:7:22:33 | username | ldap.js:64:16:64:23 | username |
482+
| ldap.js:22:7:22:33 | username | ldap.js:64:38:64:45 | username |
471483
| ldap.js:22:18:22:18 | q | ldap.js:22:18:22:24 | q.query |
472484
| ldap.js:22:18:22:24 | q.query | ldap.js:22:18:22:33 | q.query.username |
473485
| ldap.js:22:18:22:33 | q.query.username | ldap.js:22:7:22:33 | username |
474-
| ldap.js:25:24:25:31 | username | ldap.js:25:13:25:57 | `(\|(nam ... ame}))` |
486+
| ldap.js:25:13:25:57 | `(\|(nam ... ame}))` | ldap.js:28:30:28:34 | opts1 |
487+
| ldap.js:25:13:25:57 | `(\|(nam ... ame}))` | ldap.js:28:30:28:34 | opts1 |
475488
| ldap.js:25:24:25:31 | username | ldap.js:25:13:25:57 | `(\|(nam ... ame}))` |
476489
| ldap.js:25:46:25:53 | username | ldap.js:25:13:25:57 | `(\|(nam ... ame}))` |
477-
| ldap.js:25:46:25:53 | username | ldap.js:25:13:25:57 | `(\|(nam ... ame}))` |
478-
| ldap.js:32:26:32:33 | username | ldap.js:32:15:32:59 | `(\|(nam ... ame}))` |
490+
| ldap.js:32:15:32:59 | `(\|(nam ... ame}))` | ldap.js:32:5:32:61 | { filte ... e}))` } |
491+
| ldap.js:32:15:32:59 | `(\|(nam ... ame}))` | ldap.js:32:5:32:61 | { filte ... e}))` } |
479492
| ldap.js:32:26:32:33 | username | ldap.js:32:15:32:59 | `(\|(nam ... ame}))` |
480493
| ldap.js:32:48:32:55 | username | ldap.js:32:15:32:59 | `(\|(nam ... ame}))` |
481-
| ldap.js:32:48:32:55 | username | ldap.js:32:15:32:59 | `(\|(nam ... ame}))` |
494+
| ldap.js:63:9:65:3 | parsedFilter | ldap.js:66:40:66:51 | parsedFilter |
495+
| ldap.js:63:24:65:3 | ldap.pa ... ))`\\n ) | ldap.js:63:9:65:3 | parsedFilter |
496+
| ldap.js:64:5:64:49 | `(\|(nam ... ame}))` | ldap.js:63:24:65:3 | ldap.pa ... ))`\\n ) |
497+
| ldap.js:64:16:64:23 | username | ldap.js:64:5:64:49 | `(\|(nam ... ame}))` |
498+
| ldap.js:64:38:64:45 | username | ldap.js:64:5:64:49 | `(\|(nam ... ame}))` |
499+
| ldap.js:66:40:66:51 | parsedFilter | ldap.js:66:30:66:53 | { filte ... ilter } |
500+
| ldap.js:66:40:66:51 | parsedFilter | ldap.js:66:30:66:53 | { filte ... ilter } |
482501
| marsdb-flow-to.js:10:9:10:18 | query | marsdb-flow-to.js:14:17:14:21 | query |
483502
| marsdb-flow-to.js:10:9:10:18 | query | marsdb-flow-to.js:14:17:14:21 | query |
484503
| marsdb-flow-to.js:10:17:10:18 | {} | marsdb-flow-to.js:10:9:10:18 | query |
@@ -887,8 +906,9 @@ edges
887906
| json-schema-validator.js:55:22:55:26 | query | json-schema-validator.js:50:34:50:47 | req.query.data | json-schema-validator.js:55:22:55:26 | query | This query depends on $@. | json-schema-validator.js:50:34:50:47 | req.query.data | a user-provided value |
888907
| json-schema-validator.js:59:22:59:26 | query | json-schema-validator.js:50:34:50:47 | req.query.data | json-schema-validator.js:59:22:59:26 | query | This query depends on $@. | json-schema-validator.js:50:34:50:47 | req.query.data | a user-provided value |
889908
| json-schema-validator.js:61:22:61:26 | query | json-schema-validator.js:50:34:50:47 | req.query.data | json-schema-validator.js:61:22:61:26 | query | This query depends on $@. | json-schema-validator.js:50:34:50:47 | req.query.data | a user-provided value |
890-
| ldap.js:25:13:25:57 | `(\|(nam ... ame}))` | ldap.js:20:21:20:27 | req.url | ldap.js:25:13:25:57 | `(\|(nam ... ame}))` | This query depends on $@. | ldap.js:20:21:20:27 | req.url | a user-provided value |
891-
| ldap.js:32:15:32:59 | `(\|(nam ... ame}))` | ldap.js:20:21:20:27 | req.url | ldap.js:32:15:32:59 | `(\|(nam ... ame}))` | This query depends on $@. | ldap.js:20:21:20:27 | req.url | a user-provided value |
909+
| ldap.js:28:30:28:34 | opts1 | ldap.js:20:21:20:27 | req.url | ldap.js:28:30:28:34 | opts1 | This query depends on $@. | ldap.js:20:21:20:27 | req.url | a user-provided value |
910+
| ldap.js:32:5:32:61 | { filte ... e}))` } | ldap.js:20:21:20:27 | req.url | ldap.js:32:5:32:61 | { filte ... e}))` } | This query depends on $@. | ldap.js:20:21:20:27 | req.url | a user-provided value |
911+
| ldap.js:66:30:66:53 | { filte ... ilter } | ldap.js:20:21:20:27 | req.url | ldap.js:66:30:66:53 | { filte ... ilter } | This query depends on $@. | ldap.js:20:21:20:27 | req.url | a user-provided value |
892912
| marsdb-flow-to.js:14:17:14:21 | query | marsdb-flow-to.js:11:17:11:24 | req.body | marsdb-flow-to.js:14:17:14:21 | query | This query depends on $@. | marsdb-flow-to.js:11:17:11:24 | req.body | a user-provided value |
893913
| marsdb.js:16:12:16:16 | query | marsdb.js:13:17:13:24 | req.body | marsdb.js:16:12:16:16 | query | This query depends on $@. | marsdb.js:13:17:13:24 | req.body | a user-provided value |
894914
| minimongo.js:18:12:18:16 | query | minimongo.js:15:17:15:24 | req.body | minimongo.js:18:12:18:16 | query | This query depends on $@. | minimongo.js:15:17:15:24 | req.body | a user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-089/untyped/ldap.js

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,10 @@ const server = http.createServer((req, res) => {
2222
let username = q.query.username;
2323

2424
var opts1 = {
25-
filter: `(|(name=${username})(username=${username}))`, // NOT OK
25+
filter: `(|(name=${username})(username=${username}))`,
2626
};
2727

28-
client.search("o=example", opts1, function (err, res) {});
28+
client.search("o=example", opts1, function (err, res) {}); // NOT OK
2929

3030
client.search(
3131
"o=example",
@@ -59,6 +59,11 @@ const server = http.createServer((req, res) => {
5959
});
6060

6161
client.search("o=example", { filter: f }, function (err, res) {});
62+
63+
const parsedFilter = ldap.parseFilter(
64+
`(|(name=${username})(username=${username}))`
65+
);
66+
client.search("o=example", { filter: parsedFilter }, function (err, res) {}); // NOT OK
6267
});
6368

6469
server.listen(389, () => {});

0 commit comments

Comments
 (0)