Ticket #6016 (closed bug: fixed)

Opened 4 years ago

Last modified 2 years ago

Dialog: Title XSS Vulnerability

Reported by: shadowman131 Owned by:
Priority: major Milestone: 1.10.0
Component: ui.dialog Version: 1.8.4
Keywords: Cc:
Blocking: Blocked by:

Description

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.

Change History

comment:1 Changed 4 years ago by scott.gonzalez

  • Status changed from new to closed
  • Resolution set to invalid

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.

comment:2 Changed 4 years ago by 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.

comment:3 Changed 4 years ago by scott.gonzalez

The documentation has been updated.

comment:4 Changed 3 years ago by scott.gonzalez

  • Status changed from closed to reopened
  • Resolution invalid deleted
  • Milestone changed from TBD to 1.9

comment:5 Changed 3 years ago by gnarf

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

comment:6 Changed 3 years ago by 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.

comment:7 Changed 2 years ago by scott.gonzalez

  • Milestone changed from 1.9.0 to 1.10.0

comment:8 Changed 2 years ago by dalekocian

  • Owner set to dalekocian
  • Status changed from reopened to assigned

comment:9 Changed 2 years ago by 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?

comment:10 Changed 2 years ago by 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.

comment:11 Changed 2 years ago by scott.gonzalez

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

comment:12 Changed 2 years ago by 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.

comment:13 Changed 2 years ago by joern.zaefferer

  • Owner dalekocian deleted
  • Status changed from assigned to open

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.

comment:14 Changed 2 years ago by 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.

comment:15 Changed 2 years ago by 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?

comment:16 Changed 2 years ago by 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.

comment:17 Changed 2 years ago by joern.zaefferer

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

comment:18 Changed 2 years ago by joern.zaefferer

  • Summary changed from XSS Vulnerability - Dialog Title to Dialog: Title XSS Vulnerability

comment:19 Changed 2 years ago by Jörn Zaefferer

  • Status changed from open to closed
  • Resolution set to fixed

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

Changeset: 7e9060c109b928769a664dbcc2c17bd21231b6f3

Note: See TracTickets for help on using tickets.