Merge pull request github#5478 from asgerf/js/shared-flow-step

Approved by erik-krogh

Author: codeql-ci

javascript/ql/src/Security/CWE-327/BadRandomness.ql

@@ -87,11 +87,6 @@ private DataFlow::Node goodRandom(DataFlow::TypeTracker t, DataFlow::SourceNode
   or
   exists(DataFlow::TypeTracker t2 | t = t2.smallstep(goodRandom(t2, source), result))
   or
-  // re-using the collection steps for `Set`.
-  exists(DataFlow::TypeTracker t2 |
-    result = CollectionsTypeTracking::collectionStep(goodRandom(t2, source), t, t2)
-  )
-  or
   InsecureRandomness::isAdditionalTaintStep(goodRandom(t.continue(), source), result) and
   // bit shifts and multiplication by powers of two are generally used for constructing larger numbers from smaller numbers.
   not exists(BinaryExpr binop | binop = result.asExpr() |

javascript/ql/src/meta/analysis-quality/TaintSteps.ql

@@ -15,17 +15,17 @@ predicate relevantStep(DataFlow::Node pred, DataFlow::Node succ) {
   (
     TaintTracking::sharedTaintStep(pred, succ)
     or
-    any(DataFlow::AdditionalFlowStep cfg).step(pred, succ)
+    DataFlow::SharedFlowStep::step(pred, succ)
     or
-    any(DataFlow::AdditionalFlowStep cfg).step(pred, succ, _, _)
+    DataFlow::SharedFlowStep::step(pred, succ, _, _)
     or
-    any(DataFlow::AdditionalFlowStep cfg).loadStep(pred, succ, _)
+    DataFlow::SharedFlowStep::loadStep(pred, succ, _)
     or
-    any(DataFlow::AdditionalFlowStep cfg).storeStep(pred, succ, _)
+    DataFlow::SharedFlowStep::storeStep(pred, succ, _)
     or
-    any(DataFlow::AdditionalFlowStep cfg).loadStoreStep(pred, succ, _, _)
+    DataFlow::SharedFlowStep::loadStoreStep(pred, succ, _, _)
     or
-    any(DataFlow::AdditionalFlowStep cfg).loadStoreStep(pred, succ, _)
+    DataFlow::SharedFlowStep::loadStoreStep(pred, succ, _)
   ) and
   not pred.getFile() instanceof IgnoredFile and
   not succ.getFile() instanceof IgnoredFile

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

@@ -84,16 +84,17 @@ private module ArrayDataFlow {
    * A step modelling the creation of an Array using the `Array.from(x)` method.
    * The step copies the elements of the argument (set, array, or iterator elements) into the resulting array.
    */
-  private class ArrayFrom extends DataFlow::AdditionalFlowStep, DataFlow::CallNode {
-    ArrayFrom() { this = DataFlow::globalVarRef("Array").getAMemberCall("from") }
-
+  private class ArrayFrom extends DataFlow::SharedFlowStep {
     override predicate loadStoreStep(
       DataFlow::Node pred, DataFlow::Node succ, string fromProp, string toProp
     ) {
-      pred = this.getArgument(0) and
-      succ = this and
-      fromProp = arrayLikeElement() and
-      toProp = arrayElement()
+      exists(DataFlow::CallNode call |
+        call = DataFlow::globalVarRef("Array").getAMemberCall("from") and
+        pred = call.getArgument(0) and
+        succ = call and
+        fromProp = arrayLikeElement() and
+        toProp = arrayElement()
+      )
     }
   }
 
@@ -103,55 +104,45 @@ private module ArrayDataFlow {
    *
    * Such a step can occur both with the `push` and `unshift` methods, or when creating a new array.
    */
-  private class ArrayCopySpread extends DataFlow::AdditionalFlowStep {
-    DataFlow::Node spreadArgument; // the spread argument containing the elements to be copied.
-    DataFlow::Node base; // the object where the elements should be copied to.
-
-    ArrayCopySpread() {
-      exists(DataFlow::MethodCallNode mcn | mcn = this |
-        mcn.getMethodName() = ["push", "unshift"] and
-        spreadArgument = mcn.getASpreadArgument() and
-        base = mcn.getReceiver().getALocalSource()
-      )
-      or
-      spreadArgument = this.(DataFlow::ArrayCreationNode).getASpreadArgument() and
-      base = this
-    }
-
+  private class ArrayCopySpread extends DataFlow::SharedFlowStep {
     override predicate loadStoreStep(
       DataFlow::Node pred, DataFlow::Node succ, string fromProp, string toProp
     ) {
-      pred = spreadArgument and
-      succ = base and
       fromProp = arrayLikeElement() and
-      toProp = arrayElement()
+      toProp = arrayElement() and
+      (
+        exists(DataFlow::MethodCallNode mcn |
+          mcn.getMethodName() = ["push", "unshift"] and
+          pred = mcn.getASpreadArgument() and
+          succ = mcn.getReceiver().getALocalSource()
+        )
+        or
+        pred = succ.(DataFlow::ArrayCreationNode).getASpreadArgument()
+      )
     }
   }
 
   /**
    * A step for storing an element on an array using `arr.push(e)` or `arr.unshift(e)`.
    */
-  private class ArrayAppendStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
-    ArrayAppendStep() {
-      this.getMethodName() = "push" or
-      this.getMethodName() = "unshift"
-    }
-
+  private class ArrayAppendStep extends DataFlow::SharedFlowStep {
     override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
       prop = arrayElement() and
-      element = this.getAnArgument() and
-      obj.getAMethodCall() = this
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = ["push", "unshift"] and
+        element = call.getAnArgument() and
+        obj.getAMethodCall() = call
+      )
     }
   }
 
   /**
-   * A step for reading/writing an element from an array inside a for-loop.
-   * E.g. a read from `foo[i]` to `bar` in `for(var i = 0; i < arr.length; i++) {bar = foo[i]}`.
+   * A node that reads or writes an element from an array inside a for-loop.
    */
-  private class ArrayIndexingStep extends DataFlow::AdditionalFlowStep, DataFlow::Node {
+  private class ArrayIndexingAccess extends DataFlow::Node {
     DataFlow::PropRef read;
 
-    ArrayIndexingStep() {
+    ArrayIndexingAccess() {
       read = this and
       TTNumber() =
         unique(InferredType type | type = read.getPropertyNameExpr().flow().analyze().getAType()) and
@@ -162,34 +153,42 @@ private module ArrayDataFlow {
         i.getVariable().getADefinition().(VariableDeclarator).getDeclStmt() = init
       )
     }
+  }
 
+  /**
+   * A step for reading/writing an element from an array inside a for-loop.
+   * E.g. a read from `foo[i]` to `bar` in `for(var i = 0; i < arr.length; i++) {bar = foo[i]}`.
+   */
+  private class ArrayIndexingStep extends DataFlow::SharedFlowStep {
     override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
-      prop = arrayElement() and
-      obj = this.(DataFlow::PropRead).getBase() and
-      element = this
+      exists(ArrayIndexingAccess access |
+        prop = arrayElement() and
+        obj = access.(DataFlow::PropRead).getBase() and
+        element = access
+      )
     }
 
     override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
-      prop = arrayElement() and
-      element = this.(DataFlow::PropWrite).getRhs() and
-      this = obj.getAPropertyWrite()
+      exists(ArrayIndexingAccess access |
+        prop = arrayElement() and
+        element = access.(DataFlow::PropWrite).getRhs() and
+        access = obj.getAPropertyWrite()
+      )
     }
   }
 
   /**
    * A step for retrieving an element from an array using `.pop()` or `.shift()`.
    * E.g. `array.pop()`.
    */
-  private class ArrayPopStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
-    ArrayPopStep() {
-      getMethodName() = "pop" or
-      getMethodName() = "shift"
-    }
-
+  private class ArrayPopStep extends DataFlow::SharedFlowStep {
     override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
-      prop = arrayElement() and
-      obj = this.getReceiver() and
-      element = this
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = ["pop", "shift"] and
+        prop = arrayElement() and
+        obj = call.getReceiver() and
+        element = call
+      )
     }
   }
 
@@ -235,12 +234,12 @@ private module ArrayDataFlow {
   /**
    * A step for creating an array and storing the elements in the array.
    */
-  private class ArrayCreationStep extends DataFlow::AdditionalFlowStep, DataFlow::ArrayCreationNode {
+  private class ArrayCreationStep extends DataFlow::SharedFlowStep {
     override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
-      exists(int i |
-        element = this.getElement(i) and
-        obj = this and
-        if this = any(PromiseAllCreation c).getArrayNode()
+      exists(DataFlow::ArrayCreationNode array, int i |
+        element = array.getElement(i) and
+        obj = array and
+        if array = any(PromiseAllCreation c).getArrayNode()
         then prop = arrayElement(i)
         else prop = arrayElement()
       )
@@ -251,56 +250,42 @@ private module ArrayDataFlow {
    * A step modelling that `splice` can insert elements into an array.
    * For example in `array.splice(i, del, e)`: if `e` is tainted, then so is `array
    */
-  private class ArraySpliceStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
-    ArraySpliceStep() { this.getMethodName() = "splice" }
-
+  private class ArraySpliceStep extends DataFlow::SharedFlowStep {
     override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
-      prop = arrayElement() and
-      element = getArgument(2) and
-      this = obj.getAMethodCall()
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = "splice" and
+        prop = arrayElement() and
+        element = call.getArgument(2) and
+        call = obj.getAMethodCall()
+      )
     }
   }
 
   /**
    * A step for modelling `concat`.
    * For example in `e = arr1.concat(arr2, arr3)`: if any of the `arr` is tainted, then so is `e`.
    */
-  private class ArrayConcatStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
-    ArrayConcatStep() { this.getMethodName() = "concat" }
-
+  private class ArrayConcatStep extends DataFlow::SharedFlowStep {
     override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = arrayElement() and
-      (pred = this.getReceiver() or pred = this.getAnArgument()) and
-      succ = this
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = "concat" and
+        prop = arrayElement() and
+        (pred = call.getReceiver() or pred = call.getAnArgument()) and
+        succ = call
+      )
     }
   }
 
   /**
    * A step for modelling that elements from an array `arr` also appear in the result from calling `slice`/`splice`/`filter`.
    */
-  private class ArraySliceStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
-    ArraySliceStep() {
-      this.getMethodName() = "slice" or
-      this.getMethodName() = "splice" or
-      this.getMethodName() = "filter"
-    }
-
+  private class ArraySliceStep extends DataFlow::SharedFlowStep {
     override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = arrayElement() and
-      pred = this.getReceiver() and
-      succ = this
-    }
-  }
-
-  /**
-   * A step for modelling `for of` iteration on arrays.
-   */
-  private class ForOfStep extends PreCallGraphStep {
-    override predicate loadStep(DataFlow::Node obj, DataFlow::Node e, string prop) {
-      exists(ForOfStmt forOf |
-        obj = forOf.getIterationDomain().flow() and
-        e = DataFlow::lvalueNode(forOf.getLValue()) and
-        prop = arrayElement()
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = ["slice", "splice", "filter"] and
+        prop = arrayElement() and
+        pred = call.getReceiver() and
+        succ = call
       )
     }
   }