Merge pull request github#3613 from erik-krogh/Reassigned

Approved by asgerf

Author: semmle-qlci

javascript/ql/src/semmle/javascript/GlobalAccessPaths.qll

@@ -419,4 +419,135 @@ module AccessPath {
     or
     result = node.getALocalSource()
   }
+
+  /**
+   * A module for reasoning dominating reads and writes to access-paths.
+   */
+  module DominatingPaths {
+    /**
+     * A classification of acccess paths into reads and writes.
+     */
+    cached
+    private newtype AccessPathKind =
+      AccessPathRead() or
+      AccessPathWrite()
+
+    /**
+     * Gets the `ranking`th access-path with `root` and `path` within `bb`.
+     * And the access-path has type `type`.
+     *
+     * Only has a result if there exists both a read and write of the access-path within `bb`.
+     */
+    private ControlFlowNode rankedAccessPath(
+      ReachableBasicBlock bb, Root root, string path, int ranking, AccessPathKind type
+    ) {
+      result =
+        rank[ranking](ControlFlowNode ref |
+          ref = getAccessTo(root, path, _) and
+          ref.getBasicBlock() = bb and
+          // Prunes the accesses where there does not exists a read and write within the same basicblock.
+          // This could be more precise, but doing it like this avoids massive joins.
+          hasRead(bb) and
+          hasWrite(bb)
+        |
+          ref order by any(int i | ref = bb.getNode(i))
+        ) and
+      result = getAccessTo(root, path, type)
+    }
+
+    /**
+     * Holds if there exists an access-path read inside the basic-block `bb`.
+     *
+     * INTERNAL: This predicate is only meant to be used inside `rankedAccessPath`.
+     */
+    pragma[noinline]
+    private predicate hasRead(ReachableBasicBlock bb) {
+      bb = getAccessTo(_, _, AccessPathRead()).getBasicBlock()
+    }
+
+    /**
+     * Holds if there exists an access-path write inside the basic-block `bb`.
+     *
+     * INTERNAL: This predicate is only meant to be used inside `rankedAccessPath`.
+     */
+    pragma[noinline]
+    private predicate hasWrite(ReachableBasicBlock bb) {
+      bb = getAccessTo(_, _, AccessPathRead()).getBasicBlock()
+    }
+
+    /**
+     * Gets a `ControlFlowNode` for an access to `path` from `root` with type `type`.
+     *
+     * This predicate uses both the AccessPath API, and the SourceNode API.
+     * This ensures that we have basic support for access-paths with ambiguous roots.
+     *
+     * There is only a result if both a read and a write of the access-path exists.
+     */
+    pragma[nomagic]
+    private ControlFlowNode getAccessTo(Root root, string path, AccessPathKind type) {
+      exists(getAReadNode(root, path)) and
+      exists(getAWriteNode(root, path)) and
+      (
+        type = AccessPathRead() and
+        result = getAReadNode(root, path)
+        or
+        type = AccessPathWrite() and
+        result = getAWriteNode(root, path)
+      )
+    }
+
+    /**
+     * Gets a `ControlFlowNode` for a read to `path` from `root`.
+     *
+     * Only used within `getAccessTo`.
+     */
+    private ControlFlowNode getAReadNode(Root root, string path) {
+      exists(DataFlow::PropRead read | read.asExpr() = result |
+        path = fromReference(read, root) or
+        read = root.getAPropertyRead(path)
+      )
+    }
+
+    /**
+     * Gets a `ControlFlowNode` for a write to `path` from `root`.
+     *
+     * Only used within `getAccessTo`.
+     */
+    private ControlFlowNode getAWriteNode(Root root, string path) {
+      result = root.getAPropertyWrite(path).getWriteNode()
+      or
+      exists(DataFlow::PropWrite write | path = fromRhs(write.getRhs(), root) |
+        result = write.getWriteNode()
+      )
+    }
+
+    /**
+     * Gets a basic-block where the access path defined by `root` and `path` is written to.
+     * And a read to the same access path exists.
+     */
+    private ReachableBasicBlock getAWriteBlock(Root root, string path) {
+      result = getAccessTo(root, path, AccessPathWrite()).getBasicBlock() and
+      exists(getAccessTo(root, path, AccessPathRead())) // helps performance
+    }
+
+    /**
+     * EXPERIMENTAL. This API may change in the future.
+     *
+     * Holds for `read` if there exists a previous write to the same access-path that dominates this read.
+     */
+    cached
+    predicate hasDominatingWrite(DataFlow::PropRead read) {
+      // within the same basic block.
+      exists(ReachableBasicBlock bb, Root root, string path, int ranking |
+        read.asExpr() = rankedAccessPath(bb, root, path, ranking, AccessPathRead()) and
+        exists(rankedAccessPath(bb, root, path, any(int prev | prev < ranking), AccessPathWrite()))
+      )
+      or
+      // across basic blocks.
+      exists(Root root, string path |
+        read.asExpr() = getAccessTo(root, path, AccessPathRead()) and
+        getAWriteBlock(root, path).strictlyDominates(read.getBasicBlock())
+      )
+    }
+  }
 }

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

