973987 - Notifications do not go through the parent

Status: RESOLVED INVALID

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

Description

[Reuben Morais [:reuben]]

ContentParent::RecvShowAlertNotification is never called, it looks like Notifications goes through the parent normally for prompting for permission but not for the actual displaying of the notification.

Paul, has the Notifications API been sec reviewed? I couldn't find any information on the wiki.

Comment 1

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

So desktop notification pre-dates me at Mozilla. A quick google lead me to https://wiki.mozilla.org/Security/Reviews/Firefox4/Desktop_Notifications_Security_Review  But this was well prior to b2g or the process model, or the web app permission model etc.  

This bug puzzles me a little though - if the RecvShowAlertNotification is never called in the parent, how is the system app (which runs in the parent) ever notified that it needs to show a notification? I'll do a little digging and try to figure it out.

In any case, desktop-notifications is one of the rare permissions that regular web apps (i.e not privileged) can obtain without prompting. That is, the default is "allow" for this permission see [1], even for type=web apps. IE although in general its bad when we enforce permissions in the child, I'm not sure that this particular permission is too critical an issue. Feel free to correct me if I am missing something and we should fix it regardless.

The pattern worries me though - I know we reviewed permissions implementation in bug 777602, but that focuses on APIs that use message manager for inter-process communication. I'm not sure that a similar across the board audit has been performed for APIs which communicate directly with IPDL instead of using message manager. I had a vague memory of Ben Turner doing an audit like this, but I can't find the bug, so I'll take an action to do revisit this.


[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsTable.jsm#179

Comment 2

[Daniel Veditz [:dveditz]]

I think Paul is saying he needs to look things up and doesn't want to unhide the bug, but he doesn't atm think it's a vulnerability.

Comment 3

[Julian Hector [:tedd] [:jhector]]

Maybe I can give some information regarding ContentParent::RecvShowAlertNotification not being called, it is actually called but from code that is generated from the PContent.ipdl file during build time.

It can be found by looking at ./objdir-gecko/ipc/ipdl/PContentParent.cpp realtive to the B2G root directory. In the ... function is a big switch-case statement, and one of the cases is:

> PContent::Msg_ShowAlertNotification__ID

which unserializes the IPC message and calls the Recv function:

> if ((!(RecvShowAlertNotification(mozilla::Move(imageUrl), mozilla::Move(title), mozilla::Move(text), mozilla::Move(textClickable), mozilla::Move(cookie), mozilla::Move(name), mozilla::Move(bidi), mozilla::Move(lang), mozilla::Move(data), mozilla::Move(principal), mozilla::Move(inPrivateBrowsing))))) {
>     mozilla::ipc::ProtocolErrorBreakpoint("Handler for ShowAlertNotification returned error code");
>     return MsgProcessingError;
> }

The code that actually sends the IPC message across the channel is also generated from the IPDL file and resides in ./objdir-gecko/ipc/ipdl/PContentChild.cpp, the class supplies a "SendShowAlertNotification" which can be called from a PContentChild instance.

Inside Gecko the code that triggers sending the message is here [1] which calls the ShowAlertNotification() function on a singleton instance of ContentChild (which itself is derived form PContentChild)

so the Recv-function is actually called but it can't be found by searching mxr or dxr since it is generated code.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/nsAlertsService.cpp?rev=0ac19d3bf7bf#81

Comment 4

[Julian Hector [:tedd] [:jhector]]

I just saw the bug in my cc list, I don't know if the issue has been resolved or not, I commented in case it is important again at some point.

Comment 5

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

Julian, can you have a look and see if we still need this for desktop (and close it if not?)

Comment 6

[Julian Hector [:tedd] [:jhector]]

Sure thing Paul.

Doing some digging in searchfox.org, it seems that this got changed in Bug 1227300 and in use by nsIAlertService, which is implemented here [1]

Doing a search for ShowAlertNotification [2] shows there are a few places using it:

> browser/components/nsBrowserGlue.js
> toolkit/components/downloads/nsDownloadManager.cpp
> toolkit/components/extensions/ext-notifications.js
> addon-sdk/source/lib/sdk/notifications.js

since, addon-sdk will be deprecated, I don't think that usage matters too much. About the other three, I am not sure.
The ShowAlert message is still part of PContent and received on the parent side. [3]

[1] http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/toolkit/components/alerts/nsAlertsService.cpp#203,219
[2] http://searchfox.org/mozilla-central/search?q=symbol:_ZN16nsIAlertsService21ShowAlertNotificationERK9nsAStringS2_S2_bS2_P11nsIObserverS2_S2_S2_S2_P12nsIPrincipalbb%2C%23showAlertNotification&redirect=false
[3] http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/dom/ipc/PContent.ipdl#784

Comment 7

[Andrew Overholt [:overholt]]

I think this still needs a decision as to whether this is a big deal or not.

Comment 8

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

Back from leave. I'm not too concerned about this (at least not enough to prioritize over other review work). Worst case scenario here is that a compromised child process can bypass the permission check for showing notifications, the above digging didn't actually confirm this was possible, and until we get all the fission stuff done, a child process can just lie about what page it currently has loaded (and assume permissions from other domains[1]). If anyone has concerns here I _could_ do a more detailed review, but I don't think its really worth it. I think i'll just make a note to consider this as part of broader permission model reviews for fission, and close this bug. (any objections, please reopen and needinfo me) 


[1] bug 1432825 among others...