more precise taint-tracking for Promise.all

Author: erik-krogh

javascript/ql/src/semmle/javascript/Promises.qll

@@ -476,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 => ...)`

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

javascript/ql/test/library-tests/Promises/flow2.js

@@ -2,20 +2,20 @@
 	var source = "source";
 
 	Promise.all([source, "clean"]).then((arr) => {
-		sink(arr); // OK - but flagged by taint-tracking.
+		sink(arr); // OK
 		sink(arr[0]); // NOT OK
-		sink(arr[1]); // OK - but flagged by taint-tracking.
+		sink(arr[1]); // OK
 	})
 
 	var [clean, tainted] = await Promise.all(["clean", source]);
-	sink(clean); // OK - but flagged by taint-tracking
+	sink(clean); // OK
 	sink(tainted); // NOT OK
 
 	var [clean2, tainted2] = await Promise.resolve(Promise.all(["clean", source]));
-	sink(clean2); // OK - but flagged by taint-tracking
+	sink(clean2); // OK
 	sink(tainted2); // NOT OK
 
-	var [clean2, tainted2] = await Promise.all(["clean", Promise.resolve(source)]);
-	sink(clean2); // OK - but flagged by taint-tracking
-	sink(tainted2); // NOT OK - but only flagged by taint-tracking
+	var [clean3, tainted3] = await Promise.all(["clean", Promise.resolve(source)]);
+	sink(clean3); // OK
+	sink(tainted3); // NOT OK - but only flagged by taint-tracking
 });

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

@@ -233,12 +233,7 @@ 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:5:8:5:10 | arr |
-| flow2.js:2:15:2:22 | "source" | flow2.js:7:8:7:13 | arr[1] |
-| flow2.js:2:15:2:22 | "source" | flow2.js:11:7:11:11 | clean |
-| flow2.js:2:15:2:22 | "source" | flow2.js:15:7:15:12 | clean2 |
-| flow2.js:2:15:2:22 | "source" | flow2.js:19:7:19:12 | clean2 |
-| flow2.js:2:15:2:22 | "source" | flow2.js:20:7:20:14 | tainted2 |
+| 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$ |