@@ -260,7 +260,8 @@ module TaintTracking {
     not any(PromiseAllCreation call).getArrayNode() = succ
     or
     // reading from a tainted object yields a tainted result
-    succ.(DataFlow::PropRead).getBase() = pred
+    succ.(DataFlow::PropRead).getBase() = pred and
+    not AccessPath::DominatingPaths::hasDominatingWrite(succ)
     or
     // iterating over a tainted iterator taints the loop variable
     exists(ForOfStmt fos |

javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

@@ -649,7 +649,8 @@ module TaintedPath {
     exists(DataFlow::PropRead read | read = dst |
       src = read.getBase() and
       read.getPropertyName() != "length" and
-      srclabel = dstlabel
+      srclabel = dstlabel and
+      not AccessPath::DominatingPaths::hasDominatingWrite(read)
     )
     or
     // string method calls of interest

javascript/ql/test/library-tests/GlobalAccessPaths/GlobalAccessPaths.expected

@@ -61,6 +61,10 @@ test_getAReferenceTo
 | test.js:39:14:39:16 | foo | foo |
 | test.js:39:14:39:20 | foo.bar | foo.bar |
 | test.js:40:3:40:10 | lazyInit | foo.bar |
+| test.js:44:13:44:18 | Object | Object |
+| test.js:44:13:44:25 | Object.create | Object.create |
+| test.js:50:7:50:12 | random | random |
+| test.js:56:7:56:12 | random | random |
 test_getAnAssignmentTo
 | other_ns.js:4:9:4:16 | NS \|\| {} | NS |
 | other_ns.js:6:12:6:13 | {} | Conflict |
@@ -71,9 +75,15 @@ test_getAnAssignmentTo
 | test.js:30:1:30:28 | functio ... on() {} | globalFunction |
 | test.js:32:1:35:1 | functio ... .baz'\\n} | destruct |
 | test.js:37:1:41:1 | functio ... Init;\\n} | lazy |
+| test.js:43:1:64:1 | functio ... // no\\n} | dominatingWrite |
 test_assignedUnique
 | GlobalClass |
 | destruct |
+| dominatingWrite |
 | f |
 | globalFunction |
 | lazy |
+hasDominatingWrite
+| test.js:48:3:48:11 | obj.prop1 |
+| test.js:57:5:57:13 | obj.prop3 |
+| test.js:62:29:62:37 | obj.prop5 |

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

@@ -9,3 +9,7 @@ query string test_getAnAssignmentTo(DataFlow::Node node) {
 }
 
 query string test_assignedUnique() { AccessPath::isAssignedInUniqueFile(result) }
+
+query DataFlow::PropRead hasDominatingWrite() {
+  AccessPath::DominatingPaths::hasDominatingWrite(result)
+}

javascript/ql/test/library-tests/GlobalAccessPaths/test.js

@@ -39,3 +39,26 @@ function lazy() {
   lazyInit = foo.bar; // 'foo.bar'
   lazyInit;
 }
+
+function dominatingWrite() {
+  var obj = Object.create();
+
+  obj.prop1; // no
+  obj.prop1 = "foo";
+  obj.prop1; // yes
+
+  if (random()) {
+    obj.prop2 = "foo";
+  }
+  obj.prop2; // no
+
+  obj.prop3 = "foo";
+  if (random()) {
+    obj.prop3; // yes
+  }
+
+  obj.prop4 = obj.prop4; // no
+
+  var foo = (obj.prop5 = 2, obj.prop5); // yes
+  var bar = (obj.prop6, obj.prop6 = 3); // no
+}