750096 - (CVE-2012-1957) JavaScript execution via special HTML in RSS view; XSS when pasting malicious content into contenteditable

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Mario Heiderich]

Attachment: test.rss

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120423123513

Steps to reproduce:

It is possible to execute JavaScript on the Feed-View - indicating that there might be a security issue leading to RCE. The bug was found as a side-effect while crafting a RSS version of the H5SC.

A PoC file is attached - please see below. The relevant part in the PoC file is this small snippet:

<description><![CDATA[<embed
src="javascript:top.SubscribeHandler.uninit=function(){alert('hello
chrome://')}">]]></description>

Firefox doesn't seem to filter <embed> elements inside the RSS <description> properly - so we can exec JavaScript. This allows us to hook into the pre-existing script tags. Note the alert in the PoC file is not a "classic website alert" but indicates privileged script exec. 

The bug was found on Ubuntu 11.10:
Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0

I also tested in Win7 with Nightly (15.0.a1 - today's build) - same effect, JavaScript execution in Feed-View.

Comment 1

[Mario Heiderich]

Attachment: Simple screenshot of the effect

Comment 2

[Mario Heiderich]

One note: The PoC might need to be refreshed once to work, I overloaded the "uninit" method - thus the alert will not happen unless you refresh the Feed-View once. 

Also, note that the headers might need to be set properly to have it work - I am using application/rss+xml.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Henri, do you know what we use to sanitize this stuff?

Comment 4

[Daniel Veditz [:dveditz]]

Confirmed that <embed> leads to code execution, but so far am not seeing a privilege escalation. You do have access to the SubscribeHandler._feedWriter element so there may be some way.

Firefox 3.6.x is fine so this may be related to the new HTML5 parser or the replacement of the old ParanoidSinkFragment. If so some add-ons might be in trouble because they may not be as diligent as the feed handler at running everything in a non-privileged sandbox.

Comment 5

[Daniel Veditz [:dveditz]]

Attachment: testcase, <embed> in <description>

This testcase demonstrates that the testcase doesn't run with privilege, nor does the later uninit() call. For an attacker triggering that is easy -- just cause the page/frame to navigate.

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Henri, do you know what we use to sanitize this stuff?

We're supposed to use nsTreeSanitizer, but it looks like it's not being used, since embed is not on its white list and it always sanitizes the src attribute for javascript: URLs.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Henri would you be a good person to take this bug?

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to David Bolter [:davidb] from comment #8)
> Henri would you be a good person to take this bug?

Normally I would, but not right now.  Sorry.  I'm semi-absent due to health issues.

Preliminary debugging suggests that the sanitizer is removing the embed element as it should, but since the element has been inserted into the document fragment once and didn't get its src removed, nsHTMLSharedObjectElement::StartObjectLoad stills end up running off a runnable using the src.

Are detached embed elements expected to start loads like detached img elements or is it a bug of that the embed element still attempts to start the load?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Are detached embed elements expected to start loads

This might have been a recent change from Josh's work to move plug-ins to content.  Josh?

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Mario, if you can find a proof of concept that shows an escalation of privilege, this would qualify for a bug bounty. As it is, this does not look to be exploitable.

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> > Are detached embed elements expected to start loads
> 
> This might have been a recent change from Josh's work to move plug-ins to
> content.  Josh?

Josh, ping?

So I've been thinking what we might do about this. I've come up with three possible solutions.

1) add a method to all elements for making the element harmless. Make the sanitizer call this method on each element that gets removed.

2) remove all attributes from all elements that the sanitizer removes.

3) move attribute sanitation to the tree op executor.

Option #1 would require changes to the classes that implement elements but would probably perform slightly better than option #2. Option #2 would be the least invasive option. Option #3 would be the safest option but would also be the ugliest in terms of encapsulation and code organization.

bz, any advice?

Comment 13

[Mario Heiderich]

I am currently blocked by other things and won't find time to look for a PoC. I am perfectly fine with simply having the issue be fixed ;)

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Critsmash triage is rating this as sec-low unless escalation of privilege can be found for this.

Comment 15

[Boris Zbarsky [:bzbarsky]]

