IMPORTANT: this issue has been cleared for public discussion and resolution by the Drupal Security Team based on the low evidence of vulnerable browsers still existing.
filter_xss does not filter out the accent grave ` character. This opens up an m-XSS (mutation-XSS) vector in some versions of IE7/8, which treats ` as a delimiter character, and we can escape out of a valid attribute, since it does not put double quotes when it's returned via innerHTML property.
The issue is not marked critical because it only affects a small percentage of older browsers under older versions, but marked as "major" since it is XSS that could be easily triggered by non-privileged users on those browsers.
Proof of concept:
<html>
<head>
</head>
<div id="a"><input value="``onmouseover=alert(1)"></div>
<div id="b"></div>
<script>b.innerHTML=a.innerHTML</script>
</html>
Attacker's Input:
``onmouseover=alert(1)
Vulnerable Browser’s Output:
<div id="a"><input value=``onmouseover=alert(1)></div>
Patched Browser’s Output:
<div id="a"><input value="``onmouseover=alert(1)"></div>
Damien Tournoud was able to find that Drupal core’s autocomplete.js is a place where this vulnerability could be exploited, in anything that builds HTML autocomplete snippets (Entity Reference, for example):
$(this.ariaLive).html($(this.selected).html());
This bug was confirmed by grendzy on IE8 version 8.0.7601.17514 (windows 7) in December of 2013. According to Wikipedia, this is the latest stable release of IE8.
However, Pere Orga (June 2015) was unable to reproduce it in IE 8.0.6001.18702 (xp) (though could in IE 7.0.5730.13 (xp)). He was testing with browserstack, which for some reason only offers IE 8.0.6001.19600 on Windows 7 (this version should only be available in XP given the "6001" part of the version number).
Mario Heiderich, the author of http://www.slideshare.net/x00mario/the-innerhtml-apocalypse, claims on slide 28 that a patch is out for IE 8, but this hasn’t been able to be confirmed by the Drupal security team.
pwolanin notes that the paper at https://cure53.de/fp170.pdf seems to suggest a couple server-side and one client-side fix in section 5. David_Rothstein also noted that "Dealing with it in JavaScript seems like a good backup (maybe for situations where text is filtered without calling filter_xss(), like https://www.drupal.org/project/wysiwyg_filter?). But I think we need to fix it server-side in filter_xss() also."
A client-side fix is available by grendzy. While not a fix for direct calls to .innerHTML, it works by overriding jQuery.html() with a version that replaces ` with "`" for IE 8, and should meet most of core/contrib’s use cases. Note however that this direction still needs work; it was tested and found not to fix the issue. (It also has a stray console.log() but that’s pretty minor.)
A server-side fix is available by pwolanin, based on the HTMLPurifier library. This works by adding a new _filter_xss_ie8_fix_attr_value(&$string) function (called from _filter_xss_attributes()) that triggers IE8 to quote properly in the presence of a ` character. This was tested and found to work.
Possibly, depending on the route taken. See "Proposed resolution."
Rafay Baloch posted Dec 2 to security:
While testing your XSS filter function i found that, you are not filtering out the accent grave which has known problems with IE8.
https://api.drupal.org/api/drupal/modules%21filter%21filter.module/funct...
The issue occurs inside of internet explorer only because treats accent grave ` as a delimiter character, and we can escape out of a valid attribute inside of an un-patched IE 8, since it does not put double quotes around our vector when it's returned via innerHTML property.
=============
Proof of concept
=============
Here is the POC that came by slightly modifying the following example at html5sec.org#59.
The POC was tested in Internet explorer version 8:
<html>
<head>
</head>
<div id="a"><input value="``onmouseover=alert(1)"></div>
<div id="b"></div>
<script>b.innerHTML=a.innerHTML</script>
</html>
Attacker's Input:
``onmouseover=alert(1)
Vulnerable Browsers Output:
<div id="a"><input value=``onmouseover=alert(1)></div>
Patched Browsers Output:
<div id="a"><input value="``onmouseover=alert(1)"></div>
When the above POC is tested inside of an unpatched Internet explorer 8, it was noticed that IE 8 does not places quotes around it when it's rendered by innerHTML property. However, When placed in a patched version of internet explorer, it places double quotes around when the string is returned back to the user, hence stopping the attack.
===
Fix
===
Currently, I am not aware of any other solutions then stripping out the accent grave character, encoding doesn't seems to solve the problem here.
==========
References
==========
http://html5sec.org/#59
http://www.slideshare.net/x00mario/the-innerhtml-apocalypse
https://cure53.de/fp170.pdf
--
Warm Regards,
Rafay Baloch
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | sec-98133-69-D6.patch.txt | 1.7 KB | pwolanin |
| #2 | sdo-98133-83-D7.patch.txt | 3.7 KB | pwolanin |
Comments
Comment #2
pwolanin commentedAttaching the most recent D7 patch - though there was some debate about:
vs
!== FALSEas the check.patches were by droplet, and myself (pwolanin)
There was also a suggestion to encode the grave accent as a HTML entity if that prevents the attack.
Comment #3
pwolanin commentedAnd an earlier D6 version
Comment #4
pwolanin commentedComment #5
pwolanin commentedComment #6
David_Rothstein commentedThis affects Drupal 8 too, which supports older browsers from a security + basic functionality point of view even if it doesn't support them otherwise (see https://www.drupal.org/node/1569578).
I'm not really sure it should be considered a critical issue though.
Comment #7
David_Rothstein commentedSome more up-to-date information from the private security issue.
On browser versions that may be vulnerable:
So all evidence points to Microsoft having fixed this for IE6 and IE7, and no one has reproduced it for IE8 recently either. Unless we believe they fixed it for older versions but not newer ones, the most likely conclusion is that they patched it somehow for IE8 too (although it's still not clear how that relates to the version numbers).
Still it may be worth considering a fix.
Some notes on that:
https://www.owasp.org/index.php/OWASP_Java_Encoder_Project#tab=Grave_Acc...
http://crackingdrupal.com/iexss.html
Which contains:
<html>
<head>
</head>
<em id="a"><code lang="``onmouseover=alert(1)">text text text</code></em>
<div id="b"></div>
<script>b.innerHTML=a.innerHTML</script>
</html>
https://github.com/ezyang/htmlpurifier/blob/v4.7.0/library/HTMLPurifier/...
`and`. If those aren't vulnerable, then converting the accent grave to one of those may be better (especially if we are including a fix within check_plain() itself, because then it's much more inline with the purpose of that function).Comment #8
catchI also think this doesn't need to be critical, it would have been in 2010 or so. The support for older browsers for security means we wouldn't automatically mark it won't fix, not that fixing it is critical (although if it's confirmed fixed in IE8 then I'm not sure we need to do anything here generally).
Comment #9
xjmDiscussed with @catch; we agreed the issue does not need to be critical at this point.
Comment #10
xjmComment #11
xjmComment #13
xjmAt this point, Drupal 8 officially does not support IE 8:
https://www.drupal.org/node/1569578
https://www.drupal.org/docs/8/system-requirements/browser-requirements
Based on that, and the fact that this issue only affected old versions of IE 8 and was not reproduced reliably, @catch, @alexpott, @effulgentsia, @Cottser, and I agreed to consider this wontfix for Drupal 8.
Moving to 7.x for the D7 maintainers to decide what they would like to do for this issue (if anything) since D7 still supports IE8.
Thanks!
Comment #14
xjmComment #15
David_Rothstein commentedThat's the same change notice I linked to above. It says that front-end support was dropped, but server-side/functional support wasn't. And I believe there is still code in the Drupal 8 filter system, form API, etc, that exists to support older versions of Internet Explorer.
If there was a decision to drop support further, I can't find any record of it. And dropping security support for older browsers should be a Drupal-wide decision, not a separate decision for different stable releases.
There is actually a discussion of that policy ongoing at #2390621: [policy, no patch] Update Drupal's browser support policy. My take is that security issues are worth fixing at least when the impact of the fix is low. (We can't know what browsers people will access a site with, and if an older browser had a certain behavior it's certainly possible some other browser in the future could wind up having the same behavior too.)
I am not sure if the fix for this issue would be low-impact or not. Note that since the comments above, @droplet actually did find some reliable ways to reproduce this on IE8 (not all installatons of IE8, but particular ones) and also found that replacing the accent grave character with
`does not fix the issue, but replacing it with`does "fix" it in the sense that IE8 doesn't know how to interpret that entity at all. So assuming all modern browsers do interpret it correctly and that no one cares about breaking the accent grave character's display on IE8 at this point, it's a possible easy fix.Comment #16
David_Rothstein commentedI forgot to mention, @droplet also suggested it might be sufficient to replace a double accent-grave (
``) only, unlike HTMLPurifier which deals with single accent grave characters also. This is because of other XSS filtering protection Drupal already has.Not sure if that's 100% guaranteed to be true, but if so it's a potentially even lower-impact fix.
Comment #17
effulgentsia commentedThis seems to me like an appealing fix.
`is explicitly defined in HTML5 and is not defined in HTML4. Perhaps this is the reason it's not recognized in IE8? Since Drupal 8'shtml.html.twiguses the HTML5 doctype, I think a filter that converts from a grave character to`is pretty reasonable. However, is this reasonable for D7 backport, which does not use HTML5?Comment #18
xjmHm I thought @catch had fixed the CR; it was not updated entirely.
https://www.drupal.org/docs/8/system-requirements/browser-requirements specifically says "Internet Explorer 9.x and later" and for me that is what we support.
Comment #19
xjm@Dries said in #1787012-139: [policy, no patch] Write D8 JS against ECMAScript 5. Prevent errors with feature detection (drop IE8 support):
So that's what I think we need to make the documentation reflect, and also why D7 still needs this fix.
Comment #20
effulgentsia commentedForm API does have some code that coddles Internet Explorer, such as the code that starts with the comment in
FormBuilder::doBuildForm():But AFAIK, that's true for IE9. I'm not aware of any D8 FAPI code that's there to coddle IE8 or lower, but if you find any, please reference that.
However, D8 does still have this code in
Xss::filter():So you're right that there's some precedent for D8 still having security-related code for unsupported IE versions.
Answered in #18 and #19.
Hm, this is basically claiming that Drupal should be responsible for security, even on browsers that don't meet the requirements in https://www.drupal.org/docs/8/system-requirements/browser-requirements. That might be a reasonable position, given that that same page also says:
Comment #21
xjmIE5 supported CSS and JavaScript. ;) If "modern" is the difference vs. IE5, IE8 is not modern either.
I guess maybe we need to check back in with the security working group as to whether we should reopen discussion to qualify Dries' decision.
Comment #22
David_Rothstein commentedHm, yes, looks like it would make Drupal 7's HTML not validate (although as a practical matter modern browsers recognize the character even on HTML4 pages).
However, I also found that for both Drupal 7 and 8
`doesn't seem to work at all on modern browsers either (even with Full HTML)... which is strange because it works with bare HTML using the same DOCTYPE that Drupal 7 or 8 use. Didn't have time to investigate further right now, but it calls into question whether this fix would actually provide any real benefit for IE8 too (maybe IE8 does interpret`in certain contexts after all).In the worst case the HTMLPurifier fix is probably good enough. It's not comprehensive if we don't protect against things like
<tag attribute="@value">(where @value is user input that has been run through Drupal's HTML-escaping), but as a practical matter protecting against this in XSS-filtering only would tend to be pretty effective.I only skimmed through #1787012: [policy, no patch] Write D8 JS against ECMAScript 5. Prevent errors with feature detection (drop IE8 support) but I don't see where it suggested dropping core support for IE8 beyond the front-end. And there even exists https://www.drupal.org/project/ie8 in contrib now, although it doesn't seem to be getting much use :)
Comment #27
gappleSupport for Internet Explorer versions prior to 11 was dropped in 8.4
https://www.drupal.org/node/2897971