refactor the js/xss query to use three flowlabels and one configuration

Author: erik-krogh

javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssCustomizations.qll

@@ -12,4 +12,29 @@ module DomBasedXss {
   class RemoteFlowSourceAsSource extends Source {
     RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
   }
+
+  /**
+   * A flow-label representing tainted values where the prefix is attacker controlled.
+   */
+  class PrefixString extends DataFlow::FlowLabel {
+    PrefixString() { this = "PrefixString" }
+  }
+
+  /** Gets the flow-label representing tainted values where the prefix is attacker controlled. */
+  PrefixString prefixLabel() { any() }
+
+  /**
+   * A sanitizer that blocks the `PrefixString` label when the start of the string is being tested as being of a particular prefix.
+   */
+  class PrefixStringSanitizer extends SanitizerGuard instanceof StringOps::StartsWith {
+    override predicate sanitizes(boolean outcome, Expr e) { none() }
+
+    override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+      super.blocks(outcome, e, label)
+      or
+      e = super.getBaseString().asExpr() and
+      label = prefixLabel() and
+      outcome = super.getPolarity()
+    }
+  }
 }

javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssQuery.qll

@@ -7,65 +7,61 @@ import javascript
 private import semmle.javascript.security.TaintedUrlSuffix
 import DomBasedXssCustomizations::DomBasedXss
 
-/**
- * DEPRECATED. Use `HtmlInjectionConfiguration` or `JQueryHtmlOrSelectorInjectionConfiguration`.
- */
-deprecated class Configuration = HtmlInjectionConfiguration;
-
 /**
  * DEPRECATED. Use `Vue::VHtmlSourceWrite` instead.
  */
 deprecated class VHtmlSourceWrite = Vue::VHtmlSourceWrite;
 
-/**
- * A taint-tracking configuration for reasoning about XSS.
- */
-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 and
-    not sink instanceof JQueryHtmlOrSelectorSink // Handled by JQueryHtmlOrSelectorInjectionConfiguration below
-  }
+/** DEPRECATED. Use `Configuration`. */
+deprecated class HtmlInjectionConfiguration = Configuration;
 
-  override predicate isSanitizer(DataFlow::Node node) {
-    super.isSanitizer(node)
-    or
-    node instanceof Sanitizer
-  }
-
-  override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
-    guard instanceof SanitizerGuard
-  }
+/** DEPRECATED. Use `Configuration`. */
+deprecated class JQueryHtmlOrSelectorInjectionConfiguration = Configuration;
 
-  override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
-    isOptionallySanitizedEdge(pred, succ)
+/**
+ * A sink that is not a URL write or a JQuery selector,
+ * assumed to be a value that is interpreted as HTML.
+ */
+class HTMLSink extends DataFlow::Node instanceof Sink {
+  HTMLSink() {
+    not this instanceof WriteURLSink and
+    not this instanceof JQueryHtmlOrSelectorSink
   }
 }
 
 /**
- * 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.
+ * A taint-tracking configuration for reasoning about XSS.
+ * Both ordinary HTML sinks, URL sinks, and JQuery selector based sinks.
+ * - HTML sinks are sinks for any tainted value
+ * - URL sinks are only sinks when the scheme is user controlled
+ * - JQuery selector sinks are sinks when the tainted value can start with `<`.
  *
- * Values are only considered tainted if they can start with the `<` character.
+ * The above is achieved using three flow labels:
+ * - TaintedUrlSuffix: a URL where the attacker only controls a suffix.
+ * - Taint: a tainted value where the attacker controls part of the value.
+ * - PrefixLabel: a tainted value where the attacker controls the prefix
  */
-class JQueryHtmlOrSelectorInjectionConfiguration extends TaintTracking::Configuration {
-  JQueryHtmlOrSelectorInjectionConfiguration() { this = "JQueryHtmlOrSelectorInjection" }
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "HtmlInjection" }
 
   override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
