Merge pull request #305 from asger-semmle/json-taint-kind

JS: Add flow label for tainted objects and sharpen NosqlInjection

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

@@ -93,7 +93,7 @@ abstract class Configuration extends string {
   }
 
   /**
-   * Holds if `source` is a source of flow labelled with `lbl` that is relevant
+   * Holds if `source` is a source of flow labeled with `lbl` that is relevant
    * for this configuration.
    */
   predicate isSource(DataFlow::Node source, FlowLabel lbl) {
@@ -108,7 +108,7 @@ abstract class Configuration extends string {
   }
 
   /**
-   * Holds if `sink` is a sink of flow labelled with `lbl` that is relevant
+   * Holds if `sink` is a sink of flow labeled with `lbl` that is relevant
    * for this configuration.
    */
   predicate isSink(DataFlow::Node sink, FlowLabel lbl) {
@@ -146,6 +146,7 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node node) {
     exists (BarrierGuardNode guard |
+      not guard instanceof LabeledBarrierGuardNode and
       isBarrierGuard(guard) and
       guard.blocks(node)
     )
@@ -161,6 +162,17 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node src, DataFlow::Node trg, FlowLabel lbl) { none() }
 
+  /**
+   * Holds if flow with label `lbl` cannot flow into `node`.
+   */
+  predicate isLabeledBarrier(DataFlow::Node node, FlowLabel lbl) {
+    exists (LabeledBarrierGuardNode guard |
+      lbl = guard.getALabel() and
+      isBarrierGuard(guard) and
+      guard.blocks(node)
+    )
+  }
+
   /**
    * Holds if data flow node `guard` can act as a barrier when appearing
    * in a condition.
@@ -297,7 +309,16 @@ abstract class BarrierGuardNode extends DataFlow::Node {
    * Holds if this node blocks expression `e` provided it evaluates to `outcome`.
    */
   abstract predicate blocks(boolean outcome, Expr e);
+}
 
