JS: Add flow through next() calls in RxJS

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`.
      */

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

@@ -1586,13 +1586,70 @@ 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.getInstanceMethod(prop)
-    |
-      succ = cls.getAReceiverNode().getAPropertyRead(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
+      pred = cls.getAReceiverNode().getAPropertyRead(prop) and
+      pred = any(DataFlow::PropRef ref).getBase()
+      or
+      pred = cls.getInstanceMethod(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)
+  }
+
+  // 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) {
+     *      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) {
+      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()
+      )
+    }
+  }
+
   predicate argumentPassingStep = FlowSteps::argumentPassing/4;
 
   /**

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()
+    )
+  }
+}

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)
-  }
 }

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)
   }
 

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)
-  }
 }

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)

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 |

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
 	}
 }
 

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 |

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 |

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);
+});