more precise charpreds in taint steps

javascript/ql/src/semmle/javascript/Arrays.qll

@@ -8,13 +8,11 @@ module ArrayTaintTracking {
   /**
    * A taint propagating data flow edge caused by the builtin array functions.
    */
-  private class ArrayFunctionTaintStep extends TaintTracking::AdditionalTaintStep {
+  private class ArrayFunctionTaintStep extends TaintTracking::AdditionalTaintStep, DataFlow::CallNode {
-    DataFlow::CallNode call;
+    ArrayFunctionTaintStep() { arrayFunctionTaintStep(_, _, this) }
-
-    ArrayFunctionTaintStep() { this = call }
 
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      arrayFunctionTaintStep(pred, succ, call)
+      arrayFunctionTaintStep(pred, succ, this)
     }
   }
 

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -230,14 +230,19 @@ module TaintTracking {
    */
   private class HeapTaintStep extends AdditionalTaintStep {
     HeapTaintStep() {
-      this = DataFlow::valueNode(_) or
+      heapStep(_, this)
-      this = DataFlow::parameterNode(_) or
-      this instanceof DataFlow::PropRead
     }
 
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      succ = this and
+      heapStep(pred, succ) and succ = this
-      exists(Expr e, Expr f | e = this.asExpr() and f = pred.asExpr() |
+    }
+  }
+
+  /** 
+   * Holds if there is taint propagation through the heap from `pred` to `succ`. 
+   */ 
+  private predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(Expr e, Expr f | e = succ.asExpr() and f = pred.asExpr() |
       // arrays with tainted elements and objects with tainted property names are tainted
       e.(ArrayExpr).getAnElement() = f
       or
@@ -256,17 +261,14 @@ module TaintTracking {
     )
     or
     // reading from a tainted object yields a tainted result
-      this = succ and
     succ.(DataFlow::PropRead).getBase() = pred
     or
     // iterating over a tainted iterator taints the loop variable
     exists(ForOfStmt fos |
-        this = DataFlow::valueNode(fos.getIterationDomain()) and
+      pred = DataFlow::valueNode(fos.getIterationDomain()) and
-        pred = this and
       succ = DataFlow::lvalueNode(fos.getLValue())
     )
   }
-  }
 
   /**
    * A taint propagating data flow edge through persistent storage.
@@ -388,12 +390,21 @@ module TaintTracking {
    * functions defined in the standard library.
    */
   private class StringManipulationTaintStep extends AdditionalTaintStep, DataFlow::ValueNode {
+    StringManipulationTaintStep() { stringManipulationStep(_, this) }
+
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
       succ = this and
-      (
+      stringManipulationStep(pred, succ)
+    }
+  }
+
+  /** 
+   * Holds if taint can propagate from `pred` to `succ` with a step related to string manipulation. 
+   */ 
+  private predicate stringManipulationStep(DataFlow::Node pred, DataFlow::ValueNode succ) {
     // string operations that propagate taint
-        exists(string name | name = astNode.(MethodCallExpr).getMethodName() |
+    exists(string name | name = succ.getAstNode().(MethodCallExpr).getMethodName() |
-          pred.asExpr() = astNode.(MethodCallExpr).getReceiver() and
+      pred.asExpr() = succ.getAstNode().(MethodCallExpr).getReceiver() and
       (
         // sorted, interesting, properties of String.prototype
         name = "anchor" or
@@ -432,7 +443,7 @@ module TaintTracking {
         name = "join"
       )
       or
-          exists(int i | pred.asExpr() = astNode.(MethodCallExpr).getArgument(i) |
+      exists(int i | pred.asExpr() = succ.getAstNode().(MethodCallExpr).getArgument(i) |
         name = "concat"
         or
         name = "replace" and i = 1
@@ -441,21 +452,21 @@ module TaintTracking {
     or
     // standard library constructors that propagate taint: `RegExp` and `String`
     exists(DataFlow::InvokeNode invk, string gv | gv = "RegExp" or gv = "String" |
-          this = invk and
+      succ = invk and
       invk = DataFlow::globalVarRef(gv).getAnInvocation() and
       pred = invk.getArgument(0)
     )
     or
     // String.fromCharCode and String.fromCodePoint
     exists(int i, MethodCallExpr mce |
-          mce = astNode and
+      mce = succ.getAstNode() and
       pred.asExpr() = mce.getArgument(i) and
       (mce.getMethodName() = "fromCharCode" or mce.getMethodName() = "fromCodePoint")
     )
     or
     // `(encode|decode)URI(Component)?` propagate taint
     exists(DataFlow::CallNode c, string name |
-          this = c and
+      succ = c and
       c = DataFlow::globalVarRef(name).getACall() and
       pred = c.getArgument(0)
     |
@@ -464,8 +475,6 @@ module TaintTracking {
       name = "encodeURIComponent" or
       name = "decodeURIComponent"
     )
-      )
-    }
   }
 
   /**