Merge pull request #8478 from asgerf/js/store-load-flow-context-sensitivity-bug

Approved by erik-krogh

Author: codeql-ci

javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll

@@ -1365,27 +1365,31 @@ private predicate loadStep(
 
 /**
  * Holds if there is flow to `base.startProp`, and `base.startProp` flows to `nd.endProp` under `cfg/summary`.
+ *
+ * If `onlyRelevantInCall` is true, the `base` object will not be propagated out of return edges, because
+ * the flow that originally reached `base.startProp` used a call edge.
  */
 pragma[nomagic]
 private predicate reachableFromStoreBase(
   string startProp, string endProp, DataFlow::Node base, DataFlow::Node nd,
-  DataFlow::Configuration cfg, PathSummary summary
+  DataFlow::Configuration cfg, PathSummary summary, boolean onlyRelevantInCall
 ) {
   exists(PathSummary s1, PathSummary s2, DataFlow::Node rhs |
-    reachableFromSource(rhs, cfg, s1)
+    reachableFromSource(rhs, cfg, s1) and
+    onlyRelevantInCall = s1.hasCall()
     or
-    reachableFromStoreBase(_, _, _, rhs, cfg, s1)
+    reachableFromStoreBase(_, _, _, rhs, cfg, s1, onlyRelevantInCall)
   |
     storeStep(rhs, nd, startProp, cfg, s2) and
     endProp = startProp and
     base = nd and
     summary =
-      MkPathSummary(false, s1.hasCall().booleanOr(s2.hasCall()), DataFlow::FlowLabel::data(),
-        DataFlow::FlowLabel::data())
+      MkPathSummary(false, s2.hasCall(), DataFlow::FlowLabel::data(), DataFlow::FlowLabel::data())
   )
   or
   exists(PathSummary newSummary, PathSummary oldSummary |
-    reachableFromStoreBaseStep(startProp, endProp, base, nd, cfg, oldSummary, newSummary) and
+    reachableFromStoreBaseStep(startProp, endProp, base, nd, cfg, oldSummary, newSummary,
+      onlyRelevantInCall) and
     summary = oldSummary.appendValuePreserving(newSummary)
   )
 }
@@ -1399,14 +1403,16 @@ private predicate reachableFromStoreBase(
 pragma[noinline]
 private predicate reachableFromStoreBaseStep(
   string startProp, string endProp, DataFlow::Node base, DataFlow::Node nd,
-  DataFlow::Configuration cfg, PathSummary oldSummary, PathSummary newSummary
+  DataFlow::Configuration cfg, PathSummary oldSummary, PathSummary newSummary,
+  boolean onlyRelevantInCall
 ) {
   exists(DataFlow::Node mid |
-    reachableFromStoreBase(startProp, endProp, base, mid, cfg, oldSummary) and
-    flowStep(mid, cfg, nd, newSummary)
+    reachableFromStoreBase(startProp, endProp, base, mid, cfg, oldSummary, onlyRelevantInCall) and
+    flowStep(mid, cfg, nd, newSummary) and
+    onlyRelevantInCall.booleanAnd(newSummary.hasReturn()) = false
     or
     exists(string midProp |
-      reachableFromStoreBase(startProp, midProp, base, mid, cfg, oldSummary) and
+      reachableFromStoreBase(startProp, midProp, base, mid, cfg, oldSummary, onlyRelevantInCall) and
       isAdditionalLoadStoreStep(mid, nd, midProp, endProp, cfg) and
       newSummary = PathSummary::level()
     )
@@ -1446,7 +1452,7 @@ private predicate storeToLoad(
     PathSummary s1, PathSummary s2
   |
     storeStep(pred, storeBase, storeProp, cfg, s1) and
-    reachableFromStoreBase(storeProp, loadProp, storeBase, loadBase, cfg, s2) and
+    reachableFromStoreBase(storeProp, loadProp, storeBase, loadBase, cfg, s2, _) and
     oldSummary = s1.appendValuePreserving(s2) and
     loadStep(loadBase, succ, loadProp, cfg, newSummary)
   )

javascript/ql/src/change-notes/2022-03-18-store-load-flow-context-sensitivity-bug.md

@@ -0,0 +1,6 @@
+---
+category: minorAnalysis
+---
+* Fixed an issue that would sometimes prevent the data-flow analysis from finding flow
+  paths through a function that stores its result on an object.
+  This may lead to more results for the security queries.

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

@@ -172,6 +172,7 @@ typeInferenceMismatch
 | string-replace.js:3:13:3:20 | source() | string-replace.js:21:6:21:41 | safe(). ...  taint) |
 | string-replace.js:3:13:3:20 | source() | string-replace.js:22:6:22:48 | safe(). ...  taint) |
 | string-replace.js:3:13:3:20 | source() | string-replace.js:24:6:24:45 | taint.r ...  + '!') |
+| summarize-store-load-in-call.js:9:15:9:22 | source() | summarize-store-load-in-call.js:9:10:9:23 | blah(source()) |
 | 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/summarize-store-load-in-call.js

@@ -0,0 +1,12 @@
+import * as dummy from 'dummy';
+
+function blah(obj) {
+    obj.prop = obj.prop + "x";
+    return obj.prop;
+}
+
+function test() {
+    sink(blah(source())); // NOT OK
+
+    blah(); // ensure more than one call site exists
+}