add mongoose.Types.ObjectId.isValid as a sanitizer-guard for NoSQL injection

Author: erik-krogh

javascript/ql/lib/semmle/javascript/security/TaintedObject.qll

@@ -120,6 +120,22 @@ module TaintedObject {
     override predicate sanitizes(boolean outcome, Expr e) { e = x and outcome = polarity }
   }
 
+  /** A guard that checks whether an input a valid string identifier using `mongoose.Types.ObjectId.isValid` */
+  class ObjectIdGuard extends SanitizerGuard instanceof API::CallNode {
+    ObjectIdGuard() {
+      this =
+        API::moduleImport("mongoose")
+            .getMember("Types")
+            .getMember("ObjectId")
+            .getMember("isValid")
+            .getACall()
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e, FlowLabel lbl) {
+      e = super.getAnArgument().asExpr() and outcome = true and lbl = label()
+    }
+  }
+
   /**
    * A sanitizer guard that validates an input against a JSON schema.
    */

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

@@ -42,6 +42,8 @@
 | mongoose.js:97:2:97:52 | Documen ... query)) |
 | mongoose.js:99:2:99:50 | Documen ... query)) |
 | mongoose.js:113:2:113:53 | Documen ... () { }) |
+| mongoose.js:134:6:134:55 | Documen ... on(){}) |
+| mongoose.js:136:6:136:55 | Documen ... on(){}) |
 | mysql.js:8:9:11:47 | connect ... ds) {}) |
 | mysql.js:14:9:16:47 | connect ... ds) {}) |
 | mysql.js:19:9:20:48 | connect ... ds) {}) |

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

@@ -283,6 +283,8 @@ nodes
 | mongoose.js:130:16:130:26 | { _id: id } |
 | mongoose.js:130:16:130:26 | { _id: id } |
 | mongoose.js:130:23:130:24 | id |
+| mongoose.js:136:33:136:37 | query |
+| mongoose.js:136:33:136:37 | query |
 | mongooseJsonParse.js:19:11:19:20 | query |
 | mongooseJsonParse.js:19:19:19:20 | {} |
 | mongooseJsonParse.js:20:19:20:44 | JSON.pa ... y.data) |
@@ -688,6 +690,8 @@ edges
 | 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:11:20:20 | query | mongoose.js:136:33:136:37 | query |
+| mongoose.js:20:11:20:20 | query | mongoose.js:136:33:136:37 | 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 |
@@ -758,6 +762,8 @@ edges
 | 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:21:19:21:32 | req.body.title | mongoose.js:136:33:136:37 | query |
+| mongoose.js:21:19:21:32 | req.body.title | mongoose.js:136:33:136:37 | 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 |
@@ -1008,6 +1014,7 @@ edges
 | 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 object depends on a $@. | mongoose.js:115:32:115:45 | req.query.cond | 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 object depends on a $@. | mongoose.js:115:32:115:45 | req.query.cond | 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 object depends on a $@. | mongoose.js:115:11:115:22 | req.query.id | user-provided value |
+| mongoose.js:136:33:136:37 | query | mongoose.js:21:19:21:26 | req.body | mongoose.js:136:33:136:37 | query | This query object depends on a $@. | mongoose.js:21:19:21:26 | req.body | 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 object depends on a $@. | mongooseJsonParse.js:20:30:20:43 | req.query.data | 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 object depends on a $@. | mongooseModelClient.js:10:22:10:29 | req.body | 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 object depends on a $@. | mongooseModelClient.js:12:22:12:29 | req.body | user-provided value |

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

@@ -104,7 +104,7 @@ app.post('/documents/find', (req, res) => {
 	new innocent(X, Y, query);
 
 	function getQueryConstructor() {
-		return Mongoose.Query;
+	return Mongoose.Query;
 	}
 
 	var C = getQueryConstructor();
@@ -129,4 +129,10 @@ app.post('/documents/find', (req, res) => {
 	Document.updateOne(cond, Y); // NOT OK
 	Document.find({ _id: id }); // NOT OK
 	Document.find({ _id: { $eq: id } }); // OK
+
+    if (Mongoose.Types.ObjectId.isValid(query)) {
+    	Document.findByIdAndUpdate(query, X, function(){}); // OK - is sanitized
+    } else {
+    	Document.findByIdAndUpdate(query, X, function(){}); // NOT OK
+    }
 });