Merge pull request #2761 from erik-krogh/UrlSearch

Approved by asgerf
2025-12-24 04:36:35 +01:00 · 2020-03-31 09:46:48 +01:00
parent 5c920eb625 4864e77430
commit 0feb7f87e4
9 changed files with 232 additions and 31 deletions
--- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -262,6 +262,17 @@ 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()
+  }
 }

 /**
@@ -548,6 +559,19 @@ 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)
+  }
 }

 /**
@@ -666,7 +690,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 three disjuncts taken together over-approximate flow through
    // higher-order calls
    callback(pred, succ) or
@@ -910,14 +934,22 @@ 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, prop)
+  cfg.isAdditionalLoadStoreStep(pred, succ, loadProp, storeProp)
+  or
+  loadProp = storeProp and
+  (
+    any(AdditionalFlowStep s).loadStoreStep(pred, succ, loadProp)
+    or
+    cfg.isAdditionalLoadStoreStep(pred, succ, loadProp)
+  )
 }

 /**
@@ -963,7 +995,7 @@ private predicate reachableFromStoreBase(
        s2.getEndLabel())
  )
  or
-  exists(PathSummary oldSummary, PathSummary newSummary |
+  exists(PathSummary newSummary, PathSummary oldSummary |
    reachableFromStoreBaseStep(prop, rhs, nd, cfg, oldSummary, newSummary) and
    summary = oldSummary.appendValuePreserving(newSummary)
  )
@@ -980,12 +1012,16 @@ private predicate reachableFromStoreBaseStep(
  string prop, DataFlow::Node rhs, DataFlow::Node nd, DataFlow::Configuration cfg,
  PathSummary oldSummary, PathSummary newSummary
 ) {
-  exists(DataFlow::Node mid | reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) |
+  exists(DataFlow::Node mid |
+    reachableFromStoreBase(prop, rhs, mid, cfg, oldSummary) and
    flowStep(mid, cfg, nd, newSummary)
    or
-    isAdditionalLoadStoreStep(mid, nd, prop, cfg) and
+    exists(string midProp |
+      reachableFromStoreBase(midProp, rhs, mid, cfg, oldSummary) and
+      isAdditionalLoadStoreStep(mid, nd, midProp, prop, cfg) and
      newSummary = PathSummary::level()
    )
+  )
 }

 /**
--- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
@@ -558,8 +558,8 @@ 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) {
    exists(DataFlow::GlobalVarRefNode urlSearchParams, NewExpr newUrlSearchParams |
@@ -568,34 +568,92 @@ module TaintTracking {
      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 a `URL` that stores a value that can be obtained
+   * with a `get` or `getAll` call to the `searchParams` property.
+   */
+  private string hiddenUrlPseudoProperty() { result = "$hiddenSearchPararms" }
+
+  /**
+   * A pseudo-property on a `URLSearchParams` that can be obtained
+   * with a `get` or `getAll` call.
+   */
+  private string getableUrlPseudoProperty() { result = "$gettableSearchPararms" }
+
  /**
   * 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, DataFlow::ValueNode {
+    /**
+     * 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) and succ = this
+    }

-    UrlSearchParamsTaintStep() {
-      // either this is itself an `URLSearchParams` object
-      isUrlSearchParams(this, source)
+    /**
+     * Holds if `pred` should be stored in the object `succ` under the property `prop`.
+     *
+     * This step is used to model 3 facts:
+     * 1) A `URL` constructed using `url = new URL(input)` transfers taint from `input` to `url.searchParams`, `url.hash`, and `url.search`.
+     * 2) Accessing the `searchParams` on a `URL` results in a `URLSearchParams` object (See the loadStoreStep method on this class and hiddenUrlPseudoProperty())
+     * 3) A `URLSearchParams` object (either `url.searchParams` or `new URLSearchParams(input)`) has a tainted value,
+     *    which can be accessed using a `get` or `getAll` call. (See getableUrlPseudoProperty())
+     */
+    override predicate storeStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      succ = this and
+      (
+        (
+          prop = "searchParams" or
+          prop = "hash" or
+          prop = "search" 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 = getableUrlPseudoProperty() and
+        isUrlSearchParams(succ, pred)
      )
    }

-    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      pred = source and succ = this
+    /**
+     * Holds if the property `loadStep` should be copied from the object `pred` to the property `storeStep` of object `succ`.
+     *
+     * This step is used to copy the value of our pseudo-property that can later be accessed using a `get` or `getAll` call.
+     * For an expression `url.searchParams`, the property `hiddenUrlPseudoProperty()` from the `url` object is stored in the property `getableUrlPseudoProperty()` on `url.searchParams`.
+     */
+    override predicate loadStoreStep(
+      DataFlow::Node pred, DataFlow::Node succ, string loadProp, string storeProp
+    ) {
+      succ = this and
+      loadProp = hiddenUrlPseudoProperty() and
+      storeProp = getableUrlPseudoProperty() and
+      exists(DataFlow::PropRead read | read = succ |
+        read.getPropertyName() = "searchParams" and
+        read.getBase() = pred
+      )
+    }
+
+    /**
+     * 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 pseudo-property `getableUrlPseudoProperty()`.
+     */
+    override predicate loadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      succ = this and
+      prop = getableUrlPseudoProperty() 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%")
+      )
    }
  }

