Ticket #2529228 (closed defect)
ReporterReed Loden |
Opened: 12/16/10 Last modified: 04/10/12 Status: closed Type: defect Resolution: fixed |
Owner Jenny Donnelly |
Target Release: 2.9.0 Priority: P2 (high) |
|---|---|---|---|
| Summary: | doc: Default formatters in autocomplete allow for stored XSS | ||
| Description: | From http://yuilibrary.com/forum/viewtopic.php?p=12923 : " Heads up everyone, if you use widgets such as autocomplete or datatable, you may be vulnerable to a stored XSS attack if you aren't specifying explicit formatters for textual data. The default Datatable supplies a formatter (formatText) which renders strings as text, escaping HTML brackets and ampersands. For autocomplete, you are going to have to copy the formatText implementation and The same problem happens with the YUI Menu widget as well. In addition, the API documentation is misleading. In the documentation for the addItem method, it says one parameter represents a "String I'm running into the same problem the above user is reporting, so making sure this is filed. If you want multiple tickets for this, let me know, and I'll be happy to file them. |
||
| Type: | defect | Observed in Version: | 2.8.2 |
| Component: | AutoComplete | Severity: | S2 (high) |
| Assigned To: | Jenny Donnelly | Target Release: | 2.9.0 |
| Location: | Example | Priority: | P2 (high) |
| Tags: | autocomplete, xss | Relates To: | #2529231 |
| Browsers: | All | ||
| URL: | http://yuilibrary.com/forum/viewtopic.php?p=12923 | ||
| Test Information: | |||
Change History
|
Posted: 12/16/10
|
|
Posted: 12/16/10
|
|
Posted: 12/22/10
Thanks for filing this. In hindsight, it is regrettable that the current ability to pass in HTML is the default behavior. Instead, text formatting should be the default and HTML formatting should be a configuration. That said, I believe this is more of a documentation issue to clarify that data passed into the default formatter will not be scrubbed. Here's the plan for 2.9.0: I am also filing a copy of this ticket for Menu. |
|
Posted: 12/22/10
|
|
Posted: 01/19/11
|
|
Posted: 01/28/11
MITRE has taken it upon itself to assign this CVE-2010-4710. This is now a publicly tracked security issue. Maybe this will affect your prioritization of this issue? <item type="CAN" name="CVE-2010-4710" seq="2010-4710"> |
|
Posted: 01/31/11
Reed, we're making a sweep of the 2.x codebase to identify other places where either function signatures or documentation are unclear about whether or not strings will be escaped before being displayed. Once we've finished this sweep (most likely later today) we'll have a better idea of how soon we can provide either code changes or documentation changes or both to address this issue. Regarding the prioritization of this issue: when this was first brought to our attention, it was in the context of it being something to watch out for, and we had planned to update the documentation in the next release. If we had known there had already been an incident in the wild (like the Bugzilla XSS that led to http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2010-4569 ), we would not have waited until the next scheduled release to address this. We were viewing this as purely a documentation issue, but in hindsight we do have a responsibility to ensure that our APIs are safe by default, and are clearly described as unsafe when it's not possible to be safe by default. |
|
Posted: 01/31/11
Well, I had hoped it was clear from the forum post I quoted that at least some other person had run into this issue in the wild. As for the Bugzilla issue, we don't discuss security issues publicly until a release has been made, and there didn't seem to be any way to comment privately on the issue here, so I (purposely) didn't mention it. It would be nice if there was a way to privately report potential security issues to you to allow for better communication between YUI and security researchers. Even just the ability to mark a ticket as private when filing would be very useful... Let me know if you need anything else from me. |
|
Posted: 02/1/11
Will address before 2.9.0. |
|
Posted: 02/1/11
Status update: we've completed initial reviews of both the 2.x and 3.x trees looking for potentially unsafe uses of innerHTML or for cases where documentation doesn't explicitly state that strings will end up in innerHTML without being escaped. Authors of individual components will perform deeper reviews, but so far there have been no major surprises: in most cases, documentation just needs to be more explicit about how strings will be used; in a small number of cases, the default behavior needs to be changed so that content is escaped or displayed by means other than innerHTML. While there's not much YUI can do to prevent a determined implementer from using data unsafely, we can at least make it harder for them to do so unwittingly. We plan to provide a 2.8.3 patch release within the next few weeks to address the code issues that have been identified, including the AutoComplete issue. At the moment there doesn't appear to be a need for a YUI 3 patch release, but this is pending deeper analysis. We will also create an official YUI security policy, and will provide an email address to which security concerns may be reported privately. Details will be announced via the usual channels (blog, Twitter, forums) once we have this in place. |
|
Posted: 03/9/11
In the API documentation, all parameters that accept strings destined for innerHTML are noted by type HTML instead of String. Added the formatEscapedResult() method for implementers which will escape potentially untrustworthy result data for insertion into DOM. |
|
Posted: 04/13/11
|
|
Posted: 04/10/12
Can anyone give a proof of concept? |
https://github.com/yui/yui2/blob/master/src/datatable/js/DataTable.js#L898 even mentions this:
formatText : function(el, oRecord, oColumn, oData) {
var value = (lang.isValue(oData)) ? oData : "";
//TODO: move to util function
el.innerHTML = value.toString().replace(/&/g, "&").replace(/</g, "<").replace(/>/g, ">");
},
Should move that to a util function and offer it for other modules such as autocomplete.