Merge pull request #4367 from erik-krogh/sql-api

JS: Fixing an API-graph gotcha in `SQL.qll`

Author: max-schaefer

javascript/ql/src/semmle/javascript/ApiGraphs.qll

@@ -24,7 +24,10 @@ module API {
      * Gets a data-flow node corresponding to a use of the API component represented by this node.
      *
      * For example, `require('fs').readFileSync` is a use of the function `readFileSync` from the
-     * `fs` module, and `require('fs').readFileSync(file)` is a use of the result of that function.
+     * `fs` module, and `require('fs').readFileSync(file)` is a use of the return of that function.
+     *
+     * This includes indirect uses found via data flow, meaning that in
+     * `f(obj.foo); function f(x) {};` both `obj.foo` and `x` are uses of the `foo` member from `obj`.
      *
      * As another example, in the assignment `exports.plusOne = (x) => x+1` the two references to
      * `x` are uses of the first parameter of `plusOne`.
@@ -35,6 +38,34 @@ module API {
       )
     }
 
+    /**
+     * Gets an immediate use of the API component represented by this node.
+     *
+     * For example, `require('fs').readFileSync` is a an immediate use of the `readFileSync` member
+     * from the `fs` module.
+     *
+     * Unlike `getAUse()`, this predicate only gets the immediate references, not the indirect uses
+     * found via data flow. This means that in `const x = fs.readFile` only `fs.readFile` is a reference
+     * to the `readFile` member of `fs`, neither `x` nor any node that `x` flows to is a reference to
+     * this API component.
+     */
+    DataFlow::SourceNode getAnImmediateUse() { Impl::use(this, result) }
+
+    /**
+     * Gets a call to the function represented by this API component.
+     */
+    DataFlow::CallNode getACall() { result = getReturn().getAnImmediateUse() }
+
+    /**
+     * Gets a `new` call to the function represented by this API component.
+     */
+    DataFlow::NewNode getAnInstantiation() { result = getInstance().getAnImmediateUse() }
+
+    /**
+     * Gets an invocation (with our without `new`) to the function represented by this API component.
+     */
+    DataFlow::InvokeNode getAnInvocation() { result = getACall() or result = getAnInstantiation() }
+
     /**
      * Gets a data-flow node corresponding to the right-hand side of a definition of the API
      * component represented by this node.

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

@@ -31,24 +31,27 @@ private module MySql {
   /** Gets the package name `mysql` or `mysql2`. */
   API::Node mysql() { result = API::moduleImport(["mysql", "mysql2"]) }
 
-  /** Gets a call to `mysql.createConnection`. */
-  API::Node createConnection() { result = mysql().getMember("createConnection").getReturn() }
+  /** Gets a reference to `mysql.createConnection`. */
+  API::Node createConnection() { result = mysql().getMember("createConnection") }
 
