Skip to content

Commit a063d7d

Browse files
yoffyoff
committed
Python: sinks -> decodings
Query operators that interpret JavaScript are no longer considered sinks. Instead they are considered decodings and the output is the tainted dictionary. The state changes to `DictInput` to reflect that the user now controls a dangerous dictionary. This fixes the spurious result and moves the error reporting to a more logical place.
1 parent d9f63e1 commit a063d7d

File tree

4 files changed

+44
-27
lines changed

4 files changed

+44
-27
lines changed

‎python/ql/lib/semmle/python/frameworks/NoSQL.qll

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -123,40 +123,49 @@ private module NoSql {
123123
}
124124

125125
/** The `$where` query operator executes a string as JavaScript. */
126-
private class WhereQueryOperator extends API::CallNode, NoSqlQuery::Range {
126+
private class WhereQueryOperator extends DataFlow::Node, Decoding::Range {
127+
API::Node dictionary;
127128
DataFlow::Node query;
128129

129130
WhereQueryOperator() {
130-
this = mongoCollection().getMember(mongoCollectionMethodName()).getACall() and
131-
query = this.getParameter(0).getSubscript("$where").asSink()
131+
dictionary =
132+
mongoCollection().getMember(mongoCollectionMethodName()).getACall().getParameter(0) and
133+
query = dictionary.getSubscript("$where").asSink() and
134+
this = dictionary.asSink()
132135
}
133136

134-
override DataFlow::Node getQuery() { result = query }
137+
override DataFlow::Node getAnInput() { result = query }
135138

136-
override predicate interpretsDict() { none() }
139+
override DataFlow::Node getOutput() { result = this }
137140

138-
override predicate vulnerableToStrings() { any() }
141+
override string getFormat() { result = "NoSQL" }
142+
143+
override predicate mayExecuteInput() { any() }
139144
}
140145

141146
/** The `$function` query operator executes its `body` string as JavaScript. */
142-
private class FunctionQueryOperator extends API::CallNode, NoSqlQuery::Range {
147+
private class FunctionQueryOperator extends DataFlow::Node, Decoding::Range {
148+
API::Node dictionary;
143149
DataFlow::Node query;
144150

145151
FunctionQueryOperator() {
146-
this = mongoCollection().getMember(mongoCollectionMethodName()).getACall() and
147-
query =
148-
this.getParameter(0)
149-
.getASubscript*()
150-
.getSubscript("$function")
151-
.getSubscript("body")
152-
.asSink()
152+
dictionary =
153+
mongoCollection()
154+
.getMember(mongoCollectionMethodName())
155+
.getACall()
156+
.getParameter(0)
157+
.getASubscript*() and
158+
query = dictionary.getSubscript("$function").getSubscript("body").asSink() and
159+
this = dictionary.asSink()
153160
}
154161

155-
override DataFlow::Node getQuery() { result = query }
162+
override DataFlow::Node getAnInput() { result = query }
163+
164+
override DataFlow::Node getOutput() { result = this }
156165

157-
override predicate interpretsDict() { none() }
166+
override string getFormat() { result = "NoSQL" }
158167

159-
override predicate vulnerableToStrings() { any() }
168+
override predicate mayExecuteInput() { any() }
160169
}
161170

162171
/**

‎python/ql/lib/semmle/python/security/dataflow/NoSQLInjectionCustomizations.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ module NoSqlInjection {
7575

7676
/** A JSON decoding converts a string to a dictionary. */
7777
class JsonDecoding extends Decoding, StringToDictConversion {
78-
JsonDecoding() { this.getFormat() = "JSON" }
78+
JsonDecoding() { this.getFormat() in ["JSON", "NoSQL"] }
7979

8080
override DataFlow::Node getAnInput() { result = Decoding.super.getAnInput() }
8181

‎python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,19 @@ edges
22
| PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:1:26:1:32 | GSSA Variable request |
33
| PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:26:21:26:27 | ControlFlowNode for request |
44
| PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:42:14:42:20 | ControlFlowNode for request |
5+
| PoC/server.py:1:26:1:32 | GSSA Variable request | PoC/server.py:51:14:51:20 | ControlFlowNode for request |
56
| PoC/server.py:26:5:26:17 | SSA variable author_string | PoC/server.py:27:25:27:37 | ControlFlowNode for author_string |
67
| PoC/server.py:26:21:26:27 | ControlFlowNode for request | PoC/server.py:26:5:26:17 | SSA variable author_string |
78
| PoC/server.py:27:5:27:10 | SSA variable author | PoC/server.py:30:27:30:44 | ControlFlowNode for Dict |
89
| PoC/server.py:27:14:27:38 | ControlFlowNode for Attribute() | PoC/server.py:27:5:27:10 | SSA variable author |
910
| PoC/server.py:27:25:27:37 | ControlFlowNode for author_string | PoC/server.py:27:14:27:38 | ControlFlowNode for Attribute() |
10-
| PoC/server.py:42:5:42:10 | SSA variable author | PoC/server.py:46:27:46:68 | ControlFlowNode for Dict |
1111
| PoC/server.py:42:5:42:10 | SSA variable author | PoC/server.py:46:38:46:67 | ControlFlowNode for BinaryExpr |
1212
| PoC/server.py:42:14:42:20 | ControlFlowNode for request | PoC/server.py:42:5:42:10 | SSA variable author |
13+
| PoC/server.py:46:38:46:67 | ControlFlowNode for BinaryExpr | PoC/server.py:46:27:46:68 | ControlFlowNode for Dict |
14+
| PoC/server.py:51:5:51:10 | SSA variable author | PoC/server.py:53:17:53:70 | ControlFlowNode for BinaryExpr |
15+
| PoC/server.py:51:14:51:20 | ControlFlowNode for request | PoC/server.py:51:5:51:10 | SSA variable author |
16+
| PoC/server.py:53:17:53:70 | ControlFlowNode for BinaryExpr | PoC/server.py:60:37:60:57 | ControlFlowNode for Dict |
17+
| PoC/server.py:60:37:60:57 | ControlFlowNode for Dict | PoC/server.py:60:27:60:58 | ControlFlowNode for Dict |
1318
| flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_mongoengine_bad.py:1:26:1:32 | GSSA Variable request |
1419
| flask_mongoengine_bad.py:1:26:1:32 | GSSA Variable request | flask_mongoengine_bad.py:19:21:19:27 | ControlFlowNode for request |
1520
| flask_mongoengine_bad.py:1:26:1:32 | GSSA Variable request | flask_mongoengine_bad.py:26:21:26:27 | ControlFlowNode for request |
@@ -76,16 +81,16 @@ edges
7681
| pymongo_test.py:13:5:13:15 | SSA variable json_search | pymongo_test.py:15:42:15:62 | ControlFlowNode for Dict |
7782
| pymongo_test.py:13:19:13:43 | ControlFlowNode for Attribute() | pymongo_test.py:13:5:13:15 | SSA variable json_search |
7883
| pymongo_test.py:13:30:13:42 | ControlFlowNode for unsafe_search | pymongo_test.py:13:19:13:43 | ControlFlowNode for Attribute() |
79-
| pymongo_test.py:29:5:29:12 | SSA variable event_id | pymongo_test.py:33:34:33:73 | ControlFlowNode for Dict |
8084
| pymongo_test.py:29:5:29:12 | SSA variable event_id | pymongo_test.py:33:45:33:72 | ControlFlowNode for Fstring |
8185
| pymongo_test.py:29:16:29:51 | ControlFlowNode for Attribute() | pymongo_test.py:29:5:29:12 | SSA variable event_id |
8286
| pymongo_test.py:29:27:29:33 | ControlFlowNode for request | pymongo_test.py:29:27:29:50 | ControlFlowNode for Subscript |
8387
| pymongo_test.py:29:27:29:50 | ControlFlowNode for Subscript | pymongo_test.py:29:16:29:51 | ControlFlowNode for Attribute() |
84-
| pymongo_test.py:39:5:39:12 | SSA variable event_id | pymongo_test.py:43:34:43:73 | ControlFlowNode for Dict |
88+
| pymongo_test.py:33:45:33:72 | ControlFlowNode for Fstring | pymongo_test.py:33:34:33:73 | ControlFlowNode for Dict |
8589
| pymongo_test.py:39:5:39:12 | SSA variable event_id | pymongo_test.py:43:45:43:72 | ControlFlowNode for Fstring |
8690
| pymongo_test.py:39:16:39:51 | ControlFlowNode for Attribute() | pymongo_test.py:39:5:39:12 | SSA variable event_id |
8791
| pymongo_test.py:39:27:39:33 | ControlFlowNode for request | pymongo_test.py:39:27:39:50 | ControlFlowNode for Subscript |
8892
| pymongo_test.py:39:27:39:50 | ControlFlowNode for Subscript | pymongo_test.py:39:16:39:51 | ControlFlowNode for Attribute() |
93+
| pymongo_test.py:43:45:43:72 | ControlFlowNode for Fstring | pymongo_test.py:43:34:43:73 | ControlFlowNode for Dict |
8994
nodes
9095
| PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember |
9196
| PoC/server.py:1:26:1:32 | GSSA Variable request | semmle.label | GSSA Variable request |
@@ -99,6 +104,11 @@ nodes
99104
| PoC/server.py:42:14:42:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
100105
| PoC/server.py:46:27:46:68 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict |
101106
| PoC/server.py:46:38:46:67 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr |
107+
| PoC/server.py:51:5:51:10 | SSA variable author | semmle.label | SSA variable author |
108+
| PoC/server.py:51:14:51:20 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
109+
| PoC/server.py:53:17:53:70 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr |
110+
| PoC/server.py:60:27:60:58 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict |
111+
| PoC/server.py:60:37:60:57 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict |
102112
| flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember |
103113
| flask_mongoengine_bad.py:1:26:1:32 | GSSA Variable request | semmle.label | GSSA Variable request |
104114
| flask_mongoengine_bad.py:19:5:19:17 | SSA variable unsafe_search | semmle.label | SSA variable unsafe_search |
@@ -183,7 +193,7 @@ subpaths
183193
#select
184194
| PoC/server.py:30:27:30:44 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:30:27:30:44 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
185195
| PoC/server.py:46:27:46:68 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:46:27:46:68 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
186-
| PoC/server.py:46:38:46:67 | ControlFlowNode for BinaryExpr | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:46:38:46:67 | ControlFlowNode for BinaryExpr | This NoSQL query contains an unsanitized $@. | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
196+
| PoC/server.py:60:27:60:58 | ControlFlowNode for Dict | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | PoC/server.py:60:27:60:58 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | PoC/server.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
187197
| flask_mongoengine_bad.py:22:34:22:44 | ControlFlowNode for json_search | flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_mongoengine_bad.py:22:34:22:44 | ControlFlowNode for json_search | This NoSQL query contains an unsanitized $@. | flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
188198
| flask_mongoengine_bad.py:30:39:30:59 | ControlFlowNode for Dict | flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_mongoengine_bad.py:30:39:30:59 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | flask_mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
189199
| flask_pymongo_bad.py:14:31:14:51 | ControlFlowNode for Dict | flask_pymongo_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | flask_pymongo_bad.py:14:31:14:51 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | flask_pymongo_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
@@ -195,6 +205,4 @@ subpaths
195205
| mongoengine_bad.py:61:29:61:49 | ControlFlowNode for Dict | mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | mongoengine_bad.py:61:29:61:49 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | mongoengine_bad.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
196206
| pymongo_test.py:15:42:15:62 | ControlFlowNode for Dict | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:15:42:15:62 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
197207
| pymongo_test.py:33:34:33:73 | ControlFlowNode for Dict | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:33:34:33:73 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
198-
| pymongo_test.py:33:45:33:72 | ControlFlowNode for Fstring | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:33:45:33:72 | ControlFlowNode for Fstring | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
199208
| pymongo_test.py:43:34:43:73 | ControlFlowNode for Dict | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:43:34:43:73 | ControlFlowNode for Dict | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |
200-
| pymongo_test.py:43:45:43:72 | ControlFlowNode for Fstring | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | pymongo_test.py:43:45:43:72 | ControlFlowNode for Fstring | This NoSQL query contains an unsanitized $@. | pymongo_test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |

‎python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/PoC/server.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ def by_where():
5050
def by_function():
5151
author = request.args['author']
5252
search = {
53-
"body": 'function(author) { return(author === "'+author+'") }', # $ result=BAD
53+
"body": 'function(author) { return(author === "'+author+'") }',
5454
"args": [ "$author" ],
5555
"lang": "js"
5656
}
@@ -64,11 +64,11 @@ def by_function():
6464
def by_function_arg():
6565
author = request.args['author']
6666
search = {
67-
"body": 'function(author, target) { return(author === target) }', # $ result=OK
67+
"body": 'function(author, target) { return(author === target) }',
6868
"args": [ "$author", author ],
6969
"lang": "js"
7070
}
71-
post = posts.find_one({'$expr': {'$function': search}}) # $ SPURIOUS: result=BAD
71+
post = posts.find_one({'$expr': {'$function': search}}) # $ result=OK
7272
return show_post(post, author)
7373

7474
@app.route('/', methods=['GET'])

0 commit comments

Comments
 (0)