From dd9aec056c7266274fb8df5cf2a787a1595b7fdf Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 3 Apr 2020 11:47:53 +0200 Subject: [PATCH 01/15] handle basic dynamic method dispatch for jQuery methods --- javascript/ql/src/semmle/javascript/frameworks/jQuery.qll | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 From 55edfed1ee0a03aa8d03836afb420896a58bd930 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 3 Apr 2020 11:48:10 +0200 Subject: [PATCH 02/15] support jQuery().get() returning a DOM node --- javascript/ql/src/semmle/javascript/DOM.qll | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) 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()) + ) } } } From 14b551f887c61c9e654ba7250a0c49d203e2f421 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 3 Apr 2020 11:48:25 +0200 Subject: [PATCH 03/15] Xss through DOM --- .../ql/src/Security/CWE-079/XssThroughDom.ql | 21 +++ .../javascript/security/dataflow/Xss.qll | 6 + .../security/dataflow/XssThroughDom.qll | 141 ++++++++++++++++++ .../Security/CWE-079/XssThroughDom.expected | 63 ++++++++ .../Security/CWE-079/XssThroughDom.qlref | 1 + .../Security/CWE-079/xss-through-dom.js | 62 ++++++++ 6 files changed, 294 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-079/XssThroughDom.ql create mode 100644 javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.expected create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.qlref create mode 100644 javascript/ql/test/query-tests/Security/CWE-079/xss-through-dom.js 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..a87ed4955a2 --- /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/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..36e735283a3 --- /dev/null +++ b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll @@ -0,0 +1,141 @@ +/** + * 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 + + /** + * 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 HasNodePropertySanitizerGuard + } + } + + /** + * 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 = ["name", "value"] + } + + /** + * 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 this + .getReceiver() + .(DataFlow::CallNode) + .getAnArgument() + .(StringOps::ConcatenationRoot) + .getConstantStringParts() + .substring(0, 1) = "<" + } + } + + /** + * 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() + } + } + + /** + * The precense of a `nodeType` or `jquery` property indicates that the value is a DOM node, and not the text of a DOM node. + * + * This sanitizer helps prune infeasible paths in type-overloaded functions. + */ + class HasNodePropertySanitizerGuard extends TaintTracking::SanitizerGuardNode { + DataFlow::PropRead read; + + HasNodePropertySanitizerGuard() { + read = this and + read.getPropertyName() = ["nodeType", "jquery"] + } + + override predicate sanitizes(boolean outcome, Expr e) { + e = read.getBase().asExpr() and outcome = true + } + } +} 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..f32705554de --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.expected @@ -0,0 +1,63 @@ +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") | +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") | +#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 | 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..98947e96ff3 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-079/xss-through-dom.js @@ -0,0 +1,62 @@ +(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. +})(); \ No newline at end of file From 1b80f46f30477c52c90e6c4697f92594f865d362 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Fri, 3 Apr 2020 15:54:27 +0200 Subject: [PATCH 04/15] add QHelp for js/xss-through-dom query --- .../src/Security/CWE-079/XssThroughDom.qhelp | 71 +++++++++++++++++++ .../CWE-079/examples/XssThroughDom.js | 4 ++ .../CWE-079/examples/XssThroughDomFixed.js | 4 ++ 3 files changed, 79 insertions(+) create mode 100644 javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp create mode 100644 javascript/ql/src/Security/CWE-079/examples/XssThroughDom.js create mode 100644 javascript/ql/src/Security/CWE-079/examples/XssThroughDomFixed.js 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..297e3739fee --- /dev/null +++ b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp @@ -0,0 +1,71 @@ + + + + +

+Writing text from a webpage to the same webpage without properly sanitizing the +input first, might allow for a cross-site scripting vulnerability. +

+

+A webpage with this vulnerability unescapes an otherwise sanitized text, +and thereby allows an attacker to use sanitized text in the DOM 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. +

+ + + +

