Merge pull request #8304 from erik-krogh/xssUrl

JS: Refactor the XSS / Client-side-url queries

Author: erik-krogh

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

@@ -26,6 +26,15 @@ module TaintedUrlSuffix {
    */
   FlowLabel label() { result instanceof TaintedUrlSuffixLabel }
 
+  /** Gets a remote flow source that is a tainted URL query or fragment part from `window.location`. */
+  ClientSideRemoteFlowSource source() {
+    result = DOM::locationRef().getAPropertyRead(["search", "hash"])
+    or
+    result = DOM::locationSource()
+    or
+    result.getKind().isUrl()
+  }
+
   /** 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.

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

@@ -8,8 +8,6 @@ import javascript
 import semmle.javascript.security.dataflow.RemoteFlowSources
 
 module ClientSideUrlRedirect {
-  private import Xss::DomBasedXss as DomBasedXss
-
   /**
    * A data flow source for unvalidated URL redirect vulnerabilities.
    */
@@ -21,7 +19,12 @@ module ClientSideUrlRedirect {
   /**
    * A data flow sink for unvalidated URL redirect vulnerabilities.
    */
-  abstract class Sink extends DataFlow::Node { }
+  abstract class Sink extends DataFlow::Node {
+    /** Holds if the sink can execute JavaScript code in the current context. */
+    predicate isXssSink() {
+      none() // overwritten in subclasses
+    }
+  }
 
   /**
    * A sanitizer for unvalidated URL redirect vulnerabilities.
@@ -86,37 +89,46 @@ module ClientSideUrlRedirect {
    * A sink which is used to set the window location.
    */
   class LocationSink extends Sink, DataFlow::ValueNode {
+    boolean xss;
+
     LocationSink() {
       // A call to a `window.navigate` or `window.open`
       exists(string name | name = ["navigate", "open", "openDialog", "showModalDialog"] |
         this = DataFlow::globalVarRef(name).getACall().getArgument(0)
-      )
+      ) and
+      xss = false
       or
       // A call to `location.replace` or `location.assign`
       exists(DataFlow::MethodCallNode locationCall, string name |
         locationCall = DOM::locationRef().getAMethodCall(name) and
         this = locationCall.getArgument(0)
       |
         name = ["replace", "assign"]
-      )
+      ) and
+      xss = true
       or
       // An assignment to `location`
-      exists(Assignment assgn | isLocation(assgn.getTarget()) and astNode = assgn.getRhs())
+      exists(Assignment assgn | isLocation(assgn.getTarget()) and astNode = assgn.getRhs()) and
+      xss = true
       or
       // An assignment to `location.href`, `location.protocol` or `location.hostname`
       exists(DataFlow::PropWrite pw, string propName |
         pw = DOM::locationRef().getAPropertyWrite(propName) and
         this = pw.getRhs()
       |
-        propName = ["href", "protocol", "hostname"]
+        propName = ["href", "protocol", "hostname"] and
+        (if propName = "href" then xss = true else xss = false)
       )
       or
       // A redirection using the AngularJS `$location` service
       exists(AngularJS::ServiceReference service |
         service.getName() = "$location" and
         this.asExpr() = service.getAMethodCall("url").getArgument(0)
-      )
+      ) and
+      xss = false
     }
+
+    override predicate isXssSink() { xss = true }
   }
 
   /**
@@ -156,16 +168,23 @@ module ClientSideUrlRedirect {
   }
 
   /**
-   * A script or iframe `src` attribute, viewed as a `ScriptUrlSink`.
+   * A write to a `href` or similar attribute viewed as a `ScriptUrlSink`.
    */
