Merge pull request #765 from asger-semmle/class-receiver-propagation

JS: support flow out of "this" in constructor call

Author: Max Schaefer

change-notes/1.20/analysis-javascript.md

@@ -8,7 +8,7 @@
   - server-side code, for example [hapi](https://hapijs.com/)
 * File classification has been improved to recognize additional generated files, for example files from [HTML Tidy](html-tidy.org).
 
-* The taint tracking library now recognizes flow through persistent storage and callbacks in certain cases. This may give more results for the security queries.
+* The taint tracking library now recognizes flow through persistent storage, class fields, and callbacks in certain cases. This may give more results for the security queries.
 
 ## New queries
 
@@ -41,3 +41,4 @@
 * `DataFlow::SourceNode` is no longer an abstract class; to add new source nodes, extend `DataFlow::SourceNode::Range` instead.
 * Subclasses of `DataFlow::PropRead` are no longer automatically made source nodes; you now need to additionally define a corresponding subclass of `DataFlow::SourceNode::Range` to achieve this.
 * The deprecated libraries `semmle.javascript.DataFlow` and `semmle.javascript.dataflow.CallGraph` have been removed; they are both superseded by `semmle.javascript.dataflow.DataFlow`.
+* The predicate `DataFlow::returnedPropWrite` was intended for internal use only and is no longer available.

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

@@ -606,22 +606,19 @@ private predicate storeStep(
   basicStoreStep(pred, succ, prop) and
   summary = PathSummary::level()
   or
-  exists(Function f, DataFlow::Node mid, DataFlow::Node base |
-    // `f` stores its parameter `pred` in property `prop` of a value that it returns,
+  exists(Function f, DataFlow::Node mid |
+    // `f` stores its parameter `pred` in property `prop` of a value that flows back to the caller,
     // and `succ` is an invocation of `f`
     reachableFromInput(f, succ, pred, mid, cfg, summary) and
-    returnedPropWrite(f, base, prop, mid)
+    (
+      returnedPropWrite(f, _, prop, mid)
+      or
+      succ instanceof DataFlow::NewNode and
+      receiverPropWrite(f, prop, mid)
+    )
   )
 }
 
-/**
- * Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`.
- */
-predicate returnedPropWrite(Function f, DataFlow::SourceNode base, string prop, DataFlow::Node rhs) {
-  base.hasPropertyWrite(prop, rhs) and
-  base.flowsToExpr(f.getAReturnedExpr())
-}
-
 /**
  * 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

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

@@ -802,6 +802,13 @@ module DataFlow {
    */
   predicate thisNode(DataFlow::Node node, StmtContainer container) { node = TThisNode(container) }
 
+  /**
+   * Gets the node representing the receiver of the given function, or `this` in the given top-level.
+   *
+   * Has no result if `container` is an arrow function.
+   */
+  DataFlow::ThisNode thisNode(StmtContainer container) { result = TThisNode(container) }
+
   /**
    * A classification of flows that are not modeled, or only modeled incompletely, by
    * `DataFlowNode`:

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

@@ -167,12 +167,16 @@ private module NodeTracking {
     basicStoreStep(pred, succ, prop) and
     summary = PathSummary::level()
     or
-    exists(Function f, DataFlow::Node mid, DataFlow::SourceNode base |
-      // `f` stores its parameter `pred` in property `prop` of a value that it returns,
+    exists(Function f, DataFlow::Node mid |
+      // `f` stores its parameter `pred` in property `prop` of a value that flows back to the caller,
       // and `succ` is an invocation of `f`
       reachableFromInput(f, succ, pred, mid, summary) and
-      base.hasPropertyWrite(prop, mid) and
-      base.flowsToExpr(f.getAReturnedExpr())
+      (
+        returnedPropWrite(f, _, prop, mid)
+        or
+        succ instanceof DataFlow::NewNode and
+        receiverPropWrite(f, prop, mid)
+      )
     )
   }
 

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

@@ -115,12 +115,14 @@ predicate callStep(DataFlow::Node pred, DataFlow::Node succ) {
 
 /**
  * Holds if there is a flow step from `pred` to `succ` through returning
- * from a function call.
+ * from a function call or the receiver flowing out of a constructor call.
  */
 predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) {
-  exists(Function f |
-    returnExpr(f, pred, _) and
-    calls(succ, f)
+  exists(Function f | calls(succ, f) |
+    returnExpr(f, pred, _)
+    or
+    succ instanceof DataFlow::NewNode and
+    DataFlow::thisNode(pred, f)
   )
 }
 
@@ -264,6 +266,21 @@ predicate callback(DataFlow::Node arg, DataFlow::SourceNode cb) {
   )
 }
 
+/**
+ * Holds if `f` may return `base`, which has a write of property `prop` with right-hand side `rhs`.
+ */
+predicate returnedPropWrite(Function f, DataFlow::SourceNode base, string prop, DataFlow::Node rhs) {
+  base.hasPropertyWrite(prop, rhs) and
+  base.flowsToExpr(f.getAReturnedExpr())
+}
+
+/**
+ * Holds if `f` may assign `rhs` to `this.prop`.
+ */
+predicate receiverPropWrite(Function f, string prop, DataFlow::Node rhs) {
+  DataFlow::thisNode(f).hasPropertyWrite(prop, rhs)
+}
+
 /**
  * A utility class that is equivalent to `boolean` but does not require type joining.
  */

javascript/ql/test/library-tests/InterProceduralFlow/constructor-calls.js

@@ -0,0 +1,12 @@
+import * as dummy from 'dummy';
+
+class C {
+  constructor() {
+    this.field = new Object();
+  }
+}
+
+function test() {
+  let x = new C().field;
+  foo(x);
+}

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -1,4 +1,10 @@
 | advanced-callgraph.js:2:13:2:20 | source() | advanced-callgraph.js:6:22:6:22 | v |
+| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:18:8:18:14 | c.taint |
+| constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:22:8:22:19 | c_safe.taint |
+| constructor-calls.js:10:16:10:23 | source() | constructor-calls.js:26:8:26:14 | d.taint |
+| constructor-calls.js:10:16:10:23 | source() | constructor-calls.js:30:8:30:19 | d_safe.taint |
+| constructor-calls.js:14:15:14:22 | source() | constructor-calls.js:17:8:17:14 | c.param |
+| constructor-calls.js:14:15:14:22 | source() | constructor-calls.js:25:8:25:14 | d.param |
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:17:14:17:14 | x |
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:20:14:20:14 | y |
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:30:14:30:20 | x.value |

javascript/ql/test/library-tests/TaintTracking/constructor-calls.js

@@ -0,0 +1,31 @@
+class EcmaClass {
+  constructor(param) {
+    this.param = param;
+    this.taint = source();
+  }
+}
+
+function JsClass(param) {
+  this.param = param;
+  this.taint = source();
+}
+
+function test() {
+  let taint = source();
+
+  let c = new EcmaClass(taint);
+  sink(c.param); // NOT OK
+  sink(c.taint); // NOT OK
+  
+  let c_safe = new EcmaClass("safe");
+  sink(c_safe.param); // OK
+  sink(c_safe.taint); // NOT OK
+  
+  let d = new JsClass(taint);
+  sink(d.param); // NOT OK
+  sink(d.taint); // NOT OK
+  
+  let d_safe = new JsClass("safe");
+  sink(d_safe.param); // OK
+  sink(d_safe.taint); // NOT OK
+}