JavaScript: Track flow through property getter functions.

Max Schaefer · Max Schaefer · commit b3e8103dce64 · 2019-06-28T08:51:27.000+01:00
diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -697,11 +697,60 @@ private predicate storeStep(
   )
 }
 
+/**
+ * Holds if `f` may `read` property `prop` of parameter `parm`.
+ */
+private predicate parameterPropRead(
+  Function f, DataFlow::Node invk, DataFlow::Node arg, string prop, DataFlow::PropRead read,
+  DataFlow::Configuration cfg
+) {
+  exists(DataFlow::SourceNode parm |
+    callInputStep(f, invk, arg, parm, cfg) and
+    read = parm.getAPropertyRead(prop)
+  )
+}
+
+/**
+ * Holds if `nd` may flow into a return statement of `f` under configuration `cfg`
+ * (possibly through callees) along a path summarized by `summary`.
+ */
+private predicate reachesReturn(
+  Function f, DataFlow::Node nd, DataFlow::Configuration cfg, PathSummary summary
+) {
+  isRelevant(nd, cfg) and
+  returnExpr(f, nd, _) and
+  summary = PathSummary::level()
+  or
+  exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary |
+    flowStep(nd, cfg, mid, oldSummary) and
+    reachesReturn(f, mid, cfg, newSummary) and
+    summary = oldSummary.append(newSummary)
+  )
+}
+
+/**
+ * Holds if property `prop` of `pred` may flow into `succ` along a path summarized by
+ * `summary`.
+ */
+private predicate loadStep(
+  DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg,
+  PathSummary summary
+) {
+  basicLoadStep(pred, succ, prop) and
+  summary = PathSummary::level()
+  or
+  exists(Function f, DataFlow::PropRead read |
+    parameterPropRead(f, succ, pred, prop, read, cfg) and
+    reachesReturn(f, read, cfg, summary)
+  )
+}
+
 /**
  * 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 summarized by `summary`.
  */