-  class SrcAttributeUrlSink extends ScriptUrlSink, DataFlow::ValueNode {
-    SrcAttributeUrlSink() {
+  class AttributeUrlSink extends ScriptUrlSink {
+    AttributeUrlSink() {
+      // e.g. `$("<a>", {href: sink}).appendTo("body")`
       exists(DOM::AttributeDefinition attr |
-        attr.getElement().getName() = ["script", "iframe"] and
-        attr.getName() = "src" and
+        not attr instanceof JsxAttribute and // handled more precisely in `ReactAttributeWriteUrlSink`.
+        attr.getName() = DOM::getAPropertyNameInterpretedAsJavaScriptUrl()
+      |
         this = attr.getValueNode()
       )
+      or
+      // e.g. node.setAttribute("href", sink)
+      any(DomMethodCallExpr call).interpretsArgumentsAsURL(this.asExpr())
     }
+
+    override predicate isXssSink() { any() }
   }
 
   /**
@@ -179,6 +198,8 @@ module ClientSideUrlRedirect {
         this = DataFlow::valueNode(pw.getRhs())
       )
     }
+
+    override predicate isXssSink() { any() }
   }
 
   /**
@@ -195,6 +216,8 @@ module ClientSideUrlRedirect {
         this = attr.getValue().flow()
       )
     }
+
+    override predicate isXssSink() { any() }
   }
 
   /**

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

@@ -94,7 +94,17 @@ class DomMethodCallExpr extends MethodCallExpr {
       name = "createElement" and argPos = 0
       or
       name = "appendChild" and argPos = 0
-      or
+    )
+  }
+
+  /**
+   * Holds if `arg` is an argument that is used as an URL.
+   */
+  predicate interpretsArgumentsAsURL(Expr arg) {
+    exists(int argPos, string name |
+      arg = this.getArgument(argPos) and
+      name = this.getMethodName()
+    |
       (
         name = "setAttribute" and argPos = 1
         or

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

@@ -12,4 +12,14 @@ 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() }
 }

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
-  }
-
-  override predicate isSanitizer(DataFlow::Node node) {
-    super.isSanitizer(node)
-    or
-    node instanceof Sanitizer
-  }
+/** DEPRECATED. Use `Configuration`. */
+deprecated class HtmlInjectionConfiguration = Configuration;
 
-  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,26 @@ 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()
+  }
+}
+
+/**
+ * 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, TaintTracking::LabeledSanitizerGuardNode instanceof StringOps::StartsWith {
+  override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+    e = super.getBaseString().asExpr() and
+    label = prefixLabel() and
+    outcome = super.getPolarity()
   }
 }

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

@@ -253,6 +253,15 @@ module DomBasedXss {
     }
   }
 
+  import ClientSideUrlRedirectCustomizations::ClientSideUrlRedirect as ClientSideUrlRedirect
+
+  /**
+   * A write to a URL which may execute JavaScript code.
+   */
+  class WriteURLSink extends Sink instanceof ClientSideUrlRedirect::Sink {
+    WriteURLSink() { super.isXssSink() }
+  }
+
   /**
    * An expression whose value is interpreted as HTML or CSS
    * and may be inserted into the DOM.
@@ -347,7 +356,7 @@ module DomBasedXss {
   /**
    * A write to the `template` option of a Vue instance, viewed as an XSS sink.
    */
-  class VueTemplateSink extends DomBasedXss::Sink {
+  class VueTemplateSink extends Sink {
     VueTemplateSink() {
       // Note: don't use Vue::Component#getTemplate as it includes an unwanted getALocalSource() step
       this = any(Vue::Component c).getOption("template")
@@ -358,7 +367,7 @@ module DomBasedXss {
    * The tag name argument to the `createElement` parameter of the
    * `render` method of a Vue instance, viewed as an XSS sink.
    */
-  class VueCreateElementSink extends DomBasedXss::Sink {
+  class VueCreateElementSink extends Sink {
     VueCreateElementSink() {
       exists(Vue::Component c, DataFlow::FunctionNode f |
         f.flowsTo(c.getRender()) and
@@ -370,12 +379,12 @@ module DomBasedXss {
   /**
    * A Vue `v-html` attribute, viewed as an XSS sink.
    */
-  class VHtmlSink extends Vue::VHtmlAttribute, DomBasedXss::Sink { }
+  class VHtmlSink extends Vue::VHtmlAttribute, Sink { }
 
   /**
    * A raw interpolation tag in a template file, viewed as an XSS sink.
    */
-  class TemplateSink extends DomBasedXss::Sink {
+  class TemplateSink extends Sink {
     TemplateSink() {
       exists(Templating::TemplatePlaceholderTag tag |
         tag.isRawInterpolation() and
@@ -388,7 +397,7 @@ module DomBasedXss {
    * A value being piped into the `safe` pipe in a template file,
    * disabling subsequent HTML escaping.
    */
-  class SafePipe extends DomBasedXss::Sink {
+  class SafePipe extends Sink {
     SafePipe() { this = Templating::getAPipeCall("safe").getArgument(0) }
   }