Closed Bug 650001 (CVE-2011-2369) Opened 14 years ago Closed 14 years ago

SVG innerHTML getter doesn't encode entities, enabling XSS

Categories

(Core :: DOM: Serializers, defect)

Product:
Core
Component:
DOM: Serializers
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla6
Tracking Flags:
Tracking Status
firefox5 + fixed
blocking2.0 --- -
status2.0 --- wanted
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: mario, Assigned: hsivonen)

References

(
URL
)

Details

(Keywords: testcase, Whiteboard: [sg:moderate] high?)

Attachments

(3 files)

Make the HTML serializer pay attention to namespaces
13.10 KB, patch
bzbarsky
: review+
sayrer
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Mochitest
1.97 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Patch for mozilla-2.0
13.07 KB, patch
dveditz
: approval2.0+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.2.16) 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>&lt;img src=x onerror=alert(1)&gt;<p> // also works with crippled named entities (!!) <!doctype html><svg><style>&ltimg src=x onerror=alert(1)&gt<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 &lt and &gt (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>&ltimg src=x onerror=alert(1)&gt<p> // no effect <!doctype html><svg><style>&ltimg src=x onerror=alert(1)&gt<p> // XSS The problem is not the innerHTML getter as far as I can see, but the fact that &gt; and &lt; get auto-decoded and enable XSS. Other tested UAs decode &#x61; 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
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)
Attached patch Mochitest — — Splinter Review
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!
Keywords: testcase
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+
Blocks: 652243
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?
Attachment #526199 - Flags: approval2.0?
Attachment #526199 - Flags: approval-mozilla-aurora?
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.
Attachment #526199 - Flags: approval2.0?
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
Closed: 14 years ago
Flags: in-testsuite?
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]
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.
Alias: CVE-2011-2369
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.
Group: core-security
Blocks: mXSS
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.

Attachment

General

Creator:
Created:
Updated:
Size: