From 4ed1a40f1d401ec1d052fdb115b6c1835d31982e Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 30 Aug 2021 14:54:26 +0200
Subject: [PATCH 1/8] add steps for `next()` call in RxJS

---
 .../lib/semmle/javascript/frameworks/RxJS.qll | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/javascript/ql/lib/semmle/javascript/frameworks/RxJS.qll b/javascript/ql/lib/semmle/javascript/frameworks/RxJS.qll
index 4131b9bd61e1..8468e74ba707 100644
--- a/javascript/ql/lib/semmle/javascript/frameworks/RxJS.qll
+++ b/javascript/ql/lib/semmle/javascript/frameworks/RxJS.qll
@@ -102,3 +102,44 @@ private class RxJsPipeMapStep extends TaintTracking::SharedTaintStep {
     )
   }
 }
+
+/**
+ * Gets a instance of the `Subject` class from RxJS.
+ */
+private API::Node getARxJSSubject() {
+  result =
+    API::moduleImport("rxjs")
+        .getMember(["Subject", "BehaviorSubject", "ReplaySubject", "AsyncSubject"])
+        .getInstance()
+  or
+  result =
+    API::Node::ofType("rxjs", ["Subject", "BehaviorSubject", "ReplaySubject", "AsyncSubject"])
+}
+
+/**
+ * A step from a call to the `next` method of a `Subject` to a read of the value.
+ */
+private class RxJSNextStep extends DataFlow::SharedFlowStep {
+  override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    exists(API::CallNode call |
+      call = getARxJSSubject().getMember("next").getACall() and
+      pred = call.getArgument(0) and
+      succ = call.getReceiver().getALocalSource() and
+      prop = "value"
+    )
+  }
+
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(API::Node subject |
+      subject = getARxJSSubject() and
+      pred = subject.getMember("next").getACall().getArgument(0) and
+      succ =
+        subject
+            .getMember("subscribe")
+            .getParameter(0)
+            .getMember("next")
+            .getParameter(0)
+            .getAnImmediateUse()
+    )
+  }
+}

From 51a73a4431d54274840af70d8d05e24ca8201239 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 30 Aug 2021 14:55:10 +0200
Subject: [PATCH 2/8] add `localFieldStep` as a `DataFlow::SharedFlowStep`, and
 support writes on nested properties

