diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md index 14a95faab9d..14ca3b82d3c 100644 --- 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 new file mode 100644 index 00000000000..5aa2fe63253 --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp @@ -0,0 +1,70 @@ + + + + +

+Extracting text from a DOM node and interpreting it as HTML can lead to a cross-site scripting vulnerability. +

+

+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. +

+
+ + +

+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. +

+
+ + +

+The following example shows a webpage using a data-target attribute +to select and manipulate a DOM element using the JQuery library. In the example, the +data-target attribute is read into the target variable, and the +$ function is then supposed to use the target variable as a CSS +selector to determine which element should be manipulated. +

+ +

+However, if an attacker can control the data-target attribute, +then the value of target can be used to cause the $ function +to execute arbitary JavaScript. +

+

+The above vulnerability can be fixed by using $.find instead of $. +The $.find function will only interpret target as a CSS selector +and never as HTML, thereby preventing an XSS attack. +

+ +
+ + +
  • +OWASP: +DOM based +XSS Prevention Cheat Sheet. +
  • +
  • +OWASP: +XSS +(Cross Site Scripting) Prevention Cheat Sheet. +
  • +
  • +OWASP +DOM Based XSS. +
  • +
  • +OWASP +Types of Cross-Site +Scripting. +
  • +
  • +Wikipedia: Cross-site scripting. +
  • +
    +
    diff --git a/javascript/ql/src/Security/CWE-079/XssThroughDom.ql b/javascript/ql/src/Security/CWE-079/XssThroughDom.ql new file mode 100644 index 00000000000..b4ae70d7e8b --- /dev/null +++ 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 new file mode 100644 index 00000000000..cfbc3c08069 --- /dev/null +++ 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 new file mode 100644 index 00000000000..3bc9e36267d --- /dev/null +++ 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 index 71f0239ed27..fcce5a4759f 100644 --- 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 index 82a78d3df79..e814610ac5f 100644 --- 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 index 47aff96c9c3..98dcb59992f 100644 --- 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 new file mode 100644 index 00000000000..16a645954fe --- /dev/null +++ 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 $("

    " + ... ) 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 new file mode 100644 index 00000000000..7963d42b65e --- /dev/null +++ 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 new file mode 100644 index 00000000000..3226decda37 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.qlref @@ -0,0 +1 @@ +Security/CWE-079/XssThroughDom.ql diff --git a/javascript/ql/test/query-tests/Security/CWE-079/xss-through-dom.js b/javascript/ql/test/query-tests/Security/CWE-079/xss-through-dom.js new file mode 100644 index 00000000000..0f8a1ff7a09 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/xss-through-dom.js @@ -0,0 +1,70 @@ +(function () { + $("#id").html($("textarea").val()); // NOT OK. + + $("#id").html($(".some-element").text()); // NOT OK. + + $("#id").html($(".some-element").attr("foo", "bar")); // OK. + $("#id").html($(".some-element").attr({"foo": "bar"})); // OK. + $("#id").html($(".some-element").attr("data-target")); // NOT OK. + + $("#id").html( + document.getElementById("foo").innerText // NOT OK. + ); + + $("#id").html( + document.getElementById("foo").innerHTML // OK - only repeats existing XSS. + ); + + $("#id").html( + document.getElementById("foo").textContent // NOT OK. + ); + + $("#id").html( + document.querySelectorAll("textarea")[0].value // NOT OK. + ); + + $("#id").html( + document.getElementById('div1').getAttribute('data-target') // NOT OK + ); + + function safe1(x) { // overloaded function. + if (x.jquery) { + var foo = $(x); // OK + } + + } + safe1($("textarea").val()); + + function safe2(x) { // overloaded function. + if (typeof x === "object") { + var foo = $(x); // OK + } + } + safe2($("textarea").val()); + + + $("#id").html( + $("

    " + something() + "

    ").text() // OK - this is for a flow-step to catch, not this query. + ); + + + $("#id").get(0).innerHTML = $("textarea").val(); // NOT OK. + + var base = $("#id"); + base[html ? 'html' : 'text']($("textarea").val()); // NOT OK. + + $("#id").get(0).innerHTML = $("input").get(0).name; // NOT OK. + $("#id").get(0).innerHTML = $("input").get(0).getAttribute("name"); // NOT OK. + + $("#id").get(0).innerHTML = $("input").getAttribute("id"); // OK. + + $("#id").get(0).innerHTML = $(document).find("option").attr("value"); // NOT OK. + + var valMethod = $("textarea").val; + $("#id").get(0).innerHTML = valMethod(); // NOT OK + + var myValue = $(document).find("option").attr("value"); + if(myValue.property) { + $("#id").get(0).innerHTML = myValue; // OK. + } +})(); \ No newline at end of file