JS: Move $() sink into separate dataflow config

javascript/ql/src/Security/CWE-079/Xss.ql

@@ -15,8 +15,13 @@ import javascript
 import semmle.javascript.security.dataflow.DomBasedXss::DomBasedXss
 import DataFlow::PathGraph
 
-from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
-where cfg.hasFlowPath(source, sink)
+from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where
+  (
+    cfg instanceof HtmlInjectionConfiguration or
+    cfg instanceof JQuerySelectorInjectionConfiguration
+  ) and
+  cfg.hasFlowPath(source, sink)
 select sink.getNode(), source, sink,
   sink.getNode().(Sink).getVulnerabilityKind() + " vulnerability due to $@.", source.getNode(),
   "user-provided value"

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

@@ -8,15 +8,23 @@ import javascript
 module DomBasedXss {
   import DomBasedXssCustomizations::DomBasedXss
 
+  /**
+   * DEPRECATED. Use `HtmlInjectionConfiguration` or `JQuerySelectorInjectionConfiguration`.
+   */
+  deprecated class Configuration = HtmlInjectionConfiguration;
+
   /**
    * A taint-tracking configuration for reasoning about XSS.
    */
-  class Configuration extends TaintTracking::Configuration {
-    Configuration() { this = "DomBasedXss" }
+  class HtmlInjectionConfiguration extends TaintTracking::Configuration {
+    HtmlInjectionConfiguration() { this = "HtmlInjection" }
 
     override predicate isSource(DataFlow::Node source) { source instanceof Source }
 
-    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+    override predicate isSink(DataFlow::Node sink) {
+      sink instanceof Sink and
+      not sink instanceof JQuerySelectorSink // Handled by JQuerySelectorInjectionConfiguration below
+    }
 
     override predicate isSanitizer(DataFlow::Node node) {
       super.isSanitizer(node)
@@ -28,59 +36,68 @@ module DomBasedXss {
       guard instanceof SanitizerGuard
     }
 
-    override predicate isAdditionalStoreStep(
-      DataFlow::Node pred, DataFlow::SourceNode succ, string prop
-    ) {
-      exists(DataFlow::PropRead read |
-        pred = read.getBase() and
-        succ = read and
-        read.getPropertyName() = "hash" and
-        prop = urlSuffixPseudoProperty()
-      )
-    }
-
-    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 |
-        call.getMethodName() = ["substr", "substring", "slice"] and
-        not call.getArgument(0).getIntValue() = 0 and
-        pred = call.getReceiver() and
-        succ = call and
-        prop = urlSuffixPseudoProperty()
-      )
-      or
-      exists(DataFlow::MethodCallNode call |
-        call.getMethodName() = "exec" and pred = call.getArgument(0)
-        or
-        call.getMethodName() = "match" and pred = call.getReceiver()
-      |
-        succ = call and
-        prop = urlSuffixPseudoProperty()
-      )
-      or
-      exists(StringSplitCall split |
-        split.getSeparator() = ["#", "?"] and
-        pred = split.getBaseString() and
-        succ = split.getASubstringRead(1) and
-        prop = urlSuffixPseudoProperty()
-      )
-    }
-
     override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
       DomBasedXss::isOptionallySanitizedEdge(pred, succ)
     }
   }
 
-  private string urlSuffixPseudoProperty() { result = "$UrlSuffix$" }
+  /**
+   * A taint-tracking configuration for reasoning about injection into the jQuery `$` function
+   * or similar, where the interpretation of the input string depends on its first character.
+   *
+   * Values are only considered tainted if they can start with the `<` character.
+   */
+  class JQuerySelectorInjectionConfiguration extends TaintTracking::Configuration {
+    JQuerySelectorInjectionConfiguration() { this = "JQuerySelectorInjection" }
+
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      // Reuse any source not derived from location
+      source instanceof Source and
+      not source = DOM::locationRef() 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
+    }
+
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      sink instanceof JQuerySelectorSink and label.isTaint()
+    }
+
+    override predicate isAdditionalFlowStep(
+      DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
+      DataFlow::FlowLabel succlbl
+    ) {
+      exists(TaintTracking::AdditionalTaintStep step |
+        step.step(pred, succ) and
+        predlbl.isData() and
+        succlbl.isTaint()
+      )
+    }
+
+    override predicate isSanitizer(DataFlow::Node node) {
+      super.isSanitizer(node)
+      or
+      node instanceof Sanitizer
+    }
+
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+      guard instanceof SanitizerGuard
+    }
+
+    override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
+      DomBasedXss::isOptionallySanitizedEdge(pred, succ)
+      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
+      )
+    }
+  }
 }

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