---
 .../ql/lib/semmle/javascript/dataflow/DataFlow.qll    | 11 ++++++++++-
 .../CWE-078/UnsafeShellCommandConstruction.expected   | 10 ++++++++++
 .../ql/test/query-tests/Security/CWE-078/lib/lib.js   |  2 +-
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
index 1d5f49d6e2f4..ecae8f60f117 100644
--- a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
@@ -1586,13 +1586,22 @@ module DataFlow {
    */
   predicate localFieldStep(DataFlow::Node pred, DataFlow::Node succ) {
     exists(ClassNode cls, string prop |
-      pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyWrite(prop).getRhs() or
+      pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyWrite(prop).getRhs()
+      or
+      // add support for writes on nested properties
+      pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyRead(prop) and
+      pred = any(DataFlow::PropRef ref).getBase()
+      or
       pred = cls.getInstanceMethod(prop)
     |
       succ = cls.getAReceiverNode().getAPropertyRead(prop)
     )
   }
 
+  private class LocalFieldStep extends DataFlow::SharedFlowStep {
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) { localFieldStep(pred, succ) }
+  }
+
   predicate argumentPassingStep = FlowSteps::argumentPassing/4;
 
   /**
diff --git a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected
index 3ff369a3b8dd..38168d2d4bcf 100644
--- a/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected
@@ -166,6 +166,10 @@ nodes
 | lib/lib.js:277:23:277:26 | opts |
 | lib/lib.js:277:23:277:30 | opts.bla |
 | lib/lib.js:277:23:277:30 | opts.bla |
+| lib/lib.js:279:19:279:22 | opts |
+| lib/lib.js:279:19:279:26 | opts.bla |
+| lib/lib.js:281:23:281:35 | this.opts.bla |
+| lib/lib.js:281:23:281:35 | this.opts.bla |
 | lib/lib.js:307:39:307:42 | name |
 | lib/lib.js:307:39:307:42 | name |
 | lib/lib.js:308:23:308:26 | name |
@@ -468,8 +472,13 @@ edges
 | lib/lib.js:268:22:268:24 | obj | lib/lib.js:268:22:268:32 | obj.version |
 | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:26 | opts |
 | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:26 | opts |
+| lib/lib.js:276:8:276:11 | opts | lib/lib.js:279:19:279:22 | opts |
+| lib/lib.js:276:8:276:11 | opts | lib/lib.js:279:19:279:22 | opts |
 | lib/lib.js:277:23:277:26 | opts | lib/lib.js:277:23:277:30 | opts.bla |
 | lib/lib.js:277:23:277:26 | opts | lib/lib.js:277:23:277:30 | opts.bla |
+| lib/lib.js:279:19:279:22 | opts | lib/lib.js:279:19:279:26 | opts.bla |
+| lib/lib.js:279:19:279:26 | opts.bla | lib/lib.js:281:23:281:35 | this.opts.bla |
+| lib/lib.js:279:19:279:26 | opts.bla | lib/lib.js:281:23:281:35 | this.opts.bla |
 | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name |
 | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name |
 | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name |
@@ -636,6 +645,7 @@ edges
 | lib/lib.js:261:11:261:33 | "rm -rf ...  + name | lib/lib.js:257:35:257:38 | name | lib/lib.js:261:30:261:33 | name | $@ based on $@ is later used in $@. | lib/lib.js:261:11:261:33 | "rm -rf ...  + name | String concatenation | lib/lib.js:257:35:257:38 | name | library input | lib/lib.js:261:3:261:34 | cp.exec ... + name) | shell command |
 | lib/lib.js:268:10:268:32 | "rm -rf ... version | lib/lib.js:267:46:267:48 | obj | lib/lib.js:268:22:268:32 | obj.version | $@ based on $@ is later used in $@. | lib/lib.js:268:10:268:32 | "rm -rf ... version | String concatenation | lib/lib.js:267:46:267:48 | obj | library input | lib/lib.js:268:2:268:33 | cp.exec ... ersion) | shell command |
 | lib/lib.js:277:11:277:30 | "rm -rf " + opts.bla | lib/lib.js:276:8:276:11 | opts | lib/lib.js:277:23:277:30 | opts.bla | $@ based on $@ is later used in $@. | lib/lib.js:277:11:277:30 | "rm -rf " + opts.bla | String concatenation | lib/lib.js:276:8:276:11 | opts | library input | lib/lib.js:277:3:277:31 | cp.exec ... ts.bla) | shell command |
+| lib/lib.js:281:11:281:35 | "rm -rf ... pts.bla | lib/lib.js:276:8:276:11 | opts | lib/lib.js:281:23:281:35 | this.opts.bla | $@ based on $@ is later used in $@. | lib/lib.js:281:11:281:35 | "rm -rf ... pts.bla | String concatenation | lib/lib.js:276:8:276:11 | opts | library input | lib/lib.js:281:3:281:36 | cp.exec ... ts.bla) | shell command |
 | lib/lib.js:308:11:308:26 | "rm -rf " + name | lib/lib.js:307:39:307:42 | name | lib/lib.js:308:23:308:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:308:11:308:26 | "rm -rf " + name | String concatenation | lib/lib.js:307:39:307:42 | name | library input | lib/lib.js:308:3:308:27 | cp.exec ... + name) | shell command |
 | lib/lib.js:315:10:315:25 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:315:22:315:25 | name | $@ based on $@ is later used in $@. | lib/lib.js:315:10:315:25 | "rm -rf " + name | String concatenation | lib/lib.js:314:40:314:43 | name | library input | lib/lib.js:315:2:315:26 | cp.exec ... + name) | shell command |
 | lib/lib.js:320:11:320:26 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:320:23:320:26 | name | $@ based on $@ is later used in $@. | lib/lib.js:320:11:320:26 | "rm -rf " + name | String concatenation | lib/lib.js:314:40:314:43 | name | library input | lib/lib.js:320:3:320:27 | cp.exec ... + name) | shell command |
diff --git a/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js b/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js
index f87f10f83a85..9df87ac96fa4 100644
--- a/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js
+++ b/javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js
@@ -278,7 +278,7 @@ module.exports.Foo = class Foo {
 		this.opts = {};
 		this.opts.bla = opts.bla
 
-		cp.exec("rm -rf " + this.opts.bla); // NOT OK - but FN [INCONSISTENCY]
+		cp.exec("rm -rf " + this.opts.bla); // NOT OK
 	}
 }
 

From ca063663972f054641054867c4a89dd26ca3a288 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 30 Aug 2021 14:55:15 +0200
Subject: [PATCH 3/8] add test

---
 .../CodeInjection/CodeInjection.expected      | 19 +++++++++++++++++
 .../HeuristicSourceCodeInjection.expected     | 17 +++++++++++++++
 .../Security/CWE-094/CodeInjection/rxjs.js    | 21 +++++++++++++++++++
 3 files changed, 57 insertions(+)
 create mode 100644 javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/rxjs.js

diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected
index a0b4a89d54fa..890047be35d7 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/CodeInjection.expected
@@ -110,6 +110,15 @@ nodes
 | react.js:10:56:10:77 | documen ... on.hash |
 | react.js:10:56:10:77 | documen ... on.hash |
 | react.js:10:56:10:77 | documen ... on.hash |
+| rxjs.js:9:9:9:35 | taint |
+| rxjs.js:9:17:9:35 | req.param("wobble") |
+| rxjs.js:9:17:9:35 | req.param("wobble") |
+| rxjs.js:12:16:12:20 | taint |
+| rxjs.js:14:12:14:12 | v |
+| rxjs.js:15:12:15:12 | v |
+| rxjs.js:15:12:15:12 | v |
+| rxjs.js:19:10:19:22 | subject.value |
+| rxjs.js:19:10:19:22 | subject.value |
 | template-sinks.js:17:9:17:31 | tainted |
 | template-sinks.js:17:19:17:31 | req.query.foo |
 | template-sinks.js:17:19:17:31 | req.query.foo |
@@ -232,6 +241,14 @@ edges
 | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted |
 | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted |
 | react.js:10:56:10:77 | documen ... on.hash | react.js:10:56:10:77 | documen ... on.hash |
+| rxjs.js:9:9:9:35 | taint | rxjs.js:12:16:12:20 | taint |
+| rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:9:9:9:35 | taint |
+| rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:9:9:9:35 | taint |
+| rxjs.js:12:16:12:20 | taint | rxjs.js:14:12:14:12 | v |
+| rxjs.js:12:16:12:20 | taint | rxjs.js:19:10:19:22 | subject.value |
+| rxjs.js:12:16:12:20 | taint | rxjs.js:19:10:19:22 | subject.value |
+| rxjs.js:14:12:14:12 | v | rxjs.js:15:12:15:12 | v |
+| rxjs.js:14:12:14:12 | v | rxjs.js:15:12:15:12 | v |
 | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:19:17:19:23 | tainted |
 | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:19:17:19:23 | tainted |
 | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:20:16:20:22 | tainted |
@@ -313,6 +330,8 @@ edges
 | react-native.js:8:32:8:38 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:8:32:8:38 | tainted | $@ flows to here and is interpreted as code. | react-native.js:7:17:7:33 | req.param("code") | User-provided value |
 | react-native.js:10:23:10:29 | tainted | react-native.js:7:17:7:33 | req.param("code") | react-native.js:10:23:10:29 | tainted | $@ flows to here and is interpreted as code. | react-native.js:7:17:7:33 | req.param("code") | User-provided value |
 | react.js:10:56:10:77 | documen ... on.hash | react.js:10:56:10:77 | documen ... on.hash | react.js:10:56:10:77 | documen ... on.hash | $@ flows to here and is interpreted as code. | react.js:10:56:10:77 | documen ... on.hash | User-provided value |
+| rxjs.js:15:12:15:12 | v | rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:15:12:15:12 | v | $@ flows to here and is interpreted as code. | rxjs.js:9:17:9:35 | req.param("wobble") | User-provided value |
+| rxjs.js:19:10:19:22 | subject.value | rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:19:10:19:22 | subject.value | $@ flows to here and is interpreted as code. | rxjs.js:9:17:9:35 | req.param("wobble") | User-provided value |
 | template-sinks.js:19:17:19:23 | tainted | template-sinks.js:17:19:17:31 | req.query.foo | template-sinks.js:19:17:19:23 | tainted | $@ flows to here and is interpreted as a template, which may contain code. | template-sinks.js:17:19:17:31 | req.query.foo | User-provided value |
 | template-sinks.js:20:16:20:22 | tainted | template-sinks.js:17:19:17:31 | req.query.foo | template-sinks.js:20:16:20:22 | tainted | $@ flows to here and is interpreted as a template, which may contain code. | template-sinks.js:17:19:17:31 | req.query.foo | User-provided value |
 | template-sinks.js:21:18:21:24 | tainted | template-sinks.js:17:19:17:31 | req.query.foo | template-sinks.js:21:18:21:24 | tainted | $@ flows to here and is interpreted as a template, which may contain code. | template-sinks.js:17:19:17:31 | req.query.foo | User-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected
index d1a6a3deef3c..82a89178cd9b 100644
--- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/HeuristicSourceCodeInjection.expected
@@ -114,6 +114,15 @@ nodes
 | react.js:10:56:10:77 | documen ... on.hash |
 | react.js:10:56:10:77 | documen ... on.hash |
 | react.js:10:56:10:77 | documen ... on.hash |
+| rxjs.js:9:9:9:35 | taint |
+| rxjs.js:9:17:9:35 | req.param("wobble") |
+| rxjs.js:9:17:9:35 | req.param("wobble") |
+| rxjs.js:12:16:12:20 | taint |
+| rxjs.js:14:12:14:12 | v |
+| rxjs.js:15:12:15:12 | v |
+| rxjs.js:15:12:15:12 | v |
+| rxjs.js:19:10:19:22 | subject.value |
+| rxjs.js:19:10:19:22 | subject.value |
 | template-sinks.js:17:9:17:31 | tainted |
 | template-sinks.js:17:19:17:31 | req.query.foo |
 | template-sinks.js:17:19:17:31 | req.query.foo |
@@ -240,6 +249,14 @@ edges
 | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted |
 | react-native.js:7:17:7:33 | req.param("code") | react-native.js:7:7:7:33 | tainted |
 | react.js:10:56:10:77 | documen ... on.hash | react.js:10:56:10:77 | documen ... on.hash |
+| rxjs.js:9:9:9:35 | taint | rxjs.js:12:16:12:20 | taint |
+| rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:9:9:9:35 | taint |
+| rxjs.js:9:17:9:35 | req.param("wobble") | rxjs.js:9:9:9:35 | taint |
+| rxjs.js:12:16:12:20 | taint | rxjs.js:14:12:14:12 | v |
+| rxjs.js:12:16:12:20 | taint | rxjs.js:19:10:19:22 | subject.value |
+| rxjs.js:12:16:12:20 | taint | rxjs.js:19:10:19:22 | subject.value |
+| rxjs.js:14:12:14:12 | v | rxjs.js:15:12:15:12 | v |
+| rxjs.js:14:12:14:12 | v | rxjs.js:15:12:15:12 | v |
 | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:19:17:19:23 | tainted |
 | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:19:17:19:23 | tainted |
 | template-sinks.js:17:9:17:31 | tainted | template-sinks.js:20:16:20:22 | tainted |
diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/rxjs.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/rxjs.js
new file mode 100644
index 000000000000..37f4b7631402
--- /dev/null
+++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/rxjs.js
@@ -0,0 +1,21 @@
+var express = require('express');
+
+var app = express();
+
+import { BehaviorSubject } from 'rxjs';
+
+
+app.get('/some/path', function(req, res) {
+  const taint = req.param("wobble");
+
+  const subject = new BehaviorSubject();
+  subject.next(taint);
+  subject.subscribe({
+    next: (v) => {
+      eval(v); // NOT OK
+    }
+  });
+  setTimeout(() => {
+    eval(subject.value); // NOT OK
+  }, 100);
+});

From d6aa30cc157834efcf453cc9a80779263f47f868 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Wed, 15 Sep 2021 17:54:33 +0200
Subject: [PATCH 4/8] performance attempt

---
 .../ql/lib/semmle/javascript/ApiGraphs.qll    |  8 +++++++
 .../javascript/dataflow/Configuration.qll     | 22 +++++++++++++------
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll
index 0f8dd3178d7b..f35e74f829b1 100644
--- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll
+++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll
@@ -865,6 +865,14 @@ module API {
       result = awaited(call, DataFlow::TypeTracker::end())
     }
 
+    cached
+    predicate memberEdge(TApiNode pred, string member, TApiNode succ) {
+      edge(pred, Label::member(member), succ)
+    }
+
+    cached
+    predicate anyMemberEdge(TApiNode pred, TApiNode succ) { memberEdge(pred, _, succ) }
+
     /**
      * Holds if there is an edge from `pred` to `succ` in the API graph that is labeled with `lbl`.
      */