--- a/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll
@@ -23,5 +23,31 @@ module DomBasedXss {
      or
      node instanceof Sanitizer
    }
+
+    override predicate isAdditionalLoadStoreStep(
+      DataFlow::Node pred, DataFlow::Node succ, string predProp, string succProp
+    ) {
+      exists(DataFlow::PropRead read |
+        pred = read.getBase() and
+        succ = read and
+        read.getPropertyName() = "hash" and
+        predProp = "hash" and
+        succProp = urlSuffixPseudoProperty()
+      )
    }
+
+    override predicate isAdditionalLoadStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      exists(DataFlow::MethodCallNode call, string name |
+        name = "substr" or name = "substring" or name = "slice"
+      |
+        call.getMethodName() = name and
+        not call.getArgument(0).getIntValue() = 0 and
+        pred = call.getReceiver() and
+        succ = call and
+        prop = urlSuffixPseudoProperty()
+      )
+    }
+  }
+
+  private string urlSuffixPseudoProperty() { result = "$UrlSuffix$" }
 }
--- a/javascript/ql/test/library-tests/CustomLoadStoreSteps/test.expected
+++ b/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 |
--- a/javascript/ql/test/library-tests/CustomLoadStoreSteps/test.ql
+++ b/javascript/ql/test/library-tests/CustomLoadStoreSteps/test.ql
@@ -19,6 +19,18 @@ 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
--- a/javascript/ql/test/library-tests/CustomLoadStoreSteps/tst.js
+++ b/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. 
+	
 })();
--- a/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/Xss.expected
@@ -333,6 +333,14 @@ 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') |
+| tst.js:347:20:347:36 | document.location |
+| tst.js:347:20:347:36 | document.location |
+| tst.js:349:5:349:30 | getUrl( ... ring(1) |
+| tst.js:349:5:349:30 | getUrl( ... ring(1) |
 | typeahead.js:20:13:20:45 | target |
 | typeahead.js:20:22:20:38 | document.location |
 | typeahead.js:20:22:20:38 | document.location |
@@ -643,6 +651,14 @@ 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') |
+| tst.js:347:20:347:36 | document.location | tst.js:349:5:349:30 | getUrl( ... ring(1) |
+| tst.js:347:20:347:36 | document.location | tst.js:349:5:349:30 | getUrl( ... ring(1) |
+| tst.js:347:20:347:36 | document.location | tst.js:349:5:349:30 | getUrl( ... ring(1) |
+| tst.js:347:20:347:36 | document.location | tst.js:349:5:349:30 | getUrl( ... ring(1) |
 | 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 |
@@ -742,6 +758,8 @@ 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 |
+| tst.js:349:5:349:30 | getUrl( ... ring(1) | tst.js:347:20:347:36 | document.location | tst.js:349:5:349:30 | getUrl( ... ring(1) | Cross-site scripting vulnerability due to $@. | tst.js:347:20:347:36 | 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 |
--- a/javascript/ql/test/query-tests/Security/CWE-079/XssWithAdditionalSources.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-079/XssWithAdditionalSources.expected
@@ -333,6 +333,14 @@ 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') |
+| tst.js:347:20:347:36 | document.location |
+| tst.js:347:20:347:36 | document.location |
+| tst.js:349:5:349:30 | getUrl( ... ring(1) |
+| tst.js:349:5:349:30 | getUrl( ... ring(1) |
 | typeahead.js:9:28:9:30 | loc |
 | typeahead.js:9:28:9:30 | loc |
 | typeahead.js:10:16:10:18 | loc |
@@ -647,6 +655,14 @@ 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') |
+| tst.js:347:20:347:36 | document.location | tst.js:349:5:349:30 | getUrl( ... ring(1) |
+| tst.js:347:20:347:36 | document.location | tst.js:349:5:349:30 | getUrl( ... ring(1) |
+| tst.js:347:20:347:36 | document.location | tst.js:349:5:349:30 | getUrl( ... ring(1) |
+| tst.js:347:20:347:36 | document.location | tst.js:349:5:349:30 | getUrl( ... ring(1) |
 | 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 |
--- a/javascript/ql/test/query-tests/Security/CWE-079/tst.js
+++ b/javascript/ql/test/query-tests/Security/CWE-079/tst.js
@@ -325,3 +325,27 @@ 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'));
+
+  // OK (.get is not defined on a URL)
+  let myUrl = getTaintedUrl();
+  $('name').html(myUrl.get('name'));
+
+}
+
+
+function hash() {
+  function getUrl() {
+    return new URL(document.location);
+  }
+  $(getUrl().hash.substring(1)); // NOT OK
+
+}