JS: Use TaintedUrlSuffix flow label in jQuery xss

Author: asgerf

javascript/ql/src/semmle/javascript/StringConcatenation.qll

@@ -34,7 +34,7 @@ module StringConcatenation {
     or
     exists(DataFlow::ArrayCreationNode array, DataFlow::MethodCallNode call |
       call = array.getAMethodCall("join") and
-      call.getArgument(0).mayHaveStringValue("") and
+      (call.getArgument(0).mayHaveStringValue("") or call.getNumArgument() = 0) and
       (
         // step from array element to array
         result = array.getElement(n) and

javascript/ql/src/semmle/javascript/security/TaintedUrlSuffix.qll

@@ -0,0 +1,111 @@
+/**
+ * Provides a flow label for reasoning about URLs with a tainted query and fragment part,
+ * which we collectively refer to as the "suffix" of the URL.
+ */
+import javascript
+
+/**
+ * Provides a flow label for reasoning about URLs with a tainted query and fragment part,
+ * which we collectively refer to as the "suffix" of the URL.
+ */
+module TaintedUrlSuffix {
+  private import DataFlow
+
+  /**
+   * The flow label representing a URL with a tainted query and fragment part.
+   *
+   * Can also be accessed using `TaintedUrlSuffix::label()`.
+   */
+  class TaintedUrlSuffixLabel extends FlowLabel {
+    TaintedUrlSuffixLabel() {
+      this = "tainted-url-suffix"
+    }
+  }
+
+  /**
+   * Gets the flow label representing a URL with a tainted query and fragment part.
+   */
+  FlowLabel label() { result instanceof TaintedUrlSuffixLabel }
+
+  /** Holds for `pred -> succ` is a step of form `x -> x.p` */
+  private predicate isSafeLocationProp(DataFlow::PropRead read) {
+    // Ignore properties that refer to the scheme, domain, port, auth, or path.
+    exists (string name | name = read.getPropertyName() |
+      name = "protocol" or
+      name = "scheme" or
+      name = "host" or
+      name = "hostname" or
+      name = "domain" or
+      name = "origin" or
+      name = "port" or
+      name = "path" or
+      name = "pathname" or
+      name = "username" or
+      name = "password" or
+      name = "auth"
+    )
+  }
+
+  /**
+   * Holds if there is a flow step `src -> dst` involving the URL suffix taint label.
+   *
+   * This handles steps through string operations, promises, URL parsers, and URL accessors.
+   */
+  predicate step(Node src, Node dst, FlowLabel srclbl, FlowLabel dstlbl) {
+    // Inherit all ordinary taint steps except `x -> x.p` steps
+    srclbl = label() and
+    dstlbl = label() and
+    TaintTracking::sharedTaintStep(src, dst) and
+    not isSafeLocationProp(dst)
+    or
+    // Transition from URL suffix to full taint when extracting the query/fragment part.
+    srclbl = label() and
+    dstlbl.isTaint() and
+    (
+      exists(MethodCallNode call, string name |
+        src = call.getReceiver() and
+        dst = call and
+        name = call.getMethodName()
+      |
+        // Substring that is not a prefix
+        name = ["substring", "substr", "slice"] and
+        not call.getArgument(0).getIntValue() = 0
+        or
+        // Split around '#' or '?' and extract the suffix
+        name = "split" and
+        call.getArgument(0).getStringValue() = ["#", "?"] and
+        not exists(call.getAPropertyRead("0")) // Avoid false flow to the prefix
+        or
+        // Replace '#' and '?' with nothing
+        name = "replace" and
+        call.getArgument(0).getStringValue() = ["#", "?"] and
+        call.getArgument(1).getStringValue() = ""
+        or
+        // The `get` call in `url.searchParams.get(x)` and `url.hashParams.get(x)`
+        // The step should be safe since nothing else reachable by this flow label supports a method named 'get'.
+        name = "get"
+        or
+        // Methods on URL objects from the Closure library
+        name = "getDecodedQuery" or
+        name = "getFragment" or
+        name = "getParameterValue" or
+        name = "getParameterValues" or
+        name = "getQueryData"
+      )
+      or
+      exists(PropRead read |
+        src = read.getBase() and
+        dst = read and
+        // Unlike the `search` property, the `query` property from `url.parse` does not include the `?`.
+        read.getPropertyName() = "query"
+      )
+      or
+      // Assume calls to regexp.exec always extract query/fragment parameters.
+      exists(MethodCallNode call |
+        call = any(RegExpLiteral re).flow().(DataFlow::SourceNode).getAMethodCall("exec") and
+        src = call.getArgument(0) and
+        dst = call
+      )
+    )
+  }
+}

javascript/ql/src/semmle/javascript/security/dataflow/DomBasedXss.qll

@@ -4,6 +4,7 @@
  */
 
 import javascript
+private import semmle.javascript.security.TaintedUrlSuffix
 
 module DomBasedXss {
   import DomBasedXssCustomizations::DomBasedXss
@@ -61,26 +62,14 @@ module DomBasedXss {
       not source = [DOM::locationRef(), DOM::locationRef().getAPropertyRead()] and
       label.isTaint()
       or
-      source = DOM::locationSource() and
-      label.isData() // Require transformation before reaching sink
-      or
-      source = DOM::locationRef().getAPropertyRead(["hash", "search"]) and
-      label.isData() // Require transformation before reaching sink
+      source = [DOM::locationSource(), DOM::locationRef().getAPropertyRead(["hash", "search"])] and
+      label = TaintedUrlSuffix::label()
     }
 
     override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
       sink instanceof JQueryHtmlOrSelectorSink and label.isTaint()
     }
 
-    override predicate isAdditionalFlowStep(
-      DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
-      DataFlow::FlowLabel succlbl
-    ) {
-      TaintTracking::sharedTaintStep(pred, succ) and
-      predlbl.isData() and
-      succlbl.isTaint()
-    }
-
     override predicate isSanitizer(DataFlow::Node node) {
       super.isSanitizer(node)
       or
@@ -91,15 +80,14 @@ module DomBasedXss {
       guard instanceof SanitizerGuard
     }
 
-    override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
-      DomBasedXss::isOptionallySanitizedEdge(pred, succ)
+    override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl) {
+      TaintedUrlSuffix::step(src, trg, inlbl, outlbl)
       or
-      // Avoid stepping from location -> location.hash, as the .hash is already treated as a source
-      // (with a different flow label)
-      exists(DataFlow::PropRead read |
-        read = DOM::locationRef().getAPropertyRead(["hash", "search"]) and
-        pred = read.getBase() and
-        succ = read
+      exists(DataFlow::Node operator |
+        StringConcatenation::taintStep(src, trg, operator, _) and
+        StringConcatenation::getOperand(operator, 0).getStringValue() = "<" + any(string s) and
+        inlbl = TaintedUrlSuffix::label() and
+        outlbl.isTaint()
       )
     }
   }