Merge pull request #12826 from asgerf/js/more-call-graph-steps

asgerf · web-flow · commit 5272810ad961 · 2023-04-17T13:50:59.000+02:00
JS: Improvements to type-tracking through 'extend' and 'this'
diff --git a/javascript/ql/lib/semmle/javascript/Extend.qll b/javascript/ql/lib/semmle/javascript/Extend.qll
@@ -157,10 +157,12 @@ private class FunctionalExtendCallShallow extends ExtendCall {
 }
 
 /**
- * A taint propagating data flow edge from the objects flowing into an extend call to its return value
+ * A value-preserving data flow edge from the objects flowing into an extend call to its return value
  * and to the source of the destination object.
+ *
+ * Since all object properties are preserved, we model this as a value-preserving step.
  */
-private class ExtendCallTaintStep extends TaintTracking::SharedTaintStep {
+private class ExtendCallStep extends PreCallGraphStep {
   override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
     exists(ExtendCall extend |
       pred = extend.getASourceOperand() and succ = extend.getDestinationOperand().getALocalSource()
diff --git a/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
@@ -806,6 +806,10 @@ private predicate basicFlowStepNoBarrier(
   callStep(pred, succ) and
   summary = PathSummary::call()
   or
+  // Implied receiver flow
+  CallGraph::impliedReceiverStep(pred, succ) and
+  summary = PathSummary::call()
+  or
   // Flow out of function
   returnStep(pred, succ) and
   summary = PathSummary::return()
diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/CallGraphs.qll
@@ -241,22 +241,26 @@ module CallGraph {
     )
   }
 
-  private predicate shouldTrackObjectWithMethods(DataFlow::SourceNode node) {
+  private DataFlow::FunctionNode getAMethodOnPlainObject(DataFlow::SourceNode node) {
     (
       (
         node instanceof DataFlow::ObjectLiteralNode
         or
         node instanceof DataFlow::FunctionNode
       ) and
-      node.getAPropertySource() instanceof DataFlow::FunctionNode
+      result = node.getAPropertySource()
       or
-      exists(node.(DataFlow::ObjectLiteralNode).getPropertyGetter(_))
+      result = node.(DataFlow::ObjectLiteralNode).getPropertyGetter(_)
       or
-      exists(node.(DataFlow::ObjectLiteralNode).getPropertySetter(_))
+      result = node.(DataFlow::ObjectLiteralNode).getPropertySetter(_)
     ) and
     not node.getTopLevel().isExterns()
   }
 
+  private predicate shouldTrackObjectWithMethods(DataFlow::SourceNode node) {
+    exists(getAMethodOnPlainObject(node))
+  }
+
   /**
    * Gets a step summary for tracking object literals.
    *
@@ -273,4 +277,22 @@ module CallGraph {
     or
     StepSummary::step(getAnAllocationSiteRef(node), result, objectWithMethodsStep())
   }
+
+  /**
+   * Holds if `pred` is assumed to flow to `succ` because a method is stored on an object that is assumed
+   * to be the receiver of calls to that method.
+   *
+   * For example, object literal below is assumed to flow to the receiver of the `foo` function:
+   * ```js
+   * let obj = {};
+   * obj.foo = function() {}
+   * ```
+   */
+  cached
+  predicate impliedReceiverStep(DataFlow::SourceNode pred, DataFlow::SourceNode succ) {
+    exists(DataFlow::SourceNode host |
+      pred = getAnAllocationSiteRef(host) and
+      succ = getAMethodOnPlainObject(host).getReceiver()
+    )
+  }
 }
diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/StepSummary.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/StepSummary.qll
@@ -94,6 +94,10 @@ private module Cached {
     DataFlow::localFieldStep(pred, succ) and
     summary = LevelStep()
     or
+    // Implied flow of host object into 'this' of a method
+    CallGraph::impliedReceiverStep(pred, succ) and
+    summary = CallStep()
+    or
     exists(string prop |
       basicStoreStep(pred, succ, prop) and
       summary = StoreStep(prop)
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll
@@ -55,6 +55,22 @@ class Configuration extends TaintTracking::Configuration {
     )
   }
 
+  override predicate isSanitizerEdge(
+    DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel lbl
+  ) {
+    // Suppress the value-preserving step src -> dst in `extend(dst, src)`. This is modeled as a value-preserving
+    // step because it preserves all properties, but the destination is not actually Object.prototype.
+    exists(ExtendCall call |
+      pred = call.getASourceOperand() and
+      (
+        succ = call.getDestinationOperand().getALocalSource()
+        or
+        succ = call
+      ) and
+      lbl instanceof ObjectPrototype
+    )
+  }
+
   override predicate isAdditionalFlowStep(
     DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
   ) {
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll
@@ -31,6 +31,13 @@ module UnsafeJQueryPlugin {
    */
   abstract class Sanitizer extends DataFlow::Node { }
 
+  /**
+   * The receiver of a function, seen as a sanitizer.
+   *
+   * Plugins often do `$(this)` to coerce an existing DOM element to a jQuery object.
+   */
+  private class ThisSanitizer extends Sanitizer instanceof DataFlow::ThisNode { }
+
   /**
    * An argument that may act as an HTML fragment rather than a CSS selector, as a sink for remote unsafe jQuery plugins.
    */
diff --git a/javascript/ql/src/change-notes/2023-04-14-more-call-graph-steps.md b/javascript/ql/src/change-notes/2023-04-14-more-call-graph-steps.md
@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Improved the call graph to better handle the case where a function is stored on
+  a plain object and subsequently copied to a new host object via an `extend` call.
diff --git a/javascript/ql/test/query-tests/Security/CWE-020/UntrustedDataToExternalAPI/UntrustedDataToExternalAPI.expected b/javascript/ql/test/query-tests/Security/CWE-020/UntrustedDataToExternalAPI/UntrustedDataToExternalAPI.expected
@@ -36,6 +36,8 @@ nodes
 | tst-UntrustedDataToExternalAPI.js:33:14:33:22 | untrusted |
 | tst-UntrustedDataToExternalAPI.js:34:34:34:42 | untrusted |
 | tst-UntrustedDataToExternalAPI.js:34:34:34:42 | untrusted |
+| tst-UntrustedDataToExternalAPI.js:41:7:41:8 | {} |
+| tst-UntrustedDataToExternalAPI.js:41:7:41:8 | {} |
 | tst-UntrustedDataToExternalAPI.js:41:11:45:1 | {\\n    x ... usted\\n} |
 | tst-UntrustedDataToExternalAPI.js:41:11:45:1 | {\\n    x ... usted\\n} |
 | tst-UntrustedDataToExternalAPI.js:42:8:42:16 | untrusted |
@@ -83,6 +85,8 @@ edges
 | tst-UntrustedDataToExternalAPI.js:24:21:24:41 | JSON.pa ... rusted) | tst-UntrustedDataToExternalAPI.js:24:20:24:42 | [JSON.p ... usted)] |
 | tst-UntrustedDataToExternalAPI.js:24:21:24:41 | JSON.pa ... rusted) | tst-UntrustedDataToExternalAPI.js:24:20:24:42 | [JSON.p ... usted)] |
 | tst-UntrustedDataToExternalAPI.js:24:32:24:40 | untrusted | tst-UntrustedDataToExternalAPI.js:24:21:24:41 | JSON.pa ... rusted) |
+| tst-UntrustedDataToExternalAPI.js:41:11:45:1 | {\\n    x ... usted\\n} | tst-UntrustedDataToExternalAPI.js:41:7:41:8 | {} |
+| tst-UntrustedDataToExternalAPI.js:41:11:45:1 | {\\n    x ... usted\\n} | tst-UntrustedDataToExternalAPI.js:41:7:41:8 | {} |
 | tst-UntrustedDataToExternalAPI.js:42:8:42:16 | untrusted | tst-UntrustedDataToExternalAPI.js:41:11:45:1 | {\\n    x ... usted\\n} |
 | tst-UntrustedDataToExternalAPI.js:42:8:42:16 | untrusted | tst-UntrustedDataToExternalAPI.js:41:11:45:1 | {\\n    x ... usted\\n} |
 | tst-UntrustedDataToExternalAPI.js:43:8:43:16 | untrusted | tst-UntrustedDataToExternalAPI.js:41:11:45:1 | {\\n    x ... usted\\n} |
@@ -101,4 +105,5 @@ edges
 | tst-UntrustedDataToExternalAPI.js:30:13:30:30 | getDeepUntrusted() | tst-UntrustedDataToExternalAPI.js:3:17:3:27 | window.name | tst-UntrustedDataToExternalAPI.js:30:13:30:30 | getDeepUntrusted() | Call to external-lib() [param 0] with untrusted data from $@. | tst-UntrustedDataToExternalAPI.js:3:17:3:27 | window.name | window.name |
 | tst-UntrustedDataToExternalAPI.js:33:14:33:22 | untrusted | tst-UntrustedDataToExternalAPI.js:3:17:3:27 | window.name | tst-UntrustedDataToExternalAPI.js:33:14:33:22 | untrusted | Call to external-lib.get.[callback].[param 'res'].send() [param 0] with untrusted data from $@. | tst-UntrustedDataToExternalAPI.js:3:17:3:27 | window.name | window.name |
 | tst-UntrustedDataToExternalAPI.js:34:34:34:42 | untrusted | tst-UntrustedDataToExternalAPI.js:3:17:3:27 | window.name | tst-UntrustedDataToExternalAPI.js:34:34:34:42 | untrusted | Call to external-lib.get.[callback].[param 'req'].app.locals.something.foo() [param 0] with untrusted data from $@. | tst-UntrustedDataToExternalAPI.js:3:17:3:27 | window.name | window.name |
+| tst-UntrustedDataToExternalAPI.js:41:7:41:8 | {} | tst-UntrustedDataToExternalAPI.js:3:17:3:27 | window.name | tst-UntrustedDataToExternalAPI.js:41:7:41:8 | {} | Call to lodash.merge() [param 0] with untrusted data from $@. | tst-UntrustedDataToExternalAPI.js:3:17:3:27 | window.name | window.name |
 | tst-UntrustedDataToExternalAPI.js:41:11:45:1 | {\\n    x ... usted\\n} | tst-UntrustedDataToExternalAPI.js:3:17:3:27 | window.name | tst-UntrustedDataToExternalAPI.js:41:11:45:1 | {\\n    x ... usted\\n} | Call to lodash.merge() [param 1] with untrusted data from $@. | tst-UntrustedDataToExternalAPI.js:3:17:3:27 | window.name | window.name |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
@@ -1119,6 +1119,10 @@ nodes
 | tst.js:494:18:494:30 | location.hash |
 | tst.js:494:18:494:40 | locatio ... bstr(1) |
 | tst.js:494:18:494:40 | locatio ... bstr(1) |
+| tst.js:501:33:501:63 | decodeU ... n.hash) |
+| tst.js:501:33:501:63 | decodeU ... n.hash) |
+| tst.js:501:43:501:62 | window.location.hash |
+| tst.js:501:43:501:62 | window.location.hash |
 | typeahead.js:20:13:20:45 | target |
 | typeahead.js:20:22:20:45 | documen ... .search |
 | typeahead.js:20:22:20:45 | documen ... .search |
