From 978d2db25215a79723b2506854e382fd7b045c7c Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Thu, 26 Nov 2020 15:24:56 +0000 Subject: [PATCH] JavaScript: Add models for more Mongoose methods. --- javascript/change-notes/2020-11-30-nosql.md | 2 + .../src/Security/CWE-089/SqlInjection.qhelp | 6 ++ .../semmle/javascript/frameworks/NoSQL.qll | 13 ++- .../CWE-089/untyped/DatabaseAccesses.expected | 1 + .../CWE-089/untyped/SqlInjection.expected | 94 +++++++++++++++++++ .../Security/CWE-089/untyped/mongoose.js | 20 ++++ 6 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 javascript/change-notes/2020-11-30-nosql.md diff --git a/javascript/change-notes/2020-11-30-nosql.md b/javascript/change-notes/2020-11-30-nosql.md new file mode 100644 index 00000000000..1aaee78edcf --- /dev/null +++ b/javascript/change-notes/2020-11-30-nosql.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* 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. diff --git a/javascript/ql/src/Security/CWE-089/SqlInjection.qhelp b/javascript/ql/src/Security/CWE-089/SqlInjection.qhelp index db86f591e85..8cdc2419d47 100644 --- a/javascript/ql/src/Security/CWE-089/SqlInjection.qhelp +++ b/javascript/ql/src/Security/CWE-089/SqlInjection.qhelp @@ -17,6 +17,11 @@ Most database connector libraries offer a way of safely embedding untrusted data into a query by means of query parameters or prepared statements.

+

+For NoSQL queries, make use of an operator like MongoDB's $eq +to ensure that untrusted data is interpreted as a literal value and not as +a query object. +

