Bug 650001 (CVE-2011-2369)

SVG innerHTML getter doesn't encode entities, enabling XSS

RESOLVED FIXED in Firefox 5

Status

()

Core
Serializers
--
major
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Mario Heiderich, Assigned: hsivonen)

Tracking

(Blocks: 1 bug, {testcase})

unspecified
mozilla6
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5+ fixed, blocking2.0 -, status2.0 wanted, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:moderate] high?, URL)

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
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?
(Reporter)

Comment 2

6 years ago
@: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.
(Assignee)

Comment 5

6 years ago
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
(Assignee)

Comment 6

6 years ago
Meant to take this, too.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Comment 7

6 years ago
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)
(Assignee)

Comment 8

6 years ago
Created attachment 526200 [details] [diff] [review]
Mochitest
Attachment #526200 - Flags: review?(bzbarsky)
(Assignee)

Comment 9

6 years ago
Note: This hasn't been through the tryserver, since pushing this to try would make the patch public.
(Reporter)

Comment 10

6 years ago
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!

Updated

6 years ago
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
(Assignee)

Comment 13

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....
(Assignee)

Comment 15

6 years ago
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.  :(
(Assignee)

Comment 17

6 years ago
dveditz, what should the landing plan be here?

Updated

6 years ago
tracking-firefox5: --- → +
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.
Attachment #526199 - Flags: approval2.0?
blocking2.0: --- → -
status2.0: --- → wanted
status-firefox5: --- → affected
tracking-firefox5: --- → +

Comment 20

6 years ago
let's not wait for the shadow-repo to be setup for this one. we'll get it going soon, though.

Comment 21

6 years ago
let's get this onto m-c so we can see it working. if it looks good there we'll approve for aurora.
(Assignee)

Comment 22

6 years ago
(In reply to comment #18)
> Land on trunk and everywhere you have approvals to land.

Should I defer the test case landing?
(Assignee)

Comment 23

6 years ago
Fix without test landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/7299264c5204
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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 25

6 years ago
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+
(Assignee)

Comment 26

6 years ago
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+
(Assignee)

Comment 29

6 years ago
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
(Assignee)

Comment 31

6 years ago
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

Updated

3 years ago
Blocks: 974208
(Assignee)

Comment 32

3 years ago
Landed the test finally:
https://hg.mozilla.org/integration/mozilla-inbound/rev/383435371058
Flags: in-testsuite? → in-testsuite+
(Assignee)

Updated

3 years ago
Whiteboard: [sg:moderate] high? [test case needs landing when bug no longer confidential] → [sg:moderate] high?
https://hg.mozilla.org/mozilla-central/rev/383435371058
You need to log in before you can comment on or make changes to this bug.