@@ -2271,6 +2275,10 @@ edges
 | tst.js:494:18:494:30 | location.hash | tst.js:494:18:494:40 | locatio ... bstr(1) |
 | tst.js:494:18:494:30 | location.hash | tst.js:494:18:494:40 | locatio ... bstr(1) |
 | tst.js:494:18:494:30 | location.hash | tst.js:494:18:494:40 | locatio ... bstr(1) |
+| tst.js:501:43:501:62 | window.location.hash | tst.js:501:33:501:63 | decodeU ... n.hash) |
+| tst.js:501:43:501:62 | window.location.hash | tst.js:501:33:501:63 | decodeU ... n.hash) |
+| tst.js:501:43:501:62 | window.location.hash | tst.js:501:33:501:63 | decodeU ... n.hash) |
+| tst.js:501:43:501:62 | window.location.hash | tst.js:501:33:501:63 | decodeU ... n.hash) |
 | typeahead.js:20:13:20:45 | target | typeahead.js:21:12:21:17 | target |
 | typeahead.js:20:22:20:45 | documen ... .search | typeahead.js:20:13:20:45 | target |
 | typeahead.js:20:22:20:45 | documen ... .search | typeahead.js:20:13:20:45 | target |
@@ -2559,6 +2567,7 @@ edges
 | tst.js:486:22:486:24 | url | tst.js:471:13:471:36 | documen ... .search | tst.js:486:22:486:24 | url | Cross-site scripting vulnerability due to $@. | tst.js:471:13:471:36 | documen ... .search | user-provided value |
 | tst.js:491:23:491:45 | locatio ... bstr(1) | tst.js:491:23:491:35 | location.hash | tst.js:491:23:491:45 | locatio ... bstr(1) | Cross-site scripting vulnerability due to $@. | tst.js:491:23:491:35 | location.hash | user-provided value |
 | tst.js:494:18:494:40 | locatio ... bstr(1) | tst.js:494:18:494:30 | location.hash | tst.js:494:18:494:40 | locatio ... bstr(1) | Cross-site scripting vulnerability due to $@. | tst.js:494:18:494:30 | location.hash | user-provided value |