@@ -52,5 +57,6 @@ immune to injection attacks.
  • Wikipedia: SQL injection.
    • +
  • MongoDB: $eq operator.
    •  diff --git a/javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll b/javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll index 31671cbc423..0002a057b8e 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll @@ -237,9 +237,17 @@ private module Mongoose { // implement lots of the MongoDB collection interface MongoDB::CollectionMethodSignatures::interpretsArgumentAsQuery(name, n) or - name = "findByIdAndUpdate" and n = 1 + name = "find" + ["ById", "One"] + "AndUpdate" and n = 1 or - name = "where" and n = 0 + name in ["delete" + ["Many", "One"], "geoSearch", "remove", "replaceOne", "where"] and + n = 0 + or + name in [ + "find" + ["", "ById", "One"], + "find" + ["ById", "One"] + "And" + ["Delete", "Remove", "Update"], + "update" + ["", "Many", "One"] + ] and + n = 0 } /** @@ -262,6 +270,7 @@ private module Mongoose { name = "findOneAndReplace" or name = "findOneAndUpdate" or name = "geosearch" or + name = "remove" or name = "replaceOne" or name = "update" or name = "updateMany" or diff --git a/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected b/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected index 5b0956053e5..d17ff8d1cc4 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected +++ b/javascript/ql/test/query-tests/Security/CWE-089/untyped/DatabaseAccesses.expected @@ -32,6 +32,7 @@ | mongoose.js:96:2:96:52 | Documen ... query)) | | mongoose.js:97:2:97:52 | Documen ... query)) | | mongoose.js:99:2:99:50 | Documen ... query)) | +| mongoose.js:113:2:113:53 | Documen ... () { }) | | socketio.js:11:5:11:54 | db.run( ... ndle}`) | | tst2.js:7:3:7:62 | sql.que ... ms.id}` | | tst2.js:9:3:9:85 | new sql ... + "'") | diff --git a/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected b/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected index 2577a24fb97..e33f30d01ba 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected @@ -138,6 +138,45 @@ nodes | mongoose.js:96:46:96:50 | query | | mongoose.js:111:14:111:18 | query | | mongoose.js:111:14:111:18 | query | +| mongoose.js:113:31:113:35 | query | +| mongoose.js:113:31:113:35 | query | +| mongoose.js:115:6:115:22 | id | +| mongoose.js:115:11:115:22 | req.query.id | +| mongoose.js:115:11:115:22 | req.query.id | +| mongoose.js:115:25:115:45 | cond | +| mongoose.js:115:32:115:45 | req.query.cond | +| mongoose.js:115:32:115:45 | req.query.cond | +| mongoose.js:116:22:116:25 | cond | +| mongoose.js:116:22:116:25 | cond | +| mongoose.js:117:21:117:24 | cond | +| mongoose.js:117:21:117:24 | cond | +| mongoose.js:118:21:118:24 | cond | +| mongoose.js:118:21:118:24 | cond | +| mongoose.js:119:18:119:21 | cond | +| mongoose.js:119:18:119:21 | cond | +| mongoose.js:120:22:120:25 | cond | +| mongoose.js:120:22:120:25 | cond | +| mongoose.js:121:16:121:19 | cond | +| mongoose.js:121:16:121:19 | cond | +| mongoose.js:122:19:122:22 | cond | +| mongoose.js:122:19:122:22 | cond | +| mongoose.js:123:20:123:21 | id | +| mongoose.js:123:20:123:21 | id | +| mongoose.js:124:28:124:31 | cond | +| mongoose.js:124:28:124:31 | cond | +| mongoose.js:125:28:125:31 | cond | +| mongoose.js:125:28:125:31 | cond | +| mongoose.js:126:28:126:31 | cond | +| mongoose.js:126:28:126:31 | cond | +| mongoose.js:127:18:127:21 | cond | +| mongoose.js:127:18:127:21 | cond | +| mongoose.js:128:22:128:25 | cond | +| mongoose.js:128:22:128:25 | cond | +| mongoose.js:129:21:129:24 | cond | +| mongoose.js:129:21:129:24 | cond | +| mongoose.js:130:16:130:26 | { _id: id } | +| mongoose.js:130:16:130:26 | { _id: id } | +| mongoose.js:130:23:130:24 | id | | mongooseJsonParse.js:19:11:19:20 | query | | mongooseJsonParse.js:19:19:19:20 | {} | | mongooseJsonParse.js:20:19:20:44 | JSON.pa ... y.data) | @@ -369,6 +408,8 @@ edges | mongoose.js:20:11:20:20 | query | mongoose.js:96:46:96:50 | query | | mongoose.js:20:11:20:20 | query | mongoose.js:111:14:111:18 | query | | mongoose.js:20:11:20:20 | query | mongoose.js:111:14:111:18 | query | +| mongoose.js:20:11:20:20 | query | mongoose.js:113:31:113:35 | query | +| mongoose.js:20:11:20:20 | query | mongoose.js:113:31:113:35 | query | | mongoose.js:20:19:20:20 | {} | mongoose.js:20:11:20:20 | query | | mongoose.js:21:19:21:26 | req.body | mongoose.js:21:19:21:32 | req.body.title | | mongoose.js:21:19:21:26 | req.body | mongoose.js:21:19:21:32 | req.body.title | @@ -437,8 +478,45 @@ edges | mongoose.js:21:19:21:32 | req.body.title | mongoose.js:96:46:96:50 | query | | mongoose.js:21:19:21:32 | req.body.title | mongoose.js:111:14:111:18 | query | | mongoose.js:21:19:21:32 | req.body.title | mongoose.js:111:14:111:18 | query | +| mongoose.js:21:19:21:32 | req.body.title | mongoose.js:113:31:113:35 | query | +| mongoose.js:21:19:21:32 | req.body.title | mongoose.js:113:31:113:35 | query | | mongoose.js:24:25:24:29 | query | mongoose.js:24:24:24:30 | [query] | | mongoose.js:24:25:24:29 | query | mongoose.js:24:24:24:30 | [query] | +| mongoose.js:115:6:115:22 | id | mongoose.js:123:20:123:21 | id | +| mongoose.js:115:6:115:22 | id | mongoose.js:123:20:123:21 | id | +| mongoose.js:115:6:115:22 | id | mongoose.js:130:23:130:24 | id | +| mongoose.js:115:11:115:22 | req.query.id | mongoose.js:115:6:115:22 | id | +| mongoose.js:115:11:115:22 | req.query.id | mongoose.js:115:6:115:22 | id | +| mongoose.js:115:25:115:45 | cond | mongoose.js:116:22:116:25 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:116:22:116:25 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:117:21:117:24 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:117:21:117:24 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:118:21:118:24 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:118:21:118:24 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:119:18:119:21 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:119:18:119:21 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:120:22:120:25 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:120:22:120:25 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:121:16:121:19 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:121:16:121:19 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:122:19:122:22 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:122:19:122:22 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:124:28:124:31 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:124:28:124:31 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:125:28:125:31 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:125:28:125:31 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:126:28:126:31 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:126:28:126:31 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:127:18:127:21 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:127:18:127:21 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:128:22:128:25 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:128:22:128:25 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:129:21:129:24 | cond | +| mongoose.js:115:25:115:45 | cond | mongoose.js:129:21:129:24 | cond | +| mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:115:25:115:45 | cond | +| mongoose.js:115:32:115:45 | req.query.cond | mongoose.js:115:25:115:45 | cond | +| mongoose.js:130:23:130:24 | id | mongoose.js:130:16:130:26 | { _id: id } | +| mongoose.js:130:23:130:24 | id | mongoose.js:130:16:130:26 | { _id: id } | | mongooseJsonParse.js:19:11:19:20 | query | mongooseJsonParse.js:23:19:23:23 | query | | mongooseJsonParse.js:19:11:19:20 | query | mongooseJsonParse.js:23:19:23:23 | query | | mongooseJsonParse.js:19:19:19:20 | {} | mongooseJsonParse.js:19:11:19:20 | query | @@ -551,6 +629,22 @@ edges | 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 | | 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 | | 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | | 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 | | 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 | | 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-089/untyped/mongoose.js b/javascript/ql/test/query-tests/Security/CWE-089/untyped/mongoose.js index 3818b5e05f9..baeb13b1fbc 100644 --- a/javascript/ql/test/query-tests/Security/CWE-089/untyped/mongoose.js +++ b/javascript/ql/test/query-tests/Security/CWE-089/untyped/mongoose.js @@ -109,4 +109,24 @@ app.post('/documents/find', (req, res) => { var C = getQueryConstructor(); new C(X, Y, query); // NOT OK + + Document.findOneAndUpdate(X, query, function () { }); // NOT OK + + let id = req.query.id, cond = req.query.cond; + Document.deleteMany(cond); // NOT OK + Document.deleteOne(cond); // NOT OK + Document.geoSearch(cond); // NOT OK + Document.remove(cond); // NOT OK + Document.replaceOne(cond, Y); // NOT OK + Document.find(cond); // NOT OK + Document.findOne(cond); // NOT OK + Document.findById(id); // NOT OK + Document.findOneAndDelete(cond); // NOT OK + Document.findOneAndRemove(cond); // NOT OK + Document.findOneAndUpdate(cond, Y); // NOT OK + Document.update(cond, Y); // NOT OK + Document.updateMany(cond, Y); // NOT OK + Document.updateOne(cond, Y); // NOT OK + Document.find({ _id: id }); // NOT OK + Document.find({ _id: { $eq: id } }); // OK });