Last Comment Bug 650001 - (CVE-2011-2369) SVG innerHTML getter doesn't encode entities, enabling XSS
(CVE-2011-2369)
: SVG innerHTML getter doesn't encode entities, enabling XSS
Status: RESOLVED FIXED
[sg:moderate] high?
: testcase
Product: Core
Classification: Components
Component: Serializers (show other bugs)
: unspecified
: All All
: -- major (vote)
: mozilla6
Assigned To: Henri Sivonen (:hsivonen)
:
: Andrew Overholt [:overholt]
Mentors:
http://html5sec.org/innerhtml?xss=%3C...
Depends on:
Blocks: mXSS 652243
  Show dependency treegraph
 
Reported: 2011-04-14 08:33 PDT by Mario Heiderich
Modified: 2014-02-27 14:40 PST (History)
8 users (show)
hsivonen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
-
wanted
unaffected
unaffected


Attachments
Make the HTML serializer pay attention to namespaces (13.10 KB, patch)
2011-04-15 01:00 PDT, Henri Sivonen (:hsivonen)
bzbarsky: review+
sayrer: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Mochitest (1.97 KB, patch)
2011-04-15 01:00 PDT, Henri Sivonen (:hsivonen)
bzbarsky: review+
Details | Diff | Splinter Review
Patch for mozilla-2.0 (13.07 KB, patch)
2011-05-21 16:40 PDT, Cameron Kaiser [:spectre]
dveditz: approval2.0+
Details | Diff | Splinter Review

Description Mario Heiderich 2011-04-14 08:33:34 PDT
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
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-04-14 08:54:14 PDT
This seems like just a problem in the innerHTML getter, right?
Comment 2 Mario Heiderich 2011-04-14 09:07:57 PDT
@: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.
Comment 3 Daniel Veditz [:dveditz] 2011-04-14 16:35:48 PDT
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>
Comment 4 Daniel Veditz [:dveditz] 2011-04-14 16:38:15 PDT
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.
Comment 5 Henri Sivonen (:hsivonen) 2011-04-14 23:04:29 PDT
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.
Comment 6 Henri Sivonen (:hsivonen) 2011-04-14 23:47:48 PDT
Meant to take this, too.
Comment 7 Henri Sivonen (:hsivonen) 2011-04-15 01:00:08 PDT
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.
Comment 8 Henri Sivonen (:hsivonen) 2011-04-15 01:00:35 PDT
Created attachment 526200 [details] [diff] [review]
Mochitest
Comment 9 Henri Sivonen (:hsivonen) 2011-04-15 01:01:31 PDT
Note: This hasn't been through the tryserver, since pushing this to try would make the patch public.
Comment 10 Mario Heiderich 2011-04-15 03:40:22 PDT
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 11 Boris Zbarsky [:bz] (still a bit busy) 2011-04-21 22:39:59 PDT
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 12 Boris Zbarsky [:bz] (still a bit busy) 2011-04-21 22:40:46 PDT
Comment on attachment 526200 [details] [diff] [review]
Mochitest

r=me
Comment 13 Henri Sivonen (:hsivonen) 2011-04-25 22:30:10 PDT
Which repos should I push this to and when? Should I push the test at the same time with the fix?
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-04-25 22:35:37 PDT
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 15 Henri Sivonen (:hsivonen) 2011-04-26 06:08:05 PDT
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?
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-04-26 06:46:57 PDT
I would have said no, but I'm not sure what the new mozilla-shadow setup is...  That never got clearly explained.  :(
Comment 17 Henri Sivonen (:hsivonen) 2011-05-02 05:45:33 PDT
dveditz, what should the landing plan be here?
Comment 18 Daniel Veditz [:dveditz] 2011-05-04 12:05:42 PDT
Land on trunk and everywhere you have approvals to land.
Comment 19 Daniel Veditz [:dveditz] 2011-05-05 11:31:15 PDT
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.
Comment 20 Robert Sayre 2011-05-05 14:31:06 PDT
let's not wait for the shadow-repo to be setup for this one. we'll get it going soon, though.
Comment 21 Asa Dotzler [:asa] 2011-05-05 14:43:13 PDT
let's get this onto m-c so we can see it working. if it looks good there we'll approve for aurora.
Comment 22 Henri Sivonen (:hsivonen) 2011-05-06 05:39:52 PDT
(In reply to comment #18)
> Land on trunk and everywhere you have approvals to land.

Should I defer the test case landing?
Comment 23 Henri Sivonen (:hsivonen) 2011-05-09 00:05:22 PDT
Fix without test landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/7299264c5204
Comment 24 neil@parkwaycc.co.uk 2011-05-09 01:15:37 PDT
(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 Robert Sayre 2011-05-09 19:28:13 PDT
Comment on attachment 526199 [details] [diff] [review]
Make the HTML serializer pay attention to namespaces

approved for mozilla-aurora
Comment 26 Henri Sivonen (:hsivonen) 2011-05-10 01:18:57 PDT
Thanks. Pushed to Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/d80e7d97c3ea
Comment 27 Cameron Kaiser [:spectre] 2011-05-21 16:40:11 PDT
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 28 Daniel Veditz [:dveditz] 2011-06-03 16:47:34 PDT
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"
Comment 29 Henri Sivonen (:hsivonen) 2011-06-12 11:01:59 PDT
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.)
Comment 30 Daniel Veditz [:dveditz] 2011-06-16 00:42:29 PDT
We're releasing the fix next week. Sure, should be OK to check in the test now given the moderate severity.
Comment 31 Henri Sivonen (:hsivonen) 2011-06-21 05:42:54 PDT
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.
Comment 32 Henri Sivonen (:hsivonen) 2014-02-27 05:02:15 PST
Landed the test finally:
https://hg.mozilla.org/integration/mozilla-inbound/rev/383435371058
Comment 33 Ryan VanderMeulen [:RyanVM] 2014-02-27 14:40:35 PST
https://hg.mozilla.org/mozilla-central/rev/383435371058

Note You need to log in before you can comment on or make changes to this bug.