Skip to content

Commit edbbc84

Browse files
codeql-cicodeql-ci
authored
Merge pull request #4753 from max-schaefer/js/more-nosql-query-args
Approved by asgerf, mchammer01
2 parents 04bacf4 + 978d2db commit edbbc84

File tree

6 files changed

+134
-2
lines changed

6 files changed

+134
-2
lines changed

‎javascript/change-notes/2020-11-30-nosql.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The query "Database query built from user-controlled sources" (`js/sql-injection`) has been improved to recognize more Mongoose APIs that may interpret untrusted user input as a query.

‎javascript/ql/src/Security/CWE-089/SqlInjection.qhelp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,11 @@ Most database connector libraries offer a way of safely
1717
embedding untrusted data into a query by means of query parameters
1818
or prepared statements.
1919
</p>
20+
<p>
21+
For NoSQL queries, make use of an operator like MongoDB's <code>$eq</code>
22+
to ensure that untrusted data is interpreted as a literal value and not as
23+
a query object.
24+
</p>
2025
</recommendation>
2126

2227
<example>
@@ -52,5 +57,6 @@ immune to injection attacks.
5257

5358
<references>
5459
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/SQL_injection">SQL injection</a>.</li>
60+
<li>MongoDB: <a href="https://docs.mongodb.com/manual/reference/operator/query/eq">$eq operator</a>.</li>
5561
</references>
5662
</qhelp>

‎javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,9 +237,17 @@ private module Mongoose {
237237
// implement lots of the MongoDB collection interface
238238
MongoDB::CollectionMethodSignatures::interpretsArgumentAsQuery(name, n)
239239
or
240-
name = "findByIdAndUpdate" and n = 1
240+
name = "find" + ["ById", "One"] + "AndUpdate" and n = 1
241241
or
242-
name = "where" and n = 0
242+
name in ["delete" + ["Many", "One"], "geoSearch", "remove", "replaceOne", "where"] and
243+
n = 0
244+
or
245+
name in [
246+
"find" + ["", "ById", "One"],
247+
"find" + ["ById", "One"] + "And" + ["Delete", "Remove", "Update"],
248+
"update" + ["", "Many", "One"]
249+
] and
250+
n = 0
243251
}
244252

245253
/**
@@ -262,6 +270,7 @@ private module Mongoose {
262270
name = "findOneAndReplace" or
263271
name = "findOneAndUpdate" or
264272
name = "geosearch" or
273+
name = "remove" or
265274
name = "replaceOne" or
266275
name = "update" or
267276
name = "updateMany" or

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
| mongoose.js:96:2:96:52 | Documen ... query)) |
3333
| mongoose.js:97:2:97:52 | Documen ... query)) |
3434
| mongoose.js:99:2:99:50 | Documen ... query)) |
35+
| mongoose.js:113:2:113:53 | Documen ... () { }) |
3536
| socketio.js:11:5:11:54 | db.run( ... ndle}`) |
3637
| tst2.js:7:3:7:62 | sql.que ... ms.id}` |
3738
| tst2.js:9:3:9:85 | new sql ... + "'") |

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

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,45 @@ nodes
138138
| mongoose.js:96:46:96:50 | query |
139139
| mongoose.js:111:14:111:18 | query |
140140
| mongoose.js:111:14:111:18 | query |
141+
| mongoose.js:113:31:113:35 | query |
142+
| mongoose.js:113:31:113:35 | query |
143+
| mongoose.js:115:6:115:22 | id |
144+
| mongoose.js:115:11:115:22 | req.query.id |
145+
| mongoose.js:115:11:115:22 | req.query.id |
146+
| mongoose.js:115:25:115:45 | cond |
147+
| mongoose.js:115:32:115:45 | req.query.cond |
148+
| mongoose.js:115:32:115:45 | req.query.cond |
149+
| mongoose.js:116:22:116:25 | cond |
150+
| mongoose.js:116:22:116:25 | cond |
151+
| mongoose.js:117:21:117:24 | cond |
152+
| mongoose.js:117:21:117:24 | cond |
153+
| mongoose.js:118:21:118:24 | cond |
154+
| mongoose.js:118:21:118:24 | cond |
155+
| mongoose.js:119:18:119:21 | cond |
156+
| mongoose.js:119:18:119:21 | cond |
157+
| mongoose.js:120:22:120:25 | cond |
158+
| mongoose.js:120:22:120:25 | cond |
159+
| mongoose.js:121:16:121:19 | cond |
160+
| mongoose.js:121:16:121:19 | cond |
161+
| mongoose.js:122:19:122:22 | cond |
162+
| mongoose.js:122:19:122:22 | cond |
163+
| mongoose.js:123:20:123:21 | id |
164+
| mongoose.js:123:20:123:21 | id |
165+
| mongoose.js:124:28:124:31 | cond |
166+
| mongoose.js:124:28:124:31 | cond |
167+
| mongoose.js:125:28:125:31 | cond |
168+
| mongoose.js:125:28:125:31 | cond |
169+
| mongoose.js:126:28:126:31 | cond |
170+
| mongoose.js:126:28:126:31 | cond |
171+
| mongoose.js:127:18:127:21 | cond |
172+
| mongoose.js:127:18:127:21 | cond |
173+
| mongoose.js:128:22:128:25 | cond |
174+
| mongoose.js:128:22:128:25 | cond |
175+
| mongoose.js:129:21:129:24 | cond |
176+
| mongoose.js:129:21:129:24 | cond |
177+
| mongoose.js:130:16:130:26 | { _id: id } |
178+
| mongoose.js:130:16:130:26 | { _id: id } |
179+
| mongoose.js:130:23:130:24 | id |
141180
| mongooseJsonParse.js:19:11:19:20 | query |
142181
| mongooseJsonParse.js:19:19:19:20 | {} |
143182
| mongooseJsonParse.js:20:19:20:44 | JSON.pa ... y.data) |
@@ -369,6 +408,8 @@ edges
369408
| mongoose.js:20:11:20:20 | query | mongoose.js:96:46:96:50 | query |
370409
| mongoose.js:20:11:20:20 | query | mongoose.js:111:14:111:18 | query |
371410
| mongoose.js:20:11:20:20 | query | mongoose.js:111:14:111:18 | query |
411+
| mongoose.js:20:11:20:20 | query | mongoose.js:113:31:113:35 | query |
412+
| mongoose.js:20:11:20:20 | query | mongoose.js:113:31:113:35 | query |
372413
| mongoose.js:20:19:20:20 | {} | mongoose.js:20:11:20:20 | query |
373414
| mongoose.js:21:19:21:26 | req.body | mongoose.js:21:19:21:32 | req.body.title |
374415
| mongoose.js:21:19:21:26 | req.body | mongoose.js:21:19:21:32 | req.body.title |
@@ -437,8 +478,45 @@ edges
437478
| mongoose.js:21:19:21:32 | req.body.title | mongoose.js:96:46:96:50 | query |
438479
| mongoose.js:21:19:21:32 | req.body.title | mongoose.js:111:14:111:18 | query |
439480
| mongoose.js:21:19:21:32 | req.body.title | mongoose.js:111:14:111:18 | query |
481+
| mongoose.js:21:19:21:32 | req.body.title | mongoose.js:113:31:113:35 | query |
482+
| mongoose.js:21:19:21:32 | req.body.title | mongoose.js:113:31:113:35 | query |
440483
| mongoose.js:24:25:24:29 | query | mongoose.js:24:24:24:30 | [query] |
441484
| mongoose.js:24:25:24:29 | query | mongoose.js:24:24:24:30 | [query] |
485+
| mongoose.js:115:6:115:22 | id | mongoose.js:123:20:123:21 | id |
486+
| mongoose.js:115:6:115:22 | id | mongoose.js:123:20:123:21 | id |
487+
| mongoose.js:115:6:115:22 | id | mongoose.js:130:23:130:24 | id |
488+
| mongoose.js:115:11:115:22 | req.query.id | mongoose.js:115:6:115:22 | id |
489+
| mongoose.js:115:11:115:22 | req.query.id | mongoose.js:115:6:115:22 | id |
490+
| mongoose.js:115:25:115:45 | cond | mongoose.js:116:22:116:25 | cond |
491+
| mongoose.js:115:25:115:45 | cond | mongoose.js:116:22:116:25 | cond |
492+
| mongoose.js:115:25:115:45 | cond | mongoose.js:117:21:117:24 | cond |
493+
| mongoose.js:115:25:115:45 | cond | mongoose.js:117:21:117:24 | cond |
494+
| mongoose.js:115:25:115:45 | cond | mongoose.js:118:21:118:24 | cond |
495+
| mongoose.js:115:25:115:45 | cond | mongoose.js:118:21:118:24 | cond |
496+
| mongoose.js:115:25:115:45 | cond | mongoose.js:119:18:119:21 | cond |
497+
| mongoose.js:115:25:115:45 | cond | mongoose.js:119:18:119:21 | cond |
498+
| mongoose.js:115:25:115:45 | cond | mongoose.js:120:22:120:25 | cond |
499+
| mongoose.js:115:25:115:45 | cond | mongoose.js:120:22:120:25 | cond |
500+
| mongoose.js:115:25:115:45 | cond | mongoose.js:121:16:121:19 | cond |
501+
| mongoose.js:115:25:115:45 | cond | mongoose.js:121:16:121:19 | cond |
502+
| mongoose.js:115:25:115:45 | cond | mongoose.js:122:19:122:22 | cond |
503+
| mongoose.js:115:25:115:45 | cond | mongoose.js:122:19:122:22 | cond |
504+
| mongoose.js:115:25:115:45 | cond | mongoose.js:124:28:124:31 | cond |
505+
| mongoose.js:115:25:115:45 | cond | mongoose.js:124:28:124:31 | cond |
506+
| mongoose.js:115:25:115:45 | cond | mongoose.js:125:28:125:31 | cond |
507+
| mongoose.js:115:25:115:45 | cond | mongoose.js:125:28:125:31 | cond |
508+
| mongoose.js:115:25:115:45 | cond | mongoose.js:126:28:126:31 | cond |
509+
| mongoose.js:115:25:115:45 | cond | mongoose.js:126:28:126:31 | cond |
510+
| mongoose.js:115:25:115:45 | cond | mongoose.js:127:18:127:21 | cond |
511+
| mongoose.js:115:25:115:45 | cond | mongoose.js:127:18:127:21 | cond |
512+
| mongoose.js:115:25:115:45 | cond | mongoose.js:128:22:128:25 | cond |
513+
| mongoose.js:115:25:115:45 | cond | mongoose.js:128:22:128:25 | cond |
514+
| mongoose.js:115:25:115:45 | cond | mongoose.js:129:21:129:24 | cond |
515+
| mongoose.js:115:25:115:45 | cond | mongoose.js:129:21:129:24 | cond |
516+
| mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:115:25:115:45 | cond |
517+
| mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:115:25:115:45 | cond |
518+
| mongoose.js:130:23:130:24 | id | mongoose.js:130:16:130:26 | { _id: id } |
519+
| mongoose.js:130:23:130:24 | id | mongoose.js:130:16:130:26 | { _id: id } |
442520
| mongooseJsonParse.js:19:11:19:20 | query | mongooseJsonParse.js:23:19:23:23 | query |
443521
| mongooseJsonParse.js:19:11:19:20 | query | mongooseJsonParse.js:23:19:23:23 | query |
444522
| mongooseJsonParse.js:19:19:19:20 | {} | mongooseJsonParse.js:19:11:19:20 | query |
@@ -551,6 +629,22 @@ edges
551629
| mongoose.js:94:51:94:55 | query | mongoose.js:21:19:21:26 | req.body | mongoose.js:94:51:94:55 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
552630
| mongoose.js:96:46:96:50 | query | mongoose.js:21:19:21:26 | req.body | mongoose.js:96:46:96:50 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
553631
| mongoose.js:111:14:111:18 | query | mongoose.js:21:19:21:26 | req.body | mongoose.js:111:14:111:18 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
632+
| mongoose.js:113:31:113:35 | query | mongoose.js:21:19:21:26 | req.body | mongoose.js:113:31:113:35 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
633+
| mongoose.js:116:22:116:25 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:116:22:116:25 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
634+
| mongoose.js:117:21:117:24 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:117:21:117:24 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
635+
| mongoose.js:118:21:118:24 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:118:21:118:24 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
636+
| mongoose.js:119:18:119:21 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:119:18:119:21 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
637+
| mongoose.js:120:22:120:25 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:120:22:120:25 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
638+
| mongoose.js:121:16:121:19 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:121:16:121:19 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
639+
| mongoose.js:122:19:122:22 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:122:19:122:22 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
640+
| mongoose.js:123:20:123:21 | id | mongoose.js:115:11:115:22 | req.query.id | mongoose.js:123:20:123:21 | id | This query depends on $@. | mongoose.js:115:11:115:22 | req.query.id | a user-provided value |
641+
| mongoose.js:124:28:124:31 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:124:28:124:31 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
642+
| mongoose.js:125:28:125:31 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:125:28:125:31 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
643+
| mongoose.js:126:28:126:31 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:126:28:126:31 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
644+
| mongoose.js:127:18:127:21 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:127:18:127:21 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
645+
| mongoose.js:128:22:128:25 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:128:22:128:25 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
646+
| mongoose.js:129:21:129:24 | cond | mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:129:21:129:24 | cond | This query depends on $@. | mongoose.js:115:32:115:45 | req.query.cond | a user-provided value |
647+
| mongoose.js:130:16:130:26 | { _id: id } | mongoose.js:115:11:115:22 | req.query.id | mongoose.js:130:16:130:26 | { _id: id } | This query depends on $@. | mongoose.js:115:11:115:22 | req.query.id | a user-provided value |
554648
| mongooseJsonParse.js:23:19:23:23 | query | mongooseJsonParse.js:20:30:20:43 | req.query.data | mongooseJsonParse.js:23:19:23:23 | query | This query depends on $@. | mongooseJsonParse.js:20:30:20:43 | req.query.data | a user-provided value |
555649
| mongooseModelClient.js:11:16:11:24 | { id: v } | mongooseModelClient.js:10:22:10:29 | req.body | mongooseModelClient.js:11:16:11:24 | { id: v } | This query depends on $@. | mongooseModelClient.js:10:22:10:29 | req.body | a user-provided value |
556650
| mongooseModelClient.js:12:16:12:34 | { id: req.body.id } | mongooseModelClient.js:12:22:12:29 | req.body | mongooseModelClient.js:12:16:12:34 | { id: req.body.id } | This query depends on $@. | mongooseModelClient.js:12:22:12:29 | req.body | a user-provided value |

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,4 +109,24 @@ app.post('/documents/find', (req, res) => {
109109

110110
var C = getQueryConstructor();
111111
new C(X, Y, query); // NOT OK
112+
113+
Document.findOneAndUpdate(X, query, function () { }); // NOT OK
114+
115+
let id = req.query.id, cond = req.query.cond;
116+
Document.deleteMany(cond); // NOT OK
117+
Document.deleteOne(cond); // NOT OK
118+
Document.geoSearch(cond); // NOT OK
119+
Document.remove(cond); // NOT OK
120+
Document.replaceOne(cond, Y); // NOT OK
121+
Document.find(cond); // NOT OK
122+
Document.findOne(cond); // NOT OK
123+
Document.findById(id); // NOT OK
124+
Document.findOneAndDelete(cond); // NOT OK
125+
Document.findOneAndRemove(cond); // NOT OK
126+
Document.findOneAndUpdate(cond, Y); // NOT OK
127+
Document.update(cond, Y); // NOT OK
128+
Document.updateMany(cond, Y); // NOT OK
129+
Document.updateOne(cond, Y); // NOT OK
130+
Document.find({ _id: id }); // NOT OK
131+
Document.find({ _id: { $eq: id } }); // OK
112132
});

0 commit comments

Comments
 (0)