-    // Reuse any source not derived from location
     source instanceof Source and
-    not source = [DOM::locationRef(), DOM::locationRef().getAPropertyRead()] and
-    label.isTaint()
+    (label.isTaint() or label = prefixLabel()) and
+    not source = TaintedUrlSuffix::source()
     or
-    source = [DOM::locationSource(), DOM::locationRef().getAPropertyRead(["hash", "search"])] and
+    source = TaintedUrlSuffix::source() and
     label = TaintedUrlSuffix::label()
   }
 
   override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
-    sink instanceof JQueryHtmlOrSelectorSink and label.isTaint()
+    sink instanceof HTMLSink and
+    label = [TaintedUrlSuffix::label(), prefixLabel(), DataFlow::FlowLabel::taint()]
+    or
+    sink instanceof JQueryHtmlOrSelectorSink and
+    label = [DataFlow::FlowLabel::taint(), prefixLabel()]
+    or
+    sink instanceof WriteURLSink and
+    label = prefixLabel()
   }
 
   override predicate isSanitizer(DataFlow::Node node) {
@@ -78,6 +74,32 @@ class JQueryHtmlOrSelectorInjectionConfiguration extends TaintTracking::Configur
     guard instanceof SanitizerGuard
   }
 
+  override predicate isLabeledBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+    super.isLabeledBarrier(node, lbl)
+    or
+    // copy all taint barriers to the TaintedUrlSuffix/PrefixLabel label. This copies both the ordinary sanitizers and the sanitizer-guards.
+    super.isLabeledBarrier(node, DataFlow::FlowLabel::taint()) and
+    lbl = [TaintedUrlSuffix::label(), prefixLabel()]
+    or
+    // any non-first string-concatenation leaf is a barrier for the prefix label.
+    exists(StringOps::ConcatenationRoot root |
+      node = root.getALeaf() and
+      not node = root.getFirstLeaf() and
+      lbl = prefixLabel()
+    )
+    or
+    // we assume that `.join()` calls have a prefix, and thus block the prefix label.
+    node = any(DataFlow::MethodCallNode call | call.getMethodName() = "join") and
+    lbl = prefixLabel()
+  }
+
+  override predicate isSanitizerEdge(
+    DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel label
+  ) {
+    isOptionallySanitizedEdge(pred, succ) and
+    label = [DataFlow::FlowLabel::taint(), prefixLabel(), TaintedUrlSuffix::label()]
+  }
+
   override predicate isAdditionalFlowStep(
     DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
   ) {
@@ -89,5 +111,15 @@ class JQueryHtmlOrSelectorInjectionConfiguration extends TaintTracking::Configur
       inlbl = TaintedUrlSuffix::label() and
       outlbl.isTaint()
     )
+    or
+    // inherit all ordinary taint steps for prefixLabel
+    inlbl = prefixLabel() and
+    outlbl = prefixLabel() and
+    TaintTracking::sharedTaintStep(src, trg)
+    or
+    // steps out of taintedSuffixlabel to taint-label are also a steps to prefixLabel.
+    TaintedUrlSuffix::step(src, trg, TaintedUrlSuffix::label(), DataFlow::FlowLabel::taint()) and
+    inlbl = TaintedUrlSuffix::label() and
+    outlbl = prefixLabel()
   }
 }

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

@@ -17,12 +17,7 @@ import semmle.javascript.security.dataflow.DomBasedXssQuery
 import DataFlow::PathGraph
 
 from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
-where
-  (
-    cfg instanceof HtmlInjectionConfiguration or
-    cfg instanceof JQueryHtmlOrSelectorInjectionConfiguration
-  ) and
-  cfg.hasFlowPath(source, sink)
+where cfg.hasFlowPath(source, sink)
 select sink.getNode(), source, sink,
   sink.getNode().(Sink).getVulnerabilityKind() + " vulnerability due to $@.", source.getNode(),
   "user-provided value"