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.)

Marked as sec-low since this is desirable security control for the future, but not critical to launch.

See also https://bugzilla.mozilla.org/show_bug.cgi?id=900551#c7

Our team will take this as a Gecko work.

Remove the 2.0? tag until bug 900551 comment #16 is clear.

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.

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.

Forgot to switch out transaction type processing.

Forgot about readding app implicit perms, cleaned up transaction type building.

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.

(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++?

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.

I think you wanted to list that this /blocks/ bug 903683?

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.

Hi Kyle! I was wondering if there's any update on this one:)

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.

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.

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.

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.

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.

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(|

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

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.

https://hg.mozilla.org/integration/b2g-inbound/rev/a880d4e0d656 https://hg.mozilla.org/integration/b2g-inbound/rev/b01c7abafbdf

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

New try with fixed xpc shell tests. https://tbpl.mozilla.org/?tree=Try&rev=6c31c2ed45c1

https://hg.mozilla.org/integration/b2g-inbound/rev/744da032a2de https://hg.mozilla.org/integration/b2g-inbound/rev/9d38e8901847

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

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.

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

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

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

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

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

May we know the ETA for this bug? I'm asking because it's blocking bug 843452, which is also 2.1 committed feature.

https://hg.mozilla.org/integration/b2g-inbound/rev/b7a7dfbe4e3f https://hg.mozilla.org/integration/b2g-inbound/rev/b89c84a850f9 https://hg.mozilla.org/integration/b2g-inbound/rev/fa269ea53937

Comment on attachment 8480666 [details] Patch 4 (v1) - Fix permissions names in gaia atom tests Afraid this has not resolved the issue.

Backed out in: http://hg.mozilla.org/integration/b2g-inbound/rev/7c97c5f7a05e

Last patch actually contained some changes that should only be done after Bug 900551 lands. Landing just the permissions name changes here.

https://hg.mozilla.org/integration/b2g-inbound/rev/c107147819ea https://hg.mozilla.org/integration/b2g-inbound/rev/565b3b3526a7 https://hg.mozilla.org/integration/b2g-inbound/rev/3ecbe14009fb https://hg.mozilla.org/integration/b2g-inbound/rev/ab60d3c61f55

Clean try run for bug 846200: https://tbpl.mozilla.org/?tree=Try&rev=0f48ca820119

Gaia commit https://github.com/mozilla-b2g/gaia/commit/1a26b5a73f6f1b34b8886149d8879a546e43b59f

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

Unable to verify this bug until all depends on bugs (bug 1058158, bug 1055898) are closed out and verified

In addition to comment 53, I am also unable to verify this bug due to it being a back end issue

(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.

(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.

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.

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

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.

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"} },

(I'm see navigator.mozSettings is null, so I assume have the permission wrong )

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?

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.

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?

I've tried with and without settings-api. And yes, it's a privileged app.

Moving settings api issues discussion to bug 1107674