JS: Add XSS-through-dom query #3191

erik-krogh · 2020-04-02T10:13:26Z

Adds a new query js/xss-through-dom.

Relevant CVEs:

CVE	source	sink
CVE-2019-14862	element.name	IE7 document.createElement(..)
CVE-2016-10735	element.getAttribute("data-target")	$(..)
CVE-2019-8331	element.getAttribute('data-original-title')	$().html(..)
CVE-2019-15482	$().text()	$(..)
CVE-2019-12313	element.innerText	element.insertAdjacentHTML(..)
CVE-2018-19048	$().val()	element.innerHTML
CVE-2018-8035	element.value	$.jGrowl(..)

All of the above CVEs reads a string from a DOM node (attribute or text-node), where that string is controlled by someone else (e.g. a client, another library, rendered by a server...).
This string then ends up in an XSS sink, and a safe text thereby becomes unsafe HTML.

So the query does not flag vulnerabilities that are immediately exploitable. An attacker needs a write primitive to the relevant location.
But the point is that the write primitive is safe on its own, and an xss-through-dom vulnerability will escalate the attack by unescaping the text.

The query currently flags a lot of results, many of these are just bad style and not necessarily exploitable.

Here are some results: https://lgtm.com/query/2351488529036590711/
Many of the results are the old bootstrap CVE (CVE-2016-10735).
(That CVE existed across many versions of bootstrap. The query does not currently flag all versions).

