copyPropertyStep works interprocedurally

2026-05-03 12:45:27 +02:00 · 2020-01-17 12:01:02 +01:00
parent 06e898f53b
commit 6ad62e32e0
4 changed files with 57 additions and 36 deletions
--- a/javascript/ql/src/semmle/javascript/Promises.qll
+++ b/javascript/ql/src/semmle/javascript/Promises.qll
@@ -142,7 +142,7 @@ private module ExceptionalPromiseFlow {
      this = promise
    }

-    override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
      prop = rejectField() and
      (
        pred = promise.getRejectParameter().getACall().getArgument(0) or
@@ -185,14 +185,14 @@ private module ExceptionalPromiseFlow {
      succ = getCallback(1).getParameter(0)
    }
    
-    override predicate copyProperty(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    override predicate copyProperty(DataFlow::Node pred, DataFlow::Node succ, string prop) {
      not exists(this.getArgument(1)) and
      prop = rejectField() and
      pred = getReceiver() and
      succ = this
    }
    
-    override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
      prop = rejectField() and
      pred = getCallback([0..1]).getExceptionalReturn() and
      succ = this
@@ -213,7 +213,7 @@ private module ExceptionalPromiseFlow {
      succ = getCallback(0).getParameter(0)
    }

-    override predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    override predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) {
      prop = rejectField() and
      pred = getCallback([0..1]).getExceptionalReturn() and
      succ = this
@@ -228,7 +228,7 @@ private module ExceptionalPromiseFlow {
      this.getMethodName() = "finally"
    }

-    override predicate copyProperty(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) {
+    override predicate copyProperty(DataFlow::Node pred, DataFlow::Node succ, string prop) {
      prop = rejectField() and
      pred = getReceiver() and
      succ = this
--- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -226,10 +226,8 @@ abstract class Configuration extends string {

  /**
   * Holds if the `pred` should be stored in the object `succ` under the property `prop`.
-   *
-   * `succ` is a DataFlow::SourceNode, as this is assumed by the `isAdditionalCopyPropertyStep` predicate.
   */
-  predicate isAdditionalStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
+  predicate isAdditionalStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }

  /**
   * Holds if the property `prop` of the object `pred` should be loaded into `succ`. 
@@ -237,9 +235,9 @@ abstract class Configuration extends string {
  predicate isAdditionalLoadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }

  /**
-   * Holds if the property `prop` should be copied from the object `pred` to the object `succ`. 
+   * Holds if the property `prop` should be copied from the object `pred` to the object `succ`.
   */
-  predicate isAdditionalCopyPropertyStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
+  predicate isAdditionalCopyPropertyStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
 }

 /**
@@ -470,11 +468,9 @@ abstract class AdditionalFlowStep extends DataFlow::Node {

  /**
   * Holds if the `pred` should be stored in the object `succ` under the property `prop`.
-   *
-   * `succ` is a DataFlow::SourceNode, as this is assumed by the `copyProperty` predicate.
   */
  cached
-  predicate store(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
+  predicate store(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }

  /**
   * Holds if the property `prop` of the object `pred` should be loaded into `succ`.
@@ -486,7 +482,7 @@ abstract class AdditionalFlowStep extends DataFlow::Node {
   * Holds if the property `prop` should be copied from the object `pred` to the object `succ`.
   */
  cached
-  predicate copyProperty(DataFlow::Node pred, DataFlow::SourceNode succ, string prop) { none() }
+  predicate copyProperty(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
 }

 /**
@@ -765,7 +761,7 @@ private predicate storeStep(
      returnedPropWrite(f, _, prop, mid)
      or
      exists(DataFlow::SourceNode base | 
-        isAdditionalStoreStep(mid, _, prop, cfg)
+        isAdditionalStoreStep(mid, base, prop, cfg)
        and
        base.flowsToExpr(f.getAReturnedExpr())
      )
@@ -811,27 +807,31 @@ private predicate reachesReturn(
  )
 }

+/**
+ * Holds if the property `prop` of the object `pred` should be loaded into `succ`.
+ */
 private predicate isAdditionalLoadStep(DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) { 
  any(AdditionalFlowStep s).load(pred, succ, prop)
  or
  cfg.isAdditionalLoadStep(pred, succ, prop)
 }

-private predicate isAdditionalStoreStep(DataFlow::Node pred, DataFlow::SourceNode succ, string prop, DataFlow::Configuration cfg) {
+/**
+ * Holds if the `pred` should be stored in the object `succ` under the property `prop`.
+ */
+private predicate isAdditionalStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) {
  any(AdditionalFlowStep s).store(pred, succ, prop)
  or
  cfg.isAdditionalStoreStep(pred, succ, prop)
 }

-private predicate isAdditionalCopyPropertyStep(DataFlow::SourceNode pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) {
-  exists(DataFlow::Node predNode, DataFlow::SourceNode succNode |
-    pred = predNode.getALocalSource() and
-    succ.getALocalSource() = succNode
-  |
-    any(AdditionalFlowStep s).copyProperty(predNode, succNode, prop)
-    or
-    cfg.isAdditionalCopyPropertyStep(predNode, succNode, prop)
-  )
+/**
+ * Holds if the property `prop` should be copied from the object `pred` to the object `succ`.
+ */
+private predicate isAdditionalCopyPropertyStep(DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg) {
+  any(AdditionalFlowStep s).copyProperty(pred, succ, prop)
+  or
+  cfg.isAdditionalCopyPropertyStep(pred, succ, prop)
 }

 /**
@@ -869,7 +869,12 @@ private predicate reachableFromStoreBase(
  or
  exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary |
    reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) and
-    flowStep(mid, cfg, nd, newSummary) and
+    (
+      flowStep(mid, cfg, nd, newSummary) 
+      or
+      existsCopyProperty(mid, nd, prop, cfg) and
+      newSummary = PathSummary::level()
+    ) and
    summary = oldSummary.appendValuePreserving(newSummary)
  )
 }
@@ -885,28 +890,33 @@ pragma[noinline]
 private predicate flowThroughProperty(
  DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg, PathSummary summary
 ) {
-  exists(string prop, DataFlow::Node storeBase, DataFlow::Node loadBase, PathSummary oldSummary, PathSummary newSummary |
-    reachableFromStoreBase(prop, pred, storeBase, cfg, oldSummary) and
-    (storeBase = loadBase or existsCopyProperty(storeBase, loadBase, prop, cfg)) and
-    loadStep(loadBase, succ, prop, cfg, newSummary) and
+  exists(string prop, DataFlow::Node base, PathSummary oldSummary, PathSummary newSummary |
+    reachableFromStoreBase(prop, pred, base, cfg, oldSummary) and
+    loadStep(base, succ, prop, cfg, newSummary) and
    summary = oldSummary.append(newSummary)
  )
 }

+/**
+ * Holds if the property `prop` is copied from `fromNode` to `toNode`.
+ */
+bindingset[prop, cfg]
+private predicate existsCopyProperty(DataFlow::Node fromNode, DataFlow::Node toNode, string prop, DataFlow::Configuration cfg) {
+  fromNode = toNode
+  or
+  existsCopyPropertyRecursive(fromNode, toNode, prop, cfg)
+}
+
 /**
 * Holds if the property `prop` is copied from `fromNode` to `toNode` using at least 1 step.
 *
 * The recursion of this predicate has been unfolded once compared to a naive implementation in order to avoid having no constraint on `prop`.
 * Therefore a caller of this predicate should also test whether the `toNode` and `fromNode` are equal.
 */
-private predicate existsCopyProperty(DataFlow::Node fromNode, DataFlow::Node toNode, string prop, DataFlow::Configuration cfg) {
+private predicate existsCopyPropertyRecursive(DataFlow::Node fromNode, DataFlow::Node toNode, string prop, DataFlow::Configuration cfg) {
  exists(DataFlow::Node mid |
    isAdditionalCopyPropertyStep(fromNode, mid, prop, cfg) and
-    (
-      existsCopyProperty(mid, toNode, prop, cfg)
-      or
-      mid = toNode
-    )
+    existsCopyProperty(mid, toNode, prop, cfg)
  )
 }

--- a/javascript/ql/test/library-tests/Promises/flow.js
+++ b/javascript/ql/test/library-tests/Promises/flow.js
@@ -69,4 +69,9 @@
 	} catch(e) {
 		sink(e); // NOT OK!
 	}
+	
+	function chainedPromise() {
+		return new Promise((resolve, reject) => reject(source)).then(() => {});
+	}
+	chainedPromise().then(() => {}).catch(e => sink(e)); // NOT OK!
 })();
--- a/javascript/ql/test/library-tests/Promises/tests.expected
+++ b/javascript/ql/test/library-tests/Promises/tests.expected
@@ -33,6 +33,7 @@ test_PromiseDefinition_getExecutor
 | flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:55:23:55:57 | (resolv ... source) |
 | flow.js:60:12:60:59 | new Pro ... ource)) | flow.js:60:24:60:58 | (resolv ... source) |
 | flow.js:65:9:65:56 | new Pro ... ource)) | flow.js:65:21:65:55 | (resolv ... source) |
+| flow.js:74:10:74:57 | new Pro ... ource)) | flow.js:74:22:74:56 | (resolv ... source) |
 | 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  } |
@@ -53,6 +54,7 @@ test_PromiseDefinition
 | flow.js:55:11:55:58 | new Pro ... ource)) |
 | flow.js:60:12:60:59 | new Pro ... ource)) |
 | flow.js:65:9:65:56 | new Pro ... ource)) |
+| flow.js:74:10:74:57 | new Pro ... ource)) |
 | 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  }) |
@@ -65,6 +67,7 @@ test_PromiseDefinition_getAResolveHandler
 | flow.js:42:2:42:49 | new Pro ... ource)) | flow.js:42:56:42:64 | () => { } |
 | flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:56:19:56:26 | () => {} |
 | 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 | () => {} |
 | 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  } |
@@ -82,6 +85,7 @@ test_PromiseDefinition_getRejectParameter
 | flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:55:33:55:38 | reject |
 | flow.js:60:12:60:59 | new Pro ... ource)) | flow.js:60:34:60:39 | reject |
 | flow.js:65:9:65:56 | new Pro ... ource)) | flow.js:65:31:65:36 | reject |
+| flow.js:74:10:74:57 | new Pro ... ource)) | flow.js:74:32:74:37 | 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 |
@@ -99,6 +103,7 @@ test_PromiseDefinition_getResolveParameter
 | flow.js:55:11:55:58 | new Pro ... ource)) | flow.js:55:24:55:30 | resolve |
 | flow.js:60:12:60:59 | new Pro ... ource)) | flow.js:60:25:60:31 | resolve |
 | flow.js:65:9:65:56 | new Pro ... ource)) | flow.js:65:22:65:28 | resolve |
+| flow.js:74:10:74:57 | new Pro ... ource)) | flow.js:74:23:74:29 | 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,4 +131,5 @@ flow
 | flow.js:2:15:2:22 | "source" | flow.js:58:24:58:24 | x |
 | flow.js:2:15:2:22 | "source" | flow.js:62:22:62:22 | x |
 | flow.js:2:15:2:22 | "source" | flow.js:70:8:70:8 | e |
+| flow.js:2:15:2:22 | "source" | flow.js:76:50:76:50 | e |
 | interflow.js:3:18:3:25 | "source" | interflow.js:18:10:18:14 | error |