Merge pull request github#3076 from esbena/js/even-more-mongoose-improvements

Approved by erik-krogh

Author: semmle-qlci

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

@@ -209,16 +209,61 @@ private module Mongoose {
     result = getAMongooseInstance().getAMemberCall("createConnection")
   }
 
+  /**
+   * A Mongoose function invocation.
+   */
+  private class InvokeNode extends DataFlow::InvokeNode {
+    /**
+     * Holds if this invocation returns an object of type `Query`.
+     */
+    abstract predicate returnsQuery();
+
+    /**
+     * Holds if this invocation returns a `Query` that evaluates to one or
+     * more Documents (`asArray` is false if it evaluates to a single
+     * Document).
+     */
+    abstract predicate returnsDocumentQuery(boolean asArray);
+
+    /**
+     * Holds if this invocation interprets `arg` as a query.
+     */
+    abstract predicate interpretsArgumentAsQuery(DataFlow::Node arg);
+  }
+
   /**
    * Provides classes modeling the Mongoose Model class
    */
   module Model {
+    private class ModelInvokeNode extends InvokeNode, DataFlow::MethodCallNode {
+      ModelInvokeNode() { this = ref().getAMethodCall() }
+
+      override predicate returnsQuery() { MethodSignatures::returnsQuery(getMethodName()) }
+
+      override predicate returnsDocumentQuery(boolean asArray) {
+        MethodSignatures::returnsDocumentQuery(getMethodName(), asArray)
+      }
+
+      override predicate interpretsArgumentAsQuery(DataFlow::Node arg) {
+        exists(int n |
+          MethodSignatures::interpretsArgumentAsQuery(this.getMethodName(), n) and
+          arg = this.getArgument(n)
+        )
+      }
+    }
+
     /**
      * Gets a data flow node referring to a Mongoose Model object.
      */
     private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
       (
-        result = getAMongooseInstance().getAMemberCall("model") or
+        result = getAMongooseInstance().getAMemberCall("model")
+        or
+        exists(DataFlow::SourceNode conn | conn = createConnection() |
+          result = conn.getAMemberCall("model") or
+          result = conn.getAPropertyRead("models").getAPropertyRead()
+        )
+        or
         result.hasUnderlyingType("mongoose", "Model")
       ) and
       t.start()
@@ -271,41 +316,59 @@ private module Mongoose {
         name = "updateOne" or
         name = "where"
       }
+
+      /**
+       * Holds if Document method `name` returns a query that results in
+       * one or more documents, the documents are wrapped in an array
+       * if `asArray` is true.
+       */
+      predicate returnsDocumentQuery(string name, boolean asArray) {
+        asArray = false and name = "findOne"
+        or
+        asArray = true and name = "find"
+      }
     }
   }
 
   /**
    * Provides classes modeling the Mongoose Query class
    */
   module Query {
-    /**
-     * A Mongoose query object as a result of a Model method call.
-     */
-    private class QueryFromModel extends DataFlow::MethodCallNode {
-      QueryFromModel() {
-        exists(string name |
-          Model::MethodSignatures::returnsQuery(name) and
-          Model::ref().getAMethodCall(name) = this
+    private class QueryInvokeNode extends InvokeNode, DataFlow::MethodCallNode {
+      QueryInvokeNode() { this = ref().getAMethodCall() }
+
+      override predicate returnsQuery() { MethodSignatures::returnsQuery(getMethodName()) }
+
+      override predicate returnsDocumentQuery(boolean asArray) {
+        MethodSignatures::returnsDocumentQuery(getMethodName(), asArray)
+      }
+
+      override predicate interpretsArgumentAsQuery(DataFlow::Node arg) {
+        exists(int n |
+          MethodSignatures::interpretsArgumentAsQuery(this.getMethodName(), n) and
+          arg = this.getArgument(n)
         )
       }
     }
 
-    /**
-     * A Mongoose query object as a result of a Query constructor invocation.
-     */
-    class QueryFromConstructor extends DataFlow::NewNode {
-      QueryFromConstructor() {
+    private class NewQueryInvokeNode extends InvokeNode {
+      NewQueryInvokeNode() {
         this = getAMongooseInstance().getAPropertyRead("Query").getAnInstantiation()
       }
+
+      override predicate returnsQuery() { any() }
+
+      override predicate returnsDocumentQuery(boolean asArray) { none() }
+
+      override predicate interpretsArgumentAsQuery(DataFlow::Node arg) { arg = this.getArgument(2) }
     }
 
     /**
      * Gets a data flow node referring to a Mongoose query object.
      */
     private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
       (
-        result instanceof QueryFromConstructor or
-        result instanceof QueryFromModel or
+        result.(InvokeNode).returnsQuery() or
         result.hasUnderlyingType("mongoose", "Query")
       ) and
       t.start()
@@ -444,6 +507,152 @@ private module Mongoose {
         name = "within" or
         name = "wtimeout"
       }
+
+      /**
+       * Holds if Query method `name` returns a query that results in
+       * one or more documents, the documents are wrapped in an array
+       * if `asArray` is true.
+       */
+      predicate returnsDocumentQuery(string name, boolean asArray) {
+        asArray = false and name = "findOne"
+        or
+        asArray = true and name = "find"
+      }
+    }
+  }
+
+  /**
+   * Provides classes modeling the Mongoose Document class
+   */
+  module Document {
+    private class DocumentInvokeNode extends InvokeNode, DataFlow::MethodCallNode {
+      DocumentInvokeNode() { this = ref().getAMethodCall() }
+
+      override predicate returnsQuery() { MethodSignatures::returnsQuery(getMethodName()) }
+
+      override predicate returnsDocumentQuery(boolean asArray) {
+        MethodSignatures::returnsDocumentQuery(getMethodName(), asArray)
+      }
+
+      override predicate interpretsArgumentAsQuery(DataFlow::Node arg) {
+        exists(int n |
+          MethodSignatures::interpretsArgumentAsQuery(this.getMethodName(), n) and
+          arg = this.getArgument(n)
+        )
+      }
+    }
+
+    /**
+     * A Mongoose Document that is retrieved from the backing database.
+     */
+    class RetrievedDocument extends DataFlow::SourceNode {
+      RetrievedDocument() {
+        exists(boolean asArray, DataFlow::ParameterNode param |
+          exists(InvokeNode call |
+            call.returnsDocumentQuery(asArray) and
+            param = call.getCallback(call.getNumArgument() - 1).getParameter(1)
+          )
+          or
+          exists(
+            DataFlow::SourceNode base, DataFlow::MethodCallNode call, string executor,
+            int paramIndex
+          |
+            executor = "then" and paramIndex = 0
+            or
+            executor = "exec" and paramIndex = 1
+          |
+            base = Query::ref() and
+            call = base.getAMethodCall(executor) and
+            param = call.getCallback(0).getParameter(paramIndex) and
+            exists(DataFlow::MethodCallNode pred |
+              // limitation: look at the previous method call
+              Query::MethodSignatures::returnsDocumentQuery(pred.getMethodName(), asArray) and
+              pred.getAMethodCall() = call
+            )
+          )
+        |
+          asArray = false and this = param
+          or
+          asArray = true and
+          exists(DataFlow::PropRead access |
+            // limitation: look for direct accesses
+            access = param.getAPropertyRead() and
+            not exists(access.getPropertyName()) and
+            this = access
+          )
+        )
+      }
+    }
+
+    /**
+     * Gets a data flow node referring to a Mongoose Document object.
+     */
+    private DataFlow::SourceNode ref(DataFlow::TypeTracker t) {
+      (
+        result instanceof RetrievedDocument or
+        result.hasUnderlyingType("mongoose", "Document")
+      ) and
+      t.start()
+      or
+      exists(DataFlow::TypeTracker t2, DataFlow::SourceNode succ | succ = ref(t2) |
+        result = succ.track(t2, t)
+        or
+        result = succ.getAMethodCall(any(string name | MethodSignatures::returnsDocument(name))) and
+        t = t2.continue()
+      )
+    }
+
+    /**
+     * Gets a data flow node referring to a Mongoose Document object.
+     */
+    DataFlow::SourceNode ref() { result = ref(DataFlow::TypeTracker::end()) }
+
+    private module MethodSignatures {
+      /**
+       * Holds if Document method `name` returns a Query.
+       */
+      predicate returnsQuery(string name) {
+        // Documents are subtypes of Models
+        Model::MethodSignatures::returnsQuery(name) or
+        name = "replaceOne" or
+        name = "update" or
+        name = "updateOne"
+      }
+
+      /**
+       * Holds if Document method `name` interprets parameter `n` as a query.
+       */
+      predicate interpretsArgumentAsQuery(string name, int n) {
+        // Documents are subtypes of Models
+        Model::MethodSignatures::interpretsArgumentAsQuery(name, n)
+        or
+        n = 0 and
+        (
+          name = "replaceOne" or
+          name = "update" or
+          name = "updateOne"
+        )
+      }
+
+      /**
+       * Holds if Document method `name` returns a query that results in
+       * one or more documents, the documents are wrapped in an array
+       * if `asArray` is true.
+       */
+      predicate returnsDocumentQuery(string name, boolean asArray) {
+        // Documents are subtypes of Models
+        Model::MethodSignatures::returnsDocumentQuery(name, asArray)
+      }
+
+      /**
+       * Holds if Document method `name` returns a Document.
+       */
+      predicate returnsDocument(string name) {
+        name = "depopulate" or
+        name = "init" or
+        name = "populate" or
+        name = "overwrite"
+      }
     }
   }
 
@@ -468,53 +677,39 @@ private module Mongoose {
    * An expression that is interpreted as a (part of a) MongoDB query.
    */
   class MongoDBQueryPart extends NoSQL::Query {
-    MongoDBQueryPart() {
-      exists(DataFlow::MethodCallNode mcn, string method, int n |
-        Model::MethodSignatures::interpretsArgumentAsQuery(method, n) and
-        mcn = Model::ref().getAMethodCall(method) and
-        this = mcn.getArgument(n).asExpr()
-      )
-      or
-      this = any(Query::QueryFromConstructor c).getArgument(2).asExpr()
-      or
-      exists(string method, int n | Query::MethodSignatures::interpretsArgumentAsQuery(method, n) |
-        this = Query::ref().getAMethodCall(method).getArgument(n).asExpr()
-      )
-    }
+    MongoDBQueryPart() { any(InvokeNode call).interpretsArgumentAsQuery(this.flow()) }
   }
 
   /**
    * An evaluation of a MongoDB query.
    */
-  class MongoDBQueryEvaluation extends DatabaseAccess {
-    DataFlow::MethodCallNode mcn;
+  class ShorthandQueryEvaluation extends DatabaseAccess {
+    InvokeNode invk;
+
+    ShorthandQueryEvaluation() {
+      this = invk and
+      // shorthand for execution: provide a callback
+      invk.returnsQuery() and
+      exists(invk.getCallback(invk.getNumArgument() - 1))
+    }
 
-    MongoDBQueryEvaluation() {
-      this = mcn and
-      (
-        exists(string method |
-          Model::MethodSignatures::returnsQuery(method) and
-          mcn = Model::ref().getAMethodCall(method) and
-          // callback provided to a Model method call
-          exists(mcn.getCallback(mcn.getNumArgument() - 1))
-        )
-        or
-        Query::ref().getAMethodCall() = mcn and
-        (
-          // explicit execution using a Query method call
-          exists(string executor | executor = "exec" or executor = "then" or executor = "catch" |
-            mcn.getMethodName() = executor
-          )
-          or
-          // callback provided to a Query method call
-          exists(mcn.getCallback(mcn.getNumArgument() - 1))
-        )
+    override DataFlow::Node getAQueryArgument() {
+      // NB: the complete information is not easily accessible for deeply chained calls
+      invk.interpretsArgumentAsQuery(result)
+    }
+  }
+
+  class ExplicitQueryEvaluation extends DatabaseAccess {
+    ExplicitQueryEvaluation() {
+      // explicit execution using a Query method call
+      exists(string executor | executor = "exec" or executor = "then" or executor = "catch" |
+        Query::ref().getAMethodCall(executor) = this
       )
     }
 
     override DataFlow::Node getAQueryArgument() {
-      // NB: this does not account for all of the chained calls leading to this execution
-      mcn.getAnArgument().asExpr().(MongoDBQueryPart).flow() = result
+      // NB: the complete information is not easily accessible for deeply chained calls
+      none()
     }
   }
 }

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

@@ -14,6 +14,19 @@
 | mongoose.js:65:2:65:51 | Documen ... on(){}) |
 | mongoose.js:67:2:68:27 | new Mon ... on(){}) |
 | mongoose.js:71:2:77:9 | Documen ... .exec() |
+| mongoose.js:84:2:84:52 | Documen ... query)) |
+| mongoose.js:85:2:85:52 | Documen ... query)) |
+| mongoose.js:86:2:86:57 | Documen ... query)) |
+| mongoose.js:87:2:87:57 | Documen ... query)) |
+| mongoose.js:88:2:88:52 | Documen ... query)) |
+| mongoose.js:89:2:89:55 | Documen ... query)) |
+| mongoose.js:91:2:91:52 | Documen ... query)) |
+| mongoose.js:92:2:92:49 | Documen ... query)) |
+| mongoose.js:93:2:93:57 | Documen ... query)) |
+| mongoose.js:94:2:94:54 | Documen ... query)) |
+| mongoose.js:95:2:95:52 | Documen ... query)) |
+| mongoose.js:96:2:96:52 | Documen ... query)) |
+| mongoose.js:98:2:98:50 | Documen ... query)) |
 | 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 ...  + "'") |