Merge pull request #3641 from asger-semmle/js/pre-call-graph-steps

Approved by erik-krogh

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

@@ -1,5 +1,6 @@
 import javascript
 private import semmle.javascript.dataflow.InferredTypes
+private import semmle.javascript.dataflow.internal.PreCallGraphStep
 
 /**
  * Classes and predicates for modelling TaintTracking steps for arrays.
@@ -222,29 +223,32 @@ private module ArrayDataFlow {
    *
    * And the second parameter in the callback is the array ifself, so there is a `loadStoreStep` from the array to that second parameter.
    */
-  private class ArrayIteration extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
-    ArrayIteration() {
-      this.getMethodName() = "map" or
-      this.getMethodName() = "forEach"
-    }
-
+  private class ArrayIteration extends PreCallGraphStep {
     override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
-      prop = arrayElement() and
-      obj = this.getReceiver() and
-      element = getCallback(0).getParameter(0)
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = ["map", "forEach"] and
+        prop = arrayElement() and
+        obj = call.getReceiver() and
+        element = call.getCallback(0).getParameter(0)
+      )
     }
 
     override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
-      this.getMethodName() = "map" and
-      prop = arrayElement() and
-      element = this.getCallback(0).getAReturn() and
-      obj = this
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = "map" and
+        prop = arrayElement() and
+        element = call.getCallback(0).getAReturn() and
+        obj = call
+      )
     }
 
-    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = arrayElement() and
-      pred = this.getReceiver() and
-      succ = getCallback(0).getParameter(2)
+    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+      exists(DataFlow::MethodCallNode call |
+        call.getMethodName() = ["map", "forEach"] and
+        prop = arrayElement() and
+        pred = call.getReceiver() and
+        succ = call.getCallback(0).getParameter(2)
+      )
     }
   }
 
@@ -311,16 +315,13 @@ private module ArrayDataFlow {
   /**
    * A step for modelling `for of` iteration on arrays.
    */
-  private class ForOfStep extends DataFlow::AdditionalFlowStep, DataFlow::ValueNode {
-    ForOfStmt forOf;
-    DataFlow::Node element;
-
-    ForOfStep() { this.asExpr() = forOf.getIterationDomain() }
-
+  private class ForOfStep extends PreCallGraphStep {
     override predicate loadStep(DataFlow::Node obj, DataFlow::Node e, string prop) {
-      obj = this and
-      e = DataFlow::lvalueNode(forOf.getLValue()) and
-      prop = arrayElement()
+      exists(ForOfStmt forOf |
+        obj = forOf.getIterationDomain().flow() and
+        e = DataFlow::lvalueNode(forOf.getLValue()) and
+        prop = arrayElement()
+      )
     }
   }
 }

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

@@ -23,6 +23,7 @@ private import internal.CallGraphs
 private import internal.FlowSteps as FlowSteps
 private import internal.DataFlowNode
 private import internal.AnalyzedParameters
+private import internal.PreCallGraphStep
 
 module DataFlow {
   /**

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

@@ -335,5 +335,20 @@ abstract class AdditionalTypeTrackingStep extends DataFlow::Node {
   /**
    * Holds if type-tracking should step from `pred` to `succ`.
    */
-  abstract predicate step(DataFlow::Node pred, DataFlow::Node succ);
+  predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }
+
+  /**
+   * Holds if type-tracking should step from `pred` into the `prop` property of `succ`.
+   */
+  predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
+
+  /**
+   * Holds if type-tracking should step from the `prop` property of `pred` to `succ`.
+   */
+  predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  /**
+   * Holds if type-tracking should step from the `prop` property of `pred` to the same property in `succ`.
+   */
+  predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
 }

javascript/ql/src/semmle/javascript/dataflow/internal/PreCallGraphStep.qll

