using pseudo-properties to model URL parsing

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

@@ -602,45 +602,89 @@ module TaintTracking {
   }
 
   /**
-   * Holds if `params` is a `URLSearchParams` object providing access to
-   * the parameters encoded in `input`.
+   * Holds if `params` is a construction of a `URLSearchParams` that parses 
+   * the parameters in `input`.
    */
-  predicate isUrlSearchParams(DataFlow::SourceNode params, DataFlow::Node input) {
+  private predicate isUrlSearchParams(DataFlow::SourceNode params, DataFlow::Node input) {
     exists(DataFlow::GlobalVarRefNode urlSearchParams, NewExpr newUrlSearchParams |
       urlSearchParams.getName() = "URLSearchParams" and
       newUrlSearchParams = urlSearchParams.getAnInstantiation().asExpr() and
       params.asExpr() = newUrlSearchParams and
       input.asExpr() = newUrlSearchParams.getArgument(0)
     )
-    or
-    exists(DataFlow::NewNode newUrl |
-      newUrl = DataFlow::globalVarRef("URL").getAnInstantiation() and
-      params = newUrl.getAPropertyRead("searchParams") and
-      input = newUrl.getArgument(0)
-    )
+  }
+
+  /**
+   * A pseudo-property used to store a value on a `URLSearchParams` that 
+   * can be obtained with a `get` or `getAll` call. 
+   */
+  private string hiddenUrlPseudoProperty() {
+    result = "$hiddenSearchPararms"
   }
 
   /**
    * A taint propagating data flow edge arising from URL parameter parsing.
    */
-  private class UrlSearchParamsTaintStep extends AdditionalTaintStep, DataFlow::ValueNode {
-    DataFlow::Node source;
-
+  private class UrlSearchParamsTaintStep extends DataFlow::AdditionalFlowStep {
     UrlSearchParamsTaintStep() {
-      // either this is itself an `URLSearchParams` object
-      isUrlSearchParams(this, source)
+      this = DataFlow::globalVarRef("URL") or
+      this = DataFlow::globalVarRef("URLSearchParams")
+    }
+
+    /**
+     * Holds if `succ` is a `URLSearchParams` providing access to the 
+     * parameters encoded in `pred`. 
+     */ 
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      isUrlSearchParams(succ, pred)
+    }
+
+    /**
+     * Holds if `pred` should be stored in the object `succ` under the property `prop`.
+     * 
+     * This step is used to model 2 facts: 
+     * 1) A `URL` constructed using `url = new URL(input)` transfers taint from `input` to `url.searchParams`. 
+     * 2) A `URLSearchParams` (either `url.searchParams` or `new URLSearchParams(input)`) has a tainted value, 
+     *    which is stored in a pseudo-property, that can later be access using a `get` or `getAll` call. 
+     */
+    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      (prop = "searchParams" or prop = hiddenUrlPseudoProperty()) and
+      exists(DataFlow::NewNode newUrl | succ = newUrl  |
+        newUrl = DataFlow::globalVarRef("URL").getAnInstantiation() and
+        pred = newUrl.getArgument(0)
+      )
       or
-      // or this is a call to `get` or `getAll` on a `URLSearchParams` object
-      exists(DataFlow::SourceNode searchParams, string m |
-        isUrlSearchParams(searchParams, source) and
-        this = searchParams.getAMethodCall(m) and
-        m.matches("get%")
+      prop = hiddenUrlPseudoProperty() and
+      isUrlSearchParams(succ, pred)
+    }
+
+    /**
+     * Holds if the property `prop` should be copied from the object `pred` to the object `succ`.
+     * 
+     * This step is used to copy a value the value of our pseudo-property that can later be accessed using a `get` or `getAll` call. 
+     */
+    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = hiddenUrlPseudoProperty() and
+      exists(DataFlow::PropRead write | write = succ | 
+        write.getPropertyName() = "searchParams" and
+        write.getBase() = pred
       )
     }
 
-    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      pred = source and succ = this
-    }
+   /**
+    * Holds if the property `prop` of the object `pred` should be loaded into `succ`.
+    * 
+    * This step is used to load the value stored in the hidden pseudo-property. 
+    */
+    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) { 
+      prop = hiddenUrlPseudoProperty() and
+      // this is a call to `get` or `getAll` on a `URLSearchParams` object
+      exists(string m, DataFlow::MethodCallNode call | call = succ |
+        call.getMethodName() = m and
+        call.getReceiver() = pred and
+        m.matches("get%")
+      )
+     }
   }
 
   /**

javascript/ql/test/query-tests/Security/CWE-079/Xss.expected

@@ -333,6 +333,10 @@ nodes
 | tst.js:319:35:319:42 | location |
 | tst.js:319:35:319:42 | location |
 | tst.js:319:35:319:42 | location |
+| tst.js:330:18:330:34 | document.location |
+| tst.js:330:18:330:34 | document.location |
+| tst.js:336:18:336:35 | params.get('name') |
+| tst.js:336:18:336:35 | params.get('name') |
 | typeahead.js:20:13:20:45 | target |
 | typeahead.js:20:22:20:38 | document.location |
 | typeahead.js:20:22:20:38 | document.location |
@@ -642,6 +646,10 @@ edges
 | tst.js:313:10:313:10 | e | tst.js:314:20:314:20 | e |
 | tst.js:313:10:313:10 | e | tst.js:314:20:314:20 | e |
 | tst.js:319:35:319:42 | location | tst.js:319:35:319:42 | location |
+| tst.js:330:18:330:34 | document.location | tst.js:336:18:336:35 | params.get('name') |
+| tst.js:330:18:330:34 | document.location | tst.js:336:18:336:35 | params.get('name') |
+| tst.js:330:18:330:34 | document.location | tst.js:336:18:336:35 | params.get('name') |
+| tst.js:330:18:330:34 | document.location | tst.js:336:18:336:35 | params.get('name') |
 | typeahead.js:20:13:20:45 | target | typeahead.js:21:12:21:17 | target |
 | typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search |
 | typeahead.js:20:22:20:38 | document.location | typeahead.js:20:22:20:45 | documen ... .search |
@@ -741,6 +749,7 @@ edges
 | tst.js:306:20:306:20 | e | tst.js:304:9:304:16 | location | tst.js:306:20:306:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:304:9:304:16 | location | user-provided value |
 | tst.js:314:20:314:20 | e | tst.js:311:10:311:17 | location | tst.js:314:20:314:20 | e | Cross-site scripting vulnerability due to $@. | tst.js:311:10:311:17 | location | user-provided value |
 | tst.js:319:35:319:42 | location | tst.js:319:35:319:42 | location | tst.js:319:35:319:42 | location | Cross-site scripting vulnerability due to $@. | tst.js:319:35:319:42 | location | user-provided value |
+| tst.js:336:18:336:35 | params.get('name') | tst.js:330:18:330:34 | document.location | tst.js:336:18:336:35 | params.get('name') | Cross-site scripting vulnerability due to $@. | tst.js:330:18:330:34 | document.location | user-provided value |
 | typeahead.js:25:18:25:20 | val | typeahead.js:20:22:20:38 | document.location | typeahead.js:25:18:25:20 | val | Cross-site scripting vulnerability due to $@. | typeahead.js:20:22:20:38 | document.location | 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 |
 | winjs.js:3:43:3:49 | tainted | winjs.js:2:17:2:33 | document.location | winjs.js:3:43:3:49 | tainted | Cross-site scripting vulnerability due to $@. | winjs.js:2:17:2:33 | document.location | user-provided value |

javascript/ql/test/query-tests/Security/CWE-079/tst.js

@@ -324,4 +324,15 @@ function test2() {
 
   // OK
   $('myId').html(target.length)
+}
+
+function getTaintedUrl() {
+  return new URL(document.location);
+}
+
+function URLPseudoProperties() {
+  // NOT OK
+  let params = getTaintedUrl().searchParams;
+  $('name').html(params.get('name'));
+
 }