Closed Bug 987003 (CVE-2014-1529) Opened 6 years ago Closed 6 years ago
Arbitrary code execution from web notifications
601 bytes, text/html
1.46 KB, patch
|Details | Diff | Splinter Review|
17.32 KB, image/png
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.
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.
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)
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.
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
Does it affect also ESR 24?
(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.
(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 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 :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
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+
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.
I filed bug 988672 on just turning this stuff off altogether for image loads (and everything else other than docshells, in fact).
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?
(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!
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?
Attachment #8396587 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
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
You need to log in before you can comment on or make changes to this bug.