Merge branch 'master' of git.semmle.com:Semmle/ql into MorePathSinks

javascript/ql/src/experimental/SockJS/SockJS.qll

@@ -0,0 +1,46 @@
+/**
+ * Provides classes for working with [SockJS](http://sockjs.org).
+ */
+
+import javascript
+
+/**
+ * A model of the `SockJS` websocket data handler (https://sockjs.org).
+ */
+module SockJS {
+  /**
+   * Access to user-controlled data object received from websocket
+   * For example:
+   * ```
+   * server.on('connection', function(conn) {
+   *     conn.on('data', function(message) {
+   *       ...
+   *     });
+   *  });
+   * ```
+   */
+  class SourceFromSocketJS extends RemoteFlowSource {
+    SourceFromSocketJS() {
+      exists(
+        DataFlow::CallNode createServer, DataFlow::CallNode connNode,
+        DataFlow::CallNode dataHandlerNode
+      |
+        createServer = appCreation() and
+        connNode = createServer.getAMethodCall("on") and
+        connNode.getArgument(0).getStringValue() = "connection" and
+        dataHandlerNode = connNode.getCallback(1).getParameter(0).getAMethodCall("on") and
+        dataHandlerNode.getArgument(0).getStringValue() = "data" and
+        this = dataHandlerNode.getCallback(1).getParameter(0)
+      )
+    }
+
+    override string getSourceType() { result = "input from SockJS WebSocket" }
+  }
+
+  /**
+   * Gets a new SockJS server.
+   */
+  private DataFlow::CallNode appCreation() {
+    result = DataFlow::moduleImport("sockjs").getAMemberCall("createServer")
+  }
+}

javascript/ql/src/experimental/SockJS/examples/server.js

@@ -0,0 +1,16 @@
+const express = require('express');
+const http = require('http');
+const sockjs = require('sockjs');
+
+const app = express();
+const server = http.createServer(app);
+const sockjs_echo = sockjs.createServer({});
+sockjs_echo.on('connection', function(conn) {
+    conn.on('data', function(message) {
+        var data = JSON.parse(message);
+        conn.write(JSON.stringify(eval(data.test)));
+    });
+});
+
+sockjs_echo.installHandlers(server, {prefix:'/echo'});
+server.listen(9090, '127.0.0.1');

javascript/ql/src/semmle/javascript/Functions.qll

@@ -117,7 +117,14 @@ class Function extends @function, Parameterized, TypeParameterized, StmtContaine
   ArgumentsVariable getArgumentsVariable() { result.getFunction() = this }
 
   /** Holds if the body of this function refers to the function's `arguments` variable. */
-  predicate usesArgumentsObject() { exists(getArgumentsVariable().getAnAccess()) }
+  predicate usesArgumentsObject() {
+    exists(getArgumentsVariable().getAnAccess())
+    or
+    exists(PropAccess read |
+      read.getBase() = getVariable().getAnAccess() and
+      read.getPropertyName() = "arguments"
+    )
+  }
 
   /**
    * Holds if this function declares a parameter or local variable named `arguments`.

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

@@ -98,6 +98,17 @@ abstract class Configuration extends string {
    */
   predicate isSource(DataFlow::Node source) { none() }
 
