Merge pull request #8253 from erik-krogh/domWrite

JS: merge hasDominatingWrite and hasDominatingAssignment

Author: erik-krogh

javascript/ql/lib/semmle/javascript/DynamicPropertyAccess.qll

@@ -181,16 +181,7 @@ class DynamicPropRead extends DataFlow::SourceNode, DataFlow::ValueNode {
    * dst[x][y] = src[y];
    * ```
    */
-  predicate hasDominatingAssignment() {
-    exists(DataFlow::PropWrite write, BasicBlock bb, int i, int j, SsaVariable ssaVar |
-      write = getBase().getALocalSource().getAPropertyWrite() and
-      bb.getNode(i) = write.getWriteNode() and
-      bb.getNode(j) = astNode and
-      i < j and
-      write.getPropertyNameExpr() = ssaVar.getAUse() and
-      astNode.getIndex() = ssaVar.getAUse()
-    )
-  }
+  predicate hasDominatingAssignment() { AccessPath::DominatingPaths::hasDominatingWrite(this) }
 }
 
 /**

javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll

@@ -532,7 +532,7 @@ module AccessPath {
      */
     cached
     predicate hasDominatingWrite(DataFlow::PropRead read) {
-      Stages::FlowSteps::ref() and
+      Stages::TypeTracking::ref() and
       // within the same basic block.
       exists(ReachableBasicBlock bb, Root root, string path, int ranking |
         read.asExpr() = rankedAccessPath(bb, root, path, ranking, AccessPathRead()) and
@@ -544,6 +544,21 @@ module AccessPath {
         read.asExpr() = getAccessTo(root, path, AccessPathRead()) and
         getAWriteBlock(root, path).strictlyDominates(read.getBasicBlock())
       )
+      or
+      // Dynamic write where the same variable is used to index the read and write (in the same basic block)
+      // For example, this is true for `dst[x]` on line 2 below:
+      // ```js
+      // dst[x] = {};
+      // dst[x][y] = src[y];
+      // ```
+      exists(DataFlow::PropWrite write, BasicBlock bb, int i, int j, SsaVariable ssaVar |
+        write = read.getBase().getALocalSource().getAPropertyWrite() and
+        bb.getNode(i) = write.getWriteNode() and
+        bb.getNode(j) = read.asExpr() and
+        i < j and
+        write.getPropertyNameExpr() = ssaVar.getAUse() and
+        read.getPropertyNameExpr() = ssaVar.getAUse()
+      )
     }
   }
 }

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

@@ -534,7 +534,10 @@ module TaintTracking {
       or
       // reading from a tainted object yields a tainted result
       succ.(DataFlow::PropRead).getBase() = pred and
-      not AccessPath::DominatingPaths::hasDominatingWrite(succ) and
+      not (
+        AccessPath::DominatingPaths::hasDominatingWrite(succ) and
+        exists(succ.(DataFlow::PropRead).getPropertyName())
+      ) and
       not isSafeClientSideUrlProperty(succ) and
       not ClassValidator::isAccessToSanitizedField(succ)
       or

javascript/ql/lib/semmle/javascript/internal/CachedStages.qll

@@ -212,6 +212,8 @@ module Stages {
       any(DataFlow::Node node).hasUnderlyingType(_)
       or
       any(DataFlow::Node node).hasUnderlyingType(_, _)
+      or
+      AccessPath::DominatingPaths::hasDominatingWrite(_)
     }
   }
 
@@ -235,8 +237,6 @@ module Stages {
     predicate backref() {
       1 = 1
       or
-      AccessPath::DominatingPaths::hasDominatingWrite(_)
-      or
       DataFlow::SharedFlowStep::step(_, _)
     }
   }