+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/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(); +}); From 2d3e42e6d66aacf8c20609fae964040fa5278b2f Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 20 Apr 2020 11:50:46 +0200 Subject: [PATCH 05/15] update qhelp for xss-through-dom Co-Authored-By: Asger F --- javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp index 297e3739fee..ca99672f4a5 100644 --- a/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp +++ b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp @@ -5,8 +5,7 @@

    -Writing text from a webpage to the same webpage without properly sanitizing the -input first, might allow for a cross-site scripting vulnerability. +Extracting text from a DOM node and interpreting it as HTML can lead to a cross-site scripting vulnerability.

    A webpage with this vulnerability unescapes an otherwise sanitized text, From 12f4ce81112535089e14aabbb1c41d580f3606c3 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 20 Apr 2020 11:42:16 +0200 Subject: [PATCH 06/15] merge two cases of jQuery method calls --- javascript/ql/src/semmle/javascript/frameworks/jQuery.qll | 7 ++++--- .../test/query-tests/Security/CWE-079/xss-through-dom.js | 5 ++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll b/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll index e814610ac5f..5bd04c2e521 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll @@ -538,12 +538,13 @@ module JQuery { MethodCall() { this = dollarCall() and name = "$" or - this = ([dollar(), objectRef()]).getAMemberCall(name) - or // 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) + ( + read.getPropertyNameExpr().flow().mayHaveStringValue(name) or + read.getPropertyName() = name + ) ) or // Handle contributed JQuery objects that aren't source nodes (usually parameter uses) 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 index 98947e96ff3..c5af74f276f 100644 --- 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 @@ -58,5 +58,8 @@ $("#id").get(0).innerHTML = $("input").getAttribute("id"); // OK. - $("#id").get(0).innerHTML = $(document).find("option").attr("value"); // NOT OK. + $("#id").get(0).innerHTML = $(document).find("option").attr("value"); // NOT OK. + + var valMethod = $("textarea").val; + $("#id").get(0).innerHTML = valMethod(); // OK - Not a method call, not valid receiver for jQuery. })(); \ No newline at end of file From 73b0aa4004fe263346ca59cc0259c5599b11402e Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 20 Apr 2020 11:48:33 +0200 Subject: [PATCH 07/15] add more attributes potentially vulnerable to xss-through-dom --- .../src/semmle/javascript/security/dataflow/XssThroughDom.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll index 36e735283a3..0577be54766 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll @@ -44,7 +44,8 @@ module XssThroughDom { bindingset[result] string unsafeAttributeName() { result.regexpMatch("data-.*") or - result = ["name", "value"] + result.regexpMatch("aria-.*") or + result = ["name", "value", "title", "alt"] } /** From 9fc29ee0f8e0dc64d99dcd4f9cdfab6f3e6747e0 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Mon, 20 Apr 2020 12:59:17 +0200 Subject: [PATCH 08/15] update qhelp --- javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp index ca99672f4a5..b08423c64c0 100644 --- a/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp +++ b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp @@ -8,9 +8,9 @@ Extracting text from a DOM node and interpreting it as HTML can lead to a cross-site scripting vulnerability.

    -A webpage with this vulnerability unescapes an otherwise sanitized text, -and thereby allows an attacker to use sanitized text in the DOM to perform a -cross-site scripting attack. +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.

    From 59b94b3d1b14adc8f117f8b4b61c996d25f2c4cd Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Tue, 21 Apr 2020 13:08:06 +0200 Subject: [PATCH 09/15] revert back to having 2 separate cases in JQuery::MethodCall --- javascript/ql/src/semmle/javascript/frameworks/jQuery.qll | 7 +++---- .../query-tests/Security/CWE-079/XssThroughDom.expected | 5 +++++ .../test/query-tests/Security/CWE-079/xss-through-dom.js | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll b/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll index 5bd04c2e521..e814610ac5f 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll +++ b/javascript/ql/src/semmle/javascript/frameworks/jQuery.qll @@ -538,13 +538,12 @@ module JQuery { MethodCall() { this = dollarCall() and name = "$" or + this = ([dollar(), objectRef()]).getAMemberCall(name) + or // 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 - read.getPropertyName() = name - ) + read.getPropertyNameExpr().flow().mayHaveStringValue(name) ) or // Handle contributed JQuery objects that aren't source nodes (usually parameter uses) diff --git a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.expected b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.expected index f32705554de..7963d42b65e 100644 --- a/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.expected +++ b/javascript/ql/test/query-tests/Security/CWE-079/XssThroughDom.expected @@ -35,6 +35,9 @@ nodes | 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() | @@ -48,6 +51,7 @@ edges | 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 | @@ -61,3 +65,4 @@ edges | 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/xss-through-dom.js b/javascript/ql/test/query-tests/Security/CWE-079/xss-through-dom.js index c5af74f276f..b07f6e40fba 100644 --- 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 @@ -61,5 +61,5 @@ $("#id").get(0).innerHTML = $(document).find("option").attr("value"); // NOT OK. var valMethod = $("textarea").val; - $("#id").get(0).innerHTML = valMethod(); // OK - Not a method call, not valid receiver for jQuery. + $("#id").get(0).innerHTML = valMethod(); // NOT OK })(); \ No newline at end of file From 947e9828da632394e3c77661089c477074e37829 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 22 Apr 2020 10:07:50 +0200 Subject: [PATCH 10/15] Update javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp Co-Authored-By: mc <42146119+mchammer01@users.noreply.github.com> --- javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp index b08423c64c0..0c46d92c196 100644 --- a/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp +++ b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp @@ -17,7 +17,7 @@ If an attacker is able to control the safe sanitized text, then this vulnerabili

    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. +writing text to the page, or one of the other solutions that are mentioned in the References section below.

    From 76503d3536ca05b62b81e2f1f795e688bc8b84e6 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 22 Apr 2020 10:07:29 +0200 Subject: [PATCH 11/15] user controlled -> user-controlled --- javascript/ql/src/Security/CWE-079/XssThroughDom.ql | 2 +- .../src/semmle/javascript/security/dataflow/XssThroughDom.qll | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-079/XssThroughDom.ql b/javascript/ql/src/Security/CWE-079/XssThroughDom.ql index a87ed4955a2..b4ae70d7e8b 100644 --- a/javascript/ql/src/Security/CWE-079/XssThroughDom.ql +++ b/javascript/ql/src/Security/CWE-079/XssThroughDom.ql @@ -1,6 +1,6 @@ /** * @name Cross-site scripting through DOM - * @description Writing user controlled DOM to HTML can allow for + * @description Writing user-controlled DOM to HTML can allow for * a cross-site scripting vulnerability. * @kind path-problem * @problem.severity error diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll index 0577be54766..4708d142263 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll @@ -35,7 +35,7 @@ module XssThroughDom { } /** - * Gets an attribute name that could store user controlled data. + * 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. From 7bfea946fddaaf080b1ab7c7f5e486d1990489e0 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 22 Apr 2020 10:23:03 +0200 Subject: [PATCH 12/15] update links in xss-through-dom qhelp --- javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp index 0c46d92c196..5aa2fe63253 100644 --- a/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp +++ b/javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp @@ -56,11 +56,11 @@ OWASP:
  • OWASP -DOM Based XSS. +DOM Based XSS.
  • OWASP -Types of Cross-Site +Types of Cross-Site Scripting.
  • From a5bbfa30d153175532d9884409536133bbea9587 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 22 Apr 2020 10:23:07 +0200 Subject: [PATCH 13/15] add change note --- change-notes/1.25/analysis-javascript.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 0a29d132d0392f5b92fa780d208da2e392f67713 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 22 Apr 2020 13:50:43 +0200 Subject: [PATCH 14/15] reuse existing logic in DomBasedXss --- .../security/dataflow/XssThroughDom.qll | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll index 4708d142263..86433962bc7 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll @@ -62,13 +62,14 @@ module XssThroughDom { this.getArgument(0).mayHaveStringValue(unsafeAttributeName()) ) and // looks like a $("

    " + ... ) source, which is benign for this query. - not this - .getReceiver() - .(DataFlow::CallNode) - .getAnArgument() - .(StringOps::ConcatenationRoot) - .getConstantStringParts() - .substring(0, 1) = "<" + not exists(DataFlow::Node prefix | + DomBasedXss::isPrefixOfJQueryHtmlString(this + .getReceiver() + .(DataFlow::CallNode) + .getAnArgument(), prefix) + | + prefix.getStringValue().regexpMatch("\\s*<.*") + ) } } From ac2674181690d937efd18f072adcfacf6d627d33 Mon Sep 17 00:00:00 2001 From: Erik Krogh Kristensen Date: Wed, 22 Apr 2020 14:16:15 +0200 Subject: [PATCH 15/15] reuse existing SanitizerGuard from UnsafeJQueryPlugin --- .../security/dataflow/XssThroughDom.qll | 21 ++----------------- .../Security/CWE-079/xss-through-dom.js | 5 +++++ 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll index 86433962bc7..16a645954fe 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll @@ -12,6 +12,7 @@ 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. @@ -30,7 +31,7 @@ module XssThroughDom { override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) { guard instanceof TypeTestGuard or - guard instanceof HasNodePropertySanitizerGuard + guard instanceof UnsafeJQuery::PropertyPresenceSanitizer } } @@ -122,22 +123,4 @@ module XssThroughDom { e = typeof.getOperand() } } - - /** - * The precense of a `nodeType` or `jquery` property indicates that the value is a DOM node, and not the text of a DOM node. - * - * This sanitizer helps prune infeasible paths in type-overloaded functions. - */ - class HasNodePropertySanitizerGuard extends TaintTracking::SanitizerGuardNode { - DataFlow::PropRead read; - - HasNodePropertySanitizerGuard() { - read = this and - read.getPropertyName() = ["nodeType", "jquery"] - } - - override predicate sanitizes(boolean outcome, Expr e) { - e = read.getBase().asExpr() and outcome = true - } - } } 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 index b07f6e40fba..0f8a1ff7a09 100644 --- 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 @@ -62,4 +62,9 @@ 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