+pragma[nomagic]
 private predicate reachableFromStoreBase(
   string prop, DataFlow::Node rhs, DataFlow::Node nd, DataFlow::Configuration cfg,
   PathSummary summary
@@ -723,11 +772,14 @@ private predicate reachableFromStoreBase(
  *
  * In other words, `pred` may flow to `succ` through a property.
  */
+pragma[noinline]
 private predicate flowThroughProperty(
   DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg, PathSummary summary
 ) {
-  exists(string prop, DataFlow::Node base | reachableFromStoreBase(prop, pred, base, cfg, summary) |
-    basicLoadStep(base, succ, prop)
+  exists(string prop, DataFlow::Node base, PathSummary oldSummary, PathSummary newSummary |
+    reachableFromStoreBase(prop, pred, base, cfg, oldSummary) and
+    loadStep(base, succ, prop, cfg, newSummary) and
+    summary = oldSummary.append(newSummary)
   )
 }
 
diff --git a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll
@@ -150,6 +150,21 @@ private module NodeTracking {
     )
   }
 
+  /**
+   * Holds if `nd` may flow into a return statement of `f`
+   * (possibly through callees) along a path summarized by `summary`.
+   */
+  private predicate reachesReturn(Function f, DataFlow::Node nd, PathSummary summary) {
+    returnExpr(f, nd, _) and
+    summary = PathSummary::level()
+    or
+    exists (DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary |
+      flowStep(nd, mid, oldSummary) and
+      reachesReturn(f, mid, newSummary) and
+      summary = oldSummary.append(newSummary)
+    )
+  }
+
   /**
    * Holds if a function invoked at `invk` may return an expression into which `input`,
    * which is either an argument or a definition captured by the function, flows,
@@ -191,6 +206,21 @@ private module NodeTracking {
     )
   }
 
+  /**
+   * Holds if property `prop` of `pred` may flow into `succ` along a path summarized by
+   * `summary`.
+   */
+  private predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop,
+                            PathSummary summary) {
+    basicLoadStep(pred, succ, prop) and
+    summary = PathSummary::level()
+    or
+    exists (Function f, DataFlow::ParameterNode parm |
+      argumentPassing(succ, pred, f, parm) and
+      reachesReturn(f, parm.getAPropertyRead(prop), summary)
+    )
+  }
+
   /**
    * Holds if `rhs` is the right-hand side of a write to property `prop`, and `nd` is reachable
    * from the base of that write (possibly through callees) along a path summarized by `summary`.
@@ -216,9 +246,10 @@ private module NodeTracking {
   private predicate flowThroughProperty(
     DataFlow::Node pred, DataFlow::Node succ, PathSummary summary
   ) {
-    exists(string prop, DataFlow::Node base |
-      reachableFromStoreBase(prop, pred, base, summary) and
-      basicLoadStep(base, succ, prop)
+    exists (string prop, DataFlow::Node base, PathSummary oldSummary, PathSummary newSummary |
+      reachableFromStoreBase(prop, pred, base, oldSummary) and
+      loadStep(base, succ, prop, newSummary) and
+      summary = oldSummary.append(newSummary)
     )
   }
 
diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected b/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected
@@ -29,6 +29,7 @@
 | promises.js:12:22:12:31 | "rejected" | promises.js:24:20:24:20 | v |
 | promises.js:12:22:12:31 | "rejected" | promises.js:27:16:27:16 | v |
 | properties2.js:7:14:7:21 | "source" | properties2.js:8:12:8:24 | foo(source).p |
+| properties2.js:7:14:7:21 | "source" | properties2.js:33:13:33:20 | getP(o3) |
 | properties.js:2:16:2:24 | "tainted" | properties.js:5:14:5:23 | a.someProp |
 | properties.js:2:16:2:24 | "tainted" | properties.js:12:15:12:24 | x.someProp |
 | properties.js:2:16:2:24 | "tainted" | properties.js:14:15:14:27 | tmp1.someProp |
diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected b/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected
@@ -30,6 +30,7 @@
 | promises.js:12:22:12:31 | "rejected" | promises.js:24:20:24:20 | v |
 | promises.js:12:22:12:31 | "rejected" | promises.js:27:16:27:16 | v |
 | properties2.js:7:14:7:21 | "source" | properties2.js:8:12:8:24 | foo(source).p |
+| properties2.js:7:14:7:21 | "source" | properties2.js:33:13:33:20 | getP(o3) |
 | properties.js:2:16:2:24 | "tainted" | properties.js:5:14:5:23 | a.someProp |
 | properties.js:2:16:2:24 | "tainted" | properties.js:12:15:12:24 | x.someProp |
 | properties.js:2:16:2:24 | "tainted" | properties.js:14:15:14:27 | tmp1.someProp |
diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected b/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected
@@ -36,6 +36,7 @@
 | promises.js:12:22:12:31 | "rejected" | promises.js:27:16:27:16 | v |
 | promises.js:32:24:32:37 | "also tainted" | promises.js:38:32:38:32 | v |
 | properties2.js:7:14:7:21 | "source" | properties2.js:8:12:8:24 | foo(source).p |
+| properties2.js:7:14:7:21 | "source" | properties2.js:33:13:33:20 | getP(o3) |
 | properties.js:2:16:2:24 | "tainted" | properties.js:5:14:5:23 | a.someProp |
 | properties.js:2:16:2:24 | "tainted" | properties.js:12:15:12:24 | x.someProp |
 | properties.js:2:16:2:24 | "tainted" | properties.js:14:15:14:27 | tmp1.someProp |
diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/properties2.js b/javascript/ql/test/library-tests/InterProceduralFlow/properties2.js
@@ -21,4 +21,25 @@ var o2 = {};
 setP(o2, "not a source");
 var sink5 = o2.p;
 
+function getP(base) {
+  return base.p;
+}
+
+function getQ(base) {
+  return base.q;
+}
+
+var o3 = { p: source };
+var sink6 = getP(o3);
+var sink7 = getQ(o3);
+
+var o4 = {};
+setP(o4, source);
+var sink8 = getP(o4);
+var sink9 = getQ(o4);
+
+var o5 = {};
+setP(o5, "not a source");
+var sink10 = getP(o5);
+
 // semmle-extractor-options: --source-type module
