987003 - (CVE-2014-1529) Arbitrary code execution from web notifications

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mariusz Mlynski]

There's no security check for the icon URL in notifications -- as a result, pages with the notification permission can run arbitrary code. Websites with no such privilege can try clickjacking, especially that there's no security delay on the notification prompt.

Comment 1

[Mariusz Mlynski]

Attachment: exploit.html

This launches C:\Windows\System32\calc.exe (Windows) or alerts Components.classes (Linux). Show the notification or simply open it locally from file:// to skip the prompt.

NB: It probably won't work on Mac OS X >= 10.8 because XUL alerts aren't used there for notifications anymore.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

uh. We should handle notification icons the same way was favicons.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#853
Favicon handling does look rather limited though.
We should allow blob and data urls. and probably file urls when the page is loaded from
file system etc.

I guess it is enough to just block javascript:, although whitelisting would be nicer.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

CheckLoadURIWithPrincipal with DISALLOW_SCRIPT

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Or that can be too strict.

(I wish nsIURI would force certain types of security checks before .spec can be accessed.
Like, one would need to pass some principal when creating uri object, and when accessing it a principal would be needed too)

Comment 5

[(no longer active)]

This affects all branches including release. :(

Comment 6

[David Chan [:dchan]]

This doesn't appear to "directly" impact B2G. The icon URI is used to set the src of an <img> tag. You could set a data: / file: URI but there doesn't appear to be a way to execute script.

:pauljt
Can you think of other attack vectors? The simulator shows the one of the children of 'desktop-notifications-container' in the System app receives the src.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Why would CheckLoadURIWithPrincipal with DISALLOW_SCRIPT be too restrictive? Makes no sense to allow javascript: URIs here.

I'm a bit confused about how this ends up actually executing with privileges, though. alert.js just calls document.getElementById('alertImage').setAttribute('src', "<url>"), and I thought that executed JS/data: URIs in a sandbox. alertImage is a xul:image.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> and I thought that executed JS/data: URIs in a sandbox.

A javascript: URI in that situation will be executed in a sandbox.  But the principal of that sandbox seems to be whatever was set as the channel owner, and that gets set by NewImageChannel() to the loading principal, which in this case is system.  So the sandbox means no direct access to a Window, but you still have access to other things you normally have access to in a global.  In the case of non-system principals this is not a problem, because there really isn't anything else useful; the built-in JS APIs is about it.  But for the system principal you get Components, apparently?

We should probably use a nullprincipal here instead of the channel principal...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Or just turn off javascript: for images and be done with it.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Yeah, turning off javascript: for images sounds good to me.

Comment 11

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Or just turn off javascript: for images and be done with it.

Yes, please!

Comment 12

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to David Chan [:dchan] from comment #6)
> This doesn't appear to "directly" impact B2G. The icon URI is used to set
> the src of an <img> tag. You could set a data: / file: URI but there doesn't
> appear to be a way to execute script.
> 
> :pauljt
> Can you think of other attack vectors? The simulator shows the one of the
> children of 'desktop-notifications-container' in the System app receives the
> src.

On Firefox OS, the icon url is used to set the src of an img tag inside the system app [1]. I didn't realise that scripts like this inherited principals (I thought it was it was already null principal). I was worried that you could do things like "javascript: new MozActivity" and cause the system app to fire an activity. However, CSP protects us here, and you can use javascript: uris in the system app. 

An aside: I'm interested to learn more about the sandbox that is applied to img.src here though if someone has a link to docs or code.






[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/notifications.js#L303

Comment 13

[Paul Theriault [:pauljt] (no longer reading bugmail)]

typo: "CSP protects us here, and you CAN'T use javascript: uris in the system app."

Comment 14

[Paul Theriault [:pauljt] (no longer reading bugmail)]

PS all the more reason to work towards CSP on chrome pages aka bug 923902

Comment 15

[Boris Zbarsky [:bzbarsky]]

> I'm interested to learn more about the sandbox that is applied to img.src here

It's just a sandbox global instead of a Window global.  See http://mxr.mozilla.org/mozilla-central/source/dom/src/jsurl/nsJSProtocolHandler.cpp#266

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Does it affect also ESR 24?

Comment 17

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> We should probably use a nullprincipal here instead of the channel
> principal...

Abso-****ing-lutely.

I think we should do all of the following:
* Disable JS URIs for image loads.
* Use a new null principal for sandboxed JSURI execution.
* Think of ways to avoid this kind of problem every time people add new kinds of resource loading from API implementations. We should make this as secure by default as possible. Currently we rely on a lot of explicit security checks, which we know from experience are fragile.

Comment 18

[(no longer active)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> Does it affect also ESR 24?

This API is not exposed on ESR24.

Comment 19

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #18)

> This API is not exposed on ESR24.

Are there any APIs other than web notifications where this same sort of thing might happen?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

In this case disabling javascript: explicitly for icon is probably the safest option which can land on
all the branches. Then in a followup disable javascript: for images.

Comment 21

[(no longer active)]

(In reply to Bobby Holley (:bholley) from comment #19)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #18)
> 
> > This API is not exposed on ESR24.
> 
> Are there any APIs other than web notifications where this same sort of
> thing might happen?

That is a *very* difficult question to answer confidently.  :/

Were you hoping for something more concrete than "not off the top of my head"?

Comment 22

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #20)
> In this case disabling javascript: explicitly for icon is probably the
> safest option which can land on
> all the branches. Then in a followup disable javascript: for images.

What do you mean for "icon"?  Do you mean the icon argument passed to the Notification API?  AFAIK on desktop that ends up getting loaded into http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/resources/content/alert.xul#29 which is ultimately an HTMLImageElement, so are we talking about disabling javascript: for all images?

Comment 23

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #21)
> Were you hoping for something more concrete than "not off the top of my
> head"?

I just wanted to call attention to the fact that this issue is not inherently one with web notifications, and that we should be careful about:

* what we call "fixed"
* what we call "affected"
* opening up this security bug until the general issue is solved

Comment 24

[Boris Zbarsky [:bzbarsky]]

I suspect the safest fix for branches is using a nullprincipal for the javascript: sandbox.  I'm hoping to get around to writing that patch later today if no one else gets to it first.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Null principal for sandbox

Try run kicked off, but I expect this to be quite safe.  We should do followups for making the default behavior "do not run" instead of "run in sandbox".

Comment 26

[Boris Zbarsky [:bzbarsky]]

I did verify on Linux that the patch fixes the bug; "Components" ends up not defined in the sandbox, of course.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8396587 [details] [diff] [review]
Null principal for sandbox

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not trivially.  It's clear from the patch that a javascript: URI is involved, but not what one needs to do with it.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  No.

Which older supported branches are affected by this flaw?  Looks like 28, 29, 30.

If not all supported branches, which bug introduced the flaw?  I'm going to guess bug 782211.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  I expect this patch to apply as-is to the affected branches, but if not backporting should be a matter of minutes.

How likely is this patch to cause regressions; how much testing does it need?  I don't think this is likely to cause regressions.

Comment 28

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

Comment on attachment 8396587 [details] [diff] [review]
Null principal for sandbox

sec-approval+. Please make Aurora and Beta patches once it is in trunk and nominate them.

Comment 29

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee9b3f32f16a

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8396587 [details] [diff] [review]
Null principal for sandbox

This applies cleanly to the branches, as expected.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Let's say bug 782211, but this code is
  very old.
User impact if declined: Death and destruction.  Or at least arbitrary code
  execution, which is much the same thing.
Testing completed (on m-c, etc.): Green on try.
Risk to taking this patch (and alternatives if risky): I believe this is fairly
   low risk.  All the alternatives are worse.
String or IDL/UUID changes made by this patch: None.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I filed bug 988672 on just turning this stuff off altogether for image loads (and everything else other than docshells, in fact).

Comment 32

[Carsten Book [:Tomcat]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/760327353a9c
https://hg.mozilla.org/releases/mozilla-beta/rev/bc5ac3067e5d

Comment 34

[Daniel Veditz [:dveditz]]

What is special about notification's use of this very old code that turned into an exploit? Do we have other features using it that were similarly exploitable in older versions? Did we just break some feature that legitimately relies on that principal?

Comment 35

[moz_bug_r_a4]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #18)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> > Does it affect also ESR 24?
> 
> This API is not exposed on ESR24.

Hmm, no.  I can reproduce the testcase in Firefox ESR 24.4.0 on Windows.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> What is special about notification's use of this very old code that turned into an
> exploit?

It's taking strings a web page passes it and using them as the src of an image without doing a checkLoadURI(DISALLOW_SCRIPT) check.

We generally try to do such a checkLoadURI check for image sources (e.g. favicon).  Or some sort of scheme whitelist like in pageInfo.js (see checkProtocol()).

That said, we could in fact have other vulnerable code.  That's why we should fix bug 988672.

> Did we just break some feature that legitimately relies on that principal?

We shouldn't have, no.  Not any more than fixing bug 988672 would!

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8396587 [details] [diff] [review]
Null principal for sandbox

I guess we need this on ESR24 as well.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/bde29b7cab44

Comment 39

[Ioana (away)]

Attachment: Empty notification.png

I reproduced this issue on an older Firefox 28 build (Windows 7 64bit) and verified it on the latest Aurora and Nightly builds and on Firefox 29.0b4.

There is a followup issue here though: when the calc application used to open before, now I get an empty notification. I suppose the notification should contain some message about why it's not opening the application or something along these lines.

Comment 40

[Ioana (away)]

> There is a followup issue here though: when the calc application used to..

I will file a followup bug for this tomorrow, when I should also be able to test ESR.

Comment 41

[Frederik Braun [:freddy]]

Thanks for filing the follow-up. Putting it in See Also, so I don't have to read all the comments ;)

Comment 42

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20140403 Firefox/24.0