Merge pull request #4506 from asgerf/js/separate-jquery-config

Approved by esbena

Author: codeql-ci

change-notes/1.26/analysis-javascript.md

@@ -53,7 +53,9 @@
 | Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | More results | This query now recognizes some unsafe uses of `importScripts()` inside WebWorkers. |
 | Missing CSRF middleware (`js/missing-token-validation`) | More results | This query now recognizes writes to cookie and session variables as potentially vulnerable to CSRF attacks. |
 | Missing CSRF middleware (`js/missing-token-validation`) | Fewer results | This query now recognizes more ways of protecting against CSRF attacks. |
+| Client-side cross-site scripting (`js/xss`) | More results | This query now tracks data flow from `location.hash` more precisely. |
 
 
 ## Changes to libraries
 * The predicate `TypeAnnotation.hasQualifiedName` now works in more cases when the imported library was not present during extraction.
+* The class `DomBasedXss::Configuration` has been deprecated, as it has been split into `DomBasedXss::HtmlInjectionConfiguration` and `DomBasedXss::JQueryHtmlOrSelectorInjectionConfiguration`. Unless specifically working with jQuery sinks, subclasses should instead be based on `HtmlInjectionConfiguration`. To use both configurations in a query, see [Xss.ql](https://github.com/github/codeql/blob/main/javascript/ql/src/Security/CWE-079/Xss.ql) for an example.

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 JQueryHtmlOrSelectorInjectionConfiguration
+  ) 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 `JQueryHtmlOrSelectorInjectionConfiguration`.
+   */
+  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 JQueryHtmlOrSelectorSink // Handled by JQueryHtmlOrSelectorInjectionConfiguration 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 isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
+      DomBasedXss::isOptionallySanitizedEdge(pred, succ)
+    }
+  }
+
+  /**
+   * 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 JQueryHtmlOrSelectorInjectionConfiguration extends TaintTracking::Configuration {
+    JQueryHtmlOrSelectorInjectionConfiguration() { this = "JQueryHtmlOrSelectorInjection" }
+
+    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 JQueryHtmlOrSelectorSink and label.isTaint()
     }
 
-    override predicate isAdditionalLoadStoreStep(
-      DataFlow::Node pred, DataFlow::Node succ, string predProp, string succProp
+    override predicate isAdditionalFlowStep(
+      DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel predlbl,
+      DataFlow::FlowLabel succlbl
     ) {
-      exists(DataFlow::PropRead read |
-        pred = read.getBase() and
-        succ = read and
-        read.getPropertyName() = "hash" and
-        predProp = "hash" and
-        succProp = urlSuffixPseudoProperty()
+      exists(TaintTracking::AdditionalTaintStep step |
+        step.step(pred, succ) and
+        predlbl.isData() and
+        succlbl.isTaint()
       )
     }
 
-    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()
-      )
+    override predicate isSanitizer(DataFlow::Node node) {
+      super.isSanitizer(node)
       or
-      exists(StringSplitCall split |
-        split.getSeparator() = ["#", "?"] and
-        pred = split.getBaseString() and
-        succ = split.getASubstringRead(1) and
-        prop = urlSuffixPseudoProperty()
-      )
+      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
+      )
     }
   }
-
-  private string urlSuffixPseudoProperty() { result = "$UrlSuffix$" }
 }

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

@@ -34,18 +34,11 @@ module UnsafeJQueryPlugin {
   /**
    * An argument that may act as a HTML fragment rather than a CSS selector, as a sink for remote unsafe jQuery plugins.
    */
-  class AmbiguousHtmlOrSelectorArgument extends DataFlow::Node {
+  class AmbiguousHtmlOrSelectorArgument extends DataFlow::Node,
+    DomBasedXss::JQueryHtmlOrSelectorArgument {
     AmbiguousHtmlOrSelectorArgument() {
-      exists(JQuery::MethodCall call |
-        call.interpretsArgumentAsSelector(this) and call.interpretsArgumentAsHtml(this)
-      ) and
-      // the $-function in particular will not construct HTML for non-string values
-      analyze().getAType() = TTString() and
       // any fixed prefix makes the call unambiguous
-      not exists(DataFlow::Node prefix |
-        DomBasedXss::isPrefixOfJQueryHtmlString(this, prefix) and
-        prefix.mayHaveStringValue(_)
-      )
+      not exists(getAPrefix())
     }
   }
 

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

@@ -3,6 +3,7 @@
  */
 
 import javascript
+private import semmle.javascript.dataflow.InferredTypes
 
 /** Provides classes and predicates shared between the XSS queries. */
 module Shared {
@@ -153,19 +154,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 +183,54 @@ 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 or similar, which is interpreted as either a selector
+   * or as an HTML string depending on its first character.
+   */
+  class JQueryHtmlOrSelectorArgument extends DataFlow::Node {
+    JQueryHtmlOrSelectorArgument() {
+      exists(JQuery::MethodCall call |
+        call.interpretsArgumentAsHtml(this) and
+        call.interpretsArgumentAsSelector(this) and
+        analyze().getAType() = TTString()
+      )
+    }
+
+    /** Gets a string that flows to the prefix of this argument. */
+    string getAPrefix() { result = getAPrefixOfJQuerySelectorString(this).getStringValue() }
+  }
+
+  /**
+   * An argument to the jQuery `$` function or similar, which may be interpreted as HTML.
+   *
+   * This is the same as `JQueryHtmlOrSelectorArgument`, excluding cases where the value
+   * is prefixed by something other than `<`.
+   */
+  class JQueryHtmlOrSelectorSink extends Sink, JQueryHtmlOrSelectorArgument {
+    JQueryHtmlOrSelectorSink() {
+      // If a prefix of the string is known, it must start with '<' or be an empty string
+      forall(string strval | strval = getAPrefix() | strval.regexpMatch("(?s)\\s*<.*|"))
+    }
+  }
+
   /**
    * An expression whose value is interpreted as HTML or CSS
    * and may be inserted into the DOM.
@@ -350,11 +379,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"
       )
     }