+  /**
+   * Gets the flow label to associate with sources added by the 1-argument `isSource` predicate.
+   *
+   * For taint-tracking configurations, this defaults to `taint` and for other data-flow configurations
+   * it defaults to `data`.
+   *
+   * Overriding this predicate is rarely needed, and overriding the 2-argument `isSource` predicate
+   * should be preferred when possible.
+   */
+  FlowLabel getDefaultSourceLabel() { result = FlowLabel::data() }
+
   /**
    * Holds if `source` is a source of flow labeled with `lbl` that is relevant
    * for this configuration.
@@ -256,9 +267,11 @@ abstract class Configuration extends string {
 /**
  * A label describing the kind of information tracked by a flow configuration.
  *
- * There are two standard labels "data" and "taint", the former describing values
- * that directly originate from a flow source, the latter values that are derived
- * from a flow source via one or more transformations (such as string operations).
+ * There are two standard labels "data" and "taint".
+ * - "data" only propagates along value-preserving data flow, such as assignments
+ *   and parameter-passing, and is the default flow source for a `DataFlow::Configuration`.
+ * - "taint" additionally permits flow through transformations such as string operations,
+ *   and is the default flow source for a `TaintTracking::Configuration`.
  */
 abstract class FlowLabel extends string {
   bindingset[this]
@@ -666,7 +679,7 @@ private predicate exploratoryFlowStep(
  */
 private predicate isSource(DataFlow::Node nd, DataFlow::Configuration cfg, FlowLabel lbl) {
   (cfg.isSource(nd) or nd.(AdditionalSource).isSourceFor(cfg)) and
-  lbl = FlowLabel::data()
+  lbl = cfg.getDefaultSourceLabel()
   or
   nd.(AdditionalSource).isSourceFor(cfg, lbl)
   or
@@ -945,18 +958,31 @@ private predicate reachableFromStoreBase(
         s2.getEndLabel())
   )
   or
-  exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary |
-    reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) and
-    (
-      flowStep(mid, cfg, nd, newSummary)
-      or
-      isAdditionalLoadStoreStep(mid, nd, prop, cfg) and
-      newSummary = PathSummary::level()
-    ) and
+  exists(PathSummary oldSummary, PathSummary newSummary |
+    reachableFromStoreBaseStep(prop, rhs, nd, cfg, oldSummary, newSummary) and
     summary = oldSummary.appendValuePreserving(newSummary)
   )
 }
 
+/**
+ * Holds if `rhs` is the right-hand side of a write to property `prop`, and `nd` is reachable
+ * from the base of that write under configuration `cfg` (possibly through callees) along a
+ * path whose last step is summarized by `newSummary`, and the previous steps are summarized
+ * by `oldSummary`.
+ */
+pragma[noinline]
+private predicate reachableFromStoreBaseStep(
+  string prop, DataFlow::Node rhs, DataFlow::Node nd, DataFlow::Configuration cfg,
+  PathSummary oldSummary, PathSummary newSummary
+) {
+  exists(DataFlow::Node mid | reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) |
+    flowStep(mid, cfg, nd, newSummary)
+    or
+    isAdditionalLoadStoreStep(mid, nd, prop, cfg) and
+    newSummary = PathSummary::level()
+  )
+}
+
 /**
  * Holds if the value of `pred` is written to a property of some base object, and that base
  * object may flow into the base of property read `succ` under configuration `cfg` along
@@ -968,13 +994,29 @@ pragma[noinline]
 private predicate flowThroughProperty(
   DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg, PathSummary summary
 ) {
-  exists(string prop, DataFlow::Node base, PathSummary oldSummary, PathSummary newSummary |
-    reachableFromStoreBase(prop, pred, base, cfg, oldSummary) and
-    loadStep(base, succ, prop, cfg, newSummary) and
+  exists(PathSummary oldSummary, PathSummary newSummary |
+    storeToLoad(pred, succ, cfg, oldSummary, newSummary) and
     summary = oldSummary.append(newSummary)
   )
 }
 
+/**
+ * Holds if the value of `pred` is written to a property of some base object, and that base
+ * object may flow into the base of property read `succ` under configuration `cfg` along
+ * a path whose last step is summarized by `newSummary`, and the previous steps are summarized
+ * by `oldSummary`.
+ */
+pragma[noinline]
+private predicate storeToLoad(
+  DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg, PathSummary oldSummary,
+  PathSummary newSummary
+) {
+  exists(string prop, DataFlow::Node base |
+    reachableFromStoreBase(prop, pred, base, cfg, oldSummary) and
+    loadStep(base, succ, prop, cfg, newSummary)
+  )
+}
+
 /**
  * Holds if `arg` and `cb` are passed as arguments to a function which in turn
  * invokes `cb`, passing `arg` as its `i`th argument.

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

@@ -157,12 +157,12 @@ class InvokeNode extends DataFlow::SourceNode {
    * `name` is set to `result`.
    */
   DataFlow::ValueNode getOptionArgument(int i, string name) {
-    exists(ObjectLiteralNode obj |
-      obj.flowsTo(getArgument(i)) and
-      obj.hasPropertyWrite(name, result)
-    )
+    getOptionsArgument(i).hasPropertyWrite(name, result)
   }
 