+| tst.js:501:33:501:63 | decodeU ... n.hash) | tst.js:501:43:501:62 | window.location.hash | tst.js:501:33:501:63 | decodeU ... n.hash) | Cross-site scripting vulnerability due to $@. | tst.js:501:43:501:62 | window.location.hash | user-provided value |
 | typeahead.js:25:18:25:20 | val | typeahead.js:20:22:20:45 | documen ... .search | typeahead.js:25:18:25:20 | val | Cross-site scripting vulnerability due to $@. | typeahead.js:20:22:20:45 | documen ... .search | user-provided value |
 | v-html.vue:2:8:2:23 | v-html=tainted | v-html.vue:6:42:6:58 | document.location | v-html.vue:2:8:2:23 | v-html=tainted | Cross-site scripting vulnerability due to $@. | v-html.vue:6:42:6:58 | document.location | user-provided value |
 | various-concat-obfuscations.js:4:4:4:31 | "<div>" ... </div>" | various-concat-obfuscations.js:2:16:2:39 | documen ... .search | various-concat-obfuscations.js:4:4:4:31 | "<div>" ... </div>" | Cross-site scripting vulnerability due to $@. | various-concat-obfuscations.js:2:16:2:39 | documen ... .search | user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
@@ -1131,6 +1131,10 @@ nodes
 | tst.js:494:18:494:30 | location.hash |
 | tst.js:494:18:494:40 | locatio ... bstr(1) |
 | tst.js:494:18:494:40 | locatio ... bstr(1) |