@@ -0,0 +1,134 @@
+/**
+ * Provides an extension point for contributing flow edges prior
+ * to call graph construction and type tracking.
+ */
+
+private import javascript
+
+private newtype TUnit = MkUnit()
+
+private class Unit extends TUnit {
+  string toString() { result = "unit" }
+}
+
+/**
+ * Internal extension point for adding flow edges prior to call graph construction
+ * and type tracking.
+ *
+ * Steps added here will be added to both `AdditionalFlowStep` and `AdditionalTypeTrackingStep`.
+ *
+ * Contributing steps that rely on type tracking will lead to negative recursion.
+ */
+class PreCallGraphStep extends Unit {
+  /**
+   * Holds if there is a step from `pred` to `succ`.
+   */
+  predicate step(DataFlow::Node pred, DataFlow::Node succ) { none() }
+
+  /**
+   * Holds if there is a step from `pred` into the `prop` property of `succ`.
+   */
+  predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
+
+  /**
+   * Holds if there is a step from the `prop` property of `pred` to `succ`.
+   */
+  predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  /**
+   * Holds if there is a step from the `prop` property of `pred` to the same property in `succ`.
+   */
+  predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
+}
+
+module PreCallGraphStep {
+  /**
+   * Holds if there is a step from `pred` to `succ`.
+   */
+  cached
+  predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    any(PreCallGraphStep s).step(pred, succ)
+  }
+
+  /**
+   * Holds if there is a step from `pred` into the `prop` property of `succ`.
+   */
+  cached
+  predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    any(PreCallGraphStep s).storeStep(pred, succ, prop)
+  }
+
+  /**
+   * Holds if there is a step from the `prop` property of `pred` to `succ`.
+   */
+  cached
+  predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    any(PreCallGraphStep s).loadStep(pred, succ, prop)
+  }
+
+  /**
+   * Holds if there is a step from the `prop` property of `pred` to the same property in `succ`.
+   */
+  cached
+  predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    any(PreCallGraphStep s).loadStoreStep(pred, succ, prop)
+  }
+}
+
+private class NodeWithPreCallGraphStep extends DataFlow::Node {
+  NodeWithPreCallGraphStep() {
+    PreCallGraphStep::step(this, _)
+    or
+    PreCallGraphStep::storeStep(this, _, _)
+    or
+    PreCallGraphStep::loadStep(this, _, _)
+    or
+    PreCallGraphStep::loadStoreStep(this, _, _)
+  }
+}
+
+private class AdditionalFlowStepFromPreCallGraph extends NodeWithPreCallGraphStep,
+  DataFlow::AdditionalFlowStep {
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    pred = this and
+    PreCallGraphStep::step(this, succ)
+  }
+
+  override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    pred = this and
+    PreCallGraphStep::storeStep(this, succ, prop)
+  }
+
+  override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    pred = this and
+    PreCallGraphStep::loadStep(this, succ, prop)
+  }
+
+  override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    pred = this and
+    PreCallGraphStep::loadStoreStep(this, succ, prop)
+  }
+}
+
+private class AdditionalTypeTrackingStepFromPreCallGraph extends NodeWithPreCallGraphStep,
+  DataFlow::AdditionalTypeTrackingStep {
+  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+    pred = this and
+    PreCallGraphStep::step(this, succ)
+  }
+
+  override predicate storeStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    pred = this and
+    PreCallGraphStep::storeStep(this, succ, prop)
+  }
+
+  override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+    pred = this and
+    PreCallGraphStep::loadStep(this, succ, prop)
+  }
+
+  override predicate loadStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    pred = this and
+    PreCallGraphStep::loadStoreStep(this, succ, prop)
+  }
+}

javascript/ql/src/semmle/javascript/dataflow/internal/StepSummary.qll

@@ -110,6 +110,15 @@ module StepSummary {
       or
       basicLoadStep(pred, succ, prop) and
       summary = LoadStep(prop)
+      or
+      any(AdditionalTypeTrackingStep st).storeStep(pred, succ, prop) and
+      summary = StoreStep(prop)
+      or
+      any(AdditionalTypeTrackingStep st).loadStep(pred, succ, prop) and
+      summary = LoadStep(prop)
+      or
+      any(AdditionalTypeTrackingStep st).loadStoreStep(pred, succ, prop) and
+      summary = CopyStep(prop)
     )
     or
     any(AdditionalTypeTrackingStep st).step(pred, succ) and

javascript/ql/test/library-tests/frameworks/SQL/SqlString.expected

@@ -41,4 +41,5 @@
 | spanner.js:19:23:19:32 | "SQL code" |
 | spannerImport.js:4:8:4:17 | "SQL code" |
 | sqlite.js:7:8:7:45 | "UPDATE ... id = ?" |
+| sqliteArray.js:6:12:6:49 | "UPDATE ... id = ?" |
 | sqliteImport.js:2:8:2:44 | "UPDATE ... id = ?" |

javascript/ql/test/library-tests/frameworks/SQL/sqliteArray.js

@@ -0,0 +1,7 @@
+var sqlite = require('sqlite3');
+
+let databaseNames = [":memory:", ":foo:"];
+let databases = databaseNames.map(name => new sqlite.Database(name));
+for (let db of databases) {
+    db.run("UPDATE tbl SET name = ? WHERE id = ?", "bar", 2);
+}