@@ -153,19 +153,9 @@ module DomBasedXss {
   class LibrarySink extends Sink, DataFlow::ValueNode {
     LibrarySink() {
       // call to a jQuery method that interprets its argument as HTML
-      exists(JQuery::MethodCall call | call.interpretsArgumentAsHtml(this) |
-        // either the argument is always interpreted as HTML
-        not call.interpretsArgumentAsSelector(this)
-        or
-        // or it doesn't start with something other than `<`, and so at least
-        // _may_ be interpreted as HTML
-        not exists(DataFlow::Node prefix, string strval |
-          isPrefixOfJQueryHtmlString(this, prefix) and
-          strval = prefix.getStringValue() and
-          not strval = "" and
-          not strval.regexpMatch("\\s*<.*")
-        ) and
-        not DOM::locationRef().flowsTo(this)
+      exists(JQuery::MethodCall call |
+        call.interpretsArgumentAsHtml(this) and
+        not call.interpretsArgumentAsSelector(this) // Handled by `JQuerySelectorSink`
       )
       or
       // call to an Angular method that interprets its argument as HTML
@@ -192,16 +182,41 @@ module DomBasedXss {
    * HTML by a jQuery method.
    */
   predicate isPrefixOfJQueryHtmlString(DataFlow::Node htmlString, DataFlow::Node prefix) {
-    any(JQuery::MethodCall call).interpretsArgumentAsHtml(htmlString) and
-    prefix = htmlString
+    prefix = getAPrefixOfJQuerySelectorString(htmlString)
+  }
+
+  /**
+   * Holds if `prefix` is a prefix of `htmlString`, which may be intepreted as
+   * HTML by a jQuery method.
+   */
+  private DataFlow::Node getAPrefixOfJQuerySelectorString(DataFlow::Node htmlString) {
+    any(JQuery::MethodCall call).interpretsArgumentAsSelector(htmlString) and
+    result = htmlString
     or
-    exists(DataFlow::Node pred | isPrefixOfJQueryHtmlString(htmlString, pred) |
-      prefix = StringConcatenation::getFirstOperand(pred)
+    exists(DataFlow::Node pred | pred = getAPrefixOfJQuerySelectorString(htmlString) |
+      result = StringConcatenation::getFirstOperand(pred)
       or
-      prefix = pred.getAPredecessor()
+      result = pred.getAPredecessor()
     )
   }
 
+  /**
+   * An argument to the jQuery `$` function, which is interpreted as either a selector
+   * or as an HTML string depending on its first character.
+   */
+  class JQuerySelectorSink extends Sink {
+    JQuerySelectorSink() {
+      exists(JQuery::MethodCall call |
+        call.interpretsArgumentAsHtml(this) and
+        call.interpretsArgumentAsSelector(this) and
+        // If a prefix of the string is known, it must start with '<' or be an empty string
+        forall(string strval | strval = getAPrefixOfJQuerySelectorString(this).getStringValue() |
+          strval.regexpMatch("(?s)\\s*<.*|")
+        )
+      )
+    }
+  }
+
   /**
    * An expression whose value is interpreted as HTML or CSS
    * and may be inserted into the DOM.
@@ -350,11 +365,6 @@ module DomBasedXss {
       exists(PropAccess pacc | pacc = this.asExpr() |
         isSafeLocationProperty(pacc)
         or
-        // `$(location.hash)` is a fairly common and safe idiom
-        // (because `location.hash` always starts with `#`),
-        // so we mark `hash` as safe for the purposes of this query
-        pacc.getPropertyName() = "hash"
-        or
         pacc.getPropertyName() = "length"
       )
     }