The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla23

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: pauljt, Assigned: Carmen Jimenez Cabezas)

Tracking

(Depends on: 1 bug)

unspecified
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-, b2g18+ affected)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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).
(Reporter)

Updated

4 years ago
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: --- → ?

Comment 3

4 years ago
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.
(Reporter)

Comment 5

4 years ago
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 (https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#188)
Depends on: 776834
status-b2g18: --- → affected
tracking-b2g18: ? → +

Comment 8

4 years ago
(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.
(Assignee)

Comment 9

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

Comment 10

4 years ago
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)

Comment 11

4 years ago
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
Assignee: nobody → macajc
Status: NEW → ASSIGNED
Attachment #737555 - Flags: review?(mrbkap)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 13

4 years ago
Created attachment 737863 [details] [diff] [review]
Cu.reportError removed

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+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/1574fba0dfa4
Keywords: checkin-needed
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/1574fba0dfa4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.