Dialog: Title XSS Vulnerability

[shadowman131]

jQuery.ui.dialog's title option is not properly escaped before being set in the dom, resulting in an XSS vulnerability for applications with dynamic titles. For instance, try:

$('<div>Hi</div>').dialog({title:'<script type="text/javascript">alert("XSS");</script>"});

If this is the desired behaviour, it should at least be noted somewhere. The best reason I could think of for this to be desired would be to use icons in <img> tags in the title. For this, maybe something like:

$.ui.dialog({

title: { html: '<img src="blah">' }

});

Would be more appropriate, as the developer indicates that they mean to use full HTML and are aware of the risks.

[scott.gonzalez]

The fact that you can provide HTML doesn't create a vulnerability. This would be like saying that jQuery has an XSS vulnerability because it provides a .html() method.

[shadowman131]

I strongly disagree - as I said in the bug, at least make it apparent in the documentation. To me, a dialog title is a text attribute - I'm sure others would agree. It doesn't make sense to not escape it by default, and not even mention the lack of escaping.

[scott.gonzalez]

The documentation has been updated.

[gnarf]

Even scarier is using the title attrib of the div itself:  http://jsfiddle.net/gnarf/Hu6g5/1/

[Xavi]

I agree with shadowman13 (the OP) -- titles and labels should be html escaped by default. I feel most people assume that titles and label are purely textual attributes and as a result don't perceive them as possible XSS vulnerabilities.

For the the use case where someone wants to add an image to the title, we can require them to pass in a jquery object:

$("<div>Poppit</div>").dialog({ title: $("<img src='cheggit.png' />") });

Requiring a jquery object also improves error detection/reporting. Currently, if a user accidentally sets the title to a malformed piece of html (e.g. "<img src='cheggit.png'/"), jquery-ui will silently fail and leave the title blank. As opposed to passing in $("<img src='cheggit.png'/") which would immediately throw an exception.

IMO, this change would lead to less surprising code and better error handling.

[joern.zaefferer]

If we escape the title by default, we need a way to allow the user to pass in HTML anyway. The suggestion here by Xavi is to accept a jQuery object. Do we have any examples of that elsewhere? Other ideas or opinions?

[mikesherov]

Requiring a jQuery object everywhere a perceived XSS exists seems like a slippery slope to me. I'd rather just doc this, but I haven't really thought hard about this.

[scott.gonzalez]

My plan was to just switch to text instead of HTML, like we did for autocomplete.

[mikesherov]

Oh, if the plan is to NOT allow HTML, sure. If the plan is jQuery Obj = HTML vs. string = text, I'm against that as a generic strategy.

[joern.zaefferer]

Can we offer a simpler override then the one necessary for autocomplete? Even if its not becoming an option, this isn't exactly trivial:  https://github.com/scottgonzalez/jquery-ui-extensions/blob/master/autocomplete/jquery.ui.autocomplete.html.js

The idea about accepting a jQuery object doesn't sound so bad.

[scott.gonzalez]

An extension for dialog would be simpler. Autocomplete is complicated because of data sources and plain text searching across nodes. Accepting a jQuery object for the title option seems extremely confusing to me. I understand the logic on a technical level, but from an API standpoint, it's unintuitive, confusing, and cumbersome.

[joern.zaefferer]

Here's a potential patch against the dialog branch, extracting setting of the title into a _title method, making it pretty easy to override. The only drawback is that in order to set the current implicit default, I still have to use html:  https://gist.github.com/f945add6e6d32935d906

Is that acceptable?

[mikesherov]

I commented on the gist. I like the approach, it just appeared to me that there's an error. Perhaps I just don't know that code well enough yet.

[joern.zaefferer]

The patch snippets were misleading, along with an incomplete refactoring. Also commented on the gist.

[Jörn Zaefferer]

Dialog: Extract setting the title into a _title method, use .text() to prevent XSS. Fixes #6016 - Dialog: Title XSS Vulnerability.

Changeset: 7e9060c109b928769a664dbcc2c17bd21231b6f3