Ticket #2529228 (closed defect)

Reporter


Reed 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
formatters YUI supplies render textual data as HTML markup. In particular, if the textual data contains tags (such as script tags), those tags will be added to the page as live HTML. That may not be
what you want.

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
override your autocomlete instance's formatResult method with it. As far as I know, autocomplete doesn't supply a proper formatter for textual data, nor is there a general YUI function which takes
strings and outputs HTML text with entities.

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
specifying the text of the item to be added to the menu." Actually, the string is interpreted as an HTML fragment, not text. You have to do the HTML escaping yourself.
"

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

Reed Loden

Posted: 12/16/10

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.

Ryan Grove

YUI Developer

Posted: 12/16/10
  • priority changed to P3 (normal)

Jenny Donnelly

YUI Developer

Posted: 12/22/10
  • location changed to Library Code
  • milestone changed to 2.9.0
  • severity changed from S1 (critical) to S3 (normal)
  • status changed from new to accepted

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:
1) Update documentation
2) Provide text formatting as an option.
3) Preserve backward compatibility.

I am also filing a copy of this ticket for Menu.

Jenny Donnelly

YUI Developer

Posted: 12/22/10
  • relatesto changed to 2529231

Jenny Donnelly

YUI Developer

Posted: 01/19/11
  • location changed from Library Code to Example
  • summary changed from Default formatters in autocomplete allow for stored XSS to doc: Default formatters in autocomplete allow for stored XSS

Reed Loden

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">
<status>Candidate</status>
<phase date="20110128">Assigned</phase>
<desc>Cross-site scripting (XSS) vulnerability in the addItem method in the Menu widget in YUI before 2.9.0 allows remote attackers to inject arbitrary web script or HTML via a field that is added to a menu, related to documentation that specifies this field as a text field rather than an HTML field, a similar issue to CVE-2010-4569 and CVE-2010-4570.</desc>
<refs>
<ref source="MISC" url="http://yuilibrary.com/forum/viewtopic.php?p=12923">http://yuilibrary.com/forum/viewtopic.php?p=12923</ref>
<ref source="MISC" url="http://yuilibrary.com/projects/yui2/ticket/2529228">http://yuilibrary.com/projects/yui2/ticket/2529228</ref>
<ref source="CONFIRM" url="http://yuilibrary.com/projects/yui2/ticket/2529231">http://yuilibrary.com/projects/yui2/ticket/2529231</ref>
</refs>
</item>

Ryan Grove

YUI Developer

Posted: 01/31/11
  • priority changed from P3 (normal) to P1 (critical)
  • severity changed from S3 (normal) to S1 (critical)

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.

Reed Loden

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.

Jenny Donnelly

YUI Developer

Posted: 02/1/11
  • priority changed from P1 (critical) to P2 (high)
  • severity changed from S1 (critical) to S2 (high)

Will address before 2.9.0.

Ryan Grove

YUI Developer

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.

Jenny Donnelly

YUI Developer

Posted: 03/9/11
  • status changed from accepted to checkedin

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.

George

YUI Developer

Posted: 04/13/11
  • resolution changed to fixed
  • status changed from checkedin to closed

Andriyk0

Posted: 04/10/12

Can anyone give a proof of concept?