JS: use localFieldStep in all dataflow configurations

javascript/ql/lib/semmle/javascript/ApiGraphs.qll

@@ -683,6 +683,7 @@ module API {
     /**
      * Holds if `ref` is a use of node `nd`.
      */
+    pragma[noinline]
     cached
     predicate use(TApiNode nd, DataFlow::Node ref) {
       exists(string m, Module mod | nd = MkModuleDef(m) and mod = importableModule(m) |
@@ -737,22 +738,35 @@ module API {
       boundArgs = 0 and
       prop = ""
       or
+      result =
+        trackUseNodeHelperStep(trackUseNode(nd, false, boundArgs, prop, t.continue()), promisified,
+          boundArgs, prop)
+      or
+      t = useStep(nd, promisified, boundArgs, prop, result)
+    }
+
+    pragma[nomagic]
+    private DataFlow::SourceNode trackUseNodeHelperStep(
+      DataFlow::SourceNode start, boolean promisified, int boundArgs, string prop
+    ) {
       exists(Promisify::PromisifyCall promisify |
-        trackUseNode(nd, false, boundArgs, prop, t.continue()).flowsTo(promisify.getArgument(0)) and
+        start.flowsTo(promisify.getArgument(0)) and
         promisified = true and
         prop = "" and
-        result = promisify
+        result = promisify and
+        boundArgs = 0
       )
       or
       exists(DataFlow::PartialInvokeNode pin, DataFlow::Node pred, int predBoundArgs |
-        trackUseNode(nd, promisified, predBoundArgs, prop, t.continue()).flowsTo(pred) and
+        start.flowsTo(pred) and
         prop = "" and
         result = pin.getBoundFunction(pred, boundArgs - predBoundArgs) and
-        boundArgs in [0 .. 10]
+        boundArgs in [0 .. 10] and
+        promisified = [true, false]
       )
       or
       exists(DataFlow::Node pred, string preprop |
-        trackUseNode(nd, promisified, boundArgs, preprop, t.continue()).flowsTo(pred) and
+        start.flowsTo(pred) and
         promisified = false and
         boundArgs = 0 and
         SharedTypeTrackingStep::loadStoreStep(pred, result, prop)
@@ -761,8 +775,6 @@ module API {
         or
         preprop = ""
       )
-      or
-      t = useStep(nd, promisified, boundArgs, prop, result)
     }
 
     private import semmle.javascript.dataflow.internal.StepSummary

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

@@ -1586,13 +1586,51 @@ 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.getAReceiverNode().getAPropertyWrite(prop).getRhs()
+      or
+      pred = cls.getInstanceMethod(prop)
+    |
+      succ = cls.getAnInstanceMemberAccess(prop)
+    )
+  }
+
+  private predicate localFieldStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    exists(ClassNode cls |
+      pred = cls.getAReceiverNode().getAPropertyWrite(prop).getRhs()
+      or
       pred = cls.getInstanceMethod(prop)
     |
+      succ = cls.getConstructor().getReceiver()
+    )
+  }
+
+  private predicate localFieldLoadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    exists(ClassNode cls |
+      pred = cls.getConstructor().getReceiver() and
       succ = cls.getAReceiverNode().getAPropertyRead(prop)
     )
   }
 
