From 2a87d53db4de99d6b2994427950a16f1d76bb8c7 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 10 Oct 2018 10:11:18 +0100 Subject: [PATCH 1/2] JS: Add additional Mongoose/MongoDB sinks --- .../ql/src/semmle/javascript/frameworks/NoSQL.qll | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll b/javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll index 670d25c8c79e..cbea55340dda 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll @@ -62,9 +62,22 @@ private module MongoDB { QueryCall() { exists (string m | asExpr().(MethodCallExpr).calls(any(Collection c), m) | + m = "aggregate" and queryArgIdx = 0 or m = "count" and queryArgIdx = 0 or + m = "deleteMany" and queryArgIdx = 0 or + m = "deleteOne" and queryArgIdx = 0 or m = "distinct" and queryArgIdx = 1 or - m = "find" and queryArgIdx = 0 + m = "find" and queryArgIdx = 0 or + m = "findOne" and queryArgIdx = 0 or + m = "findOneAndDelete" and queryArgIdx = 0 or + m = "findOneAndRemove" and queryArgIdx = 0 or + m = "findOneAndDelete" and queryArgIdx = 0 or + m = "findOneAndUpdate" and queryArgIdx = 0 or + m = "remove" and queryArgIdx = 0 or + m = "replaceOne" and queryArgIdx = 0 or + m = "update" and queryArgIdx = 0 or + m = "updateMany" and queryArgIdx = 0 or + m = "updateOne" and queryArgIdx = 0 ) } From 74f115fa4017c25327316eb451257da372cc092f Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 10 Oct 2018 10:46:40 +0100 Subject: [PATCH 2/2] JS: add test case --- .../Security/CWE-089/SqlInjection.expected | 14 ++++++- .../query-tests/Security/CWE-089/mongoose.js | 39 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected index 55a095634ae1..7a99482adb8f 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected @@ -1,6 +1,18 @@ | mongodb.js:18:16:18:20 | query | This query depends on $@. | mongodb.js:13:19:13:26 | req.body | a user-provided value | | mongodb.js:39:16:39:20 | query | This query depends on $@. | mongodb.js:34:19:34:33 | req.query.title | a user-provided value | -| mongoose.js:24:19:24:23 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:27:20:27:24 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:30:25:30:29 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:33:24:33:28 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:36:31:36:35 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:39:19:39:23 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:42:22:42:26 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:45:31:45:35 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:48:31:48:35 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:51:31:51:35 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:54:25:54:29 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:57:21:57:25 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:60:25:60:29 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | +| mongoose.js:63:24:63:28 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value | | mongooseJsonParse.js:23:19:23:23 | query | This query depends on $@. | mongooseJsonParse.js:20:30:20:43 | req.query.data | a user-provided value | | tst2.js:9:27:9:84 | "select ... d + "'" | This query depends on $@. | tst2.js:9:66:9:78 | req.params.id | a user-provided value | | tst3.js:10:14:10:19 | query1 | This query depends on $@. | tst3.js:9:16:9:34 | req.params.category | a user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-089/mongoose.js b/javascript/ql/test/query-tests/Security/CWE-089/mongoose.js index 8bb2b456126a..32f2639b3c7a 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/mongoose.js +++ b/javascript/ql/test/query-tests/Security/CWE-089/mongoose.js @@ -20,7 +20,46 @@ app.post('/documents/find', (req, res) => { const query = {}; query.title = req.body.title; + // NOT OK: query is tainted by user-provided object value + Document.aggregate('type', query); + + // NOT OK: query is tainted by user-provided object value + Document.count(query); + + // NOT OK: query is tainted by user-provided object value + Document.deleteMany(query); + + // NOT OK: query is tainted by user-provided object value + Document.deleteOne(query); + + // NOT OK: query is tainted by user-provided object value + Document.distinct('type', query); + // NOT OK: query is tainted by user-provided object value Document.find(query); + + // NOT OK: query is tainted by user-provided object value + Document.findOne(query); + + // NOT OK: query is tainted by user-provided object value + Document.findOneAndDelete(query); + + // NOT OK: query is tainted by user-provided object value + Document.findOneAndRemove(query); + + // NOT OK: query is tainted by user-provided object value + Document.findOneAndUpdate(query); + + // NOT OK: query is tainted by user-provided object value + Document.replaceOne(query); + + // NOT OK: query is tainted by user-provided object value + Document.update(query); + + // NOT OK: query is tainted by user-provided object value + Document.updateMany(query); + + // NOT OK: query is tainted by user-provided object value + Document.updateOne(query); });