+| tst.js:501:33:501:63 | decodeU ... n.hash) |
+| tst.js:501:33:501:63 | decodeU ... n.hash) |
+| tst.js:501:43:501:62 | window.location.hash |
+| tst.js:501:43:501:62 | window.location.hash |
 | typeahead.js:9:28:9:30 | loc |
 | typeahead.js:9:28:9:30 | loc |
 | typeahead.js:9:28:9:30 | loc |
@@ -2333,6 +2337,10 @@ edges
 | tst.js:494:18:494:30 | location.hash | tst.js:494:18:494:40 | locatio ... bstr(1) |
 | tst.js:494:18:494:30 | location.hash | tst.js:494:18:494:40 | locatio ... bstr(1) |
 | tst.js:494:18:494:30 | location.hash | tst.js:494:18:494:40 | locatio ... bstr(1) |
+| tst.js:501:43:501:62 | window.location.hash | tst.js:501:33:501:63 | decodeU ... n.hash) |
+| tst.js:501:43:501:62 | window.location.hash | tst.js:501:33:501:63 | decodeU ... n.hash) |
+| tst.js:501:43:501:62 | window.location.hash | tst.js:501:33:501:63 | decodeU ... n.hash) |
+| tst.js:501:43:501:62 | window.location.hash | tst.js:501:33:501:63 | decodeU ... n.hash) |
 | typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |
 | typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |
 | typeahead.js:9:28:9:30 | loc | typeahead.js:10:16:10:18 | loc |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/tst.js b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/tst.js
