Merge pull request #784 from asger-semmle/dedup-promiseTaintStep

Approved by esben-semmle

Author: semmle-qlci

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

@@ -1,71 +1,9 @@
 /**
- * Provides classes for working with promises.
+ * Provides classes for modelling promise libraries.
  */
 
 import javascript
 
-/**
- * A promise object created by the standard ECMAScript 2015 `Promise` constructor.
- */
-private class ES2015PromiseDefinition extends PromiseDefinition, DataFlow::NewNode {
-  ES2015PromiseDefinition() { this = DataFlow::globalVarRef("Promise").getAnInstantiation() }
-
-  override DataFlow::FunctionNode getExecutor() { result = getCallback(0) }
-}
-
-/**
- * A data flow edge from a promise reaction to the corresponding handler.
- */
-private class PromiseFlowStep extends DataFlow::AdditionalFlowStep {
-  PromiseDefinition p;
-
-  PromiseFlowStep() { this = p }
-
-  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    pred = p.getResolveParameter().getACall().getArgument(0) and
-    succ = p.getAResolveHandler().getParameter(0)
-    or
-    pred = p.getRejectParameter().getACall().getArgument(0) and
-    succ = p.getARejectHandler().getParameter(0)
-  }
-}
-
-/**
- * Holds if taint propagates from `pred` to `succ` through promises.
- */
-private predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
-  // from `x` to `new Promise((res, rej) => res(x))`
-  pred = succ.(PromiseDefinition).getResolveParameter().getACall().getArgument(0)
-  or
-  // from `x` to `Promise.resolve(x)`
-  pred = succ.(ResolvedPromiseDefinition).getValue()
-  or
-  exists(DataFlow::MethodCallNode thn, DataFlow::FunctionNode cb |
-    thn.getMethodName() = "then" and cb = thn.getCallback(0)
-  |
-    // from `p` to `x` in `p.then(x => ...)`
-    pred = thn.getReceiver() and
-    succ = cb.getParameter(0)
-    or
-    // from `v` to `p.then(x => return v)`
-    pred = cb.getFunction().getAReturnedExpr().flow() and
-    succ = thn
-  )
-}
-
-/**
- * An additional taint step that involves promises.
- */
-private class PromiseTaintStep extends TaintTracking::AdditionalTaintStep {
-  DataFlow::Node source;
-
-  PromiseTaintStep() { promiseTaintStep(source, this) }
-
-  override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-    pred = source and succ = this
-  }
-}
-
 /**
  * Provides classes for working with the `bluebird` library (http://bluebirdjs.com).
  */
@@ -107,24 +45,3 @@ module Q {
     override DataFlow::FunctionNode getExecutor() { result = getCallback(0) }
   }
 }
-
-/**
- * A promise that is resolved with the given value.
- */
-abstract class ResolvedPromiseDefinition extends DataFlow::CallNode {
-  /**
-   * Gets the value this promise is resolved with.
-   */
-  abstract DataFlow::Node getValue();
-}
-
-/**
- * A resolved promise created by the standard ECMAScript 2015 `Promise.resolve` function.
- */
-class ResolvedES2015PromiseDefinition extends ResolvedPromiseDefinition {
-  ResolvedES2015PromiseDefinition() {
-    this = DataFlow::globalVarRef("Promise").getAMemberCall("resolve")
-  }
-
-  override DataFlow::Node getValue() { result = getArgument(0) }
-}

javascript/ql/src/semmle/javascript/StandardLibrary.qll

@@ -139,6 +139,27 @@ private class ES2015PromiseDefinition extends PromiseDefinition, DataFlow::NewNo
   override DataFlow::FunctionNode getExecutor() { result = getCallback(0) }
 }
 
+/**
+ * A promise that is resolved with the given value.
+ */
+abstract class ResolvedPromiseDefinition extends DataFlow::CallNode {
+  /**
+   * Gets the value this promise is resolved with.
+   */
+  abstract DataFlow::Node getValue();
+}
+
+/**
+ * A resolved promise created by the standard ECMAScript 2015 `Promise.resolve` function.
+ */
+class ResolvedES2015PromiseDefinition extends ResolvedPromiseDefinition {
+  ResolvedES2015PromiseDefinition() {
+    this = DataFlow::globalVarRef("Promise").getAMemberCall("resolve")
+  }
+
+  override DataFlow::Node getValue() { result = getArgument(0) }
+}
+
 /**
  * A data flow edge from a promise reaction to the corresponding handler.
  */
@@ -164,8 +185,7 @@ private predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
   pred = succ.(PromiseDefinition).getResolveParameter().getACall().getArgument(0)
   or
   // from `x` to `Promise.resolve(x)`
-  succ = DataFlow::globalVarRef("Promise").getAMemberCall("resolve") and
-  pred = succ.(DataFlow::CallNode).getArgument(0)
+  pred = succ.(ResolvedPromiseDefinition).getValue()
   or
   exists(DataFlow::MethodCallNode thn, DataFlow::FunctionNode cb |
     thn.getMethodName() = "then" and cb = thn.getCallback(0)

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -10,6 +10,8 @@
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:30:14:30:20 | x.value |
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:41:10:41:18 | id(taint) |
 | partialCalls.js:4:17:4:24 | source() | partialCalls.js:51:14:51:14 | x |
+| promise.js:4:24:4:31 | source() | promise.js:4:8:4:32 | Promise ... urce()) |
+| promise.js:5:25:5:32 | source() | promise.js:5:8:5:33 | bluebir ... urce()) |
 | thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field |
 | thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 |
 | tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |

javascript/ql/test/library-tests/TaintTracking/promise.js

@@ -0,0 +1,6 @@
+let bluebird = require('bluebird');
+
+function test() {
+  sink(Promise.resolve(source())); // NOT OK
+  sink(bluebird.resolve(source())); // NOT OK
+}