Merge pull request #3478 from erik-krogh/PromiseAll

Approved by asgerf, esbena
2026-05-01 11:45:14 +02:00 · 2020-05-20 11:03:05 +01:00
parent 29b8a0db92 aa396a39d3
commit 2bbc1c2af0
8 changed files with 119 additions and 11 deletions
--- a/javascript/ql/src/semmle/javascript/Arrays.qll
+++ b/javascript/ql/src/semmle/javascript/Arrays.qll
@@ -251,13 +251,15 @@ private module ArrayDataFlow {
  /**
   * A step for creating an array and storing the elements in the array.
   */
-  private class ArrayCreationStep extends DataFlow::AdditionalFlowStep, DataFlow::Node {
-    ArrayCreationStep() { this instanceof DataFlow::ArrayCreationNode }
-
+  private class ArrayCreationStep extends DataFlow::AdditionalFlowStep, DataFlow::ArrayCreationNode {
    override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
-      prop = arrayElement() and
-      element = this.(DataFlow::ArrayCreationNode).getAnElement() and
-      obj = this
+      exists(int i |
+        element = this.getElement(i) and
+        obj = this and
+        if this = any(PromiseAllCreation c).getArrayNode()
+        then prop = arrayElement(i)
+        else prop = arrayElement()
+      )
    }
  }

--- a/javascript/ql/src/semmle/javascript/Promises.qll
+++ b/javascript/ql/src/semmle/javascript/Promises.qll
@@ -85,7 +85,7 @@ private class ES2015PromiseDefinition extends PromiseDefinition, DataFlow::NewNo
 */
 abstract class PromiseCreationCall extends DataFlow::CallNode {
  /**
-   * Gets the value this promise is resolved with.
+   * Gets a value this promise is resolved with.
   */
  abstract DataFlow::Node getValue();
 }
@@ -95,6 +95,16 @@ abstract class PromiseCreationCall extends DataFlow::CallNode {
 */
 abstract class ResolvedPromiseDefinition extends PromiseCreationCall { }

+/**
+ * A promise that is created using a `Promise.all(array)` call.
+ */
+abstract class PromiseAllCreation extends PromiseCreationCall {
+  /**
+   * Gets a node for the array of values given to the `Promise.all(array)` call.
+   */
+  abstract DataFlow::Node getArrayNode();
+}
+
 /**
 * A resolved promise created by the standard ECMAScript 2015 `Promise.resolve` function.
 */
@@ -121,6 +131,15 @@ class AggregateES2015PromiseDefinition extends PromiseCreationCall {
  }
 }

+/**
+ * An aggregated promise created using `Promise.all()`.
+ */
+class ES2015PromiseAllDefinition extends AggregateES2015PromiseDefinition, PromiseAllCreation {
+  ES2015PromiseAllDefinition() { this.getCalleeName() = "all" }
+
+  override DataFlow::Node getArrayNode() { result = getArgument(0) }
+}
+
 /**
 * Common predicates shared between type-tracking and data-flow for promises.
 */
@@ -303,16 +322,27 @@ private module PromiseFlow {
    CreationStep() { this = promise }

    override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+      not promise instanceof PromiseAllCreation and
      prop = valueProp() and
      pred = promise.getValue() and
      succ = this
+      or
+      prop = valueProp() and
+      pred = promise.(PromiseAllCreation).getArrayNode() and
+      succ = this
    }

    override predicate loadStore(DataFlow::Node pred, DataFlow::Node succ, string prop) {
      // Copy the value of a resolved promise to the value of this promise.
+      not promise instanceof PromiseAllCreation and
      prop = valueProp() and
      pred = promise.getValue() and
      succ = this
+      or
+      promise instanceof PromiseAllCreation and
+      prop = valueProp() and
+      pred = promise.(PromiseAllCreation).getArrayNode() and
+      succ = this
    }
  }

@@ -446,7 +476,11 @@ predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
  pred = succ.(PromiseDefinition).getResolveParameter().getACall().getArgument(0)
  or
  // from `x` to `Promise.resolve(x)`
-  pred = succ.(PromiseCreationCall).getValue()
+  pred = succ.(PromiseCreationCall).getValue() and
+  not succ instanceof PromiseAllCreation
+  or
+  // from `arr` to `Promise.all(arr)`
+  pred = succ.(PromiseAllCreation).getArrayNode()
  or
  exists(DataFlow::MethodCallNode thn | thn.getMethodName() = "then" |
    // from `p` to `x` in `p.then(x => ...)`
@@ -533,6 +567,15 @@ module Bluebird {
      result = getArgument(0).getALocalSource().(DataFlow::ArrayCreationNode).getAnElement()
    }
  }