+/**
+ * A guard node that only blocks specific labels.
+ */
+abstract class LabeledBarrierGuardNode extends BarrierGuardNode {
+  /**
+   * Get a flow label blocked by this guard node.
+   */
+  abstract FlowLabel getALabel();
 }
 
 /**
@@ -570,7 +591,8 @@ private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk,
     ret.asExpr() = f.getAReturnedExpr() and
     calls(invk, f) and // Do not consider partial calls
     reachableFromInput(f, invk, input, ret, cfg, summary) and
-    not cfg.isBarrier(ret, invk)
+    not cfg.isBarrier(ret, invk) and
+    not cfg.isLabeledBarrier(invk, summary.getEndLabel())
   )
 }
 
@@ -641,7 +663,8 @@ private predicate flowStep(DataFlow::Node pred, DataFlow::Configuration cfg,
    flowThroughProperty(pred, succ, cfg, summary)
   ) and
   not cfg.isBarrier(succ) and
-  not cfg.isBarrier(pred, succ)
+  not cfg.isBarrier(pred, succ) and
+  not cfg.isLabeledBarrier(succ, summary.getEndLabel())
 }
 
 /**
@@ -666,6 +689,7 @@ private predicate reachableFromSource(DataFlow::Node nd, DataFlow::Configuration
   exists (FlowLabel lbl |
     isSource(nd, cfg, lbl) and
     not cfg.isBarrier(nd) and
+    not cfg.isLabeledBarrier(nd, lbl) and
     summary = MkPathSummary(false, false, lbl, lbl)
   )
   or
@@ -684,7 +708,8 @@ private predicate onPath(DataFlow::Node nd, DataFlow::Configuration cfg,
                          PathSummary summary) {
   reachableFromSource(nd, cfg, summary) and
   isSink(nd, cfg, summary.getEndLabel()) and
-  not cfg.isBarrier(nd)
+  not cfg.isBarrier(nd) and
+  not cfg.isLabeledBarrier(nd, summary.getEndLabel())
   or
   exists (DataFlow::Node mid, PathSummary stepSummary |
     reachableFromSource(nd, cfg, summary) and

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -130,7 +130,7 @@ module TaintTracking {
    * configurations it is used in.
    *
    * Note: For performance reasons, all subclasses of this class should be part
-   * of the standard library. Override `Configuration::isTaintSanitizerGuard`
+   * of the standard library. Override `Configuration::isSanitizer`
    * for analysis-specific taint steps.
    */
   abstract class AdditionalSanitizerGuardNode extends SanitizerGuardNode {
@@ -159,6 +159,12 @@ module TaintTracking {
 
   }
 
+  /**
+   * A sanitizer guard node that only blocks specific flow labels.
+   */
+  abstract class LabeledSanitizerGuardNode extends SanitizerGuardNode, DataFlow::LabeledBarrierGuardNode {
+  }
+
   /**
    * DEPRECATED: Override `Configuration::isAdditionalTaintStep` or use
    * `AdditionalTaintStep` instead.

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

@@ -74,10 +74,12 @@ predicate localFlowStep(DataFlow::Node pred, DataFlow::Node succ,
   any(DataFlow::AdditionalFlowStep afs).step(pred, succ) and predlbl = succlbl
   or
   exists (boolean vp | configuration.isAdditionalFlowStep(pred, succ, vp) |
-    if vp = false and (predlbl = FlowLabel::data() or predlbl = FlowLabel::taint()) then
-      succlbl = FlowLabel::taint()
-    else
-      predlbl = succlbl
+    vp = true and
+    predlbl = succlbl
+    or
+    vp = false and
+    (predlbl = FlowLabel::data() or predlbl = FlowLabel::taint()) and
+    succlbl = FlowLabel::taint()
   )
   or
   configuration.isAdditionalFlowStep(pred, succ, predlbl, succlbl)

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

@@ -488,6 +488,31 @@ module Express {
     override string getKind() {
       result = kind
     }
+
+    override predicate isUserControlledObject() {
+      kind = "body" and
+      exists (ExpressLibraries::BodyParser bodyParser, RouteHandlerExpr expr |
+        expr.getBody() = rh and
+        bodyParser.producesUserControlledObjects() and
+        bodyParser.flowsToExpr(expr.getAMatchingAncestor())
+      )
+      or
+      // If we can't find the middlewares for the route handler,
+      // but all known body parsers are deep, assume req.body is a deep object.
+      kind = "body" and
+      forall(ExpressLibraries::BodyParser bodyParser | bodyParser.producesUserControlledObjects())
+      or
+      kind = "parameter" and
+      exists (DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |
+        this.(DataFlow::MethodCallNode).calls(request, "param")
+        or
+        exists (DataFlow::PropRead base |
+          // `req.query.name`
+          base.accesses(request, "query") and
+          this = base.getAPropertyReference(_)
+        )
+      )
+    }
   }
 
   /**

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

@@ -230,4 +230,53 @@ module ExpressLibraries {
 
   }
 
+  /**
+   * An instance of the Express `body-parser` middleware.
+   */
+  class BodyParser extends DataFlow::InvokeNode {
+    string kind;
+
+    BodyParser() {
+      this = DataFlow::moduleImport("body-parser").getACall() and kind = "json"
+      or
+      exists (string moduleName |
+        (moduleName = "body-parser" or moduleName = "express") and
+        (kind = "json" or kind = "urlencoded") and
+        this = DataFlow::moduleMember(moduleName, kind).getACall()
+      )
+    }
+
+    /**
+     * Holds if this is a JSON body parser.
+     */
+    predicate isJson() {
+      kind = "json"
+    }
+
+    /**
+     * Holds if this is a URL-encoded body parser.
+     */
+    predicate isUrlEncoded() {
+      kind = "urlencoded"
+    }
+
+    /**
+     * Holds if this is an extended URL-encoded body parser.
+     *
+     * The extended URL-encoding allows for nested objects, like JSON.
+     */
+    predicate isExtendedUrlEncoded() {
+      kind = "urlencoded" and
+      not getOptionArgument(0, "extended").mayHaveBooleanValue(false)
+    }
+
+    /**
+     * Holds if this parses the input as JSON or extended URL-encoding, resulting
+     * in user-controlled objects (as opposed to user-controlled strings).
+     */
+    predicate producesUserControlledObjects() {
+      isJson() or isExtendedUrlEncoded()
+    }
+  }
+
 }

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

