Last Comment Bug 815110 - Accessing settings, wifi, mozApps.mgmt APIs without permission kills app process.
: Accessing settings, wifi, mozApps.mgmt APIs without permission kills app proc...
Status: RESOLVED FIXED
[fixed-in-birch]
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla23
Assigned To: Carmen Jimenez Cabezas
:
Mentors:
: 850609 (view as bug list)
Depends on: 776834 814226 815398
Blocks: 815105
  Show dependency treegraph
 
Reported: 2012-11-26 05:27 PST by Paul Theriault [:pauljt]
Modified: 2013-04-19 08:15 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
+
affected


Attachments
Changed DOMWifiManager so it returns null instead of creating the object if the caller doesn't have the correct permission (1.26 KB, patch)
2013-04-15 10:05 PDT, Carmen Jimenez Cabezas
mrbkap: review+
Details | Diff | Splinter Review
Cu.reportError removed (1.18 KB, patch)
2013-04-16 01:57 PDT, Carmen Jimenez Cabezas
carmen.jimenezcabezas: review+
Details | Diff | Splinter Review

Description Paul Theriault [:pauljt] 2012-11-26 05:27:51 PST
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 2012-11-26 15:00:05 PST
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) PTO Until July 5th 2012-11-26 23:30:22 PST
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) 2012-11-27 11:23:06 PST
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!) 2012-11-27 12:00:46 PST
I hope I found most of these in the followup bugs to bug 776834.
Comment 5 Paul Theriault [:pauljt] 2013-04-11 00:34:33 PDT
Nom-ing for tracking - this blocks a comprehensive testing suite for all permissions which would be nice to have in the future.
Comment 6 Frederik Braun [:freddyb] [unavailable June 4th to August 29th] 2013-04-11 00:45:39 PDT
*** Bug 850609 has been marked as a duplicate of this bug. ***
Comment 7 [:fabrice] Fabrice Desré 2013-04-11 09:55:24 PDT
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] 2013-04-12 13:38:06 PDT
(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 2013-04-15 08:22:24 PDT
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 2013-04-15 09:46:35 PDT
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 2013-04-15 10:05:51 PDT
Created 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
Comment 12 Blake Kaplan (:mrbkap) 2013-04-15 17:06:18 PDT
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 2013-04-16 01:57:24 PDT
Created attachment 737863 [details] [diff] [review]
Cu.reportError removed

Keeping the r+ from the previous version (r=mrbkap). Just removed the log as requested
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-04-19 04:57:23 PDT
https://hg.mozilla.org/projects/birch/rev/1574fba0dfa4
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-04-19 08:15:22 PDT
https://hg.mozilla.org/mozilla-central/rev/1574fba0dfa4

Note You need to log in before you can comment on or make changes to this bug.