+  pragma[noinline]
+  private ObjectLiteralNode getOptionsArgument(int i) { result.flowsTo(getArgument(i)) }
+
   /** Gets an abstract value representing possible callees of this call site. */
   final AbstractValue getACalleeValue() { result = getCalleeNode().analyze().getAValue() }
 

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

@@ -48,7 +48,16 @@ module TaintTracking {
     // overridden to provide taint-tracking specific qldoc
     override predicate isSink(DataFlow::Node sink) { super.isSink(sink) }
 
-    /** Holds if the intermediate node `node` is a taint sanitizer. */
+    /**
+     * Holds if the intermediate node `node` is a taint sanitizer, that is,
+     * tainted values can not flow into or out of `node`.
+     *
+     * Note that this only blocks flow through nodes that operate directly on the tainted value.
+     * An object _containing_ a tainted value in a property can still flow into and out of `node`.
+     * To block such objects, override `isBarrier` or use a labeled sanitizer to block the `data` flow label.
+     *
+     * For operations that _check_ if a value is tainted or safe, use `isSanitizerGuard` instead.
+     */
     predicate isSanitizer(DataFlow::Node node) { none() }
 
     /**
@@ -84,25 +93,35 @@ module TaintTracking {
      * For example, if `guard` is the comparison expression in
      * `if(x == 'some-constant'){ ... x ... }`, it could sanitize flow of
      * `x` into the "then" branch.
+     *
+     * Node that this only handles checks that operate directly on the tainted value.
+     * Objects that _contain_ a tainted value in a property may still flow across the check.
+     * To block such objects, use a labeled sanitizer guard to block the `data` label.
      */
     predicate isSanitizerGuard(SanitizerGuardNode guard) { none() }
 
-    final override predicate isBarrier(DataFlow::Node node) {
-      super.isBarrier(node) or
-      isSanitizer(node) or
-      node instanceof DataFlow::VarAccessBarrier
+    override predicate isLabeledBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+      super.isLabeledBarrier(node, lbl)
+      or
+      isSanitizer(node) and lbl.isTaint()
     }
 
-    final override predicate isBarrierEdge(DataFlow::Node source, DataFlow::Node sink) {
-      super.isBarrierEdge(source, sink) or
-      isSanitizerEdge(source, sink)
+    override predicate isBarrier(DataFlow::Node node) {
+      super.isBarrier(node)
+      or
+      // For variable accesses we block both the data and taint label, as a falsy value
+      // can't be an object, and thus can't have any tainted properties.
+      node instanceof DataFlow::VarAccessBarrier
     }
 
     final override predicate isBarrierEdge(
       DataFlow::Node source, DataFlow::Node sink, DataFlow::FlowLabel lbl
     ) {
-      super.isBarrierEdge(source, sink, lbl) or
+      super.isBarrierEdge(source, sink, lbl)
+      or
       isSanitizerEdge(source, sink, lbl)
+      or
+      isSanitizerEdge(source, sink) and lbl.isTaint()
     }
 
     final override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) {
@@ -127,6 +146,8 @@ module TaintTracking {
     ) {
       isAdditionalFlowStep(pred, succ) and valuePreserving = false
     }
+
+    override DataFlow::FlowLabel getDefaultSourceLabel() { result.isTaint() }
   }
 
   /**
@@ -157,7 +178,7 @@ module TaintTracking {
    * them.
    */
   abstract class SanitizerGuardNode extends DataFlow::BarrierGuardNode {
-    override predicate blocks(boolean outcome, Expr e) { sanitizes(outcome, e) }
+    override predicate blocks(boolean outcome, Expr e) { none() }
 
     /**
      * Holds if this node sanitizes expression `e`, provided it evaluates
@@ -166,6 +187,8 @@ module TaintTracking {
     abstract predicate sanitizes(boolean outcome, Expr e);
 
     override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+      sanitizes(outcome, e) and label.isTaint()
+      or
       sanitizes(outcome, e, label)
     }
 
@@ -180,10 +203,6 @@ module TaintTracking {
    * A sanitizer guard node that only blocks specific flow labels.
    */
   abstract class LabeledSanitizerGuardNode extends SanitizerGuardNode, DataFlow::BarrierGuardNode {
-    final override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
-      sanitizes(outcome, e, label)
-    }
-
     override predicate sanitizes(boolean outcome, Expr e) { none() }
   }
 

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

@@ -23,7 +23,7 @@ module CleartextLogging {
    * A data flow sink for clear-text logging of sensitive information.
    */
   abstract class Sink extends DataFlow::Node {
-    DataFlow::FlowLabel getLabel() { result.isDataOrTaint() }
+    DataFlow::FlowLabel getLabel() { result.isTaint() }
   }
 
   /**
@@ -127,7 +127,7 @@ module CleartextLogging {
 
     override string describe() { result = "an access to " + name }
 
-    override DataFlow::FlowLabel getLabel() { result.isData() }
+    override DataFlow::FlowLabel getLabel() { result.isTaint() }
   }
 
   /** An access to a variable or property that might contain a password. */
@@ -153,7 +153,7 @@ module CleartextLogging {
 
     override string describe() { result = "an access to " + name }
 
-    override DataFlow::FlowLabel getLabel() { result.isData() }
+    override DataFlow::FlowLabel getLabel() { result.isTaint() }
   }
 
   /** A call that might return a password. */
@@ -167,7 +167,7 @@ module CleartextLogging {
 
     override string describe() { result = "a call to " + name }
 
-    override DataFlow::FlowLabel getLabel() { result.isData() }
+    override DataFlow::FlowLabel getLabel() { result.isTaint() }
   }
 
   /** An access to the sensitive object `process.env`. */
@@ -177,7 +177,7 @@ module CleartextLogging {
     override string describe() { result = "process environment" }
 
     override DataFlow::FlowLabel getLabel() {
-      result.isData() or
+      result.isTaint() or
       result instanceof PartiallySensitiveMap
     }
   }

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

@@ -74,7 +74,7 @@ module PrototypePollution {
   private class RemoteFlowAsSource extends Source {
     RemoteFlowAsSource() { this instanceof RemoteFlowSource }
 
-    override DataFlow::FlowLabel getAFlowLabel() { result.isData() }
+    override DataFlow::FlowLabel getAFlowLabel() { result.isTaint() }
   }
 
   /**

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

@@ -53,7 +53,7 @@ module UnsafeDynamicMethodAccess {
         hasUnsafeMethods(read.getBase().getALocalSource()) and
         src = read.getPropertyNameExpr().flow() and
         dst = read and
-        (srclabel = data() or srclabel = taint()) and
+        srclabel.isTaint() and
         dstlabel = unsafeFunction()
       )
       or
@@ -62,7 +62,7 @@ module UnsafeDynamicMethodAccess {
         not PropertyInjection::isPrototypeLessObject(proj.getObject().getALocalSource()) and
         src = proj.getASelector() and
         dst = proj and
-        (srclabel = data() or srclabel = taint()) and
+        srclabel.isTaint() and
         dstlabel = unsafeFunction()
       )
     }

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

@@ -19,7 +19,7 @@ module UnsafeDynamicMethodAccess {
     /**
      * Gets the flow label relevant for this source.
      */
-    DataFlow::FlowLabel getFlowLabel() { result = data() }
+    DataFlow::FlowLabel getFlowLabel() { result = taint() }
   }
 
   /**

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

@@ -40,7 +40,7 @@ module UnvalidatedDynamicMethodCall {
       exists(DataFlow::PropRead read |
         src = read.getPropertyNameExpr().flow() and
         dst = read and
-        (srclabel = data() or srclabel = taint()) and
+        srclabel.isTaint() and
         (
           dstlabel instanceof MaybeNonFunction
           or

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

@@ -19,7 +19,7 @@ module UnvalidatedDynamicMethodCall {
     /**
      * Gets the flow label relevant for this source.
      */
-    DataFlow::FlowLabel getFlowLabel() { result = data() }
+    DataFlow::FlowLabel getFlowLabel() { result = taint() }
   }
 
   /**