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)

Product:
Core
Component:
DOM: Device Interfaces
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
2.1 S3 (29aug)
Project Flags:
feature-b2g 2.1
Tracking Flags:
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)

Patch 2 (v1) - Update mochitests to use new settings-api permissions
(deleted), patch
bent.mozilla
: review+
Details | Diff | Splinter Review
Patch 1 (v5) - Support for granting settings permissions on a per-permission basis
(deleted), patch
bent.mozilla
: review+
Details | Diff | Splinter Review
Patch 3 (v1) - Update Marionette JS WebAPI Tests to use new settings-api permissions
(deleted), patch
echou
: review+
edgar
: review+
Details | Diff | Splinter Review
Patch 5 (v1) - Fix permission granting in reporting.py module
(deleted), patch
davehunt
: review+
Details | Diff | Splinter Review
Patch 4 (v2) - Fix permissions names in gaia atom tests
(deleted), text/x-github-pull-request
davehunt
: review+
Details 
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.
Keywords: sec-low
Our team will take this as a Gecko work.
Assignee: nobody → gene.lian
blocking-b2g: --- → 2.0?
Remove the 2.0? tag until bug 900551 comment #16 is clear.
blocking-b2g: 2.0? → ---
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
Blocks: 1003242
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.
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S2 (23may)
Forgot to switch out transaction type processing.
Attachment #8418992 - Attachment is obsolete: true
Forgot about readding app implicit perms, cleaned up transaction type building.
Attachment #8419008 - Attachment is obsolete: true
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.
(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++?
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.
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
Flags: in-moztrap?(jsmith)
feature-b2g: 2.0 → 2.1
Flags: in-moztrap?(jsmith)

        Attachment #8419018 -
        Attachment is obsolete: true

        Attachment #8419018 -
        Flags: review?(anygregor)

        Attachment #8435462 -
        Flags: review?(anygregor)
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)

        Attachment #8435462 -
        Flags: review?(anygregor) → review?(bent.mozilla)

        Attachment #8435464 -
        Flags: review?(anygregor) → review?(bent.mozilla)
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)

    
  
Depends on: 903683

    
    

    
  
I think you wanted to list that this /blocks/ bug 903683?
Blocks: 903683
No longer depends on: 903683
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-

        Attachment #8435464 -
        Flags: review?(bent.mozilla) → review+
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)

    
    

    
  
Hi Kyle! I was wondering if there's any update on this one:)
Flags: needinfo?(kyle)

    
    

    
  
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)

    
    

    
  
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)

    
    

    
  
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.
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)

    
    

    
  


  
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+

    
    

    
  
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.

    
  
No longer blocks: 903683

    
    

    
  
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

    
  
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Depends on: 1055898

    
  
Keywords: dev-doc-needed

    
    

    
  
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

        Attachment #8476462 -
        Flags: review+

    
    

    
  
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+
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)

    
    

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

    
    

    
  


  

        Attachment #8480666 -
        Flags: review?(zcampbell)

    
  
Blocks: 1053873

    
    

    
  
Comment on attachment 8480666 [details]
Patch 4 (v1) - Fix permissions names in gaia atom tests

Afraid this has not resolved the issue.

        Attachment #8480666 -
        Flags: review?(zcampbell) → review-

    
    

    
  


  
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

        Attachment #8481078 -
        Flags: review?(dave.hunt)

        Attachment #8481086 -
        Flags: review?(dave.hunt)

        Attachment #8481078 -
        Flags: review?(dave.hunt) → review+

        Attachment #8481086 -
        Flags: review?(dave.hunt) → review+

    
    

    
  
Unable to verify this bug until all depends on bugs (bug 1058158, bug 1055898) are closed out and verified
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)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)

    
    

    
  
(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.
Flags: needinfo?(ptheriault)
Flags: needinfo?(fabrice)

    
    

    
  
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)

    
    

    
  
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)

    
    

    
  
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?
Flags: needinfo?(bent.mozilla)

    
    

    
  
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)

    
    

    
  
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)

    
    

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

    
    

    
  
Moving settings api issues discussion to bug 1107674
Flags: needinfo?(bent.mozilla)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: