generalize isAdditionalLoadStoreStep such that it loads and stores different properties

Author: erik-krogh

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

@@ -246,6 +246,15 @@ abstract class Configuration extends string {
   predicate isAdditionalLoadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
     none()
   }
+
+  /**
+   * EXPERIMENTAL. This API may change in the future.
+   *
+   * Holds if the property `loadProp` should be copied from the object `pred` to the property `storeProp` of object `succ`.
+   */
+  predicate isAdditionalLoadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string loadProp, string storeProp) {
+    none()
+  }
 }
 
 /**
@@ -515,6 +524,17 @@ abstract class AdditionalFlowStep extends DataFlow::Node {
    */
   cached
   predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { none() }
+
+  /**
+   * EXPERIMENTAL. This API may change in the future.
+   *
+   * Holds if the property `loadProp` should be copied from the object `pred` to the property `storeProp` of object `succ`.
+   */
+  cached
+  predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string loadProp, string storeProp) { 
+    loadProp = storeProp and
+    loadStoreStep(pred, succ, loadProp)
+  }
 }
 
 /**
@@ -619,7 +639,7 @@ private predicate exploratoryFlowStep(
   basicLoadStep(pred, succ, _) or
   isAdditionalStoreStep(pred, succ, _, cfg) or
   isAdditionalLoadStep(pred, succ, _, cfg) or
-  isAdditionalLoadStoreStep(pred, succ, _, cfg) or
+  isAdditionalLoadStoreStep(pred, succ, _, _, cfg) or
   // the following two disjuncts taken together over-approximate flow through
   // higher-order calls
   callback(pred, succ) or
@@ -859,14 +879,21 @@ private predicate isAdditionalStoreStep(
 }
 
 /**
- * Holds if the property `prop` should be copied from the object `pred` to the object `succ`.
+ * Holds if the property `loadProp` should be copied from the object `pred` to the property `storeProp` of object `succ`.
  */
 private predicate isAdditionalLoadStoreStep(
-  DataFlow::Node pred, DataFlow::Node succ, string prop, DataFlow::Configuration cfg
+  DataFlow::Node pred, DataFlow::Node succ, string loadProp, string storeProp, DataFlow::Configuration cfg
 ) {
-  any(AdditionalFlowStep s).loadStoreStep(pred, succ, prop)
+  any(AdditionalFlowStep s).loadStoreStep(pred, succ, loadProp, storeProp)
+  or
+  cfg.isAdditionalLoadStoreStep(pred, succ, loadProp, storeProp)
   or
-  cfg.isAdditionalLoadStoreStep(pred, succ, prop)
+  loadProp = storeProp and
+  (
+    any(AdditionalFlowStep s).loadStoreStep(pred, succ, loadProp)
+    or
+    cfg.isAdditionalLoadStoreStep(pred, succ, loadProp)
+  )
 }
 
 /**
@@ -904,12 +931,14 @@ private predicate reachableFromStoreBase(
   or
   exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary |
     reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) and
-    (
-      flowStep(mid, cfg, nd, newSummary)
-      or
-      isAdditionalLoadStoreStep(mid, nd, prop, cfg) and
-      newSummary = PathSummary::level()
-    ) and
+    flowStep(mid, cfg, nd, newSummary) and
+    summary = oldSummary.appendValuePreserving(newSummary)
+  )
+  or
+  exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary, string midProp |
+    reachableFromStoreBase(midProp, rhs, mid, cfg, oldSummary) and
+    isAdditionalLoadStoreStep(mid, nd, midProp, prop, cfg) and
+    newSummary = PathSummary::level() and
     summary = oldSummary.appendValuePreserving(newSummary)
   )
 }

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

@@ -1 +1,2 @@
 | tst.js:4:15:4:22 | "source" | tst.js:9:7:9:24 | readTaint(tainted) |
+| tst.js:4:15:4:22 | "source" | tst.js:15:7:15:20 | tainted3.other |

javascript/ql/test/library-tests/CustomLoadStoreSteps/test.ql

@@ -19,6 +19,14 @@ class Configuration extends TaintTracking::Configuration {
     ) and
     prop = "bar"
   }
+  
+  // calling .copy("foo", "bar") actually moves a property from "foo" to "bar".
+  override predicate isAdditionalLoadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string loadProp, string storeProp) {
+    exists(DataFlow::MethodCallNode call | call.getMethodName() = "copy" and call = succ and pred = call.getReceiver() | 
+      call.getArgument(0).mayHaveStringValue(loadProp) and
+      call.getArgument(1).mayHaveStringValue(storeProp)
+    )
+  }
 }
 
 from DataFlow::Node pred, DataFlow::Node succ, Configuration cfg

javascript/ql/test/library-tests/CustomLoadStoreSteps/tst.js

@@ -6,5 +6,15 @@
 	function readTaint(x) {
 		return x.foo;
 	}
-	sink(readTaint(tainted));
+	sink(readTaint(tainted)); // NOT OK
+	
+	
+	var tainted2 = {myProp: source};
+	
+	var tainted3 = tainted2.copy("myProp", "other");
+	sink(tainted3.other); // NOT OK.
+	
+	var tainted4 = tainted2.copy("other", "myProp"); // does nothing, there is no "other" on tainted2.
+	sink(tainted4.other); // OK. 
+	
 })();