Merge pull request #3191 from erik-krogh/XssDom

Approved by esbena, mchammer01
github · Apr 23, 2020 · da32926 · da32926
2 parents f696594 + ac26741
commit da32926
Show file tree

Hide file tree

Showing 12 changed files with 389 additions and 6 deletions.
diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md
@@ -7,7 +7,7 @@
 
 | **Query**                                                                       | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
 |---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-
+| Cross-site scripting through DOM (`js/xss-through-dom`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities where existing text from the DOM is used as HTML. Results are not shown on LGTM by default. |
 
 ## Changes to existing queries
 

diff --git a/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp
@@ -0,0 +1,70 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Extracting text from a DOM node and interpreting it as HTML can lead to a cross-site scripting vulnerability.
+</p>
+<p>
+A webpage with this vulnerability reads text from the DOM, and afterwards adds the text as HTML to the DOM.
+Using text from the DOM as HTML effectively unescapes the text, and thereby invalidates any escaping done on the text. 
+If an attacker is able to control the safe sanitized text, then this vulnerability can be exploited to perform a cross-site scripting attack. 
+</p>
+</overview>
+
+<recommendation>
+<p>
+To guard against cross-site scripting, consider using contextual output encoding/escaping before
+writing text to the page, or one of the other solutions that are mentioned in the References section below.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows a webpage using a <code>data-target</code> attribute 
+to select and manipulate a DOM element using the JQuery library. In the example, the 
+<code>data-target</code> attribute is read into the <code>target</code> variable, and the 
+<code>$</code> function is then supposed to use the <code>target</code> variable as a CSS 
+selector to determine which element should be manipulated.
+</p>
+<sample src="examples/XssThroughDom.js" />
+<p>
+However, if an attacker can control the <code>data-target</code> attribute, 
+then the value of <code>target</code> can be used to cause the <code>$</code> function
+to execute arbitary JavaScript. 
+</p>
+<p>
+The above vulnerability can be fixed by using <code>$.find</code> instead of <code>$</code>. 
+The <code>$.find</code> function will only interpret <code>target</code> as a CSS selector 
+and never as HTML, thereby preventing an XSS attack. 
+</p>
+<sample src="examples/XssThroughDomFixed.js" />
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://cheatsheetseries.owasp.org/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.html">DOM based
+XSS Prevention Cheat Sheet</a>.
+</li>
+<li>
+OWASP:
+<a href="https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html">XSS
+(Cross Site Scripting) Prevention Cheat Sheet</a>.
+</li>
+<li>
+OWASP
+<a href="https://owasp.org/www-community/attacks/DOM_Based_XSS">DOM Based XSS</a>.
+</li>
+<li>
+OWASP
+<a href="https://owasp.org/www-community/Types_of_Cross-Site_Scripting">Types of Cross-Site
+Scripting</a>.
+</li>
+<li>
+Wikipedia: <a href="http://en.wikipedia.org/wiki/Cross-site_scripting">Cross-site scripting</a>.
+</li>
+</references>
+</qhelp>
diff --git a/javascript/ql/src/Security/CWE-079/XssThroughDom.ql b/javascript/ql/src/Security/CWE-079/XssThroughDom.ql
@@ -0,0 +1,21 @@
+/**
+ * @name Cross-site scripting through DOM
+ * @description Writing user-controlled DOM to HTML can allow for
+ *              a cross-site scripting vulnerability.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision medium
+ * @id js/xss-through-dom
+ * @tags security
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-116
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.XssThroughDom::XssThroughDom
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Cross-site scripting vulnerability due to $@.",
+  source.getNode(), "DOM text"
diff --git a/javascript/ql/src/Security/CWE-079/examples/XssThroughDom.js b/javascript/ql/src/Security/CWE-079/examples/XssThroughDom.js
@@ -0,0 +1,4 @@
+$("button").click(function () {
+    var target = this.attr("data-target");
+    $(target).hide();
+});
diff --git a/javascript/ql/src/Security/CWE-079/examples/XssThroughDomFixed.js b/javascript/ql/src/Security/CWE-079/examples/XssThroughDomFixed.js
@@ -0,0 +1,4 @@
+$("button").click(function () {
+    var target = this.attr("data-target");
+    $.find(target).hide();
+});
diff --git a/javascript/ql/src/semmle/javascript/DOM.qll b/javascript/ql/src/semmle/javascript/DOM.qll
@@ -4,6 +4,7 @@
 
 import javascript
 import semmle.javascript.frameworks.Templating
+private import semmle.javascript.dataflow.InferredTypes
 
 module DOM {
   /**
@@ -292,10 +293,18 @@ module DOM {
 
     private class DefaultRange extends Range {
       DefaultRange() {
-        this.asExpr().(VarAccess).getVariable() instanceof DOMGlobalVariable or
-        this = domValueRef().getAPropertyRead() or
-        this = domElementCreationOrQuery() or
+        this.asExpr().(VarAccess).getVariable() instanceof DOMGlobalVariable
+        or
+        this = domValueRef().getAPropertyRead()
+        or
+        this = domElementCreationOrQuery()
+        or
         this = domElementCollection()
+        or
+        exists(JQuery::MethodCall call | this = call and call.getMethodName() = "get" |
+          call.getNumArgument() = 1 and
+          forex(InferredType t | t = call.getArgument(0).analyze().getAType() | t = TTNumber())
+        )
       }
     }
   }

diff --git a/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll b/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll
@@ -538,9 +538,13 @@ module JQuery {
     MethodCall() {
       this = dollarCall() and name = "$"
       or
-      this = dollar().getAMemberCall(name)
+      this = ([dollar(), objectRef()]).getAMemberCall(name)
       or
-      this = objectRef().getAMethodCall(name)
+      // Handle basic dynamic method dispatch (e.g. `$element[html ? 'html' : 'text'](content)`)
+      exists(DataFlow::PropRead read | read = this.getCalleeNode() |
+        read.getBase().getALocalSource() = [dollar(), objectRef()] and
+        read.getPropertyNameExpr().flow().mayHaveStringValue(name)
+      )
       or
       // Handle contributed JQuery objects that aren't source nodes (usually parameter uses)
       getReceiver() = legacyObjectSource() and

diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll b/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll
@@ -413,3 +413,9 @@ module StoredXss {
 
   private class UriEncodingSanitizer extends Sanitizer, Shared::UriEncodingSanitizer { }
 }
+
+/** Provides classes and predicates for the XSS through DOM query. */
+module XssThroughDom {
+  /** A data flow source for XSS through DOM vulnerabilities. */
+  abstract class Source extends Shared::Source { }
+}
diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll
@@ -0,0 +1,126 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about
+ * cross-site scripting vulnerabilities through the DOM.
+ */
+
+import javascript
+
+/**
+ * Classes and predicates for the XSS through DOM query.
+ */
+module XssThroughDom {
+  import Xss::XssThroughDom
+  private import semmle.javascript.security.dataflow.Xss::DomBasedXss as DomBasedXss
+  private import semmle.javascript.dataflow.InferredTypes
+  private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQuery
+
+  /**
+   * A taint-tracking configuration for reasoning about XSS through the DOM.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "XssThroughDOM" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof DomBasedXss::Sink }
+
+    override predicate isSanitizer(DataFlow::Node node) {
+      super.isSanitizer(node) or
+      node instanceof DomBasedXss::Sanitizer
+    }
+
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+      guard instanceof TypeTestGuard or
+      guard instanceof UnsafeJQuery::PropertyPresenceSanitizer
+    }
+  }
+
+  /**
+   * Gets an attribute name that could store user-controlled data.
+   *
+   * Attributes such as "id", "href", and "src" are often used as input to HTML.
+   * However, they are either rarely controlable by a user, or already a sink for other XSS vulnerabilities.
+   * Such attributes are therefore ignored.
+   */
+  bindingset[result]
+  string unsafeAttributeName() {
+    result.regexpMatch("data-.*") or
+    result.regexpMatch("aria-.*") or
+    result = ["name", "value", "title", "alt"]
+  }
+
+  /**
+   * A source for text from the DOM from a JQuery method call.
+   */
+  class JQueryTextSource extends Source, JQuery::MethodCall {
+    JQueryTextSource() {
+      (
+        this.getMethodName() = ["text", "val"] and this.getNumArgument() = 0
+        or
+        this.getMethodName() = "attr" and
+        this.getNumArgument() = 1 and
+        forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and
+        this.getArgument(0).mayHaveStringValue(unsafeAttributeName())
+      ) and
+      // looks like a $("<p>" + ... ) source, which is benign for this query.
+      not exists(DataFlow::Node prefix |
+        DomBasedXss::isPrefixOfJQueryHtmlString(this
+              .getReceiver()
+              .(DataFlow::CallNode)
+              .getAnArgument(), prefix)
+      |
+        prefix.getStringValue().regexpMatch("\\s*<.*")
+      )
+    }
+  }
+
+  /**
+   * A source for text from the DOM from a DOM property read or call to `getAttribute()`.
+   */
+  class DOMTextSource extends Source {
+    DOMTextSource() {
+      exists(DataFlow::PropRead read | read = this |
+        read.getBase().getALocalSource() = DOM::domValueRef() and
+        exists(string propName | propName = ["innerText", "textContent", "value", "name"] |
+          read.getPropertyName() = propName or
+          read.getPropertyNameExpr().flow().mayHaveStringValue(propName)
+        )
+      )
+      or
+      exists(DataFlow::MethodCallNode mcn | mcn = this |
+        mcn.getReceiver().getALocalSource() = DOM::domValueRef() and
+        mcn.getMethodName() = "getAttribute" and
+        mcn.getArgument(0).mayHaveStringValue(unsafeAttributeName())
+      )
+    }
+  }
+
+  /**
+   * A test of form `typeof x === "something"`, preventing `x` from being a string in some cases.
+   *
+   * This sanitizer helps prune infeasible paths in type-overloaded functions.
+   */
+  class TypeTestGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
+    override EqualityTest astNode;
+    TypeofExpr typeof;
+    boolean polarity;
+
+    TypeTestGuard() {
+      astNode.getAnOperand() = typeof and
+      (
+        // typeof x === "string" sanitizes `x` when it evaluates to false
+        astNode.getAnOperand().getStringValue() = "string" and
+        polarity = astNode.getPolarity().booleanNot()
+        or
+        // typeof x === "object" sanitizes `x` when it evaluates to true
+        astNode.getAnOperand().getStringValue() != "string" and
+        polarity = astNode.getPolarity()
+      )
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      polarity = outcome and
+      e = typeof.getOperand()
+    }
+  }
+}
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.expected b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.expected
@@ -0,0 +1,68 @@
+nodes
+| xss-through-dom.js:2:16:2:34 | $("textarea").val() |
+| xss-through-dom.js:2:16:2:34 | $("textarea").val() |
+| xss-through-dom.js:2:16:2:34 | $("textarea").val() |
+| xss-through-dom.js:4:16:4:40 | $(".som ... .text() |
+| xss-through-dom.js:4:16:4:40 | $(".som ... .text() |
+| xss-through-dom.js:4:16:4:40 | $(".som ... .text() |
+| xss-through-dom.js:8:16:8:53 | $(".som ... arget") |
+| xss-through-dom.js:8:16:8:53 | $(".som ... arget") |
+| xss-through-dom.js:8:16:8:53 | $(".som ... arget") |
+| xss-through-dom.js:11:3:11:42 | documen ... nerText |
+| xss-through-dom.js:11:3:11:42 | documen ... nerText |
+| xss-through-dom.js:11:3:11:42 | documen ... nerText |
+| xss-through-dom.js:19:3:19:44 | documen ... Content |
+| xss-through-dom.js:19:3:19:44 | documen ... Content |
+| xss-through-dom.js:19:3:19:44 | documen ... Content |
+| xss-through-dom.js:23:3:23:48 | documen ... ].value |
+| xss-through-dom.js:23:3:23:48 | documen ... ].value |
+| xss-through-dom.js:23:3:23:48 | documen ... ].value |
+| xss-through-dom.js:27:3:27:61 | documen ... arget') |
+| xss-through-dom.js:27:3:27:61 | documen ... arget') |
+| xss-through-dom.js:27:3:27:61 | documen ... arget') |
+| xss-through-dom.js:51:30:51:48 | $("textarea").val() |
+| xss-through-dom.js:51:30:51:48 | $("textarea").val() |
+| xss-through-dom.js:51:30:51:48 | $("textarea").val() |
+| xss-through-dom.js:54:31:54:49 | $("textarea").val() |
+| xss-through-dom.js:54:31:54:49 | $("textarea").val() |
+| xss-through-dom.js:54:31:54:49 | $("textarea").val() |
+| xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name |
+| xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name |
+| xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name |
+| xss-through-dom.js:57:30:57:67 | $("inpu ... "name") |
+| xss-through-dom.js:57:30:57:67 | $("inpu ... "name") |
+| xss-through-dom.js:57:30:57:67 | $("inpu ... "name") |
+| xss-through-dom.js:61:30:61:69 | $(docum ... value") |
+| xss-through-dom.js:61:30:61:69 | $(docum ... value") |
+| xss-through-dom.js:61:30:61:69 | $(docum ... value") |
+| xss-through-dom.js:64:30:64:40 | valMethod() |
+| xss-through-dom.js:64:30:64:40 | valMethod() |
+| xss-through-dom.js:64:30:64:40 | valMethod() |
+edges
+| xss-through-dom.js:2:16:2:34 | $("textarea").val() | xss-through-dom.js:2:16:2:34 | $("textarea").val() |
+| xss-through-dom.js:4:16:4:40 | $(".som ... .text() | xss-through-dom.js:4:16:4:40 | $(".som ... .text() |
+| xss-through-dom.js:8:16:8:53 | $(".som ... arget") | xss-through-dom.js:8:16:8:53 | $(".som ... arget") |
+| xss-through-dom.js:11:3:11:42 | documen ... nerText | xss-through-dom.js:11:3:11:42 | documen ... nerText |
+| xss-through-dom.js:19:3:19:44 | documen ... Content | xss-through-dom.js:19:3:19:44 | documen ... Content |
+| xss-through-dom.js:23:3:23:48 | documen ... ].value | xss-through-dom.js:23:3:23:48 | documen ... ].value |
+| xss-through-dom.js:27:3:27:61 | documen ... arget') | xss-through-dom.js:27:3:27:61 | documen ... arget') |
+| xss-through-dom.js:51:30:51:48 | $("textarea").val() | xss-through-dom.js:51:30:51:48 | $("textarea").val() |
+| xss-through-dom.js:54:31:54:49 | $("textarea").val() | xss-through-dom.js:54:31:54:49 | $("textarea").val() |
+| xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name | xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name |
+| xss-through-dom.js:57:30:57:67 | $("inpu ... "name") | xss-through-dom.js:57:30:57:67 | $("inpu ... "name") |
+| xss-through-dom.js:61:30:61:69 | $(docum ... value") | xss-through-dom.js:61:30:61:69 | $(docum ... value") |
+| xss-through-dom.js:64:30:64:40 | valMethod() | xss-through-dom.js:64:30:64:40 | valMethod() |
+#select
+| xss-through-dom.js:2:16:2:34 | $("textarea").val() | xss-through-dom.js:2:16:2:34 | $("textarea").val() | xss-through-dom.js:2:16:2:34 | $("textarea").val() | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:2:16:2:34 | $("textarea").val() | DOM text |
+| xss-through-dom.js:4:16:4:40 | $(".som ... .text() | xss-through-dom.js:4:16:4:40 | $(".som ... .text() | xss-through-dom.js:4:16:4:40 | $(".som ... .text() | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:4:16:4:40 | $(".som ... .text() | DOM text |
+| xss-through-dom.js:8:16:8:53 | $(".som ... arget") | xss-through-dom.js:8:16:8:53 | $(".som ... arget") | xss-through-dom.js:8:16:8:53 | $(".som ... arget") | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:8:16:8:53 | $(".som ... arget") | DOM text |
+| xss-through-dom.js:11:3:11:42 | documen ... nerText | xss-through-dom.js:11:3:11:42 | documen ... nerText | xss-through-dom.js:11:3:11:42 | documen ... nerText | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:11:3:11:42 | documen ... nerText | DOM text |
+| xss-through-dom.js:19:3:19:44 | documen ... Content | xss-through-dom.js:19:3:19:44 | documen ... Content | xss-through-dom.js:19:3:19:44 | documen ... Content | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:19:3:19:44 | documen ... Content | DOM text |
+| xss-through-dom.js:23:3:23:48 | documen ... ].value | xss-through-dom.js:23:3:23:48 | documen ... ].value | xss-through-dom.js:23:3:23:48 | documen ... ].value | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:23:3:23:48 | documen ... ].value | DOM text |
+| xss-through-dom.js:27:3:27:61 | documen ... arget') | xss-through-dom.js:27:3:27:61 | documen ... arget') | xss-through-dom.js:27:3:27:61 | documen ... arget') | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:27:3:27:61 | documen ... arget') | DOM text |
+| xss-through-dom.js:51:30:51:48 | $("textarea").val() | xss-through-dom.js:51:30:51:48 | $("textarea").val() | xss-through-dom.js:51:30:51:48 | $("textarea").val() | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:51:30:51:48 | $("textarea").val() | DOM text |
+| xss-through-dom.js:54:31:54:49 | $("textarea").val() | xss-through-dom.js:54:31:54:49 | $("textarea").val() | xss-through-dom.js:54:31:54:49 | $("textarea").val() | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:54:31:54:49 | $("textarea").val() | DOM text |
+| xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name | xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name | xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:56:30:56:51 | $("inpu ... 0).name | DOM text |
+| xss-through-dom.js:57:30:57:67 | $("inpu ... "name") | xss-through-dom.js:57:30:57:67 | $("inpu ... "name") | xss-through-dom.js:57:30:57:67 | $("inpu ... "name") | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:57:30:57:67 | $("inpu ... "name") | DOM text |
+| xss-through-dom.js:61:30:61:69 | $(docum ... value") | xss-through-dom.js:61:30:61:69 | $(docum ... value") | xss-through-dom.js:61:30:61:69 | $(docum ... value") | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:61:30:61:69 | $(docum ... value") | DOM text |
+| xss-through-dom.js:64:30:64:40 | valMethod() | xss-through-dom.js:64:30:64:40 | valMethod() | xss-through-dom.js:64:30:64:40 | valMethod() | Cross-site scripting vulnerability due to $@. | xss-through-dom.js:64:30:64:40 | valMethod() | DOM text |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.qlref b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.qlref
@@ -0,0 +1 @@
+Security/CWE-079/XssThroughDom.ql