With this PR we get 2 CVE TPs, and one step closer to 4 more CVE TPs.
(I don't know how many of the last 4 we can reasonably get. 3 of them miss have missing type/data-flow, and the last is a missing $.jGrowl sink)

An evaluation shows that the performance overhead is not that big.

TODO:

Generalize the new SanitizerGuardNodes to DomBasedXss?
change-note

asgerf · 2020-04-18T11:50:03Z

javascript/ql/src/semmle/javascript/frameworks/jQuery.qll

+      exists(DataFlow::PropRead read | read = this.getCalleeNode() |
+        read.getBase().getALocalSource() = [dollar(), objectRef()] and
+        read.getPropertyNameExpr().flow().mayHaveStringValue(name)
+      )


Can we merge this with the above case? It seems to almost subsume it with the only caveat about this only recognizing method calls.

With the current version we loose support for non-method calls, which I think is OK.
I can quickly add it back by adding a .getLocalSource().

Hm, we'd lose quite a few calls. They don't look very interesting I'll give you that, but I think we should keep them.

Yeah, I'm ok with loosing non-method calls, but I'm not comfortable with loosing the reflective calls.

I'll revert it back to having 2 cases.

asgerf · 2020-04-18T12:27:39Z

javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll

+  bindingset[result]
+  string unsafeAttributeName() {
+    result.regexpMatch("data-.*") or
+    result = ["name", "value"]


I'd suggest including aria-.* and maybe title and alt as well.

asgerf · 2020-04-18T12:51:33Z

javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp

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


This opening paragraph feels a little too generic. I'd try to be a little more concrete, for example:

Suggested change

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.

and then in the next paragraph explain why that's the case, i.e. that it invalidates any escaping already performed on that data.

Co-Authored-By: Asger F <asgerf@github.com>

asgerf

LGTM

@mchammer01 can we ask you for a doc review?

mchammer01

Yes of course @asgerf 😃. Here is my review:
@erik-krogh - this looks good 👍
There is a small number of minor comments for your consideration.
Also could you add an update to the change notes (the page with a table summarizing the new and updated queries, and that also reports whether query results are shown on LGTM by default or not)? Thanks.
Hope this helps.

mchammer01 · 2020-04-21T15:08:01Z

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


Shouldn't this be user-controlled (with an hyphen)?

javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp

mchammer01 · 2020-04-21T15:20:59Z

javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp

+</li>
+<li>
+OWASP
+<a href="https://www.owasp.org/index.php/DOM_Based_XSS">DOM Based XSS</a>.


This reference and the one below have a note at the top of the page, saying that their content hasn't been moved to a new platform yet. Just wondering whether we're still happy to link to these URLs (I am not sure whether the URLs will change once the content is ported onto the new platform, so feel free to ignore if my comment is irrelevant).

The links already redirect to the new wiki.
(https://www.owasp.org/index.php/DOM_Based_XSS redirects to https://owasp.org/www-community/attacks/DOM_Based_XSS).

I'll update the links, and the warning will hopefully disappear from their site eventually.

Co-Authored-By: mc <42146119+mchammer01@users.noreply.github.com>

erik-krogh · 2020-04-22T08:24:51Z

@erik-krogh - this looks good
There is a small number of minor comments for your consideration.
Also could you add an update to the change notes (the page with a table summarizing the new and updated queries, and that also reports whether query results are shown on LGTM by default or not)? Thanks.

I've added the change note.

Hope this helps.

It did 👍

esbena

I see some potential for reuse. Please let me know if they are invalid.

esbena · 2020-04-22T09:27:22Z

javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll

+          .getAnArgument()
+          .(StringOps::ConcatenationRoot)
+          .getConstantStringParts()
+          .substring(0, 1) = "<"


Did you look into reusing some of this logic, or at least using a regexp instead of substring? https://github.com/Semmle/ql/blob/1b88c9768827c37355eea473cba4e24796be310a/javascript/ql/src/semmle/javascript/security/dataflow/Xss.qll#L80-L83

I can't reuse all of it.
But I can use isPrefixOfJQueryHtmlString, and I'll copy-paste the regexp.

esbena · 2020-04-22T09:30:14Z

javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll

+          read.getPropertyName() = propName or
+          read.getPropertyNameExpr().flow().mayHaveStringValue(propName)


I created https://github.com/github/codeql-javascript-team/issues/98 for this pattern just now.

Lets do that in a later PR.

Absolutely.

esbena · 2020-04-22T09:33:46Z

javascript/ql/src/semmle/javascript/security/dataflow/XssThroughDom.qll

+   *
+   * This sanitizer helps prune infeasible paths in type-overloaded functions.
+   */
+  class TypeTestGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {


Have you considered the sanitizers for js/unsafe-jquery-plugin?https://github.com/Semmle/ql/blob/1b88c9768827c37355eea473cba4e24796be310a/javascript/ql/src/semmle/javascript/security/dataflow/UnsafeJQueryPluginCustomizations.qll#L187-L190

They are also used to eliminate some non-string values.

That sanitizer is a generalization of one of my sanitizers, so that is perfect 👍

erik-krogh · 2020-04-22T12:50:11Z

@mchammer01: Can you approve the changes?

mchammer01

Apologies @erik-krogh, I had the day off yesterday.
LGTM, thanks for the documentation updates ✨

erik-krogh added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Apr 2, 2020

erik-krogh force-pushed the XssDom branch from 411723d to 2f8b780 Compare April 3, 2020 09:48

erik-krogh added 2 commits April 17, 2020 10:32

handle basic dynamic method dispatch for jQuery methods

dd9aec0

support jQuery().get() returning a DOM node

55edfed

erik-krogh force-pushed the XssDom branch from 98a15e2 to 16ec45a Compare April 17, 2020 08:44

erik-krogh added 2 commits April 17, 2020 10:54

Xss through DOM

14b551f

add QHelp for js/xss-through-dom query

1b80f46

erik-krogh force-pushed the XssDom branch from 16ec45a to 1b80f46 Compare April 17, 2020 08:54

erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Apr 17, 2020

erik-krogh marked this pull request as ready for review April 17, 2020 08:58

erik-krogh requested review from mchammer01 and a team as code owners April 17, 2020 08:58

erik-krogh removed the request for review from mchammer01 April 17, 2020 09:00

asgerf reviewed Apr 18, 2020

View reviewed changes

erik-krogh and others added 5 commits April 20, 2020 11:50

update qhelp for xss-through-dom

2d3e42e

Co-Authored-By: Asger F <asgerf@github.com>

merge two cases of jQuery method calls

12f4ce8

add more attributes potentially vulnerable to xss-through-dom

73b0aa4

update qhelp

9fc29ee

revert back to having 2 separate cases in JQuery::MethodCall

59b94b3

asgerf reviewed Apr 21, 2020

View reviewed changes

mchammer01 reviewed Apr 21, 2020

View reviewed changes

erik-krogh and others added 5 commits April 22, 2020 10:07

Update javascript/ql/src/Security/CWE-079/XssThroughDom.qhelp

947e982

Co-Authored-By: mc <42146119+mchammer01@users.noreply.github.com>

user controlled -> user-controlled

76503d3

Merge remote-tracking branch 'upstream/master' into XssDom

8811455

update links in xss-through-dom qhelp

7bfea94

add change note

a5bbfa3

esbena reviewed Apr 22, 2020

View reviewed changes

reuse existing logic in DomBasedXss

0a29d13

reuse existing SanitizerGuard from UnsafeJQueryPlugin

ac26741

esbena approved these changes Apr 22, 2020

View reviewed changes

mchammer01 approved these changes Apr 23, 2020

View reviewed changes

semmle-qlci merged commit da32926 into github:master Apr 23, 2020

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

	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.

		read.getPropertyName() = propName or
		read.getPropertyNameExpr().flow().mayHaveStringValue(propName)

JS: Add XSS-through-dom query #3191

JS: Add XSS-through-dom query #3191

Uh oh!

Conversation

erik-krogh commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Apr 22, 2020

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erik-krogh commented Apr 22, 2020

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erik-krogh commented Apr 2, 2020 •

edited

Loading

erik-krogh Apr 21, 2020 •

edited

Loading