add data-flow steps for resolved promises using pseudo-properties

2026-04-30 03:05:15 +02:00 · 2020-01-21 15:52:50 +01:00
parent 6648e2751f
commit d8b25ef5a2
3 changed files with 87 additions and 6 deletions
--- a/javascript/ql/src/semmle/javascript/Promises.qll
+++ b/javascript/ql/src/semmle/javascript/Promises.qll
@@ -121,9 +121,17 @@ class AggregateES2015PromiseDefinition extends PromiseCreationCall {
 }

 /**
- * This module defines how exceptional data-flow propagates into and out a Promise.
+ * This module defines how data-flow propagates into and out of a Promise.
+ * The data-flow is based on pseudo-properties rather than tainting the Promise object (which is what `PromiseTaintStep` does).
 */
-private module ExceptionalPromiseFlow {
+private module PromiseFlow {
+  /**
+   * Gets the pseudo-field used to describe resolved values in a promise.
+   */
+  string resolveField() {
+    result = "$PromiseResolveField$"
+  }
+
  /**
   * Gets the pseudo-field used to describe rejected values in a promise.
   */
@@ -134,7 +142,7 @@ private module ExceptionalPromiseFlow {
  /**
   * A flow step describing a promise definition.
   *
-   * The rejected value is written to a pseudo-field on the promise.
+   * The resolved/rejected value is written to a pseudo-field on the promise.
   */
  class PromiseDefitionStep extends DataFlow::AdditionalFlowStep {
    PromiseDefinition promise;
@@ -143,6 +151,10 @@ private module ExceptionalPromiseFlow {
    }

    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = resolveField() and
+      pred = promise.getResolveParameter().getACall().getArgument(0) and
+      succ = this
+      or
      prop = rejectField() and
      (
        pred = promise.getRejectParameter().getACall().getArgument(0) or
@@ -152,6 +164,23 @@ private module ExceptionalPromiseFlow {
    }
  }
  
+  /**
+   * A flow step describing the a Promise.resolve (and similar) call.
+   */
+  class CreationStep extends DataFlow::AdditionalFlowStep {
+    PromiseCreationCall promise;
+    CreationStep() {
+      this = promise
+    }
+
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = resolveField() and
+      pred = promise.getValue() and
+      succ = this
+    }
+  }
+
+
  /**
   * A load step loading the pseudo-field describing that the promise is rejected.
   * The rejected value is thrown as a exception.
@@ -165,6 +194,10 @@ private module ExceptionalPromiseFlow {
    }

    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = resolveField() and
+      succ = this and
+      pred = operand
+      or
      prop = rejectField() and
      succ = await.getExceptionTarget() and
      pred = operand
@@ -180,6 +213,10 @@ private module ExceptionalPromiseFlow {
    }

    override predicate load(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = resolveField() and
+      pred = getReceiver() and
+      succ = getCallback(0).getParameter(0)
+      or
      prop = rejectField() and
      pred = getReceiver() and
      succ = getCallback(1).getParameter(0)
@@ -193,12 +230,16 @@ private module ExceptionalPromiseFlow {
    }
    
    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = resolveField() and
+      pred = getCallback([0..1]).getAReturn() and
+      succ = this
+      or
      prop = rejectField() and
      pred = getCallback([0..1]).getExceptionalReturn() and
      succ = this
    }
  }
-  
+
  /**
   * A flow step describing the data-flow related to the `.catch` method of a promise.
   */
@@ -213,10 +254,20 @@ private module ExceptionalPromiseFlow {
      succ = getCallback(0).getParameter(0)
    }

+    override predicate copyProperty(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = resolveField() and
+      pred = getReceiver().getALocalSource() and
+      succ = this
+    }
+
    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
      prop = rejectField() and
      pred = getCallback(0).getExceptionalReturn() and
      succ = this
+      or
+      prop = resolveField() and
+      pred = getCallback(0).getAReturn() and
+      succ = this
    }
  }

@@ -229,10 +280,17 @@ private module ExceptionalPromiseFlow {
    }

    override predicate copyProperty(DataFlow::Node pred, DataFlow::Node succ, string prop) {
-      prop = rejectField() and
+      (prop = resolveField() or prop = rejectField()) and
      pred = getReceiver() and
      succ = this
    }
+
+    // a similar thing can also happen if a rejected promise is returned.
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = rejectField() and
+      pred = getCallback(0).getExceptionalReturn() and
+      succ = this
+    }
  }
 }

@@ -258,10 +316,14 @@ predicate promiseTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
    succ = thn
  )
  or
-  // from `p` to `p.catch(..)`
  exists(DataFlow::MethodCallNode catch | catch.getMethodName() = "catch" |
+    // from `p` to `p.catch(..)`
    pred = catch.getReceiver() and
    succ = catch
+    or
+    // from `v` to `p.catch(x => return v)`
+    pred = catch.getCallback(0).getAReturn() and
+    succ = catch
  )
  or
  // from `p` to `p.finally(..)`
--- a/javascript/ql/test/library-tests/Promises/flow.js
+++ b/javascript/ql/test/library-tests/Promises/flow.js
@@ -99,4 +99,8 @@
 	}
 	var foo = returnsRejected(new Promise((resolve, reject) => reject(source)));
 	sink(foo); // NOT OK!
+	
+	new Promise((resolve, reject) => reject("BLA")).catch(x => {return source}).then(x => sink(x)); // NOT OK
+	
+	new Promise((resolve, reject) => reject("BLA")).finally(x => {throw source}).catch(x => sink(x)); // NOT OK
 })();
--- a/javascript/ql/test/library-tests/Promises/tests.expected
+++ b/javascript/ql/test/library-tests/Promises/tests.expected
@@ -18,6 +18,8 @@ test_PromiseDefinition_getARejectHandler
 | flow.js:32:2:32:49 | new Pro ... ource)) | flow.js:32:57:32:68 | x => sink(x) |
 | flow.js:42:2:42:49 | new Pro ... ource)) | flow.js:42:67:42:75 | () => { } |
 | flow.js:48:2:48:36 | new Pro ... urce }) | flow.js:48:44:48:55 | x => sink(x) |
+| flow.js:103:2:103:48 | new Pro ... "BLA")) | flow.js:103:56:103:75 | x => {return source} |
+| flow.js:105:2:105:48 | new Pro ... "BLA")) | flow.js:105:58:105:76 | x => {throw source} |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:20:6:22:3 | (v) =>  ...  v;\\n  } |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:23:18:25:3 | (v) =>  ...  v;\\n  } |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:26:20:28:3 | (v) =>  ...  v;\\n  } |
@@ -38,12 +40,15 @@ test_PromiseDefinition_getExecutor
 | flow.js:86:23:86:70 | new Pro ... ource)) | flow.js:86:35:86:69 | (resolv ... source) |
 | flow.js:91:21:91:68 | new Pro ... ource)) | flow.js:91:33:91:67 | (resolv ... source) |
 | flow.js:100:28:100:75 | new Pro ... ource)) | flow.js:100:40:100:74 | (resolv ... source) |
+| flow.js:103:2:103:48 | new Pro ... "BLA")) | flow.js:103:14:103:47 | (resolv ... ("BLA") |
+| flow.js:105:2:105:48 | new Pro ... "BLA")) | flow.js:105:14:105:47 | (resolv ... ("BLA") |
 | interflow.js:11:12:15:6 | new Pro ... \\n    }) | interflow.js:11:24:15:5 | functio ... ;\\n    } |
 | promises.js:3:17:5:4 | new Pro ... );\\n  }) | promises.js:3:29:5:3 | functio ... e);\\n  } |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:10:30:17:3 | (res, r ... e);\\n  } |
 | promises.js:33:19:35:6 | new Pro ... \\n    }) | promises.js:33:31:35:5 | functio ... ;\\n    } |
 | promises.js:43:19:45:6 | Q.Promi ... \\n    }) | promises.js:43:29:45:5 | functio ... ;\\n    } |
 test_PromiseDefinition_getAFinallyHandler
+| flow.js:105:2:105:48 | new Pro ... "BLA")) | flow.js:105:58:105:76 | x => {throw source} |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:26:20:28:3 | (v) =>  ...  v;\\n  } |
 test_PromiseDefinition
 | flow.js:7:11:7:59 | new Pro ... ource)) |
@@ -62,6 +67,8 @@ test_PromiseDefinition
 | flow.js:86:23:86:70 | new Pro ... ource)) |
 | flow.js:91:21:91:68 | new Pro ... ource)) |
 | flow.js:100:28:100:75 | new Pro ... ource)) |