+  /**
+   * A step that models steps to and from fields of a class.
+   */
+  private class LocalFieldStep extends DataFlow::SharedFlowStep {
+    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      localFieldLoadStep(pred, succ, prop)
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      exists(DataFlow::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)
+    }
+  }
+
   predicate argumentPassingStep = FlowSteps::argumentPassing/4;
 
   /**

javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll

@@ -83,8 +83,6 @@ class Configuration extends TaintTracking::Configuration {
       inlbl.isTaint() and
       outlbl instanceof ObjectPrototype
     )
-    or
-    DataFlow::localFieldStep(pred, succ) and inlbl = outlbl
   }
 
   override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {

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/test/library-tests/ClassNode/tests.expected

@@ -1,7 +1,3 @@
-fieldStep
-| tst2.js:3:14:3:14 | x | tst2.js:7:5:7:10 | this.x |
-| tst2.js:3:14:3:14 | x | tst2.js:8:25:8:30 | this.x |
-| tst2.js:3:14:3:14 | x | tst2.js:12:12:12:17 | this.x |
 getAReceiverNode
 | fields.ts:1:1:3:1 | class B ... mber;\\n} | fields.ts:1:12:1:11 | this |
 | fields.ts:5:1:13:1 | class F ... > {};\\n} | fields.ts:6:5:6:4 | this |

javascript/ql/test/library-tests/ClassNode/tests.ql

@@ -1,9 +1,5 @@
 import javascript
 
-query predicate fieldStep(DataFlow::Node pred, DataFlow::Node succ) {
-  DataFlow::localFieldStep(pred, succ)
-}
-
 query predicate getAReceiverNode(DataFlow::ClassNode cls, DataFlow::SourceNode recv) {
   cls.getAReceiverNode() = recv
 }

javascript/ql/test/library-tests/frameworks/EventEmitter/test.expected

@@ -12,6 +12,7 @@ flowSteps
 | tst.js:20:17:20:21 | "foo" | tst.js:19:16:19:18 | foo |
 | tst.js:21:17:21:21 | "bar" | tst.js:19:39:19:41 | bar |
 | tst.js:28:17:28:22 | "blab" | tst.js:25:16:25:20 | event |
+| tst.js:30:38:30:37 | this | tst.js:36:52:36:51 | this |
 | tst.js:34:18:34:22 | "BOH" | tst.js:33:17:33:17 | x |
 | tst.js:40:20:40:27 | "yabity" | tst.js:39:19:39:19 | x |
 | tst.js:46:28:46:38 | 'FirstData' | tst.js:43:45:43:49 | first |

javascript/ql/test/library-tests/frameworks/Firebase/tests.expected

@@ -19,6 +19,7 @@ firebaseRef
 | tst.js:54:5:54:37 | this.fi ... ef('x') |
 | tst.js:58:1:58:61 | new Fir ... /news') |
 | tst.js:59:1:59:38 | new Fir ... /news') |
+| tst.js:70:1:70:12 | box2.x.ref() |
 firebaseSnapshot
 | tst.js:5:1:8:2 | fb.data ... ent;\\n}) |
 | tst.js:5:38:5:38 | x |

javascript/ql/test/library-tests/frameworks/Firebase/tst.js

@@ -67,6 +67,6 @@ class Box {
 }
 let box1 = new Box(fb.database());
 let box2 = new Box(whatever());
-box2.x.ref(); // not a firebase ref
+box2.x.ref(); // is a firebase ref
 
 functions.https.onRequest((req, res) => { res.send(req.params.foo); });

javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction.expected

@@ -250,6 +250,13 @@ nodes
 | lib/lib.js:498:45:498:48 | name |
 | lib/lib.js:499:31:499:34 | name |
 | lib/lib.js:499:31:499:34 | name |
+| lib/lib.js:504:14:504:18 | taint |
+| lib/lib.js:505:16:505:20 | taint |
+| lib/lib.js:510:14:510:18 | taint |
+| lib/lib.js:510:14:510:18 | taint |
+| lib/lib.js:511:9:511:13 | taint |
+| lib/lib.js:515:27:515:36 | this.taint |
+| lib/lib.js:515:27:515:36 | this.taint |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name |
 | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -566,6 +573,12 @@ edges
 | lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
 | lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
 | lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name |
+| lib/lib.js:504:14:504:18 | taint | lib/lib.js:505:16:505:20 | taint |
+| lib/lib.js:505:16:505:20 | taint | lib/lib.js:515:27:515:36 | this.taint |
+| lib/lib.js:505:16:505:20 | taint | lib/lib.js:515:27:515:36 | this.taint |
+| lib/lib.js:510:14:510:18 | taint | lib/lib.js:511:9:511:13 | taint |
+| lib/lib.js:510:14:510:18 | taint | lib/lib.js:511:9:511:13 | taint |
+| lib/lib.js:511:9:511:13 | taint | lib/lib.js:504:14:504:18 | taint |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
 | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name |
@@ -657,6 +670,7 @@ edges
 | lib/lib.js:478:27:478:46 | config.installedPath | lib/lib.js:477:33:477:38 | config | lib/lib.js:478:27:478:46 | config.installedPath | $@ based on $@ is later used in $@. | lib/lib.js:478:27:478:46 | config.installedPath | Path concatenation | lib/lib.js:477:33:477:38 | config | library input | lib/lib.js:479:12:479:20 | exec(cmd) | shell command |
 | lib/lib.js:483:13:483:33 | ' my na ...  + name | lib/lib.js:482:40:482:43 | name | lib/lib.js:483:30:483:33 | name | $@ based on $@ is later used in $@. | lib/lib.js:483:13:483:33 | ' my na ...  + name | String concatenation | lib/lib.js:482:40:482:43 | name | library input | lib/lib.js:485:2:485:20 | cp.exec(cmd + args) | shell command |
 | lib/lib.js:499:19:499:34 | "rm -rf " + name | lib/lib.js:498:45:498:48 | name | lib/lib.js:499:31:499:34 | name | $@ based on $@ is later used in $@. | lib/lib.js:499:19:499:34 | "rm -rf " + name | String concatenation | lib/lib.js:498:45:498:48 | name | library input | lib/lib.js:499:3:499:35 | MyThing ... + name) | shell command |
+| lib/lib.js:515:15:515:36 | "rm -rf ... s.taint | lib/lib.js:510:14:510:18 | taint | lib/lib.js:515:27:515:36 | this.taint | $@ based on $@ is later used in $@. | lib/lib.js:515:15:515:36 | "rm -rf ... s.taint | String concatenation | lib/lib.js:510:14:510:18 | taint | library input | lib/lib.js:515:3:515:37 | cp.exec ... .taint) | shell command |
 | lib/subLib2/compiled-file.ts:4:13:4:28 | "rm -rf " + name | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | $@ based on $@ is later used in $@. | lib/subLib2/compiled-file.ts:4:13:4:28 | "rm -rf " + name | String concatenation | lib/subLib2/compiled-file.ts:3:26:3:29 | name | library input | lib/subLib2/compiled-file.ts:4:5:4:29 | cp.exec ... + name) | shell command |
 | lib/subLib2/special-file.js:4:10:4:25 | "rm -rf " + name | lib/subLib2/special-file.js:3:28:3:31 | name | lib/subLib2/special-file.js:4:22:4:25 | name | $@ based on $@ is later used in $@. | lib/subLib2/special-file.js:4:10:4:25 | "rm -rf " + name | String concatenation | lib/subLib2/special-file.js:3:28:3:31 | name | library input | lib/subLib2/special-file.js:4:2:4:26 | cp.exec ... + name) | shell command |
 | lib/subLib3/my-file.ts:4:10:4:25 | "rm -rf " + name | lib/subLib3/my-file.ts:3:28:3:31 | name | lib/subLib3/my-file.ts:4:22:4:25 | name | $@ based on $@ is later used in $@. | lib/subLib3/my-file.ts:4:10:4:25 | "rm -rf " + name | String concatenation | lib/subLib3/my-file.ts:3:28:3:31 | name | library input | lib/subLib3/my-file.ts:4:2:4:26 | cp.exec ... + name) | shell command |

javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCat.expected

@@ -55,6 +55,7 @@ syncCommand
 | command-line-parameter-command-injection.js:27:2:27:58 | cp.exec ... tion"`) |
 | lib/lib.js:419:2:419:52 | cp.exec ...  true}) |
 | lib/lib.js:420:2:420:49 | cp.spaw ...  true}) |
+| lib/lib.js:515:3:515:37 | cp.exec ... .taint) |
 | other.js:7:5:7:36 | require ... nc(cmd) |
 | other.js:9:5:9:35 | require ... nc(cmd) |
 | other.js:12:5:12:30 | require ... nc(cmd) |

javascript/ql/test/query-tests/Security/CWE-078/lib/lib.js

@@ -499,4 +499,19 @@ module.exports.myCommand = function (myCommand) {
 		MyThing.cp.exec("rm -rf " + name); // NOT OK
 	}
 });
-
+
+class Base {
+	constructor(taint) {
+		this.taint = taint;
+	}
+}
+
+module.exports.Sub = class Sub extends Base {
+	constructor(taint) {
+		super(taint);
+	}
+
+	doBadStuff() {
+		cp.execSync("rm -rf " + this.taint); // NOT OK
+	}
+}