13.10 KB, patch
|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:22.214.171.124) 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>
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: In-line SVG auto-decode enabling XSS → SVG innerHTML getter doesn't encode entities, enabling XSS
Whiteboard: [sg:moderate] high?
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.
Component: SVG → Serializers
QA Contact: general → dom-to-text
Meant to take this, too.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
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.
Attachment #526199 - Flags: review?(bzbarsky)
Created attachment 526200 [details] [diff] [review] Mochitest
Attachment #526200 - Flags: review?(bzbarsky)
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....
Attachment #526199 - Flags: review?(bzbarsky) → review+
Comment on attachment 526200 [details] [diff] [review] Mochitest r=me
Attachment #526200 - Flags: review?(bzbarsky) → review+
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.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
tracking-firefox5: + → ---
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.
blocking2.0: --- → -
status2.0: --- → wanted
status-firefox5: --- → affected
tracking-firefox5: --- → +
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(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
Attachment #526199 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks. Pushed to Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/d80e7d97c3ea
status-firefox5: affected → fixed
Whiteboard: [sg:moderate] high? → [sg:moderate] high? [test case needs landing when bug no longer confidential]
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.
Attachment #534254 - Flags: review?(dveditz)
Attachment #534254 - Flags: review?(dveditz) → approval2.0?
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"
Attachment #534254 - Flags: approval2.0? → approval2.0+
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
Flags: in-testsuite? → in-testsuite+
Whiteboard: [sg:moderate] high? [test case needs landing when bug no longer confidential] → [sg:moderate] high?
You need to log in before you can comment on or make changes to this bug.