815110 - Accessing settings, wifi, mozApps.mgmt APIs without permission kills app process.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

If content attempts to access the settings, wifi or mozApps.mgmt APIs without the correct permission, the current process is killed. As far as I know this is because we kill apps which make IPDL requests without appropriate permission but I was expecting to see this behavior on in the case a process had become corrupted or compromised, not just a based on an errant JavaScript call.

Example calls that crash the process are respectively:

settings: navigator.mozSettings.createLock().get('screen.automatic-brightness')
wifi: window.navigator.mozWifiManager
mozApps.mgmt: navigator.mozApps.mgmt.getAll();

I don't think this is a showstopper (unless there are deeper implications of process killing?), but it is preventing automated permissions testing(bug 815105).

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

These results seem to imply that we're not applying permission checks at the DOM boundary, which is rather scary indeed :(.

However, app processes being killed in these cases is what we want.  You're unintentionally simulating a compromised app process.

Comment 2

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

I'm not at all convinced that we're doing all the parent process checks that we should be doing. I'd feel a lot more comfortable if we checked this on the DOM side as well.

Comment 3

[Doug Turner (:dougt)]

Having the permission check in the child process is important to have, but having the permission check in the parent is a hard requirement (to support the sandbox).  We'd like to see these the child process nicely fail if the application doesn't have the right permission (like throw an error, or whatever), but we will not block basecamp on it.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I hope I found most of these in the followup bugs to bug 776834.

Comment 5

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

Nom-ing for tracking - this blocks a comprehensive testing suite for all permissions which would be nice to have in the future.

Comment 7

[[:fabrice] Fabrice Desré]

Btw this should be fixed for the mozApps API. mozApps.mgmt returns null at the DOM level in the child if the webapps-manage permission is not set (https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#188)

Comment 8

[David Chan [:dchan]]

(In reply to Paul Theriault [:pauljt] from comment #5)
> Nom-ing for tracking - this blocks a comprehensive testing suite for all
> permissions which would be nice to have in the future.

I'm currently using a temporary workaround by adding the required permission to the parent before testing. 
mochi.test:8888 on the desktop builds
and the test-container app on b2g builds.

Comment 9

[Carmen Jimenez Cabezas]

The permission check for mozSettings was introduced on Bug 815398. 
I'm going to check the other two checks and add them if needed.

Comment 10

[Carmen Jimenez Cabezas]

The permission check for webapps-manage was added on bug 814226. 
For DOMWifiManager, for some reason it checks the permission on each API call instead of just returning null if the caller doesn't have the permission.
I'm uploading a patch that returns null early if the app doesn't have the correct permission.

Comment 11

[Carmen Jimenez Cabezas]

Attachment: Changed DOMWifiManager so it returns null instead of creating the object if the caller doesn't have the correct permission

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 737555 [details] [diff] [review]
Changed DOMWifiManager so it returns null instead of creating the object if the caller doesn't have the correct permission

Review of attachment 737555 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for this patch without the Cu.reportError. I'd like to look at another patch if you want to keep the warning.

::: dom/wifi/DOMWifiManager.js
@@ +69,5 @@
>      // Only pages with perm set can use the wifi manager.
>      this._hasPrivileges = perm == Ci.nsIPermissionManager.ALLOW_ACTION;
>  
> +    if (!this._hasPrivileges) {
> +      Cu.reportError("NO WIFI MANAGE PERMISSION FOR: " + principal.origin + "\n");

I don't know if it's worth the Cu.reportError here given that this won't actually throw – is there a way that we can report a warning instead? Also, any warning that we do throw shouldn't be in all caps.

Comment 13

[Carmen Jimenez Cabezas]

Attachment: Cu.reportError removed

Keeping the r+ from the previous version (r=mrbkap). Just removed the log as requested

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/1574fba0dfa4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1574fba0dfa4