diff --git a/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
index 835c2f7e6269..bae97ea63979 100644
--- a/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
@@ -1160,18 +1160,18 @@ private predicate reachableFromInput(
 }
 
 /**
- * Holds if there is a level step from `pred` to `succ` under `cfg` that can be appended
- * to a path represented by `oldSummary` yielding a path represented by `newSummary`.
+ * Holds if there is a level step from `mid` to `nd` under `cfg` that can be appended
+ * to a path represented by `oldSummary` yielding a path represented by `summary`.
  */
-pragma[noinline]
+pragma[noopt]
 private predicate appendStep(
-  DataFlow::Node pred, DataFlow::Configuration cfg, PathSummary oldSummary, DataFlow::Node succ,
-  PathSummary newSummary
+  DataFlow::Node mid, DataFlow::Configuration cfg, PathSummary oldSummary, DataFlow::Node nd,
+  PathSummary summary
 ) {
   exists(PathSummary stepSummary |
-    flowStep(pred, cfg, succ, stepSummary) and
+    flowStep(mid, cfg, nd, stepSummary) and
     stepSummary.isLevel() and
-    newSummary = oldSummary.append(stepSummary)
+    summary = oldSummary.append(stepSummary)
   )
 }
 
@@ -1317,6 +1317,13 @@ private predicate reachesReturn(
   summary = PathSummary::level() and
   callInputStep(f, _, _, _, _) // check that a relevant result can exist.
   or
+  reachesReturnRec(f, read, cfg, summary)
+}
+
+pragma[noopt]
+private predicate reachesReturnRec(
+  Function f, DataFlow::Node read, DataFlow::Configuration cfg, PathSummary summary
+) {
   exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary |
     flowStep(read, cfg, mid, oldSummary) and
     reachesReturn(f, mid, cfg, newSummary) and
@@ -1607,6 +1614,7 @@ private predicate flowIntoHigherOrderCall(
  * Holds if there is a flow step from `pred` to `succ` described by `summary`
  * under configuration `cfg`.
  */
+pragma[noinline]
 private predicate flowStep(
   DataFlow::Node pred, DataFlow::Configuration cfg, DataFlow::Node succ, PathSummary summary
 ) {

From 7b59adc32945ca1cee5162defe79a8b99606aecf Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Tue, 21 Sep 2021 09:01:35 +0200
Subject: [PATCH 5/8] split localFieldStep into a load/store step for the
 DataFlow step

---
 .../semmle/javascript/dataflow/DataFlow.qll   | 29 ++++++++++++++++---
 .../dataflow/UnsafeHtmlConstructionQuery.qll  |  4 ---
 .../dataflow/UnsafeJQueryPluginQuery.qll      |  2 --
 .../UnsafeShellCommandConstructionQuery.qll   |  4 ---
 .../ql/src/Security/CWE-094/CodeInjection.ql  |  1 +
 5 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
index ecae8f60f117..1dea4b8a6fc9 100644
--- a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
@@ -1586,6 +1586,15 @@ module DataFlow {
    */
   predicate localFieldStep(DataFlow::Node pred, DataFlow::Node succ) {
     exists(ClassNode cls, string prop |
+      localFieldStoreStep(cls, pred, _, prop) and
+      localFieldLoadStep(cls, _, succ, prop)
+    )
+  }
+
+  private predicate localFieldStoreStep(
+    ClassNode cls, DataFlow::Node pred, DataFlow::Node succ, string prop
+  ) {
+    (
       pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyWrite(prop).getRhs()
       or
       // add support for writes on nested properties
@@ -1593,13 +1602,25 @@ module DataFlow {
       pred = any(DataFlow::PropRef ref).getBase()
       or
       pred = cls.getInstanceMethod(prop)
-    |
-      succ = cls.getAReceiverNode().getAPropertyRead(prop)
-    )
+    ) and
+    succ = cls.getConstructor().getReceiver()
+  }
+
+  private predicate localFieldLoadStep(
+    ClassNode cls, DataFlow::Node pred, DataFlow::Node succ, string prop
+  ) {
+    pred = cls.getConstructor().getReceiver() and
+    succ = cls.getAReceiverNode().getAPropertyRead(prop)
   }
 
   private class LocalFieldStep extends DataFlow::SharedFlowStep {
-    override predicate step(DataFlow::Node pred, DataFlow::Node succ) { localFieldStep(pred, succ) }
+    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      localFieldLoadStep(_, pred, succ, prop)
+    }
+
+    override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+      localFieldStoreStep(_, pred, succ, prop)
+    }
   }
 
   predicate argumentPassingStep = FlowSteps::argumentPassing/4;
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll
index 56af9ad7ab30..21dd94079b7b 100644
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionQuery.qll
@@ -35,8 +35,4 @@ class Configration extends TaintTracking::Configuration {
     super.hasFlowPath(source, sink) and
     DataFlow::hasPathWithoutUnmatchedReturn(source, sink)
   }
-
-  override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-    DataFlow::localFieldStep(pred, succ)
-  }
 }
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll
index a84e7474ea39..980c533e8c7b 100644
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginQuery.qll
@@ -26,8 +26,6 @@ class Configuration extends TaintTracking::Configuration {
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node sink) {
-    // jQuery plugins tend to be implemented as classes that store data in fields initialized by the constructor.
-    DataFlow::localFieldStep(src, sink) or
     aliasPropertyPresenceStep(src, sink)
   }
 
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll
index 219731dcbde7..648e9d537f18 100644
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionQuery.qll
@@ -32,8 +32,4 @@ class Configuration extends TaintTracking::Configuration {
     super.hasFlowPath(source, sink) and
     DataFlow::hasPathWithoutUnmatchedReturn(source, sink)
   }
