Merge pull request #1529 from xiemaisi/js/getter-summaries

Approved by asger-semmle
2026-05-01 19:55:15 +02:00 · 2019-07-02 06:16:34 +01:00
parent 3b126d9c4e 7f95c20345
commit b0b152aaaa
10 changed files with 153 additions and 19 deletions
--- a/change-notes/1.22/analysis-javascript.md
+++ b/change-notes/1.22/analysis-javascript.md
@@ -10,6 +10,8 @@
  - [exec-async](https://www.npmjs.com/package/exec-async)
  - [express](https://www.npmjs.com/package/express)
  - [remote-exec](https://www.npmjs.com/package/remote-exec)
+
+* Support for tracking data flow and taint through getter functions (that is, functions that return a property of one of their arguments) and through the receiver object of method calls has been improved. This may produce more security alerts.
  
 ## New queries

--- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -521,7 +521,7 @@ private predicate exploratoryFlowStep(
 ) {
  basicFlowStep(pred, succ, _, cfg) or
  basicStoreStep(pred, succ, _) or
-  loadStep(pred, succ, _) or
+  basicLoadStep(pred, succ, _) or
  // the following two disjuncts taken together over-approximate flow through
  // higher-order calls
  callback(pred, succ) or
@@ -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) |
-    loadStep(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)
  )
 }

@@ -743,7 +795,7 @@ private predicate summarizedHigherOrderCall(
 ) {
  exists(
    Function f, DataFlow::InvokeNode outer, DataFlow::InvokeNode inner, int j,
-    DataFlow::Node innerArg, DataFlow::ParameterNode cbParm, PathSummary oldSummary
+    DataFlow::Node innerArg, DataFlow::SourceNode cbParm, PathSummary oldSummary
  |
    reachableFromInput(f, outer, arg, innerArg, cfg, oldSummary) and
    argumentPassing(outer, cb, f, cbParm) and
@@ -786,6 +838,7 @@ private predicate summarizedHigherOrderCall(
 * - The flow label mapping of the summary corresponds to the transformation from `arg` to the
 *   invocation of the callback.
 */
+pragma[nomagic]
 private predicate higherOrderCall(
  DataFlow::Node arg, DataFlow::SourceNode callback, int i, DataFlow::Configuration cfg,
  PathSummary summary
--- a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll
@@ -102,7 +102,7 @@ private module NodeTracking {
      or
      basicStoreStep(mid, nd, _)
      or
-      loadStep(mid, nd, _)
+      basicLoadStep(mid, nd, _)
      or
      callback(mid, nd)
      or
@@ -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::SourceNode 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
-      loadStep(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)
    )
  }

@@ -232,7 +263,7 @@ private module NodeTracking {
  ) {
    exists(
      Function f, DataFlow::InvokeNode outer, DataFlow::InvokeNode inner, int j,
-      DataFlow::Node innerArg, DataFlow::ParameterNode cbParm, PathSummary oldSummary
+      DataFlow::Node innerArg, DataFlow::SourceNode cbParm, PathSummary oldSummary
    |
      reachableFromInput(f, outer, arg, innerArg, oldSummary) and
      argumentPassing(outer, cb, f, cbParm) and
--- a/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TypeTracking.qll
@@ -83,7 +83,7 @@ module StepSummary {
      basicStoreStep(pred, succ, prop) and
      summary = StoreStep(prop)
      or
-      loadStep(pred, succ, prop) and
+      basicLoadStep(pred, succ, prop) and
      summary = LoadStep(prop)
    )
  }
--- a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
@@ -125,20 +125,27 @@ private module CachedSteps {
   */
  cached
  predicate argumentPassing(
-    DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, DataFlow::ParameterNode parm
+    DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, DataFlow::SourceNode parm
  ) {
    calls(invk, f) and
-    exists(int i |
-      f.getParameter(i) = parm.getParameter() and
-      not parm.isRestParameter() and
-      arg = invk.getArgument(i)
+    (
+      exists(int i, Parameter p |
+        f.getParameter(i) = p and
+        not p.isRestParameter() and
+        arg = invk.getArgument(i) and
+        parm = DataFlow::parameterNode(p)
+      )
+      or
+      arg = invk.(DataFlow::CallNode).getReceiver() and
+      parm = DataFlow::thisNode(f)
    )
    or
-    exists(DataFlow::Node callback, int i |
+    exists(DataFlow::Node callback, int i, Parameter p |
      invk.(DataFlow::AdditionalPartialInvokeNode).isPartialArgument(callback, arg, i) and
      partiallyCalls(invk, callback, f) and
-      parm.getParameter() = f.getParameter(i) and
-      not parm.isRestParameter()
+      f.getParameter(i) = p and
+      not p.isRestParameter() and
+      parm = DataFlow::parameterNode(p)
    )
  }

@@ -296,7 +303,7 @@ private module CachedSteps {
   * that is, `succ` is a read of property `prop` from `pred`.
   */
  cached
-  predicate loadStep(DataFlow::Node pred, DataFlow::PropRead succ, string prop) {
+  predicate basicLoadStep(DataFlow::Node pred, DataFlow::PropRead succ, string prop) {
    succ.accesses(pred, prop)
  }

--- 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 |
@@ -37,6 +38,7 @@
 | tst2.js:3:17:3:26 | "tainted2" | tst2.js:11:15:11:24 | g(source2) |
 | tst2.js:6:24:6:37 | "also tainted" | tst2.js:10:15:10:24 | g(source1) |
 | tst2.js:6:24:6:37 | "also tainted" | tst2.js:11:15:11:24 | g(source2) |
+| tst6.mjs:12:14:12:21 | "source" | tst6.mjs:14:12:14:16 | a.m() |
 | tst.js:2:17:2:22 | "src1" | tst.js:39:17:39:17 | x |
 | tst.js:2:17:2:22 | "src1" | tst.js:41:19:41:19 | x |
 | tst.js:2:17:2:22 | "src1" | tst.js:45:17:45:17 | x |
--- 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 |
@@ -38,6 +39,7 @@
 | tst2.js:3:17:3:26 | "tainted2" | tst2.js:11:15:11:24 | g(source2) |
 | tst2.js:6:24:6:37 | "also tainted" | tst2.js:10:15:10:24 | g(source1) |
 | tst2.js:6:24:6:37 | "also tainted" | tst2.js:11:15:11:24 | g(source2) |
+| tst6.mjs:12:14:12:21 | "source" | tst6.mjs:14:12:14:16 | a.m() |
 | tst.js:2:17:2:22 | "src1" | tst.js:39:17:39:17 | x |
 | tst.js:2:17:2:22 | "src1" | tst.js:41:19:41:19 | x |
 | tst.js:2:17:2:22 | "src1" | tst.js:45:17:45:17 | x |
--- 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 |
@@ -47,6 +48,7 @@
 | tst4.js:2:16:2:24 | "tainted" | tst4.js:15:15:15:31 | id(still_tainted) |
 | tst4.js:2:16:2:24 | "tainted" | tst4.js:16:15:16:28 | p.also_tainted |
 | tst4.js:2:16:2:24 | "tainted" | tst4.js:17:15:17:28 | substr(source) |
+| tst6.mjs:12:14:12:21 | "source" | tst6.mjs:14:12:14:16 | a.m() |
 | tst.js:2:17:2:22 | "src1" | tst.js:3:15:3:29 | String(source1) |
 | tst.js:2:17:2:22 | "src1" | tst.js:4:15:4:29 | RegExp(source1) |
 | tst.js:2:17:2:22 | "src1" | tst.js:5:15:5:33 | new String(source1) |
--- 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
--- a/javascript/ql/test/library-tests/InterProceduralFlow/tst6.mjs
+++ b/javascript/ql/test/library-tests/InterProceduralFlow/tst6.mjs
@@ -0,0 +1,14 @@
+class A {
+  constructor(f) {
+    this._f = f;
+
+  }
+
+  m() {
+    return this._f;
+  }
+}
+
+var source = "source";
+var a = new A(source);
+var sink = a.m();