+
+  /**
+   * A promise created using `Promise.all`:
+   */
+  class BluebirdPromiseAllDefinition extends AggregateBluebirdPromiseDefinition, PromiseAllCreation {
+    BluebirdPromiseAllDefinition() { this.getCalleeName() = "all" }
+
+    override DataFlow::Node getArrayNode() { result = getArgument(0) }
+  }
 }

 /**
--- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -607,6 +607,16 @@ module PseudoProperties {
   */
  string arrayElement() { result = pseudoProperty("arrayElement") }

+  /**
+   * Gets a pseudo-property for the location of the `i`th element in an `Array`.
+   */
+  bindingset[i]
+  string arrayElement(int i) {
+    i < 5 and result = i.toString()
+    or
+    result = arrayElement()
+  }
+
  /**
   * Gets a pseudo-property for the location of elements in some array-like object. (Set, Array, or Iterator).
   */
--- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
@@ -241,9 +241,6 @@ module TaintTracking {
   */
  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
      exists(Property prop | e.(ObjectExpr).getAProperty() = prop |
        prop.isComputed() and f = prop.getNameExpr()
      )
@@ -258,6 +255,10 @@ module TaintTracking {
      e.(ArrayExpr).getAnElement().(SpreadElement).getOperand() = f
    )
    or
+    // arrays with tainted elements and objects with tainted property names are tainted
+    succ.(DataFlow::ArrayCreationNode).getAnElement() = pred and
+    not any(PromiseAllCreation call).getArrayNode() = succ
+    or
    // reading from a tainted object yields a tainted result
    succ.(DataFlow::PropRead).getBase() = pred
    or
--- a/javascript/ql/test/library-tests/Promises/AdditionalPromises.expected
+++ b/javascript/ql/test/library-tests/Promises/AdditionalPromises.expected
@@ -1,4 +1,5 @@
 | additional-promises.js:2:13:2:57 | new Pin ... ct) {}) |
+| flow2.js:4:2:4:31 | Promise ... lean"]) |
 | flow.js:7:11:7:59 | new Pro ... ource)) |
 | flow.js:10:11:10:58 | new Pro ... ource)) |
 | flow.js:13:11:13:58 | new Pro ... ource)) |
--- a/javascript/ql/test/library-tests/Promises/flow2.js
+++ b/javascript/ql/test/library-tests/Promises/flow2.js
@@ -0,0 +1,21 @@
+(async function () {
+	var source = "source";
+
+	Promise.all([source, "clean"]).then((arr) => {
+		sink(arr); // OK
+		sink(arr[0]); // NOT OK
+		sink(arr[1]); // OK
+	})
+
+	var [clean, tainted] = await Promise.all(["clean", source]);
+	sink(clean); // OK
+	sink(tainted); // NOT OK
+
+	var [clean2, tainted2] = await Promise.resolve(Promise.all(["clean", source]));
+	sink(clean2); // OK
+	sink(tainted2); // NOT OK
+
+	var [clean3, tainted3] = await Promise.all(["clean", Promise.resolve(source)]);
+	sink(clean3); // OK
+	sink(tainted3); // NOT OK - but only flagged by taint-tracking
+});
--- a/javascript/ql/test/library-tests/Promises/tests.expected
+++ b/javascript/ql/test/library-tests/Promises/tests.expected
@@ -1,4 +1,14 @@
 test_ResolvedPromiseDefinition
+| flow2.js:4:2:4:31 | Promise ... lean"]) | flow2.js:4:15:4:20 | source |
+| flow2.js:4:2:4:31 | Promise ... lean"]) | flow2.js:4:23:4:29 | "clean" |
+| flow2.js:10:31:10:60 | Promise ... ource]) | flow2.js:10:44:10:50 | "clean" |
+| flow2.js:10:31:10:60 | Promise ... ource]) | flow2.js:10:53:10:58 | source |
+| flow2.js:14:33:14:79 | Promise ... urce])) | flow2.js:14:49:14:78 | Promise ... ource]) |
+| flow2.js:14:49:14:78 | Promise ... ource]) | flow2.js:14:62:14:68 | "clean" |
+| flow2.js:14:49:14:78 | Promise ... ource]) | flow2.js:14:71:14:76 | source |
+| flow2.js:18:33:18:79 | Promise ... urce)]) | flow2.js:18:46:18:52 | "clean" |
+| flow2.js:18:33:18:79 | Promise ... urce)]) | flow2.js:18:55:18:77 | Promise ... source) |
+| flow2.js:18:55:18:77 | Promise ... source) | flow2.js:18:71:18:76 | source |
 | flow.js:4:11:4:33 | Promise ... source) | flow.js:4:27:4:32 | source |
 | flow.js:20:2:20:24 | Promise ... source) | flow.js:20:18:20:23 | source |
 | flow.js:22:2:22:24 | Promise ... source) | flow.js:22:18:22:23 | source |