-  /** Gets a call to `mysql.createPool`. */
-  API::Node createPool() { result = mysql().getMember("createPool").getReturn() }
+  /** Gets a reference to `mysql.createPool`. */
+  API::Node createPool() { result = mysql().getMember("createPool") }
+
+  /** Gets a node that contains a MySQL pool created using `mysql.createPool()`. */
+  API::Node pool() { result = createPool().getReturn() }
 
   /** Gets a data flow node that contains a freshly created MySQL connection instance. */
   API::Node connection() {
-    result = createConnection()
+    result = createConnection().getReturn()
     or
-    result = createPool().getMember("getConnection").getParameter(0).getParameter(1)
+    result = pool().getMember("getConnection").getParameter(0).getParameter(1)
   }
 
   /** A call to the MySql `query` method. */
   private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
     QueryCall() {
-      exists(API::Node recv | recv = createPool() or recv = connection() |
-        this = recv.getMember("query").getReturn().getAUse()
+      exists(API::Node recv | recv = pool() or recv = connection() |
+        this = recv.getMember("query").getACall()
       )
     }
 
@@ -63,12 +66,7 @@ private module MySql {
   /** A call to the `escape` or `escapeId` method that performs SQL sanitization. */
   class EscapingSanitizer extends SQL::SqlSanitizer, MethodCallExpr {
     EscapingSanitizer() {
-      this =
-        [mysql(), createPool(), connection()]
-            .getMember(["escape", "escapeId"])
-            .getReturn()
-            .getAUse()
-            .asExpr() and
+      this = [mysql(), pool(), connection()].getMember(["escape", "escapeId"]).getACall().asExpr() and
       input = this.getArgument(0) and
       output = this
     }
@@ -79,9 +77,9 @@ private module MySql {
     string kind;
 
     Credentials() {
-      exists(API::Node call, string prop |
-        call in [createConnection(), createPool()] and
-        call.getAUse().asExpr().(CallExpr).hasOptionArgument(0, prop, this) and
+      exists(API::Node callee, string prop |
+        callee in [createConnection(), createPool()] and
+        this = callee.getParameter(0).getMember(prop).getARhs().asExpr() and
         (
           prop = "user" and kind = "user name"
           or
@@ -98,29 +96,32 @@ private module MySql {
  * Provides classes modelling the `pg` package.
  */
 private module Postgres {
-  /** Gets an expression of the form `new require('pg').Client()`. */
-  API::Node newClient() { result = API::moduleImport("pg").getMember("Client").getInstance() }
+  /** Gets a reference to the `Client` constructor in the `pg` package, for example `require('pg').Client`. */
+  API::Node newClient() { result = API::moduleImport("pg").getMember("Client") }
 
-  /** Gets a data flow node that holds a freshly created Postgres client instance. */
+  /** Gets a freshly created Postgres client instance. */
   API::Node client() {
-    result = newClient()
+    result = newClient().getInstance()
     or
     // pool.connect(function(err, client) { ... })
-    result = newPool().getMember("connect").getParameter(0).getParameter(1)
+    result = pool().getMember("connect").getParameter(0).getParameter(1)
   }
 
-  /** Gets an expression that constructs a new connection pool. */
+  /** Gets a constructor that when invoked constructs a new connection pool. */
   API::Node newPool() {
     // new require('pg').Pool()
-    result = API::moduleImport("pg").getMember("Pool").getInstance()
+    result = API::moduleImport("pg").getMember("Pool")
     or
     // new require('pg-pool')
-    result = API::moduleImport("pg-pool").getInstance()
+    result = API::moduleImport("pg-pool")
   }
 
+  /** Gets an expression that constructs a new connection pool. */
+  API::Node pool() { result = newPool().getInstance() }
+
   /** A call to the Postgres `query` method. */
   private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
-    QueryCall() { this = [client(), newPool()].getMember("query").getReturn().getAUse() }
+    QueryCall() { this = [client(), pool()].getMember("query").getACall() }
 
     override DataFlow::Node getAQueryArgument() { result = getArgument(0) }
   }
@@ -135,9 +136,8 @@ private module Postgres {
     string kind;
 
     Credentials() {
-      exists(DataFlow::InvokeNode call, string prop |
-        call = [client(), newPool()].getAUse() and
-        this = call.getOptionArgument(0, prop).asExpr() and
+      exists(string prop |
+        this = [newClient(), newPool()].getParameter(0).getMember(prop).getARhs().asExpr() and
         (
           prop = "user" and kind = "user name"
           or
@@ -178,7 +178,7 @@ private module Sqlite {
         meth = "prepare" or
         meth = "run"
       |
-        this = newDb().getMember(meth).getReturn().getAUse()
+        this = newDb().getMember(meth).getACall()
       )
     }
 
@@ -222,7 +222,7 @@ private module MsSql {
 
   /** A call to a MsSql query method. */
   private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
-    QueryCall() { this = request().getMember(["query", "batch"]).getReturn().getAUse() }
+    QueryCall() { this = request().getMember(["query", "batch"]).getACall() }
 
     override DataFlow::Node getAQueryArgument() { result = getArgument(0) }
   }
@@ -250,13 +250,13 @@ private module MsSql {
     string kind;
 
     Credentials() {
-      exists(DataFlow::InvokeNode call, string prop |
+      exists(API::Node callee, string prop |
         (
-          call = mssql().getMember("connect").getReturn().getAUse()
+          callee = mssql().getMember("connect")
           or
-          call = mssql().getMember("ConnectionPool").getInstance().getAUse()
+          callee = mssql().getMember("ConnectionPool")
         ) and
-        this = call.getOptionArgument(0, prop).asExpr() and
+        this = callee.getParameter(0).getMember(prop).getARhs().asExpr() and
         (
           prop = "user" and kind = "user name"
           or
@@ -281,7 +281,7 @@ private module Sequelize {
 
   /** A call to `Sequelize.query`. */
   private class QueryCall extends DatabaseAccess, DataFlow::MethodCallNode {
-    QueryCall() { this = newSequelize().getMember("query").getReturn().getAUse() }
+    QueryCall() { this = newSequelize().getMember("query").getACall() }
 
     override DataFlow::Node getAQueryArgument() { result = getArgument(0) }
   }
@@ -300,7 +300,7 @@ private module Sequelize {
 
     Credentials() {
       exists(NewExpr ne, string prop |
-        ne = newSequelize().getAUse().asExpr() and
+        ne = sequelize().getAnInstantiation().asExpr() and
         (
           this = ne.getArgument(1) and prop = "username"
           or
@@ -378,8 +378,7 @@ private module Spanner {
    */
   class DatabaseRunCall extends SqlExecution {
     DatabaseRunCall() {
-      this =
-        database().getMember(["run", "runPartitionedUpdate", "runStream"]).getReturn().getAUse()
+      this = database().getMember(["run", "runPartitionedUpdate", "runStream"]).getACall()
     }
   }
 
@@ -388,7 +387,7 @@ private module Spanner {
    */
   class TransactionRunCall extends SqlExecution {
     TransactionRunCall() {
-      this = transaction().getMember(["run", "runStream", "runUpdate"]).getReturn().getAUse()
+      this = transaction().getMember(["run", "runStream", "runUpdate"]).getACall()
     }
   }
 
@@ -397,8 +396,7 @@ private module Spanner {
    */
   class ExecuteSqlCall extends SqlExecution {
     ExecuteSqlCall() {
-      this =
-        v1SpannerClient().getMember(["executeSql", "executeStreamingSql"]).getReturn().getAUse()
+      this = v1SpannerClient().getMember(["executeSql", "executeStreamingSql"]).getACall()
     }
 
     override DataFlow::Node getAQueryArgument() {

javascript/ql/test/library-tests/frameworks/SQL/SqlString.expected

@@ -3,6 +3,7 @@
 | mssql2.js:5:15:5:34 | 'select 1 as number' |
 | mssql2.js:13:15:13:66 | 'create ...  table' |
 | mssql2.js:22:24:22:43 | 'select 1 as number' |
+| mssql2.js:29:30:29:81 | 'create ...  table' |
 | mysql1.js:13:18:13:43 | 'SELECT ... lution' |
 | mysql1.js:18:18:22:1 | {\\n    s ... vid']\\n} |
 | mysql1a.js:17:18:17:43 | 'SELECT ... lution' |

javascript/ql/test/library-tests/frameworks/SQL/mssql2.js

@@ -1,6 +1,6 @@
 // Adapted from https://github.com/tediousjs/node-mssql#readme
 const sql = require('mssql')
- 
+
 const request = new sql.Request()
 request.query('select 1 as number', (err, result) => {
     // ... error checks
@@ -19,7 +19,16 @@ class C {
         this.req = req;
     }
     send() {
-        this.req.query('select 1 as number', (err, result) => {})
+        this.req.query('select 1 as number', (err, result) => { })
     }
 }
 new C(new sql.Request());
+
+var obj = {
+    foo: function () {
+        return request.batch('create procedure #temporary as select * from table', (err, result) => {
+            // ... error checks
+        })
+    }
+}
+obj.foo("foo", "bar", "baz"); // An API-graphs gotcha: "baz" should not be considered a `SqlString`