JS: make NosqlInjection use object taint

Author: asger-semmle

javascript/ql/src/Security/CWE-089/SqlInjection.actual

@@ -4,6 +4,7 @@
  */
 
 import javascript
+import semmle.javascript.security.TaintedObject
 
 module NosqlInjection {
   /**
@@ -14,7 +15,16 @@ module NosqlInjection {
   /**
    * A data flow sink for SQL-injection vulnerabilities.
    */
-  abstract class Sink extends DataFlow::Node { }
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets a flow label relevant for this sink.
+     *
+     * Defaults to deeply tainted objects only.
+     */
+    DataFlow::FlowLabel getAFlowLabel() {
+      result = TaintedObject::label()
+    }
+  }
 
   /**
    * A sanitizer for SQL-injection vulnerabilities.
@@ -31,21 +41,29 @@ module NosqlInjection {
       source instanceof Source
     }
 
-    override predicate isSink(DataFlow::Node sink) {
-      sink instanceof Sink
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      TaintedObject::isSource(source, label)
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      sink.(Sink).getAFlowLabel() = label
     }
 
     override predicate isSanitizer(DataFlow::Node node) {
       super.isSanitizer(node) or
       node instanceof Sanitizer
     }
 
-    override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl) {
+      TaintedObject::step(src, trg, inlbl, outlbl)
+      or
       // additional flow step to track taint through NoSQL query objects
+      inlbl = TaintedObject::label() and
+      outlbl = TaintedObject::label() and
       exists (NoSQL::Query query, DataFlow::SourceNode queryObj |
         queryObj.flowsToExpr(query) and
-        queryObj.flowsTo(succ) and
-        pred = queryObj.getAPropertyWrite().getRhs()
+        queryObj.flowsTo(trg) and
+        src = queryObj.getAPropertyWrite().getRhs()
       )
     }
   }

javascript/ql/src/semmle/javascript/security/dataflow/NosqlInjection.qll

@@ -1,5 +1,4 @@
 | 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: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 |

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

@@ -16,6 +16,12 @@ app.post('/documents/find', (req, res) => {
 
       // NOT OK: query is tainted by user-provided object value
       doc.find(query);
+
+      // OK: user-data is coerced to a string
+      doc.find({ title: '' + query.body.title });
+
+      // OK: throws unless user-data is a string
+      doc.find({ title: query.body.title.substr(1) });
     });
 });
 
@@ -36,6 +42,6 @@ app.post('/documents/find', (req, res) => {
       let doc = db.collection('doc');
 
       // NOT OK: query is tainted by user-provided object value
-      doc.find(query);
+      doc.find(query); // Not currently detected 
     });
 });

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

@@ -0,0 +1,31 @@
+const express = require('express'),
+      mongodb = require('mongodb'),
+      bodyParser = require('body-parser');
+
+const MongoClient = mongodb.MongoClient;
+
+const app = express();
+
+app.use(bodyParser.urlencoded({ extended: false }));
+
+app.post('/documents/find', (req, res) => {
+    const query = {};
+    query.title = req.body.title;
+    MongoClient.connect('mongodb://localhost:27017/test', (err, db) => {
+      let doc = db.collection('doc');
+
+      // OK: req.body is safe
+      doc.find(query);
+    });
+});
+
+app.post('/documents/find', (req, res) => {
+    const query = {};
+    query.title = req.query.title;
+    MongoClient.connect('mongodb://localhost:27017/test', (err, db) => {
+      let doc = db.collection('doc');
+
+      // NOT OK: regardless of body parser, query value is still tainted
+      doc.find(query);
+    });
+});