@@ -188,6 +198,9 @@ test_PromiseDefinition_getACatchHandler
 | flow.js:119:2:119:48 | new Pro ... "BLA")) | flow.js:119:56:119:68 | x => resolved |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:23:18:25:3 | (v) =>  ...  v;\\n  } |
 flow
+| flow2.js:2:15:2:22 | "source" | flow2.js:6:8:6:13 | arr[0] |
+| flow2.js:2:15:2:22 | "source" | flow2.js:12:7:12:13 | tainted |
+| flow2.js:2:15:2:22 | "source" | flow2.js:16:7:16:14 | tainted2 |
 | flow.js:2:15:2:22 | "source" | flow.js:5:7:5:14 | await p1 |
 | flow.js:2:15:2:22 | "source" | flow.js:8:7:8:14 | await p2 |
 | flow.js:2:15:2:22 | "source" | flow.js:17:8:17:8 | e |
@@ -220,8 +233,23 @@ flow
 | flow.js:2:15:2:22 | "source" | flow.js:129:69:129:69 | x |
 | flow.js:2:15:2:22 | "source" | flow.js:131:43:131:43 | x |
 exclusiveTaintFlow
+| flow2.js:2:15:2:22 | "source" | flow2.js:20:7:20:14 | tainted3 |
 | interflow.js:3:18:3:25 | "source" | interflow.js:18:10:18:14 | error |
 typetrack
+| flow2.js:4:2:4:31 | Promise ... lean"]) | flow2.js:4:14:4:30 | [source, "clean"] | copy $PromiseResolveField$ |
+| flow2.js:4:2:4:31 | Promise ... lean"]) | flow2.js:4:14:4:30 | [source, "clean"] | store $PromiseResolveField$ |
+| flow2.js:4:39:4:41 | arr | flow2.js:4:2:4:31 | Promise ... lean"]) | load $PromiseResolveField$ |
+| flow2.js:10:25:10:60 | await P ... ource]) | flow2.js:10:31:10:60 | Promise ... ource]) | load $PromiseResolveField$ |
+| flow2.js:10:31:10:60 | Promise ... ource]) | flow2.js:10:43:10:59 | ["clean", source] | copy $PromiseResolveField$ |
+| flow2.js:10:31:10:60 | Promise ... ource]) | flow2.js:10:43:10:59 | ["clean", source] | store $PromiseResolveField$ |
+| flow2.js:14:27:14:79 | await P ... urce])) | flow2.js:14:33:14:79 | Promise ... urce])) | load $PromiseResolveField$ |
+| flow2.js:14:33:14:79 | Promise ... urce])) | flow2.js:14:49:14:78 | Promise ... ource]) | copy $PromiseResolveField$ |
+| flow2.js:14:33:14:79 | Promise ... urce])) | flow2.js:14:49:14:78 | Promise ... ource]) | store $PromiseResolveField$ |
+| flow2.js:14:49:14:78 | Promise ... ource]) | flow2.js:14:61:14:77 | ["clean", source] | copy $PromiseResolveField$ |
+| flow2.js:14:49:14:78 | Promise ... ource]) | flow2.js:14:61:14:77 | ["clean", source] | store $PromiseResolveField$ |
+| flow2.js:18:27:18:79 | await P ... urce)]) | flow2.js:18:33:18:79 | Promise ... urce)]) | load $PromiseResolveField$ |
+| flow2.js:18:33:18:79 | Promise ... urce)]) | flow2.js:18:45:18:78 | ["clean ... ource)] | copy $PromiseResolveField$ |
+| flow2.js:18:33:18:79 | Promise ... urce)]) | flow2.js:18:45:18:78 | ["clean ... ource)] | store $PromiseResolveField$ |
 | flow.js:20:2:20:43 | Promise ... ink(x)) | flow.js:20:36:20:42 | sink(x) | copy $PromiseResolveField$ |
 | flow.js:20:2:20:43 | Promise ... ink(x)) | flow.js:20:36:20:42 | sink(x) | store $PromiseResolveField$ |
 | flow.js:20:31:20:31 | x | flow.js:20:2:20:24 | Promise ... source) | load $PromiseResolveField$ |