-
-  override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-    DataFlow::localFieldStep(pred, succ)
-  }
 }
diff --git a/javascript/ql/src/Security/CWE-094/CodeInjection.ql b/javascript/ql/src/Security/CWE-094/CodeInjection.ql
index fbf39beaca4e..93295317c9e2 100644
--- a/javascript/ql/src/Security/CWE-094/CodeInjection.ql
+++ b/javascript/ql/src/Security/CWE-094/CodeInjection.ql
@@ -17,6 +17,7 @@
 import javascript
 import semmle.javascript.security.dataflow.CodeInjectionQuery
 import DataFlow::PathGraph
+import semmle.javascript.heuristics.AdditionalSources
 
 from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
 where cfg.hasFlowPath(source, sink)

From 3685255e6260406f1831b36e9872f02d97bc504e Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Wed, 22 Sep 2021 20:45:04 +0200
Subject: [PATCH 6/8] testing performance

---
 javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
index 1dea4b8a6fc9..bf310e05fe0d 100644
--- a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
@@ -1598,7 +1598,7 @@ module DataFlow {
       pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyWrite(prop).getRhs()
       or
       // add support for writes on nested properties
-      pred = cls.getADirectSuperClass*().getAReceiverNode().getAPropertyRead(prop) and
+      pred = cls.getAReceiverNode().getAPropertyRead(prop) and
       pred = any(DataFlow::PropRef ref).getBase()
       or
       pred = cls.getInstanceMethod(prop)

From 111f770635f9370bfd0a52bb85441af504a582aa Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Wed, 22 Sep 2021 21:08:00 +0200
Subject: [PATCH 7/8] remove bad optimization

---
 .../javascript/dataflow/Configuration.qll     | 22 ++++++-------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
index bae97ea63979..835c2f7e6269 100644
--- a/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
@@ -1160,18 +1160,18 @@ private predicate reachableFromInput(
 }
 
 /**
- * Holds if there is a level step from `mid` to `nd` under `cfg` that can be appended
- * to a path represented by `oldSummary` yielding a path represented by `summary`.
+ * Holds if there is a level step from `pred` to `succ` under `cfg` that can be appended
+ * to a path represented by `oldSummary` yielding a path represented by `newSummary`.
  */
-pragma[noopt]
+pragma[noinline]
 private predicate appendStep(
-  DataFlow::Node mid, DataFlow::Configuration cfg, PathSummary oldSummary, DataFlow::Node nd,
-  PathSummary summary
+  DataFlow::Node pred, DataFlow::Configuration cfg, PathSummary oldSummary, DataFlow::Node succ,
+  PathSummary newSummary
 ) {
   exists(PathSummary stepSummary |
-    flowStep(mid, cfg, nd, stepSummary) and
+    flowStep(pred, cfg, succ, stepSummary) and
     stepSummary.isLevel() and
-    summary = oldSummary.append(stepSummary)
+    newSummary = oldSummary.append(stepSummary)
   )
 }
 
@@ -1317,13 +1317,6 @@ private predicate reachesReturn(
   summary = PathSummary::level() and
   callInputStep(f, _, _, _, _) // check that a relevant result can exist.
   or
-  reachesReturnRec(f, read, cfg, summary)
-}
-
-pragma[noopt]
-private predicate reachesReturnRec(
-  Function f, DataFlow::Node read, DataFlow::Configuration cfg, PathSummary summary
-) {
   exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary |
     flowStep(read, cfg, mid, oldSummary) and
     reachesReturn(f, mid, cfg, newSummary) and
@@ -1614,7 +1607,6 @@ private predicate flowIntoHigherOrderCall(
  * Holds if there is a flow step from `pred` to `succ` described by `summary`
  * under configuration `cfg`.
  */
-pragma[noinline]
 private predicate flowStep(
   DataFlow::Node pred, DataFlow::Configuration cfg, DataFlow::Node succ, PathSummary summary
 ) {

From 553fe02f569a4f57639e78b9d866ee363b2a12a9 Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Thu, 23 Sep 2021 10:00:13 +0200
Subject: [PATCH 8/8] next performance attempt

---
 .../semmle/javascript/dataflow/DataFlow.qll   | 33 +++++++++++++++++--
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
index bf310e05fe0d..bfff49245b91 100644
--- a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
@@ -1613,13 +1613,40 @@ module DataFlow {
     succ = cls.getAReceiverNode().getAPropertyRead(prop)
   }
 
+  // split into many smaller steps, because that helps performance A LOT!
   private class LocalFieldStep extends DataFlow::SharedFlowStep {
-    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      localFieldLoadStep(_, pred, succ, prop)
+    /*
+     * override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+     *      exists(ClassNode cls |
+     *        pred = cls.getConstructor().getReceiver() and
+     *        succ = cls.getAReceiverNode().getAPropertyRead(prop)
+     *      )
+     *    }
+     */
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      exists(ClassNode cls |
+        pred = cls.getConstructor().getReceiver() and
+        succ = cls.getAReceiverNode()
+      )
+      or
+      exists(ClassNode cls |
+        pred = cls.getADirectSuperClass().getConstructor().getReceiver() and
+        succ = cls.getConstructor().getReceiver()
+      )
     }
 
     override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
-      localFieldStoreStep(_, pred, succ, prop)
+      exists(ClassNode cls |
+        (
+          pred = cls.getAReceiverNode().getAPropertyWrite(prop).getRhs()
+          or
+          // add support for writes on nested properties
+          pred = cls.getAReceiverNode().getAPropertyRead(prop) and
+          pred = any(DataFlow::PropRef ref).getBase()
+        ) and
+        succ = cls.getConstructor().getReceiver()
+      )
     }
   }