Closed Bug 987003 (CVE-2014-1529) Opened 6 years ago Closed 6 years ago

Arbitrary code execution from web notifications

Categories

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

28 Branch
All
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox28 + wontfix
firefox29 + verified
firefox30 + verified
firefox31 + verified
firefox-esr24 --- verified
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected

People

(Reporter: marius.mlynski, Assigned: bzbarsky)

References

Details

(Keywords: sec-critical, Whiteboard: [adv-main29+][adv-esr24.5+])

Attachments

(3 files)

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.
Attached file 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.
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.
CheckLoadURIWithPrincipal with DISALLOW_SCRIPT
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)
Flags: sec-bounty?
This affects all branches including release. :(
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.
status-b2g18: --- → ?
status-b2g-v1.2: --- → ?
status-b2g-v1.3: --- → ?
status-b2g-v1.4: --- → ?
status-b2g-v2.0: --- → ?
Flags: needinfo?(ptheriault)
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.
> 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...
Flags: needinfo?(bobbyholley)
Or just turn off javascript: for images and be done with it.
Yeah, turning off javascript: for images sounds good to me.
(In reply to Boris Zbarsky [:bz] from comment #9)
> Or just turn off javascript: for images and be done with it.

Yes, please!
(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
typo: "CSP protects us here, and you CAN'T use javascript: uris in the system app."
PS all the more reason to work towards CSP on chrome pages aka bug 923902
> 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
(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.
Flags: needinfo?(bobbyholley)
(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> Does it affect also ESR 24?

This API is not exposed on ESR24.
(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?
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.
(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"?
(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?
(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
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.
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".
Attachment #8396587 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
I did verify on Linux that the patch fixes the bug; "Components" ends up not defined in the sandbox, of course.
Attachment #8396587 - Flags: review?(bobbyholley) → review+
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.
Attachment #8396587 - Flags: sec-approval?
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.
Attachment #8396587 - Flags: sec-approval? → sec-approval+
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.
Attachment #8396587 - Flags: approval-mozilla-beta?
Attachment #8396587 - Flags: approval-mozilla-aurora?
I filed bug 988672 on just turning this stuff off altogether for image loads (and everything else other than docshells, in fact).
Attachment #8396587 - Flags: approval-mozilla-beta?
Attachment #8396587 - Flags: approval-mozilla-beta+
Attachment #8396587 - Flags: approval-mozilla-aurora?
Attachment #8396587 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/ee9b3f32f16a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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?
Flags: needinfo?(bzbarsky)
(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.
> 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!
Flags: needinfo?(bzbarsky)
Comment on attachment 8396587 [details] [diff] [review]
Null principal for sandbox

I guess we need this on ESR24 as well.
Attachment #8396587 - Flags: approval-mozilla-esr24?
Keywords: verifyme
Attachment #8396587 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Attached image 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.
> 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.
Thanks for filing the follow-up. Putting it in See Also, so I don't have to read all the comments ;)
See Also: → 988672
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20140403 Firefox/24.0
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main29+][adv-esr24.5+]
Alias: CVE-2014-1529
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.