@@ -0,0 +1,119 @@
+/**
+ * Provides methods for reasoning about the flow of deeply tainted objects, such as JSON objects
+ * parsed from user-controlled data.
+ *
+ * Deeply tainted objects are arrays or objects with user-controlled property names, containing
+ * tainted values or deeply tainted objects in their properties.
+ *
+ * To track deeply tainted objects, a flow-tracking configuration should generally include the following:
+ *
+ * 1. One or more sinks associated with the label `TaintedObject::label()`.
+ * 2. The sources from `TaintedObject::isSource`.
+ * 3. The flow steps from `TaintedObject::step`.
+ * 4. The sanitizing guards `TaintedObject::SanitizerGuard`.
+ */
+import javascript
+
+module TaintedObject {
+  private import DataFlow
+
+  private class TaintedObjectLabel extends FlowLabel {
+    TaintedObjectLabel() { this = "tainted-object" }
+  }
+
+  /**
+   * Gets the flow label representing a deeply tainted object.
+   *
+   * A "tainted object" is an array or object whose property values are all assumed to be tainted as well.
+   *
+   * Note that the presence of the this label generally implies the presence of the `taint` label as well.
+   */
+  FlowLabel label() { result instanceof TaintedObjectLabel  }
+
+  /**
+   * Holds for the flows steps that are relevant for tracking user-controlled JSON objects.
+   */
+  predicate step(Node src, Node trg, FlowLabel inlbl, FlowLabel outlbl) {
+    // JSON parsers map tainted inputs to tainted JSON
+    (inlbl = FlowLabel::data() or inlbl = FlowLabel::taint()) and
+    outlbl = label() and
+    exists (JsonParserCall parse |
+      src = parse.getInput() and
+      trg = parse.getOutput())
+    or
+    // Property reads preserve deep object taint.
+    inlbl = label() and
+    outlbl = label() and
+    trg.(PropRead).getBase() = src
+    or
+    // Property projection preserves deep object taint
+    inlbl = label() and
+    outlbl = label() and
+    trg.(PropertyProjection).getObject() = src
+    or
+    // Extending objects preserves deep object taint
+    inlbl = label() and
+    outlbl = label() and
+    exists (ExtendCall call |
+      src = call.getAnOperand() and
+      trg = call
+      or
+      src = call.getASourceOperand() and
+      trg = call.getDestinationOperand().getALocalSource())
+  }
+
+  /**
+   * Holds if `node` is a source of JSON taint and label is the JSON taint label.
+   */
+  predicate isSource(Node source, FlowLabel label) {
+    source instanceof Source and label = label()
+  }
+
+  /**
+   * A source of a user-controlled deep object.
+   */
+  abstract class Source extends DataFlow::Node {}
+
+  /** Request input accesses as a JSON source. */
+  private class RequestInputAsSource extends Source {
+    RequestInputAsSource() {
+      this.(HTTP::RequestInputAccess).isUserControlledObject()
+    }
+  }
+
+  /**
+   * Sanitizer guard that blocks deep object taint.
+   */
+  abstract class SanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode {
+    override FlowLabel getALabel() {
+      result = label()
+    }
+  }
+
+  /**
+   * A test of form `typeof x === "something"`, preventing `x` from being an object in some cases.
+   */
+  private class TypeTestGuard extends SanitizerGuard, ValueNode {
+    override EqualityTest astNode;
+    TypeofExpr typeof;
+    boolean polarity;
+
+    TypeTestGuard() {
+      astNode.getAnOperand() = typeof and
+      (
+        // typeof x === "object" sanitizes `x` when it evaluates to false
+        astNode.getAnOperand().getStringValue() = "object" and
+        polarity = astNode.getPolarity().booleanNot()
+        or
+        // typeof x === "string" sanitizes `x` when it evaluates to true
+        astNode.getAnOperand().getStringValue() != "object" and
+        polarity = astNode.getPolarity()
+      )
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      polarity = outcome and
+      e = typeof.getOperand()
+    }
+  }
+}

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

@@ -65,6 +65,11 @@ module ClientSideUrlRedirect {
       queryAccess(pred, succ) and
       f instanceof DocumentUrl and
       g = DataFlow::FlowLabel::taint()
+      or
+      // preserve document.url label in step from `location` to `location.href`
+      f instanceof DocumentUrl and
+      g instanceof DocumentUrl and
+      succ.(DataFlow::PropRead).accesses(pred, "href")
     }
   }
 

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

@@ -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,8 +41,12 @@ 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) {
@@ -40,12 +54,20 @@ module NosqlInjection {
       node instanceof Sanitizer
     }
 
-    override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+      guard instanceof TaintedObject::SanitizerGuard
+    }
+
+    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/RemoteFlowSources.qll

@@ -10,6 +10,11 @@ import semmle.javascript.security.dataflow.DOM
 abstract class RemoteFlowSource extends DataFlow::Node {
   /** Gets a string that describes the type of this remote flow source. */
   abstract string getSourceType();
+
+  /**
+   * Holds if this can be a user-controlled object, such as a JSON object parsed from user-controlled data.
+   */
+  predicate isUserControlledObject() { none() }
 }
 
 /**

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

@@ -1,5 +1,7 @@
 | 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 |
+| mongodb.js:32:18:32:45 | { title ... itle) } | This query depends on $@. | mongodb.js:26:19:26:26 | req.body | a user-provided value |
+| mongodb.js:54:16:54:20 | query | This query depends on $@. | mongodb.js:49:19:49:33 | req.query.title | a user-provided value |
+| mongodb_bodySafe.js:29:16:29:20 | query | This query depends on $@. | mongodb_bodySafe.js:24:19:24: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/mongodb.js

@@ -16,6 +16,21 @@ 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) });
+
+      let title = req.body.title;
+      if (typeof title === "string") {
+        // OK: input checked to be a string
+        doc.find({ title: title });
+
+        // NOT OK: input is parsed as JSON after string check
+        doc.find({ title: JSON.parse(title) });
+      }
     });
 });
 

javascript/ql/test/query-tests/Security/CWE-089/mongodb_bodySafe.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);
+    });
+});