13.10 KB, patch
Robert Sayre: approval-mozilla-aurora+
|Details | Diff | Splinter Review|
1.97 KB, patch
|Details | Diff | Splinter Review|
13.07 KB, patch
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (X11; U; Linux i686; de; rv:220.127.116.11) Gecko/20110323 Ubuntu/10.10 (maverick) Firefox/3.6.16 Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0 In-line SVG allows to have HTML entities in plain-text tags be auto decoded. This enables XSS by injecting entities via URL. Check the example link for a PoC. PoC: http://html5sec.org/innerhtml?xss=%3C!doctype%20html%3E%3Csvg%3E%3Cstyle%3E%26amplt;img%20src=x%20onerror=alert%281%29%26ampgt;%3Cp%3E <!doctype html><svg><style><img src=x onerror=alert(1)><p> // also works with crippled named entities (!!) <!doctype html><svg><style><img src=x onerror=alert(1)><p> The bug also triggers on innerHTML/outerHTML access (see example link). No other tested browser was affected by this quirk (IE9, GC10-12, O11). Other browsers encounter this problem by excluding a certain range of characters from being auto-decoded. FF4-6 is the only browser auto-decoding < and > (among others). Tested on FF4 - 6.0a1 Reproducible: Always Steps to Reproduce: 1. Click on the link 2. Shock 3. Awe
This seems like just a problem in the innerHTML getter, right?
@:bz Yes and no. Compare: <!doctype html><xxx><style><img src=x onerror=alert(1)><p> // no effect <!doctype html><svg><style><img src=x onerror=alert(1)><p> // XSS The problem is not the innerHTML getter as far as I can see, but the fact that > and < get auto-decoded and enable XSS. Other tested UAs decode a to a (as they should) but omit XSS critical chars.
I can confirm the problem with the innerHTML testcase link, but not when loading the above content in a file by itself. Since we're only seeing one alert(1) I agree w/bz that it's the innerHTML getter. In the testcase "canvas" DOM looks like [svg[svg-style[text]]][html-para]. Since the entities aren't re-encoded by the innerHTML getter you get what's shown in the log, which when parsed by the second innerHTML of course leads to the XSS. "canvas2" DOM looks like [svg[svg-style]][html-img][html-para] or <svg><style></style></svg><img src="x" onerror="alert(1)"><p></p>
I guessed "sg:moderate" because it seems unlikely many sites are using embedded <svg> let alone putting user-generated content in it and then reading it back out and reparsing it. But it could happen, and probably will as more browsers support in-line svg.
We should fix this by making the HTML serializer escape the contents of text descendants of style and script even though the elements with the same local names in the HTML namespace shouldn't get their content escaped in serialization.
Meant to take this, too.
Created attachment 526199 [details] [diff] [review] Make the HTML serializer pay attention to namespaces While reviewing the code, could you, please, also check that the commit message is OK for a security patch. AFAICT, it doesn't review anything that's not obvious from the code change.
Created attachment 526200 [details] [diff] [review] Mochitest
Note: This hasn't been through the tryserver, since pushing this to try would make the patch public.
I tested the fix against a list of SVG elements, HTML elements, comments, proc-insts etc - looks valid to me. The range of elements (<style>, <script>, <noframes>, <noscript>) seems sufficient. I was thinking <noembed> could be an issue as well - but wasn't. Good job!
Comment on attachment 526199 [details] [diff] [review] Make the HTML serializer pay attention to namespaces r=me, though I really wish there were more IsHTML(tagname) stuff going on in this code....
Comment on attachment 526200 [details] [diff] [review] Mochitest r=me
6 years ago
Which repos should I push this to and when? Should I push the test at the same time with the fix?
You need to get approvals for fx4 and fx5, I think. Need to push to Fx4 and aurora, and presumably m-c? Not sure about the test....
Comment on attachment 526199 [details] [diff] [review] Make the HTML serializer pay attention to namespaces (In reply to comment #14) > You need to get approvals for fx4 and fx5, I think. Requesting those approvals. > Need to push to Fx4 and aurora, and presumably m-c? Should I refrain from pushing to m-c until I have approvals to push to other repos at the same time?
I would have said no, but I'm not sure what the new mozilla-shadow setup is... That never got clearly explained. :(
dveditz, what should the landing plan be here?
Land on trunk and everywhere you have approvals to land.
Comment on attachment 526199 [details] [diff] [review] Make the HTML serializer pay attention to namespaces Don't plan any further "mozilla2.0" releases, clearing approval request for that branch.
let's not wait for the shadow-repo to be setup for this one. we'll get it going soon, though.
let's get this onto m-c so we can see it working. if it looks good there we'll approve for aurora.
(In reply to comment #18) > Land on trunk and everywhere you have approvals to land. Should I defer the test case landing?
Fix without test landed on trunk: http://hg.mozilla.org/mozilla-central/rev/7299264c5204
(In reply to comment #19) > Don't plan any further "mozilla2.0" releases, clearing approval request for > that branch. We (SeaMonkey) just (2 hours before patch landed on m-c) started spinning our 2.1rc1 from mozilla2.0; assuming we want this, would we just make a relbranch? [Also is there a way to find other bugs that might affect us?]
Comment on attachment 526199 [details] [diff] [review] Make the HTML serializer pay attention to namespaces approved for mozilla-aurora
Thanks. Pushed to Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/d80e7d97c3ea
Created attachment 534254 [details] [diff] [review] Patch for mozilla-2.0 Per security group discussion, requesting landing on mozilla-2.0. (I can't set the request-approval flag as it is disabled.) This is the same patch, modified for that branch.
Comment on attachment 534254 [details] [diff] [review] Patch for mozilla-2.0 Approved for the mozilla2.0 repository, a=dveditz for release-drivers When landed please add a link to the changeset and flip the status2.0 field to ".x-fixed"
Does the test case (attachment 526200 [details] [diff] [review]) need to be kept confidential still? Would it be OK to land the test case on m-c now? (I was silly enough qcommit a backup of the test case to my mercurial queue and now I'm wondering if I should try to strip it from my the queue history in order to be able to a backup of my queue to a public repo.)
We're releasing the fix next week. Sure, should be OK to check in the test now given the moderate severity.
FWIW, the security advisory for this bug isn't quite right. The bug wasn't in decoding character references. The bug was in encoding stuff in innerHTML *getter* in a way that didn't round-trip.
Landed the test finally: https://hg.mozilla.org/integration/mozilla-inbound/rev/383435371058