Closed
Bug 815110
Opened 12 years ago
Closed 12 years ago
Accessing settings, wifi, mozApps.mgmt APIs without permission kills app process.
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
People
(Reporter: pauljt, Assigned: macajc)
References
Details
(Whiteboard: [fixed-in-birch])
Attachments
(1 file, 1 obsolete file)
1.18 KB,
patch
|
macajc
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
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.
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•12 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•12 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:
--- → ?
Comment 7•12 years ago
|
||
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)
Updated•12 years ago
|
status-b2g18:
--- → affected
Comment 8•12 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•12 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•12 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•12 years ago
|
||
Comment 12•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Keywords: checkin-needed
Comment 14•12 years ago
|
||
Keywords: checkin-needed
Whiteboard: [fixed-in-birch]
Comment 15•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•