Closed Bug 815110 Opened 9 years ago Closed 8 years ago

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


(Core :: DOM: Device Interfaces, defect)

Not set



blocking-basecamp -
Tracking Status
b2g18 + affected


(Reporter: pauljt, Assigned: macajc)



(Whiteboard: [fixed-in-birch])


(1 file, 1 obsolete file)

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).
Blocks: 815105
No longer depends on: 815105
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.
Depends on: 815398
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.
blocking-basecamp: --- → ?
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.
blocking-basecamp: ? → -
I hope I found most of these in the followup bugs to bug 776834.
Nom-ing for tracking - this blocks a comprehensive testing suite for all permissions which would be nice to have in the future.
tracking-b2g18: --- → ?
Duplicate of this bug: 850609
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 (
Depends on: 776834
(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.
The permission check for mozSettings was introduced on Bug 815398. 
I'm going to check the other two checks and add them if needed.
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.
Assignee: nobody → macajc
Attachment #737555 - Flags: review?(mrbkap)
Depends on: 814226
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.
Attachment #737555 - Flags: review?(mrbkap) → review+
Keeping the r+ from the previous version (r=mrbkap). Just removed the log as requested
Attachment #737555 - Attachment is obsolete: true
Attachment #737863 - Flags: review+
Keywords: checkin-needed
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.