+| flow.js:103:2:103:48 | new Pro ... "BLA")) |
+| flow.js:105:2:105:48 | new Pro ... "BLA")) |
 | interflow.js:11:12:15:6 | new Pro ... \\n    }) |
 | promises.js:3:17:5:4 | new Pro ... );\\n  }) |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) |
@@ -76,6 +83,7 @@ test_PromiseDefinition_getAResolveHandler
 | flow.js:60:12:60:59 | new Pro ... ource)) | flow.js:61:21:61:28 | () => {} |
 | flow.js:74:10:74:57 | new Pro ... ource)) | flow.js:74:64:74:71 | () => {} |
 | flow.js:91:21:91:68 | new Pro ... ource)) | flow.js:91:75:91:82 | () => {} |
+| flow.js:105:2:105:48 | new Pro ... "BLA")) | flow.js:105:58:105:76 | x => {throw source} |
 | promises.js:3:17:5:4 | new Pro ... );\\n  }) | promises.js:6:16:8:3 | functio ... al;\\n  } |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:18:17:20:3 | (v) =>  ...  v;\\n  } |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:26:20:28:3 | (v) =>  ...  v;\\n  } |
@@ -97,6 +105,8 @@ test_PromiseDefinition_getRejectParameter
 | flow.js:86:23:86:70 | new Pro ... ource)) | flow.js:86:45:86:50 | reject |
 | flow.js:91:21:91:68 | new Pro ... ource)) | flow.js:91:43:91:48 | reject |
 | flow.js:100:28:100:75 | new Pro ... ource)) | flow.js:100:50:100:55 | reject |
+| flow.js:103:2:103:48 | new Pro ... "BLA")) | flow.js:103:24:103:29 | reject |
+| flow.js:105:2:105:48 | new Pro ... "BLA")) | flow.js:105:24:105:29 | reject |
 | interflow.js:11:12:15:6 | new Pro ... \\n    }) | interflow.js:11:43:11:48 | reject |
 | promises.js:3:17:5:4 | new Pro ... );\\n  }) | promises.js:3:48:3:53 | reject |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:10:36:10:38 | rej |
@@ -118,6 +128,8 @@ test_PromiseDefinition_getResolveParameter
 | flow.js:86:23:86:70 | new Pro ... ource)) | flow.js:86:36:86:42 | resolve |
 | flow.js:91:21:91:68 | new Pro ... ource)) | flow.js:91:34:91:40 | resolve |
 | flow.js:100:28:100:75 | new Pro ... ource)) | flow.js:100:41:100:47 | resolve |
+| flow.js:103:2:103:48 | new Pro ... "BLA")) | flow.js:103:15:103:21 | resolve |
+| flow.js:105:2:105:48 | new Pro ... "BLA")) | flow.js:105:15:105:21 | resolve |
 | interflow.js:11:12:15:6 | new Pro ... \\n    }) | interflow.js:11:34:11:40 | resolve |
 | promises.js:3:17:5:4 | new Pro ... );\\n  }) | promises.js:3:39:3:45 | resolve |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:10:31:10:33 | res |
@@ -126,6 +138,7 @@ test_PromiseDefinition_getResolveParameter
 test_PromiseDefinition_getACatchHandler
 | flow.js:32:2:32:49 | new Pro ... ource)) | flow.js:32:57:32:68 | x => sink(x) |
 | flow.js:48:2:48:36 | new Pro ... urce }) | flow.js:48:44:48:55 | x => sink(x) |
+| flow.js:103:2:103:48 | new Pro ... "BLA")) | flow.js:103:56:103:75 | x => {return source} |
 | promises.js:10:18:17:4 | new Pro ... );\\n  }) | promises.js:23:18:25:3 | (v) =>  ...  v;\\n  } |
 flow
 | flow.js:2:15:2:22 | "source" | flow.js:5:7:5:14 | await p1 |
@@ -150,4 +163,6 @@ flow
 | flow.js:2:15:2:22 | "source" | flow.js:84:21:84:21 | e |
 | flow.js:2:15:2:22 | "source" | flow.js:89:45:89:45 | e |
 | flow.js:2:15:2:22 | "source" | flow.js:101:7:101:9 | foo |
+| flow.js:2:15:2:22 | "source" | flow.js:103:93:103:93 | x |
+| flow.js:2:15:2:22 | "source" | flow.js:105:95:105:95 | x |
 | interflow.js:3:18:3:25 | "source" | interflow.js:18:10:18:14 | error |