@@ -313,7 +313,7 @@ function basicExceptions() {
 }
 
 function handlebarsSafeString() {
-	return new Handlebars.SafeString(location); // NOT OK!	
+	return new Handlebars.SafeString(location); // NOT OK!
 }
 
 function test2() {
@@ -355,15 +355,15 @@ function thisNodes() {
 	    var target = document.location.search
 	    this.html(target); // NOT OK. (this is a jQuery object)
 		this.innerHTML = target // OK. (this is a jQuery object)
-	
+
 		this.each(function (i, e) {
 			this.innerHTML = target; // NOT OK. (this is a DOM-node);
 			this.html(target); // OK. (this is a DOM-node);
-			
+
 			e.innerHTML = target; // NOT OK.
 		});
 	}
-	$.fn[pluginName] = myPlugin; 
+	$.fn[pluginName] = myPlugin;
 
 }
 
@@ -380,7 +380,7 @@ function test() {
 function test() {
   var target = document.location.search
 
-  
+
   $('myId').html(target); // NOT OK
 
   $('myId').html(target.taint); // NOT OK
@@ -401,7 +401,7 @@ function test() {
   if (random()) {return;}
   $('myId').html(target.taint6); // OK
 
-  
+
   if (random()) {target.taint7 = "safe";}
   $('myId').html(target.taint7); // NOT OK
 
@@ -493,3 +493,13 @@ function urlStuff() {
   const myHistory = require('history').createBrowserHistory();
   myHistory.push(location.hash.substr(1)); // NOT OK
 }
+
+function Foo() {
+  this.foo = document;
+  var obj = {
+    bar: function() {
+      this.foo.body.innerHTML = decodeURI(window.location.hash); // NOT OK
+    }
+  };
+  Object.assign(this, obj);
+}
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/UnsafeJQueryPlugin/unsafe-jquery-plugin.js b/javascript/ql/test/query-tests/Security/CWE-079/UnsafeJQueryPlugin/unsafe-jquery-plugin.js
@@ -188,8 +188,18 @@
 		}
 		// extending options
 		options = $.extend( {}, options );
-	
+
 		var target = $( options.of ); // NOT OK
 		console.log(target);
 	};
+
+	$.fn.blockReceiver = function( options ) {
+		$.extend({
+				foo() {
+					$(this); // OK
+				}
+			},
+			options,
+		);
+	};
 });
diff --git a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/tst.js b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/tst.js
@@ -103,11 +103,23 @@ app.get('/bar', (req, res) => {
 
     let object = {};
     object[taint][taint] = taint; // NOT OK
-    
+
     const bad = ["__proto__", "constructor"];
     if (bad.includes(taint)) {
         return;
     }
 
     object[taint][taint] = taint; // OK
-});
+});
+
+app.get('/assign', (req, res) => {
+    let taint = String(req.query.data);
+    let plainObj = {};
+
+    let object = Object.assign({}, plainObj[taint]);
+    object[taint] = taint; // OK - 'object' is not Object.prototype itself (but possibly a copy)
+
+    let dest = {};
+    Object.assign(dest, plainObj[taint]);
+    dest[taint] = taint; // OK - 'dest' is not Object.prototype itself (but possibly a copy)
+});
