Closed
Bug 846200
Opened 12 years ago
Closed 10 years ago
Support for granting the 'settings' permission on a per-permission basis
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr31 | --- | wontfix |
People
(Reporter: pauljt, Assigned: qdot)
References
Details
(Keywords: dev-doc-needed, sec-low, Whiteboard: [systemsfe])
Attachments
(5 files, 5 obsolete files)
3.92 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
12.92 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
6.94 KB,
patch
|
echou
:
review+
edgar
:
review+
|
Details | Diff | Splinter Review |
976 bytes,
patch
|
davehunt
:
review+
|
Details | Diff | Splinter Review |
46 bytes,
text/x-github-pull-request
|
davehunt
:
review+
|
Details | Review |
Previous bugs (841071, 763965) discussed splitting the permission for the settings API ('settings') into discrete tiers, or even having allowing permission grants on a 'per-setting' basis.A solution was proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=841071#c13:
"...we should support granting access to settings on a per-setting basis. So that we can enumerate in the permissions section of the manifest enumerate that an application has read-only access to settings A and B, and read-write access to settings C and D."
Apps could then enumerate the minimal set of permissions required, so that if the app was in some way compromised, only the specified settings would be compromised.
We may also want to support keywords or tiers, to represent classes of settings ? (e.g. might we allow something like "readwrite access to alarm.* settings" or similar, taking the clock app as an example.)
Reporter | ||
Comment 1•12 years ago
|
||
Marked as sec-low since this is desirable security control for the future, but not critical to launch.
Keywords: sec-low
Reporter | ||
Updated•11 years ago
|
Blocks: b2gGeckoSecurity
Reporter | ||
Updated•11 years ago
|
Updated•11 years ago
|
blocking-b2g: --- → 2.0?
Comment 4•11 years ago
|
||
Remove the 2.0? tag until bug 900551 comment #16 is clear.
blocking-b2g: 2.0? → ---
Comment 5•11 years ago
|
||
After confirmation, we think this issue is not necessary for our team (stream-3) to take because our partner's homescreen apps don't have to be privileged for now. Sorry we don't really have resources to take such a v2.0 feature. Actually, we have some newbies be able to work on this but it might be too urgent for them to take. Hope either stream-1 or stream-2 team can take this.
Assignee: gene.lian → nobody
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → kyle
Kyle, come talk to me before you start working on this, there are some in-flight projects that will make this tricky.
I think we'd want something like this in the manifest:
"permissions": {
"settings-wallpaper.image": {
description: "To show and change default wallpaper in background of icon grid"
access: "readwrite"
}
"settings-screen.timeout": {
description: "So that we can show a funny animation of a cat right before the screen goes black"
access: "readonly"
}
}
This should mean that the existing infrastructure for handling permissions would "just work". We'd just need to make sure that the parent process looks up the right permission name in nsIPermissionManager before granting access to a particular read/write attempt.
In PermissionsTable.jsm we also need the ability to indicate that a permission can be granted to "read" for privileged apps, but "read" and "write" for certified apps.
Updated•11 years ago
|
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S2 (23may)
Assignee | ||
Comment 9•11 years ago
|
||
Assignee | ||
Comment 10•11 years ago
|
||
Forgot to switch out transaction type processing.
Attachment #8418992 -
Attachment is obsolete: true
Assignee | ||
Comment 11•11 years ago
|
||
Forgot about readding app implicit perms, cleaned up transaction type building.
Attachment #8419008 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8419018 -
Flags: review?(anygregor)
One thing that will be tricky to implement in JS is that you can only place requests against a lock
* Between when the lock is created an the caller returns to the event loop *or*
* During the onsuccess/onerror callback from a previous request to the lock
I think the first bullet will be hard to implement in JS. We currently use nsIAppShell to implement this in C++, but that's not scriptable.
An alternative would of course be to change the current API as it's not super great. But I don't know how we'd change it in a backwards compatible way.
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #12)
> One thing that will be tricky to implement in JS is that you can only place
> requests against a lock
>
> * Between when the lock is created an the caller returns to the event loop
> *or*
> * During the onsuccess/onerror callback from a previous request to the lock
>
> I think the first bullet will be hard to implement in JS. We currently use
> nsIAppShell to implement this in C++, but that's not scriptable.
So in our current situation, we throw a task on the queue when creating a lock that closes the lock when we return to the event loop:
http://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager.js#41
In the OOP case, we can just keep doing the same thing, except this function will now fire a message to the parent to close the lock, instead of closing the lock itself. Seems like that should keep us working without having to go to C++?
Updated•11 years ago
|
feature-b2g: --- → 2.0
Ah, cool.
You should do all lock management in the child process so that you're not racing. The purpose of the lock management is just to avoid bugs and to enable "auto committing". So it's not a security issue. So it's fine to do entirely in the child process.
So having the child process fire a runnable to the event loop and "closing" the lock in the child process should work well enough.
Updated•11 years ago
|
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Updated•11 years ago
|
Flags: in-moztrap?(jsmith)
Updated•10 years ago
|
feature-b2g: 2.0 → 2.1
Updated•10 years ago
|
Flags: in-moztrap?(jsmith)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8419018 -
Attachment is obsolete: true
Attachment #8419018 -
Flags: review?(anygregor)
Attachment #8435462 -
Flags: review?(anygregor)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8435464 -
Flags: review?(anygregor)
Updated•10 years ago
|
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Updated•10 years ago
|
Attachment #8435462 -
Flags: review?(anygregor) → review?(bent.mozilla)
Updated•10 years ago
|
Attachment #8435464 -
Flags: review?(anygregor) → review?(bent.mozilla)
Updated•10 years ago
|
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Assignee | ||
Comment 18•10 years ago
|
||
I think you wanted to list that this /blocks/ bug 903683?
Updated•10 years ago
|
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Comment on attachment 8435462 [details] [diff] [review]
Patch 1 (v4) - Support for granting settings permissions on a per-permission basis
Review of attachment 8435462 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/settings/SettingsManager.js
@@ +378,5 @@
> + continue;
> + }
> + if (Services.perms.testExactPermissionFromPrincipal(this._window.document.nodePrincipal, permName) == Ci.nsIPermissionManager.ALLOW_ACTION) {
> + if(permName.indexOf("-write") != 0) {
> + this.hasAnyWritePrivileges = true;
This is wrong... You want |> 0| otherwise hasAnyWritePrivileges is almost always going to be true.
Please figure out why our tests don't catch this? Seems like a pretty big hole.
Attachment #8435462 -
Flags: review?(bent.mozilla) → review-
Updated•10 years ago
|
Attachment #8435464 -
Flags: review?(bent.mozilla) → review+
Updated•10 years ago
|
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Comment 20•10 years ago
|
||
Hi Kyle! I was wondering if there's any update on this one:)
Flags: needinfo?(kyle)
Assignee | ||
Comment 21•10 years ago
|
||
I'm working through reviews right now, but the one for the related bug 900551 is very, very long, so it's taking me a while. Not only that, bent's out until next Monday, so we won't see any of this land before then so he'll need to do another review. So hoping to land early next week.
Flags: needinfo?(kyle)
Comment 22•10 years ago
|
||
I just learned about this bug today. Will this proposal work for any setting, or will there be a whitelist of settings for which privileged apps can get permission?
I ask because our DRM-FL ("forward lock") implementation relies for its security on the fact that the settings db is accessible only to certified apps. See shared/js/omadrm/fl.js. So if there is not a whitelist, I would like it if this patch could ensure that settings with the 'oma.drm' prefix will never be accessible to privileged apps, no matter what permissions they have. That is: I think the patch needs to establish a black list of settings (and setting prefixes) that are never accessible to non-certified apps.
Rather than making this blacklist completely ad-hoc, I'd say put oma.drm.* in it and define a special certified-only prefix like "certified.*" for future use.
Setting needinfo for Kyle, Jonas and Paul so they are all aware of this security issue.
Flags: needinfo?(ptheriault)
Flags: needinfo?(kyle)
Flags: needinfo?(jonas)
Assignee | ||
Comment 23•10 years ago
|
||
There's a whitelist. When hitting the settings API, privileged apps are required to have a permission which will pertain to a specific setting. i.e. settings:wallpaper-image. This permission will have to be added to the Permissions Table, so the only way to add/remove those is between gecko version changes/upgrades. As long as we don't make a settings:oma.drm.? permission, privileged apps won't have access to those settings.
Clearing all ni's 'cause I got this.
Flags: needinfo?(ptheriault)
Flags: needinfo?(kyle)
Flags: needinfo?(jonas)
Permission will be granted on a per-setting basis. That's the whole point of this bug.
We can keep some settings as certified, some as privileged and even some as "normal" (though that would be a problem for webcompat). And applications will only get access to the settings that they request permission for. So even if the app gets hacked, the attacker will only have access to the settings that the app enumerates in its manifest.
Updated•10 years ago
|
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Assignee | ||
Comment 25•10 years ago
|
||
Fixed issue with incorrect index check. Note that at this point this is mostly a formality since 900551 will probably land right after this and it's a rewrite of most of the system.
Attachment #8435462 -
Attachment is obsolete: true
Attachment #8472767 -
Flags: review?(bent.mozilla)
Comment on attachment 8472767 [details] [diff] [review]
Patch 1 (v5) - Support for granting settings permissions on a per-permission basis
Review of attachment 8472767 [details] [diff] [review]:
-----------------------------------------------------------------
This looks great!
::: dom/settings/SettingsManager.js
@@ +236,5 @@
> + // Check each requested permission to make sure we can set it
> + let keys = Object.getOwnPropertyNames(aSettings);
> + for (let i = 0; i < keys.length; i++) {
> + if (!this._settingsManager.hasWritePermission("settings:" + keys[i])) {
> + if (DEBUG) debug("set not allowed on" + keys[i]);
Nit: add a space before the second quotation mark.
@@ +382,5 @@
> + if (permName.indexOf("settings") != 0) {
> + continue;
> + }
> + if (Services.perms.testExactPermissionFromPrincipal(this._window.document.nodePrincipal, permName) == Ci.nsIPermissionManager.ALLOW_ACTION) {
> + if(permName.indexOf("-write") > 0) {
Nit: Space between |if(|
Attachment #8472767 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 27•10 years ago
|
||
Running through try to make sure it'll land by itself, been doing too much testing of this with bug 900551 in front of it. :/
https://tbpl.mozilla.org/?tree=Try&rev=13dff985bdc5
Assignee | ||
Comment 28•10 years ago
|
||
Ok, looks like I need to backport the test ordering fixes from bug 900551 to here and retry, as well as add permissions to geolocation. Doing that and running another try, then will land.
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Comment 30•10 years ago
|
||
Comment 31•10 years ago
|
||
Reverted for xpcshell failures in test_bug808734.js:
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=b01c7abafbdf&jobname=xpcshell
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/dom/permission/tests/unit/test_bug808734.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/dom/permission/tests/unit/test_bug808734.js | 6 == 4 - See following stack:
remote: https://hg.mozilla.org/integration/b2g-inbound/rev/a3f9b8f4010b
remote: https://hg.mozilla.org/integration/b2g-inbound/rev/0563896f4c7a
Updated•10 years ago
|
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 32•10 years ago
|
||
New try with fixed xpc shell tests.
https://tbpl.mozilla.org/?tree=Try&rev=6c31c2ed45c1
Assignee | ||
Comment 33•10 years ago
|
||
This, bug 900551, and bug 1015518 were all backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/c4c490f3b301 for build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=46297008&tree=B2g-Inbound
Flags: needinfo?(kyle)
Assignee | ||
Comment 35•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/82bed38ff50a
Reverted https://hg.mozilla.org/integration/b2g-inbound/rev/c4c490f3b301. Build break in https://tbpl.mozilla.org/php/getParsedLog.php?id=46297008&tree=B2g-Inbound was due to bug 1055898, no changes made to original commits.
Flags: needinfo?(kyle)
Assignee | ||
Comment 36•10 years ago
|
||
And now backed out again due to marionette webapi test bustage on ics emulator
https://tbpl.mozilla.org/?tree=B2g-Inbound&showall=1&rev=82bed38ff50a
https://hg.mozilla.org/integration/b2g-inbound/rev/723c04adba8a
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 37•10 years ago
|
||
Reran try on offending platforms. Landing this alone since it greened up.
https://tbpl.mozilla.org/?tree=Try&rev=2f515cc84838
https://hg.mozilla.org/integration/b2g-inbound/rev/d71120161e89
https://hg.mozilla.org/integration/b2g-inbound/rev/b5d49f1885af
Assignee | ||
Comment 38•10 years ago
|
||
Backed out yet again because the patch I reapplied for bug 846200 was missing xpcshell test patch which now caused desktop failures again. Getting ahead of myself. Oi. Log follows.
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=b5d49f1885af
Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/0611006cc095
Assignee | ||
Comment 39•10 years ago
|
||
Comment 40•10 years ago
|
||
Comment on attachment 8476462 [details] [diff] [review]
Patch 3 (v1) - Update Marionette JS WebAPI Tests to use new settings-api permissions
Review of attachment 8476462 [details] [diff] [review]:
-----------------------------------------------------------------
r+ for
- dom/bluetooth/tests/marionette/head.js
- test_dom_BluetoothManager_enabled.js
- test_dom_BluetoothManager_API2.js
Attachment #8476462 -
Flags: review+
Comment 41•10 years ago
|
||
Comment on attachment 8476462 [details] [diff] [review]
Patch 3 (v1) - Update Marionette JS WebAPI Tests to use new settings-api permissions
Review of attachment 8476462 [details] [diff] [review]:
-----------------------------------------------------------------
r+ for
- test_dsds_mobile_data_connection.js
- test_mobile_data_connection.js
- test_mobile_data_ipv6.js
- test_mobile_set_radio.js
Attachment #8476462 -
Flags: review+
Updated•10 years ago
|
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Comment 42•10 years ago
|
||
May we know the ETA for this bug?
I'm asking because it's blocking bug 843452, which is also 2.1 committed feature.
Assignee | ||
Comment 43•10 years ago
|
||
Assignee | ||
Comment 44•10 years ago
|
||
Attachment #8480666 -
Flags: review?(zcampbell)
Comment 45•10 years ago
|
||
Comment on attachment 8480666 [details] [review]
Patch 4 (v1) - Fix permissions names in gaia atom tests
Afraid this has not resolved the issue.
Attachment #8480666 -
Flags: review?(zcampbell) → review-
Comment 46•10 years ago
|
||
Assignee | ||
Comment 47•10 years ago
|
||
Assignee | ||
Comment 48•10 years ago
|
||
Last patch actually contained some changes that should only be done after Bug 900551 lands. Landing just the permissions name changes here.
Attachment #8480666 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8481078 -
Flags: review?(dave.hunt)
Assignee | ||
Updated•10 years ago
|
Attachment #8481086 -
Flags: review?(dave.hunt)
Updated•10 years ago
|
Attachment #8481078 -
Flags: review?(dave.hunt) → review+
Updated•10 years ago
|
Attachment #8481086 -
Flags: review?(dave.hunt) → review+
Assignee | ||
Comment 49•10 years ago
|
||
Assignee | ||
Comment 50•10 years ago
|
||
Clean try run for bug 846200:
https://tbpl.mozilla.org/?tree=Try&rev=0f48ca820119
Assignee | ||
Comment 51•10 years ago
|
||
Comment 52•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c107147819ea
https://hg.mozilla.org/mozilla-central/rev/565b3b3526a7
https://hg.mozilla.org/mozilla-central/rev/3ecbe14009fb
https://hg.mozilla.org/mozilla-central/rev/ab60d3c61f55
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Unable to verify this bug until all depends on bugs (bug 1058158, bug 1055898) are closed out and verified
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-verify-]
In addition to comment 53, I am also unable to verify this bug due to it being a back end issue
QA Whiteboard: [QAnalyst-verify-] → [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
Comment 55•10 years ago
|
||
(In reply to Derek Harris [:DerekH] from comment #54)
> In addition to comment 53, I am also unable to verify this bug due to it
> being a back end issue
Basically you need a privileged party app that uses the settings API and the new syntax for accessing settings to verify this.
It can also be added to one of our existing test apps that are not certified.
Updated•10 years ago
|
status-firefox-esr31:
--- → wontfix
Comment 56•10 years ago
|
||
(In reply to Gregor Wagner [:gwagner] from comment #55)
> (In reply to Derek Harris [:DerekH] from comment #54)
> > In addition to comment 53, I am also unable to verify this bug due to it
> > being a back end issue
>
> Basically you need a privileged party app that uses the settings API and the
> new syntax for accessing settings to verify this.
> It can also be added to one of our existing test apps that are not certified.
Paul, Fabrice, do you know of a test app that uses settings API and is privileged? QA is unable to verify this patch unless we have one provided. any direction would be helpful.
Flags: needinfo?(ptheriault)
Flags: needinfo?(fabrice)
Comment 57•10 years ago
|
||
Currently there is no privileged test app using settings since that was a certified only permission.
The dev_apps/uitest-privileged app could be augmented to test changing the wallpaper.
Flags: needinfo?(fabrice)
Reporter | ||
Comment 58•10 years ago
|
||
Tony, not sure if you solved your problem here or not, but when looking to understand this more, I found that bug 900551 was more helpful, since that has example of setting/getting wallpaper settings using a privileged permission.
Specifically, see:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=900551&attachment=8479602
Flags: needinfo?(ptheriault)
Comment 59•10 years ago
|
||
Paul, is creating a simple app that we can use to verify this issue. We will verify this as soon as we receive that app.
Reporter | ||
Comment 60•10 years ago
|
||
So I'm stuck actually here trying to test this. Is the following permissions declaration correct (from my manifest)?
"permissions":{
"settings:wallpaper.image":{"access":"readwrite"},
"settings-api":{"access":"readwrite"}
},
Reporter | ||
Comment 61•10 years ago
|
||
(I'm see navigator.mozSettings is null, so I assume have the permission wrong )
Comment 62•10 years ago
|
||
We want to use this API for the BuddyUp project and I'm seeing the same problem.
Ben: As the reviewer, can you help Paul and I test this?
Flags: needinfo?(bent.mozilla)
Comment 63•10 years ago
|
||
I've looked at this with Alex Lissy and Fabrice and no one could figure out what the issue is here.
Kyle: See comment 61 and 62.
Flags: needinfo?(kyle)
Assignee | ||
Comment 64•10 years ago
|
||
You shouldn't need to set settings-api manually, that's done automatically by using any settings permission. Is the app you're trying to use this with privileged?
Flags: needinfo?(kyle)
Comment 65•10 years ago
|
||
I've tried with and without settings-api. And yes, it's a privileged app.
Assignee | ||
Comment 66•10 years ago
|
||
Moving settings api issues discussion to bug 1107674
Updated•10 years ago
|
Flags: needinfo?(bent.mozilla)
You need to log in
before you can comment on or make changes to this bug.
Description
•