(In reply to Henri Sivonen (:hsivonen) from comment #12)
Option #1 seems like the thing most likely to continue working correctly as new features are added to elements, of the three options.  Unless option 2 is talking about a whitelist?

Comment 16

[Henri Sivonen (:hsivonen)]

(In reply to Boris Zbarsky (:bz) from comment #15)
> (In reply to Henri Sivonen (:hsivonen) from comment #12)
> Option #1 seems like the thing most likely to continue working correctly as
> new features are added to elements, of the three options.  Unless option 2
> is talking about a whitelist?

No, option 2 is not about a whitelist. Option 2 is about removing all attributes from all elements that the sanitizer ends up removing from the tree for any reason. The possible reasons for removing an element are: the sanitizer decides to flatten the element itself (the element is removed but its children are retained), the sanitizer decides to prune the element itself (the element and the whole subtree rooted at the element are removed) and the sanitizer decided to prune an ancestor of the element.

I was actually starting to prefer option #2.

Option #3 would be insufficient as long as XML does not go through the tree  op executor.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Option #2 sounds fine, then.

Comment 18

[Henri Sivonen (:hsivonen)]

Simply implementing option #2 isn't enough. The embed handling runnable runs off the script runner queue instead of the event loop, so the sanitizer runs to late. To do with this problem, I need to put an update batch around both the parse and the sanitation. This leads to another problem: The code in the sanitizer currently assumes it's not wrapped in an update batch and calls methods that try to notify. It seems that it's necessary to find non-notifying alternatives for such tree mutations. As far as I can tell, the non-notifying alternatives use the old child  indices, which is somewhat inconvenient. Also, the parser occasionally intentionally breaks out of the update batch it itself has established. It's not clear to me yet if wrapping the parse in an extra update batch is going to be a problem in the cases where the sanitizer is used (i.e. where the parser isn't appending to a tree that's being rendered at the same time).

bz, are there non-notifying tree mutation methods that don't use the old child indices?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> bz, are there non-notifying tree mutation methods that don't use the old child indices?

Not at the moment; we're trying to get rid of the non-notifying stuff altogether, iirc...

Comment 20

[Henri Sivonen (:hsivonen)]

(In reply to Boris Zbarsky (:bz) from comment #19)
> > bz, are there non-notifying tree mutation methods that don't use the old child indices?
> 
> Not at the moment; we're trying to get rid of the non-notifying stuff
> altogether, iirc...

So the crux of the matter isn't notifying per se but inhibiting script runners from running from the start of the parse to the end of the sanitization. It just seems that establishing an update batch is the most appropriate way to inhibit  script runners from running and that mutations that do notify also attempt to fire mutation events and assert if script runners are being inhibited.

What would be the most appropriate way to inhibit script runners from running from the start of the parse to the end of the sanitization and to turn off attempts to fire mutation events during the modifications the sanitizer performs?

Comment 21

[Boris Zbarsky [:bzbarsky]]

I don't actually know.  We might need to add some API for this.  :(  Jonas?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd say we don't have a good way of preventing any and all scripts for an "extended" period, if "extended" means across multiple asynchronous calls. Script blockers are always stack-based, and I think they need to stay that way since they are a global mechanism and doesn't just block one document.

So if you need something across multiple async calls, we need something else which is per-document.

Comment 23

[Daniel Veditz [:dveditz]]

fwiw moz_bug_r_a4 could not find a way to escalate privilege here, but it's still not good having script running amok in our feed UI.

Comment 24

[Henri Sivonen (:hsivonen)]

(In reply to Jonas Sicking (:sicking) from comment #22)
> I'd say we don't have a good way of preventing any and all scripts for an
> "extended" period, if "extended" means across multiple asynchronous calls.

That's not what it means. I'm looking for a stack-based solution that's in effect for the duration of a synchronous parse and for the duration of a synchronous sanitization both invoked from a parent method on the stack.

Existing update batches successfully deal with script runners. The problem is that in order to remove a node during an update batch without triggering an assertion in the code that tries to fire a mutation event is to use the deprecated index-based methods.

So I'm looking for a solution that has the following three properties:
 * Is not deprecated.
 * Prevents script runners from running.
 * Does not try to fire mutation events while script runners are prevented from running.

If there is no such solution, I guess I should proceed with the deprecated APIs, but it's sad to add more uses of APIs that were trying to get rid of.

(In reply to Daniel Veditz [:dveditz] from comment #23)
> fwiw moz_bug_r_a4 could not find a way to escalate privilege here, but it's
> still not good having script running amok in our feed UI.

I expect there might be privilege escalation in the clipboard case.

Comment 25

[Henri Sivonen (:hsivonen)]

One thing worth exploring would be parsing into a document that's set as loadedAsData and then changing the owner of the document fragment before returning it. However, I would still like to have some sort of script runner prevention thing going on that encompasses both the parsing and the sanitization.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, cool, that is much simpler!

The assertion only means that we've suppressed the event, it doesn't actually mean that the event was fired when it was dangerous to do so.

So technically using a scriptblocker/updatebatch fulfills all the requirements you set up. But I think you additionally don't want to assert.

You could simply do that by putting a nsAutoScriptBlockerSuppressNodeRemoved around the code you want to protect.

However, keep in mind that scriptblockers doesn't actually completely block scriptrunners, they just make them execute later. So wouldn't it still be a problem that script would run for any injected <script> elements, it would just run later?

Comment 27

[Henri Sivonen (:hsivonen)]

Attachment: Potential fix

(In reply to Jonas Sicking (:sicking) Vacation June 11-22 from comment #26)
> You could simply do that by putting a nsAutoScriptBlockerSuppressNodeRemoved
> around the code you want to protect.

Awesome! This is exactly what I was looking for.

> However, keep in mind that scriptblockers doesn't actually completely block
> scriptrunners, they just make them execute later. So wouldn't it still be a
> problem that script would run for any injected <script> elements, it would
> just run later?

Script elements injected during fragment parsing are marked as already started, so they are prevented from executing even before we get to the sanitizer.

I believe the patch I'm attaching is a sufficient fix. However, to add more defense in depth, we could make the fragment parsing use a separate document that's marked as "loaded as data" and then transfer the ownership of the document fragment to another document after the sanitization. Should I do that?

Comment 28

[Henri Sivonen (:hsivonen)]

When trying to write a test case for this, I realized that <img src> and <video src> don't evaluate javascript: URLs with window as the |this| object but <embed src> does. Why do we evaluate <embed src> with window as |this|?

Comment 29

[Henri Sivonen (:hsivonen)]

Attachment: Mochitest, to be landed later

Comment 30

[Boris Zbarsky [:bzbarsky]]

> Why do we evaluate <embed src> with window as |this|?

Also <object data>.  See bug 300263 for why, though apparently no one else supports that so we should consider removing it...

Comment 31

[Henri Sivonen (:hsivonen)]

Comment on attachment 631845 [details] [diff] [review]
Potential fix

Requesting review for the solution, because it seems to work and I think we need to land something.

However, I'm not quite happy with this fix. The other cases in nsParserUtils that use the parser to create a DOM that supposed to be inert, a document marked as "loaded as data" is used. To use the same mechanism here, it would be necessary to transfer the ownership of the document fragment to the document of the context node after performing the parsing with the document that's not the final destination document. However, it appears that we don't have reusable code for forcing node adoption across different-origin documents and ideally the dummy document would use a null principal, so I gave up trying to do that.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631845 [details] [diff] [review]
Potential fix

r=me

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631859 [details] [diff] [review]
Mochitest, to be landed later

r=me, but do you want to test iframe too?

I assume the test does fail without your patch?

Comment 34

[Henri Sivonen (:hsivonen)]

Attachment: Mochitest, to be landed later, v2

(In reply to Boris Zbarsky (:bz) from comment #32)
> Comment on attachment 631845 [details] [diff] [review]
> Potential fix
> 
> r=me

Thank you. Landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c233e8733ba3

(In reply to Boris Zbarsky (:bz) from comment #33)
> Comment on attachment 631859 [details] [diff] [review]
> Mochitest, to be landed later
> 
> r=me, but do you want to test iframe too?

Attaching a revised version with iframe.

> I assume the test does fail without your patch?

Yes, it does.

Comment 35

[Henri Sivonen (:hsivonen)]

Attachment: Backport to ESR

I think we should make the security rating more severe here to make a fix eligible for ESR inclusion. In the builds without the fix for this bug, if the user copies a run of text on one Web site and the run of text has an indivisible embed element with the JavaScript URL as its src, pasting into contenteditable on a different-origin Web site runs the script in the context of that site.

I verified that both trunk and ESR suffer from this problem. I'm attaching a backport of the fix for ESR.

Comment 36

[Mario Heiderich]

Confirmed that on current FF versions. I ran the same tests against the copy&paste scenario that I ran against the RSS view; <embed> is the only one executing JavaScript.

For reference sake: I used the string you can get on html5sec.org when executing: Firebug -> vectors() (all vectors - isolated and concatenated)

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 633048 [details] [diff] [review]
Mochitest, to be landed later, v2

r=me

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 633095 [details] [diff] [review]
Backport to ESR

r=me

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c233e8733ba3

Comment 40

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in the 6/18 build of Mozilla Central using attached POCs.

Comment 41

[Henri Sivonen (:hsivonen)]

Comment on attachment 631845 [details] [diff] [review]
Potential fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 482909.

User impact if declined:
Unauthorized cross-site execution in the context of site that has a contenteditable-based editable area if the user can be convinced into copying some text from a malicious site and pasting it into the editable area. Unauthorized script access between distinct syndication items in a feed. Potentially similar vulnerabilities in Firefox extensions that use the same API as the feed reading features of Firefox.

Testing completed (on m-c, etc.): 
Has baked on mozilla-central over the weekend.

Risk to taking this patch (and alternatives if risky):
No true risk in sight. The first risk that I can think of is that some element implementation might misbehave if it's associated script runner gets deferred for longer than the element implementation expects. However, that risk since low, since the sanitizer is trying to eliminate this sort of things that would usually run off a script runner.
 
String or UUID changes made by this patch: 
None.

Comment 42

[Henri Sivonen (:hsivonen)]

Comment on attachment 633095 [details] [diff] [review]
Backport to ESR

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
I believe the user impact described in comment 41 merits more serious treatment than the current security rating of this bug (assigned prior to comments about the clipboard-based XSS) suggests.

User impact if declined:
See comment 41.

Fix Landed on Version:
Firefox 16

Risk to taking this patch (and alternatives if risky): 
See comment 41.

String or UUID changes made by this patch:
None.

Comment 43

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 631845 [details] [diff] [review]
Potential fix

[Triage Comment]
Approving for Beta/Aurora - will discuss ESR in security triage later this week.

Comment 44

[Henri Sivonen (:hsivonen)]

Thanks for the approvals. Landed:
http://hg.mozilla.org/releases/mozilla-beta/rev/b0aa9c9e9d91
http://hg.mozilla.org/releases/mozilla-aurora/rev/d6febb18a5be

Comment 46

[Daniel Veditz [:dveditz]]

Raising security severity because this affects more than originally thought. The copy/paste case might be more "moderate" than "high" given the required social engineering (but a click-dragging attack might work well), but there's also worry an unknown number of addons might rely on this same sanitization API.

Comment 47

[Henri Sivonen (:hsivonen)]

Comment on attachment 633095 [details] [diff] [review]
Backport to ESR

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
See comment 42 and comment 46.

User impact if declined: 
See comment 41.

Fix Landed on Version:
Firefox 16; also branch-landed on 15 and 14.

Risk to taking this patch (and alternatives if risky): 
See comment 41.

String or UUID changes made by this patch: 
None.

I thought I already requested approval for ESR, but a bug history says otherwise. Anyway, I'm requesting approval for ESR now in the light of comment 46.

I'll be on vacation returning on July 11, so if the ESR backport patch gets approval before that, please treat it as checkin-needed (just bug number, review and approval in the commit comment).

Comment 48

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 633095 [details] [diff] [review]
Backport to ESR

Thanks Henri - will set check-in needed.

Comment 49

[Al Billings [:abillings - ex-MoCo]]

This was marked for ESR checkin over two weeks ago. Can someone please check the patch in or will Henri be doing it after 7/11?

Comment 50

[Henri Sivonen (:hsivonen)]

Landed on esr:
https://hg.mozilla.org/releases/mozilla-esr10/rev/bdf10a513d20

Thanks for the approval.

Comment 51

[Henri Sivonen (:hsivonen)]

Editing summary as a reminder to cover the copy&paste aspect in the advisory.

Comment 52

[Daniel Veditz [:dveditz]]

Henri: don't forget to check these tests in.

Comment 53

[Henri Sivonen (:hsivonen)]

(In reply to Daniel Veditz [:dveditz] from comment #52)
> Henri: don't forget to check these tests in.

Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8ee10cf6ec

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a8ee10cf6ec

Comment 55

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719