Closed Bug 812289 Opened 12 years ago Closed 12 years ago

PermissionSettings doesn't enforce any restriction on permissions operations

Categories

(Firefox OS Graveyard :: General, defect, P4)

Product:
Firefox OS Graveyard
Component:
General
Platform:
All
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-basecamp:-, firefox19 fixed, firefox20 fixed, b2g18+ fixed)

Status:
RESOLVED FIXED
Milestone:
B2G C3 (12dec-1jan)
Project Flags:
blocking-basecamp -
Tracking Flags:
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 + fixed

People

(Reporter: amac, Assigned: amac)

References

Details

Attachments

(3 files, 10 obsolete files)

patch v9. Reviewed version with fixed exception throwing
45.51 KB, patch
sicking
: review+
sicking
: approval-mozilla-aurora+
sicking
: approval-mozilla-b2g18+
Details | Diff | Splinter Review
Interdiff for v8 and v9
513 bytes, patch
Details | Diff | Splinter Review
Patch for the API tests
6.61 KB, patch
fabrice
: review+
Details | Diff | Splinter Review 
With the current implementation of PermissionSettings there's no enforcement at all of any kind of valid operations. 

Any app with the "permissions" permission can basically grant, or revoke, any permission to any application, regardless of the application type and the permission. An application with this permission can, for example, grant the dial permission (certified only permission) to a web application. 

Even if the current UI application behaves correctly, this should be changed. Enforcement should not be on the UI side but on the platform side
I'm noming this because without fixing it we've done away with the defense in-depth for all practical senses. An attacker can get access to all APIs just by compromising an application that has the permission settings. This is precisely what was being avoided by making all apps, including certified ones, define expressly which permissions it needs/uses.
blocking-basecamp: --- → ?
I agree. The sandbox is useless without this.
blocking-basecamp: ? → +
Assignee: nobody → fabrice
Oh, I had misunderstood this bug.

Is this bug requesting that you should never be allowed to use the Permissions API in order to not grant a permission to an app if the app wouldn't otherwise have the ability to get that access?

I.e. in order to be able to grant a permission to an app through the permissions API, the app must both

* Enumerate the permission in its manifest.
* The app type must have PROMPT or ALLOW access according to the table in
  PermissionsInstaller.jsm.

Is that correct?

If so, I think this is a nice-to-have rather than a blocker. Only the settings app has access to permissions API and if an attacker can hack that they have access to most things on the phone anyway.

Lucas: What do you think?
blocking-basecamp: + → ?
Flags: needinfo?(ladamski)
Oh, I had misunderstood this bug.

Is this bug requesting that you should never be allowed to use the Permissions API in order to not grant a permission to an app if the app wouldn't otherwise have the ability to get that access?

I.e. in order to be able to grant a permission to an app through the permissions API, the app must both

* Enumerate the permission in its manifest.
* The app type must have PROMPT or ALLOW access according to the table in
  PermissionsInstaller.jsm.

Is that correct?

If so, I think this is a nice-to-have rather than a blocker. Only the settings app has access to permissions API and if an attacker can hack that they have access to most things on the phone anyway.

Lucas: What do you think?
I think we have both problems... we can't prevent apps from being granted (or denied for that matter) permissions which they neither requested in their manifest nor are permitted according to their app type.  Also, we can't readily tell the difference between an denied or implicit permission and an explicit one which has been granted or disallowed.
Flags: needinfo?(ladamski)
(In reply to Jonas Sicking (:sicking) from comment #3)
> Oh, I had misunderstood this bug.
> 
> Is this bug requesting that you should never be allowed to use the
> Permissions API in order to not grant a permission to an app if the app
> wouldn't otherwise have the ability to get that access?
> 
> I.e. in order to be able to grant a permission to an app through the
> permissions API, the app must both
> 
> * Enumerate the permission in its manifest.
Correct. 

> * The app type must have PROMPT or ALLOW access according to the table in
>   PermissionsInstaller.jsm.
Correct. It would be more correct though, to say that only permissions that have PROMPT in the table can be modified. If it has ALLOW or DENY then the permission must not be modified using the API. 



> 
> Is that correct?
> 
> If so, I think this is a nice-to-have rather than a blocker. Only the
> settings app has access to permissions API and if an attacker can hack that
> they have access to most things on the phone anyway.

Oh, I disagree. Obviously. And vociferously :). With this fixed, compromising the settings app doesn't allow 
placing silent calls. Or sending SMS. Or anything that isn't expressly requested on the manifest. Which was 
the main point in having apps declaring explicitly the permissions they need. 

Without fixing this, though, the settings app is a single, catasthropic, fail point. 
Or any app that has the permissions permission, we only use that on settings but there's nothing
that forbids using it on other apps). For all practical senses, if we don't fix this,  we have given a 
nice, supported, documented way of making irrelevant the permissions matrix and even the 
application type irrelevant. Want to have a web app that has access to the phone API? No 
problem, just use this API to give it. 

As I said, this makes defense in depth irrelevant. Or rather, we have defense in depth 
with a depth of 1. 

So, no, this is not a nice to have, this is a must fix in my book. Any restriction that isn't 
enforced is wet paper. 

> 
> Lucas: What do you think?
Lucas: I don't think I understand comment 5. The question is if this bug should be a blocker.

We certainly can post-installation tell what the contents of the manifest is. I.e. we can tell what application type (web vs. privileged vs. certified) it is. We can also tell which privileges it has requested in the permissions property. So this bug is definitely technically fixable.

However there are several ways we are screwed if the settings app gets hacked:
* Use navigator.mobileconnection to change telephony settings. I would expect
  that there are ways they can redirect traffic to a pay-for phone number,
  though I don't know that for sure.
* Use the settings API to enable debugging mode and then install a custom app
  which then gets arbitrary privileges. I think you can install even certified
  apps this way.
* Use the deviceStorage API to overwrite the contents of an app which has the
  privileges that you want access to.
* Use the deviceStorage API to overwrite the permissions database (i'm not 100%
  sure this is possible).
* Use the deviceStorage API to overwrite the app registry (I'm not sure that
  this is possible).

The last three are pretty easy to fix since nowadays I think we can grant read-only access to the DeviceStorage API to the system app.
Flags: needinfo?(ladamski)
(In reply to Jonas Sicking (:sicking) from comment #7)
> Lucas: I don't think I understand comment 5. The question is if this bug
> should be a blocker.
> 
> We certainly can post-installation tell what the contents of the manifest
> is. I.e. we can tell what application type (web vs. privileged vs.
> certified) it is. We can also tell which privileges it has requested in the
> permissions property. So this bug is definitely technically fixable.
> 
> However there are several ways we are screwed if the settings app gets
> hacked:

Having a problem anyway isn't a reason to not fix a worse problem. In any case...

> * Use navigator.mobileconnection to change telephony settings. I would expect
>   that there are ways they can redirect traffic to a pay-for phone number,
>   though I don't know that for sure.

Hmm Mobile Connection main danger is that they can block the PIN by repeatedly passing an incorrect/random value to the unlockCardLock function (I'm assuming we're talking about [1] here). There's no way to change traffic there that I can see. 

> * Use the settings API to enable debugging mode and then install a custom app
>   which then gets arbitrary privileges. I think you can install even
> certified apps this way.

Nah, it's worse than that, you can get a hosted app that behaves like a certified app if I'm reading [2] correctly. Dev mode, as it currently stands, is another accident waiting to happen. In my opinion, we're giving away too much control too easily (just changing a pref which can be done by an app does away with any practical control). If this isn't changed, developers should be made very very very very aware that browsing the web with developer mode active is not safe. At all! As a matter of fact, I'm filing another bug for that, thanks for bringing that out. 

> * Use the deviceStorage API to overwrite the contents of an app which has the
>   privileges that you want access to.
> * Use the deviceStorage API to overwrite the permissions database (i'm not
> 100%
>   sure this is possible).
> * Use the deviceStorage API to overwrite the app registry (I'm not sure that
>   this is possible).

Er all of this should not be possible. Do the deviceStorage API give raw access to all of the file system? Is it executed on the parent process (and served via IPC) or on the child process? 

 
> The last three are pretty easy to fix since nowadays I think we can grant
> read-only access to the DeviceStorage API to the system app.

Yeah, it actually requires read-only permission to all of that only. Dunno what does it uses all of those permissions for. But again, you're agreeing with me... if we don't fix this bug, whatever the settings app requests is meaningless since it can just give itself (or any other app) any permission.


[1] https://wiki.mozilla.org/WebAPI/WebMobileConnection
[2] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1095
Created Bug 813797 for the dev_mode issue.
(In reply to Antonio Manuel Amaya Calvo from comment #8)
> (In reply to Jonas Sicking (:sicking) from comment #7)
> > Lucas: I don't think I understand comment 5. The question is if this bug
> > should be a blocker.
> > 
> > We certainly can post-installation tell what the contents of the manifest
> > is. I.e. we can tell what application type (web vs. privileged vs.
> > certified) it is. We can also tell which privileges it has requested in the
> > permissions property. So this bug is definitely technically fixable.
> > 
> > However there are several ways we are screwed if the settings app gets
> > hacked:
> 
> Having a problem anyway isn't a reason to not fix a worse problem. In any
> case...

I agree it's not a reason not to fix it. But I'd say it's a reason to not hold the release for it, which is what the requirement for a blocking-basecamp bug is.

> > * Use navigator.mobileconnection to change telephony settings. I would expect
> >   that there are ways they can redirect traffic to a pay-for phone number,
> >   though I don't know that for sure.
> 
> Hmm Mobile Connection main danger is that they can block the PIN by
> repeatedly passing an incorrect/random value to the unlockCardLock function
> (I'm assuming we're talking about [1] here). There's no way to change
> traffic there that I can see. 

What about changing APN settings?

> > * Use the settings API to enable debugging mode and then install a custom app
> >   which then gets arbitrary privileges. I think you can install even
> > certified apps this way.
> 
> Nah, it's worse than that, you can get a hosted app that behaves like a
> certified app if I'm reading [2] correctly. Dev mode, as it currently
> stands, is another accident waiting to happen. In my opinion, we're giving
> away too much control too easily (just changing a pref which can be done by
> an app does away with any practical control). If this isn't changed,
> developers should be made very very very very aware that browsing the web
> with developer mode active is not safe. At all! As a matter of fact, I'm
> filing another bug for that, thanks for bringing that out. 

It does indeed seem like we should look into changing the developer mode.

Lets continue this discussion in bug 813797.

> > * Use the deviceStorage API to overwrite the contents of an app which has the
> >   privileges that you want access to.
> > * Use the deviceStorage API to overwrite the permissions database (i'm not
> > 100%
> >   sure this is possible).
> > * Use the deviceStorage API to overwrite the app registry (I'm not sure that
> >   this is possible).
> 
> Er all of this should not be possible. Do the deviceStorage API give raw
> access to all of the file system? Is it executed on the parent process (and
> served via IPC) or on the child process? 

The settings app has special permission to the DeviceStorage API which other apps do not have. It needs this to be able to see amount of used and free disk space. We can and should reduce this to read-only access though.
If I read this bug correctly, we want the following changes made the to the settings API
1. A permission X for app Y can only be modified if Y's manifest explicitly requested and was granted X
2. Permissions changes can only take on one of the follow transitions
a) Back to default state (dependent on appStatus and permissions table), e.g. user granted a PROMPT_ACTION with remember, we should be able to change back to PROMPT_ACTION
b) From PROMPT_ACTION to either ALLOW_ACTION or DENY_ACTION

There is a third state I omitted which is from ALLOW_ACTION to DENY ACTION. This wouldn't make the app less secure but could be leveraged to deny / break app functionality in a negative way.

I don't think it would be that difficult to code up and can take the bug from Fabrice.

What should we do if the API attempts to perform an illegal change? I'm leaning toward throwing an error.
My opinion after digging into the UI equivalent of this bug (bug 816806) - I'd block on bug 816806, but I wouldn't block on this bug. The settings api is a certified-only webapi, so we have control over who has the right to call it (it's under our control). If we take the gaia route and remove any exposure of the UI for certified apps, then we won't hit the issues brought up on this bug.

It's certainly beneficial to follow defense in depth principles though such that we check in multiple areas. But I think the piece that blocks is the certified app UI exposure to this API.
(In reply to Jason Smith [:jsmith] from comment #12)
> My opinion after digging into the UI equivalent of this bug (bug 816806) -
> I'd block on bug 816806, but I wouldn't block on this bug. The settings api
> is a certified-only webapi, so we have control over who has the right to
> call it (it's under our control). If we take the gaia route and remove any
> exposure of the UI for certified apps, then we won't hit the issues brought
> up on this bug.

The settings API being a certified-only webapi means we have access about which apps use it right now. The moment we release V1 into the wild, even to our partners builders, we lose that control because they have complete power to add their own apps to the actual phone builds. And there's nothing magical on the "permissions" permission that will make it to fail in case other certified app request it.

I see Fabrice's has this, but if part of the argument is not having resources to do this, Carmen or myself can take this bug to make it happen for V1. I think "it's wrong but only we use it and it's only used currently on one place" isn't a reason not to fix something that could potentially be a BIG security hole.
Antonio, feel free to steal this bug and re-assign it to you or Carmen!
Assignee: fabrice → amac
(In reply to Fabrice Desré [:fabrice] from comment #14)
> Antonio, feel free to steal this bug and re-assign it to you or Carmen!

Ok, I took this :) Thanks Fabrice.
Status: NEW → ASSIGNED
I'll describe here what I'm going to change so if anyone thinks it would be better to do it otherwise I can do it just once, hopefully :).

All this bug can be summarized in: Permissions that are not user-modifiable should not be modified by users. 

As it stands now, permissions are initially installed on PermissionsInstaller.jsm. PermissionsInstaller.jsm is the only one that currently know if a permission should be  user modifiable or not. PermissionsInstaller calls PermissionSettingsModule ('backend' part of the public permissions API) which in turn calls PermissionManager.addFromPrincipal.

So what I'm going to do is two part:

* Make PermissionsInstaller propagate his information (is a permission user-modifiable or not) towards PermissionManager via PermissionSettingsModule. I'll add this information to the Permissions database on PermissionManager also (since it makes sense to store it).
* Make PermissionSettings check this flag on "set" (and return it on "get", since the settings app needs this information, otherwise it cannot know which permissions it must show, see bug 817031). The "set" logic will be then:
   * Extract the current value of the permission. If the permission is "unknown", return (only existing permissions can be modified, new permissions should NOT be added this way).
   * If the permission is marked as modifiable, then modify the value. Otherwise, return.

Oh, as it currently stands, a permission is modifiable if

a) it's defined on the app manifest 
AND
b) its type is PROMPT_ACTION.

Unless anyone has an objection, this is what I plan to implement.
Blocks: 817031
Blocks: 817034
Antonio - That sounds fine. I agree with the points made in comment 13 and comment 16.
I know some people are not fond of the terminology, but I'm wondering if we could stick to "explicit" vs "implicit" terminology in this api.  Implicit == !prompt && !modifiable, explicit == prompt && modifiable.

If we just use modifiable, it can once again create confusion as to what that means for prompting (and vice versa).
Flags: needinfo?(ladamski)
That's a fair point, Lucas. I can just name the new field isExplicit then, with almost the same semantics as isModifiable:

isModifiable=(action!=unknown)&&isExplicit

I'll get to it then :)
Note that the installation code uses this API currently for all modifications of the permissions database.

So we'll likely have to change that to use something else since otherwise whatever security checks are added here will prevent installation/updates from working properly.
Unless I'm mistaken, the schema is something like (gotta love ASCII diagrams :P )


PermissionsInstaller ------------------------------------
                                                        |
                                                        v
ThirdPartyCode  -----> PermissionsSettings -> PermissionsSettingsModule

Thus I just intended to add the checks to PermissionsSettings, which isn't used internally to manage the permissions.
> Any app with the "permissions" permission can basically grant, or revoke, any permission to any application

Yes.  The fix is to not grant any non-trusted application this permission.  The only app that will ever probably have this permission is Settings.  So, the only way that this is a problem is if the Settings application gets hacked.  If the setting application gets hacked, we have other problems right?
blocking-basecamp: ? → -
(In reply to Doug Turner (:dougt) from comment #23)
> The fix is to not grant any non-trusted application this permission. 

s/non-trusted/non-certified/
(In reply to Doug Turner (:dougt) from comment #23)
> > Any app with the "permissions" permission can basically grant, or revoke, any permission to any application
> 
> Yes.  The fix is to not grant any non-trusted application this permission. 
> The only app that will ever probably have this permission is Settings.  So,
> the only way that this is a problem is if the Settings application gets
> hacked.  If the setting application gets hacked, we have other problems
> right?

Round and round the wheel goes :). I'll have a fix for this by this Friday. In any case, if you don't want to apply the fix, then a better solution than just saying "the solution is to ignore the problem and hope for the best" would be just to disable this API completely until it's fixed. As it stands now, this API converts the rest of the security model into wet paper.
> As it stands now, this API converts the rest of the security model into wet paper.

No it doesn't.  It means if that permission incorrectly granted to an application, then that application can grant any permission to any application.  My point is that we must not grant this permission to untrusted applications.  We need to be deliberate when granting these sorts of permissions to applications in general.  No one should get this permission save a few applications.

> I'll have a fix for this by this Friday.

Great!  We'd love to have it fixed.  Please request uplift on the patch.
Attached patch Initial version of the patch. (obsolete) — — Splinter Review 
I don't know who should review this patch, if I chose incorrectly please punt it the right way.

The patch includes some whitespace changes for lines I didn't actually touch, because I thought (incorrectly it seems) that any trailing whitespaces would be mine and I did a M-x delete-trailing-whitespace on all the files I touched after I was done with them.

I'll explain what I finally implemented on another comment.
Attachment #689238 - Flags: review?(jonas)
Attachment #689238 - Flags: review?(fabrice)
Ok, I uploaded a initial version of the patch, as promised. The patch actually has all the required functionality, with one small caveat: I'm currently not killing the child process that make incorrect petitions. The code is there but it's commented out, since there was no easy way to try it. 

What I did finally was:

* Added a new field, isExplicit (autoexplicatory I hope) to the permission database. 

* PermissionsManager fills this field upon insertion of a new permission. 
  The field is marked as true if the permission value is PROMPT_ACTION, and false 
  otherwise. 

* PermissionSettingsModule allows any modification when called from the parent
  process, but only allows updating explicit permissions when called from the 
  child process. Currently it DOES NOT kill a child process that request a change
  on a implicit permission, although it probably should since that's controlled 
  also on the child interface. It DOES kill, though, any child process that request
  a modification without having the "permissions" permission.

* PermissionSettings.set fails silently if when trying to set a permission that
  isn't "explicit". So it cannot be used to set new permissions or modify implicit
  ones.

* PermissionSettings.get now returns permvalue + "/" + permtype (for example, 
  allow/explicit, allow/implicit, deny/explicit...) except for "unknown" which it 
  still returns as "unknown" only.

And I think that's about it. I had to change PermissionsManager interface to add a new method to expose isExplicit towards PermissionSettings and PermissionSettingsModule.
And renoming again. Besides still thinking that the original problem is serious enough to block on it, while fixing it I discover another thing... the actual implementation doesn't even check if the child process invoking it has the permissions permission. 

So it's not just a question of hacking the settings app (although I still don't agree that's the original problem). With the current implementation, if *any* child process is compromised at shellcode level, then it can request (and be granted) any permission for itself or any other app. 

In any case, this is patched (pending of review of course), so I hope this argument is moot now :).
blocking-basecamp: - → ?
can you run a try build with your patch? thanks.
blocking-basecamp: ? → +
I made a bug and a PR with the application I used to test this on 
https://bugzilla.mozilla.org/show_bug.cgi?id=818954
(In reply to Fabrice Desré [:fabrice] from comment #30)
> can you run a try build with your patch? thanks.

Sure... don't know how though. Are you talking about 
https://wiki.mozilla.org/ReleaseEngineering/TryServer ?

Do I have permission to do that?
Priority: -- → P1
yes, this is the try server. If you have commit Level 1, you can use it. If you don't, I'll try to push to try later today.
I don't think I have a commit level at all. Not unless its given automatically at least, I do know I haven't done anything to get that.
You have to file an IT bug to get commit access.

Meanwhile I pushed your patch to the try server: https://tbpl.mozilla.org/?tree=Try&rev=33f9e7e6de51
Build seems to have failed with a... tar error?

DEBUG: Executing command: ['tar', '--use-compress-program', 'pigz', '-xf', '/builds/mock_mozilla/cache/mozilla-centos6-i386/root_cache/cache.tar.gz', '-C', '/builds/mock_mozilla/mozilla-centos6-i386/root/']
DEBUG: pigz abort: corrupted input -- invalid deflate data: <stdin>
DEBUG: tar: Unexpected EOF in archive
DEBUG: tar: Unexpected EOF in archive
DEBUG: tar: Error is not recoverable: exiting now
DEBUG: Child return code was: 2
ERROR: Command failed. See logs for output.

Is there something I should do with that?
Comment on attachment 689238 [details] [diff] [review]
Initial version of the patch.

Review of attachment 689238 [details] [diff] [review]:
-----------------------------------------------------------------

This is a bigger change than I think we need to make here. There's not really any need to track what's explicit and what isn't in the nsIPermissionManager. Especially given that the model that the nsIPermissionManager already uses where it's very generic and used for many other things than the B2G permissions model.

It also makes the nsIPermissionManager much more complicated because it carries state in the form of remembering if a permission has ever been set to PROMPT or not. For example if we change an API from being explicit to implicit, or the other way around, it will cause problems.

I'm also very worried about changing the nsIPermissionManager this late in the game given how many regressions we had last time we made modifications to it.

Rather than getting the nsIPermissionManager involved in the change here. Couldn't you simply make the Permissions API use the table in PermissionsInstaller.jsm to figure out of a permission is explicit or not. That way it'll be sure to return the correct answer even if we change the permissions table in the next release.


You also shouldn't create an API which returns two values for a single function. I.e. both the "is this API explicit" and "what is the state of this permission" questions are answered by the same function right now. And you should especially not return it in the form of a string that needs to be parsed. Especially since this is a security-related API.

Instead create a separate "isExplicit" API like:
bool isExplicit(in DOMString permission, in DOMString manifestURI, in DOMString origin, in bool browserFlag);

::: dom/ipc/ContentChild.cpp
@@ -189,5 @@
>  ConsoleListener::Observe(nsIConsoleMessage* aMessage)
>  {
>      if (!mChild)
>          return NS_OK;
> -    

Please don't put unrelated cleanup in functional patches like this. It makes it much harder to review the actual changes.
Attachment #689238 - Flags: review?(jonas) → review-
Attachment #689238 - Flags: review?(fabrice)
(In reply to Jonas Sicking (:sicking) from comment #37)
> Comment on attachment 689238 [details] [diff] [review]
> Initial version of the patch.
> 
> Review of attachment 689238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a bigger change than I think we need to make here. There's not
> really any need to track what's explicit and what isn't in the
> nsIPermissionManager. Especially given that the model that the
> nsIPermissionManager already uses where it's very generic and used for many
> other things than the B2G permissions model.

Actually there is a need to track this (either this way or by accesing the PermissionsTable from inside nsIPermissionManager) because nsIPermissionManager is duplicated locally (on cache form) on the child processes. So that information needs to be stored there too. I'll see if the PermissionsInstaller table is available on the child processes too (the code isn't called but I guess it should be there anyway, shouldn't it?)

> 
> It also makes the nsIPermissionManager much more complicated because it
> carries state in the form of remembering if a permission has ever been set
> to PROMPT or not. For example if we change an API from being explicit to
> implicit, or the other way around, it will cause problems.

No, it won't. Because any time the table changes (in fact, as it currently stands, any time any part of the code is modified) the permissions table is regenerated. Which is correct because not only a explicit->implicit or implicit->explicit change might have been done, also a allow->deny or deny->allow change could have been made.

In fact I just checked the code that do that on PermissionsInstaller and it's wrong:

412  if (permValue == "unknown" || permValue == "deny") {
413                 // All 'deny' permissions should be preserved
414                 continue;
415               } 

It doesn't take into account the possible deny->allow changes on the permission table. Or the deny->explicit changes either. I can fix that also if you wish, or just file a bug for future versions with that (please tell me which one you prefer). In any case, this means that the isExplicit is regenerated (for the cases that currently work, and will be for all cases) also whenever there's a PermissionTable change so it should behave correctly even then.

> 
> I'm also very worried about changing the nsIPermissionManager this late in
> the game given how many regressions we had last time we made modifications
> to it.
> 
> Rather than getting the nsIPermissionManager involved in the change here.
> Couldn't you simply make the Permissions API use the table in
> PermissionsInstaller.jsm to figure out of a permission is explicit or not.
> That way it'll be sure to return the correct answer even if we change the
> permissions table in the next release.

As I said, changing the permissions table should not be an issue. Anyway, I can try and access the table on both PermissionSettings and PermissionSettingsModule.


> 
> 
> You also shouldn't create an API which returns two values for a single
> function. I.e. both the "is this API explicit" and "what is the state of
> this permission" questions are answered by the same function right now. And
> you should especially not return it in the form of a string that needs to be
> parsed. Especially since this is a security-related API.
> 
> Instead create a separate "isExplicit" API like:
> bool isExplicit(in DOMString permission, in DOMString manifestURI, in
> DOMString origin, in bool browserFlag);

Do you mean to add that on PermissionSettings? I didn't want to do that initially because this way we save on a possible expensive function call (have to do the table lookup twice) and we don't need to change the publicly exposed API this late in the game (PermissionSettings). Anyway if that's what you prefer, I'll change it. 

> 
> ::: dom/ipc/ContentChild.cpp
> @@ -189,5 @@
> >  ConsoleListener::Observe(nsIConsoleMessage* aMessage)
> >  {
> >      if (!mChild)
> >          return NS_OK;
> > -    
> 
> Please don't put unrelated cleanup in functional patches like this. It makes
> it much harder to review the actual changes.

Ok, will try and remove that from the patch.
> I'll see if the
> PermissionsInstaller table is available on the child processes too (the code
> isn't called but I guess it should be there anyway, shouldn't it?)

Yes, it should be there.

> > Instead create a separate "isExplicit" API like:
> > bool isExplicit(in DOMString permission, in DOMString manifestURI, in
> > DOMString origin, in bool browserFlag);
> 
> Do you mean to add that on PermissionSettings?

Yes.

> I didn't want to do that
> initially because this way we save on a possible expensive function call
> (have to do the table lookup twice) and we don't need to change the publicly
> exposed API this late in the game (PermissionSettings). Anyway if that's
> what you prefer, I'll change it. 

Thanks. None of the function calls here are expensive, so we should optimize for a clean API rather than performance.

> > Please don't put unrelated cleanup in functional patches like this. It makes
> > it much harder to review the actual changes.
> 
> Ok, will try and remove that from the patch.

Thanks. I know some editors will do cleanup like this automatically, but it makes it very easy to miss important changes in the patch. Usually there are ways to turn off such automatic cleanup.
Attached patch New version, with review changes (obsolete) — — Splinter Review 
This one addresses all the review comments, I believe.

Note that I still think the original version was better, even if it was more complex. In fact, the original version allowed for implementing a correct update policy if/when the permissions matrix change, since we kept the original 'explicitness' value for each permission stored.

Anyway, this version doesn't make any changes on the database, or on nsPermissionManager. It just changes:

* PermissionsSettings.jsm
* PermissionsSettings.js
* PermissionsInstaller.jsm (to remove a circular dependency I had to move PermissionsTable to a new file).

And everything that used PermissionsTable from PermissionsInstaller. 

I had to add a new structure, a reverse lookup table, to find the correct permission for things like: device-storage:music-read.
Attachment #689238 - Attachment is obsolete: true
Attachment #689831 - Flags: review?(jonas)
Oh, another thing, related to this. As the API currently stands, I don't know how the settings APP is going to deal with the 'special' permission cases. I made myself a test app and had to do something as 'beautiful' as 

  var specialPermissions=[
    'contacts', 'device-storage:pictures', 'device-storage:music', 'device-storage:videos',
    'device-storage:apps'
  ];
  var access=["read", "write", "create"];

  for (var i=0; i<specialPermissions.length; i++) {
    for (var j=0; j<access.length; j++) {
      _permissions.push(specialPermissions[i]+"-"+access[j]);
    }
  }

to deal with that. Works, but it's uglier than ugly.
The only difference with the previous version is that it's been rebased with today's M-C changes. That is, device-storage:apps permissions for privileged apps changed from PROMPT_ACTION to DENY_ACTION, and the access is restricted to "read" now:

+                           "device-storage:apps": {
+                             app: DENY_ACTION,
+                             privileged: DENY_ACTION,
+                             certified: ALLOW_ACTION,
+                             access: ["read"]
+                           },
Attachment #689831 - Attachment is obsolete: true
Attachment #689831 - Flags: review?(jonas)
Attachment #690324 - Flags: review?(jonas)
Over the original version, the only changes are including the changes on the permission matrix (so the patch applies cleanly over the current m-c) and fixing a scary log when a non existant permission was passed to isExplicit.
Attachment #690324 - Attachment is obsolete: true
Attachment #690324 - Flags: review?(jonas)
Attachment #690392 - Flags: review?(jonas)
Over v2, changes are:
* Updated the permissions table so it applies cleanly over M-C
* Added error control to isExplicit for non defined permissions
* Added a missing Cu=components.utils on PermissionsTable for error reporting
Attachment #690392 - Attachment is obsolete: true
Attachment #690392 - Flags: review?(jonas)
Attachment #690452 - Flags: review?(jonas)
Oh, BTW, since I understand that me changing the attachment over and over must be a pain if you're reviewing, I'll not make any more changes to it until it's reviewed. So no more rebasing :).
I had to make another rebase today since the permissionstable has been modified. The only change is that, but I don't want to upload another patch in case the review is in process.

BTW, I marked Jonas as a reviewer but I don't know if it should be him or Fabrice. Can you take the review of this, Fabrice?
Flags: needinfo?(fabrice)
I'm reviewing this. It's just not a priority since I don't believe this is a blocker.

We've now added checks to the parent process so that is no longer a problem. Setting back to blocking- per comment 23
blocking-basecamp: + → -
(In reply to Jonas Sicking (:sicking) from comment #47)
> I'm reviewing this. It's just not a priority since I don't believe this is a
> blocker.
> 
> We've now added checks to the parent process so that is no longer a problem.
> Setting back to blocking- per comment 23

Ok, some things, again, just for the record:

* The added check doesn't eliminate the problem of a child process being compromised, it only restricts it so the child process that has to be compromised is the one running the settings app (or any app that has that permission). Oh, ironically, the parent checker is based on the child process having the permission to set permissions... permission that's contagious if this doesn't land.

* There's no such thing as "only the settings app does have this problem so we're golden". First because it's a permission that can be asked by any other certified app (and we're NOT the only ones that can make certified apps) and second because now the settings app is a single point of catastrophic failure. 

* We're, knowingly, shipping a defective, public, API. And shifting the blame on the app developers. They should use it with care, because if they don't, the whole security framework is as scary as a paper tiger. We're not enforcing rules anymore, we're giving guidelines. Soft ones at that. Would you ship a dial API that when you dial +0000 bricks the phone? After all the API is certified and only the communications app uses it currently, so as long as it checks for +0000 there's no problem, is it?

* And last but not least, knowing if a permission is explicit (which is part of this patch also) is required by bug 817031 and bug 817034 which are blocking. They could always copy the PermissionsTable into the app I guess. 

I'm renoming this (last time, I swear :P) because of the last point if nothing else.
blocking-basecamp: - → ?
I'm sorry, other than the last point this is not new information. We've had this exact discussion many times over.

Regarding the last point, this is something that can easily be hardcoded in the frontend.
blocking-basecamp: ? → -
Fair enough. Still... let's assume there's a vulnerability on Firefox OS (more than an assumption, on any sufficiently complex system that's a given). 

To make it interesting, make it so the vulnerability requires the app collaboration to work.

0. Convince the user to load the infected page.
1. Compromise the child process where our infected app is running. Any child process serves.
2. Ensure the settings app is running. Handily enough, the settings app has some Webactivities defined so we can launch it from other app.
3. Inject code into the running setting process. Since all child process run as the same OS user, that should not be a problem.
4. Now we can use all the APIs, from anywhere on the internet. 
5. Profit.

So in 5 steps: 
* Process separation: Voided. I don't have access to the underlying OS, but I don't need it either to: make calls, send or read SMS, copy the contact list, access the photo gallery...
* Application type: Voided. Any web page can be given access to any API. 
* Permission Matrix: Voided. See previous point. 
* And I don't know if there's any point of the security framework that's left standing.

What I don't really get is how remoting the app protocol so a child process compromise doesn't get access to reading apps is blocking, and this isn't.
If you think 3 is real, then the whole security model is voided. Please file a separate bug on that as it's unrelated to this bug. You can just get any arbitrary permission by hacking the app which has that permission. No need to involve the API discussed in this bug.
I haven't actually tried it on the kernel we're using at the phone but, yeah, it should be possible. See [1] for an example. 

And the big difference is that landing this an attacker can only get permissions for apps he can launch, and he can't make them persist. With this? Infect once, target exactly ONE app, leave it asleep, exploit whenever you wish, from wherever you wish. 


[1] https://github.com/vikasnkumar/hotpatch/blob/master/README
As Antonio explained, the key fundamental problem still exists and we do not understand why this is not bb+ as it was already triaged. We have a working patch and we are not far away from landing a proper fix.
Going to disagree with Jonas and agree with some of the opposing argument above. Additionally - Blocker of a blocker is a blocker.
blocking-basecamp: - → ?
(In reply to Jason Smith [:jsmith] from comment #54)
> Going to disagree with Jonas and agree with some of the opposing argument
> above. Additionally - Blocker of a blocker is a blocker.

Or in better wording - the implementation of bug 817034 is in hard dependency with the implementation of this bug. bug 817034 is blocking at this point.
Seems like we are expending a lot of energy discussing whether or not this is a blocker.  Given we have a patch close to done (tho some tests here wouldn't hurt, given the criticality of the permissions code) I'd like to see us focused on getting the reviewed and landed.

That said, if #3 in comment 50 is true then it seems like we have a big problem regardless since you can always inject code into whatever certified apps has the permissions you need.  Adding someone who might know.
Flags: needinfo?(gdestuynder)
Yes, can we please file a separate bug on #3 from comment 50. That's a completely orthogonal problem to this bug. And a much bigger problem if true.
(In reply to Jonas Sicking (:sicking) from comment #58)
> I filed bug 820630

I think copy/paste went wrong there :D
Flags: needinfo?(fabrice)
Indeed :)

Bug 820560 is tracking the issue now.
Flags: needinfo?(gdestuynder)
Comment on attachment 689831 [details] [diff] [review]
New version, with review changes

Review of attachment 689831 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, i was most of the way through reviewing this version, and there's no good way of moving my comments to the later versions of the patch.

I'd like to see a new patch anyway, though, so unless you made substantial changes in the later versions that should be fine. Let me know if you did?

::: dom/apps/src/PermissionsTable.jsm
@@ +352,5 @@
> +  // device-storage:music
> +  let reverseTable= {};
> +  
> +  for (let permName in PermissionsTable) {
> +    reverseTable[permName] = permName; // Obvious but helpful nonetheless

This shouldn't always be added. For example we shouldn't map "contacts" to "contacts" since there is no "contacts" permission in the database.

I think things should work if you simply remove this line.

@@ +356,5 @@
> +    reverseTable[permName] = permName; // Obvious but helpful nonetheless
> +    let permAliases = [];
> +    if (PermissionsTable[permName].access) {
> +      permAliases =
> +        permAliases.concat(expandPermissions(permName, "readwrite"));

I don't see that you need the .concat call. Simply permAliases = expandPermissions(...) should be enough.

@@ +358,5 @@
> +    if (PermissionsTable[permName].access) {
> +      permAliases =
> +        permAliases.concat(expandPermissions(permName, "readwrite"));
> +    } else if (PermissionsTable[permName].channels) {
> +      permAliases.concat(expandPermissions(permName, null, PermissionsTable[permName].channels));

This part can of course be removed now since we removed the .channels idea. Don't forget to remove this elsewhere too when syncing to tip.

@@ +361,5 @@
> +    } else if (PermissionsTable[permName].channels) {
> +      permAliases.concat(expandPermissions(permName, null, PermissionsTable[permName].channels));
> +    } else {
> +      permAliases =
> +        permAliases.concat(expandPermissions(permName));

Same here.

@@ +363,5 @@
> +    } else {
> +      permAliases =
> +        permAliases.concat(expandPermissions(permName));
> +    }
> +    for (let i=0; i<permAliases.length; i++) {

Please put spaces around '=' and '<'

::: dom/permission/PermissionSettings.js
@@ +67,5 @@
>          return "unknown";
>      }
>    },
>  
> +  _isExplicit: function(aPermName,aIntStatus) {

space after ','

@@ +83,5 @@
> +      appStatus = "app";
> +      break;
> +    }
> +
> +    debug("_isExplicit("+aPermName+", "+aIntStatus+"--"+appStatus+"): "+ 

Nit: there's a few lines with whitespace at the end of the line. Not a big deal.

@@ +115,5 @@
> +        value: aPermValue,
> +        browserFlag: aBrowserFlag
> +      });
> +    } else {
> +      debug("Caller tried to set an implicit permission or unset a permission! That could get us killed.");

Make this an early-return instead. And make it throw.

::: dom/permission/PermissionSettings.jsm
@@ +44,5 @@
>      Services.obs.addObserver(this, "profile-before-change", false);
>    },
>  
> +
> +  _isExplicit: function(aPermName,aIntStatus) {

space after ','

Move this function to PermissionsTable.jsm so that you don't have to duplicate it.

@@ +65,5 @@
> +            Ci.nsIPermissionManager.PROMPT_ACTION);
> +    
> +  },
> +
> +  _isChangeAllowed: function(aPrincipal,aPermName,aAction) {

spaces after ','

@@ +71,5 @@
> +    // Change is allowed from a child process when all of the following
> +    // conditions stand true:
> +    //   * the action isn't "unknown" (so the change isn't a delete)
> +    //   * the permission already exists on the database
> +    //   * and it's marked as explicit

Nit: It's unclear what the "it" in the last sentence refers to. Maybe something like "The permission is marked as explicit in the permissions table".

@@ +77,5 @@
> +    // permissionManager doesn't know if it's being called as a result of
> +    // a parent process or child process request. We could check
> +    // if the permission is actually explicit (and thus modifiable) or not
> +    // on permissionManager also but we currently don't.
> +    let perm = permissionManager.testExactPermissionFromPrincipal(aPrincipal, aPermName);

Stay below 80 columns

@@ +78,5 @@
> +    // a parent process or child process request. We could check
> +    // if the permission is actually explicit (and thus modifiable) or not
> +    // on permissionManager also but we currently don't.
> +    let perm = permissionManager.testExactPermissionFromPrincipal(aPrincipal, aPermName);
> +    let isExplicit = this._isExplicit(aPermName,aPrincipal.appStatus);

space after ','

@@ +82,5 @@
> +    let isExplicit = this._isExplicit(aPermName,aPrincipal.appStatus);
> +    
> +    return ( (aAction !== "unknown") &&
> +             ( perm !== Ci.nsIPermissionManager.UNKNOWN_ACTION ) &&
> +             isExplicit );

Please follow the whitespace convention of surrounding code. So remove the extra spaces around the parenthesis here.

@@ +92,5 @@
> +
> +  },
> +
> +
> +  _internalAddPermission: function _internalAddPermission(aData, aIsAnInternalCall, aCallbacks) {

the second flag isn't really for "internal" calls since even when it's true we're coming directly from an external caller calling addPermission.

Maybe flip it's value and call it "aCheckExplicit" or "aOnlyAllowExplicit" or some such.

@@ +151,5 @@
>          return "unknown";
>      }
>    },
>  
> +  isExplicit: function isExplicit(aPermName, aManifestURL, aOrigin, aBrowserFlag) {

Is this function used?

@@ +175,5 @@
>  
>      let result;
>      switch (aMessage.name) {
>        case "PermissionSettings:AddPermission":
> +        // Ok, if I'm here I've been invoked from a child process, cause PermissionsInstaller invokes

Limit to 80 columns. Though I don't really think you need this comment at all.

@@ +178,5 @@
>        case "PermissionSettings:AddPermission":
> +        // Ok, if I'm here I've been invoked from a child process, cause PermissionsInstaller invokes
> +        // this.addPermission directly.
> +        if (mm.assertPermission("permissions")) {
> +          var success=this._internalAddPermission(msg,false);

spaces around '='

@@ +183,5 @@
> +          // On failure (!success) we could (should?) kill the calling process. Something like
> +          // if (!success) {
> +          //   mm.assertPermission("just-kill-it-please");
> +          //  }
> +          // would do the trick.

Either make this change or remove this comment. I'm fine either way.
Attachment #689831 - Flags: review-
Oh, and I should say that this patch looks like a great approach in general, so I see no reason we couldn't get this landed.
Thanks for the review. I'll get to is ASAP and will download another patch later today. 

Some comments below:


(In reply to Jonas Sicking (:sicking) from comment #61)
> Comment on attachment 689831 [details] [diff] [review]
> New version, with review changes
> 
> Review of attachment 689831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, i was most of the way through reviewing this version, and there's no
> good way of moving my comments to the later versions of the patch.
> 
> I'd like to see a new patch anyway, though, so unless you made substantial
> changes in the later versions that should be fine. Let me know if you did?

No, the new versions were just rebasing to keep it up to date with permission changes and a couple of error cases that weren't treated originally (for unexistant permissions).


> 
> ::: dom/apps/src/PermissionsTable.jsm
> @@ +352,5 @@
> > +  // device-storage:music
> > +  let reverseTable= {};
> > +  
> > +  for (let permName in PermissionsTable) {
> > +    reverseTable[permName] = permName; // Obvious but helpful nonetheless
> 
> This shouldn't always be added. For example we shouldn't map "contacts" to
> "contacts" since there is no "contacts" permission in the database.
> 
> I think things should work if you simply remove this line.

I had that because that way the isExplicit code was much more simpler. If I have the reverseTable[perm] = perm in place then whenever I'm called then I can just:

* Look up the passed permission name on the reverse table, and if it exists return the adequate value from PermissionsTable.

If I don't have that then I would have to:

* Look up the passed permission name on PermissionsTable. If it exists, return the adequate value. If it doesn't exist, look the passed permission on the reverse table, and if it exists return the adequate value from PermissionsTable.

So I save a check on each call, making the code simpler IMO.

Besides, although 'contacts' isn't stored on the database, isExplicit('contacts') should return a correct value... and it can be used by an astute settings app developer to optimize his code, since he only has to call isExplicit once for a set of permissions-* permissions.

So for the time being I'm leaving this as is unless you still feel it would be better to remove it and add the extra check on isExplicit. 

> @@ +358,5 @@
> > +    if (PermissionsTable[permName].access) {
> > +      permAliases =
> > +        permAliases.concat(expandPermissions(permName, "readwrite"));
> > +    } else if (PermissionsTable[permName].channels) {
> > +      permAliases.concat(expandPermissions(permName, null, PermissionsTable[permName].channels));
> 
> This part can of course be removed now since we removed the .channels idea.
> Don't forget to remove this elsewhere too when syncing to tip.

Will do. That's also a leftover in PermissionsInstaller, will fix that also.
I forgot this one before, sorry

> @@ +151,5 @@
> >          return "unknown";
> >      }
> >    },
> >  
> > +  isExplicit: function isExplicit(aPermName, aManifestURL, aOrigin, aBrowserFlag) {
> 
> Is this function used?

No, it currently isn't. I added it here mostly for simmetry (since we have the rest of the methods from the IDL here). I'm leaving it in this version, can remove if needed.
This addresses all the review comments, except when previously indicated in comment #63.
Attachment #690452 - Attachment is obsolete: true
Attachment #692277 - Flags: review?(jonas)
Comment on attachment 692277 [details] [diff] [review]
Patch v6. Addresses review comments

Removed the review flag, I think I'm missin a [throws] on the idl
Attachment #692277 - Flags: review?(jonas) → review-
This addresses all the review comments, except when previously indicated in comment #63.
Fixes a problem on how I was throwing from PermissionSettings.get.
Attachment #692277 - Attachment is obsolete: true
Attachment #692330 - Flags: review?(jonas)
(In reply to Antonio Manuel Amaya Calvo from comment #66)
> Comment on attachment 692277 [details] [diff] [review]
> Patch v6. Addresses review comments
> 
> Removed the review flag, I think I'm missin a [throws] on the idl

Nevermind that. I wasn't getting the exception test from Gaia and was reading the wrong specification (webidl instead of XPIDL). Fixed now.
> Besides, although 'contacts' isn't stored on the database,
> isExplicit('contacts') should return a correct value... and it can be used
> by an astute settings app developer to optimize his code, since he only has
> to call isExplicit once for a set of permissions-* permissions.

I think this is where I'm not agreeing. I don't think we should have a function that can either take a manifest-permission-name *or* a permission-database-name. Given that we are dealing with security here, it's more important that APIs are explicit than that they are super convenient.

So if we have the need for both doing explicit checks for permission-database-names as well as manifest-permission-names, then we should create two separate functions.
Comment on attachment 692330 [details] [diff] [review]
Patch v7. Addressed review comments and fixed throw problem

Review of attachment 692330 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good otherwise, just have to figure out comment 69

::: dom/apps/src/PermissionsTable.jsm
@@ +375,5 @@
> +  // do I know which permission it really is? Hence this table is
> +  // born. The idea is that
> +  // reverseTable[device-storage:music-read] should return
> +  // device-storage:music
> +  let reverseTable= {};

space before '='

@@ +378,5 @@
> +  // device-storage:music
> +  let reverseTable= {};
> +
> +  for (let permName in PermissionsTable) {
> +    reverseTable[permName] = permName; // Obvious but helpful nonetheless

See comment 69. Let's figure out what to do here.

@@ +379,5 @@
> +  let reverseTable= {};
> +
> +  for (let permName in PermissionsTable) {
> +    reverseTable[permName] = permName; // Obvious but helpful nonetheless
> +    let permAliases = [];

Nit: You can even remove the "= []" part here.

::: dom/permission/PermissionSettings.js
@@ +75,5 @@
> +    let appID = appsService.getAppLocalIdByManifestURL(aManifestURL);
> +    let principal = secMan.getAppCodebasePrincipal(uri, appID, aBrowserFlag);
> +
> +    return isExplicitInPermissionsTable(aPermName, principal.appStatus);
> +    

Remove this empty line.

@@ +86,3 @@
>      let action;
> +    // Bug 812289: This isn't strictly necessary here, but it open the
> +    // way to cleansing suspicious processes 

I don't know what is meant by "cleansing" here. Could you fix this to be more specific.

Change "open" to "opens".

No need to leave the bugnumber in there. It can be found through the checkin comment.

Hmm.. are you simply saying that this isn't needed because we have a check in the parent process? If so, I don't think you need this comment at all since it's simply good hygiene and the only way to get proper API behavior (i.e. getting an exception rather than a killed process).

Maybe say something like:
"Check for invalid calls so that we throw an exception rather than get killed by parent process".

@@ +90,5 @@
> +        !this.isExplicit(aPermName, aManifestURL, aOrigin, aBrowserFlag)) {
> +      let errorMsg = "PermissionSettings.js: '" + aPermName + "'" +
> +                     " is an implicit permission for '" + aManifestURL+"'";
> +      Cu.reportError(errorMsg);
> +      throw errorMsg;

throw new Error(errorMsg);

@@ +100,5 @@
>        manifestURL: aManifestURL,
>        value: aPermValue,
>        browserFlag: aBrowserFlag
>      });
> +

Remove this empty line.

::: dom/permission/PermissionSettings.jsm
@@ +62,5 @@
> +    let isExplicit = isExplicitInPermissionsTable(aPermName, aPrincipal.appStatus);
> +    
> +    return ( (aAction !== "unknown") &&
> +             (perm !== Ci.nsIPermissionManager.UNKNOWN_ACTION) &&
> +             isExplicit );

Nit, there's still one more space around the outer-most parens than we normally use. In fact, the outermost parenthesis doesn't seem needed at all. But not a big deal. If you prefer to keep as-is it's fine.

@@ +131,5 @@
>          return "unknown";
>      }
>    },
>  
> +  isExplicit: function isExplicit(aPermName, aManifestURL, aOrigin, aBrowserFlag) {

This function isn't used at all, is it? Just remove it if it's not needed, we can always add it if it's needed later.

@@ +138,5 @@
> +    let appID = appsService.getAppLocalIdByManifestURL(aManifestURL);
> +    let principal = secMan.getAppCodebasePrincipal(uri, appID, aBrowserFlag);
> +
> +    return PermissionsTable.isExplicit(aPermName, principal.appStatus);
> +    

If we're keeping this function, remove this empty line.

@@ +156,5 @@
>      let result;
>      switch (aMessage.name) {
>        case "PermissionSettings:AddPermission":
> +        if (mm.assertPermission("permissions")) {
> +          var success = this._internalAddPermission(msg,false);

space after ','

@@ +159,5 @@
> +        if (mm.assertPermission("permissions")) {
> +          var success = this._internalAddPermission(msg,false);
> +          if (!success) { 
> +            // Just kill the calling process
> +            mm.assertPermission("permissions-modify-implicit"); 

You should probably report an error here too, like we do for the failed "permissions" assertion.

You can reuse the same reportError call by restructuring like:

success = false;
if (mm.assertPermission("permissions")) {
  success = this._internallAddPermission(...);
  if (!success) {
    success = mm.assertPermission("permissions-modify-implicit");
  }
}

if (!success) {
  Cu.reportError(...);
}
Attachment #692330 - Flags: review?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #69)
> > Besides, although 'contacts' isn't stored on the database,
> > isExplicit('contacts') should return a correct value... and it can be used
> > by an astute settings app developer to optimize his code, since he only has
> > to call isExplicit once for a set of permissions-* permissions.
> 
> I think this is where I'm not agreeing. I don't think we should have a
> function that can either take a manifest-permission-name *or* a
> permission-database-name. Given that we are dealing with security here, it's
> more important that APIs are explicit than that they are super convenient.
> 
> So if we have the need for both doing explicit checks for
> permission-database-names as well as manifest-permission-names, then we
> should create two separate functions.

The problem is that there is no easy/proofed way to know if something is a permission-database-name, a manifest-permission-name, or both. 

The only way that I can think of (and I don't know if there's any exception to the rule) would be something like:

_isAValidDatabaseName: function (aPermName) {
return ((PermissionsTable[aPermName] && !PermissionsTable[aPermName][access]) ||
       ((!PermissionsTable[aPermName] && PermissionsReverseTable[aPermName]));
}

And then I could do something like

isExplicitDatabasePerm(aPermName, aManifestURI, aOrigin, aBrowserFlags) {
  return _isAValidDatabaseName(aPermName) && 
         isExplicit(aPermName, aManifestURI, aOrigin, aBrowserFlags);
}

isExplicitManifestPerm(aPermName,aManifestURI, aOrigin, aBrowserFlags) {
  return PermissionsTable[aPermName] && 
         isExplicit(aPermName, aManifestURI, aOrigin, aBrowserFlags);
}

I can do that and make private the current isExplicit implementation if you wish. But I don't really see the point on that to be honest, cause I can't see of any viable attack coming from isExplicit returning the correct value either for the manifest-perm-name or the database-perm-name (for the cases that they're different). And I can't see a viable attack here because the expressions:

* If a database-perm-name is explicit for a given app, then his original 
  manifest-perm-name will be explicit for that app too
* If a manifest-perm-name is explicit for a given app, then all his derived
  database-perm-names will be explicit for that app too.

hold true for all database-perm-names and manifest-perm-names (and they must hold true unless the PermissionsTable structure is changed in which case the PermissionsInstaller/PermissionsSettings modules would need to be changed also).

TL;DR: 

If you want me to, I'll separate the isExplicit into two functions with the logic above. But I don't think they add any security and make the API clumsier.
> @@ +90,5 @@
> > +        !this.isExplicit(aPermName, aManifestURL, aOrigin, aBrowserFlag)) {
> > +      let errorMsg = "PermissionSettings.js: '" + aPermName + "'" +
> > +                     " is an implicit permission for '" + aManifestURL+"'";
> > +      Cu.reportError(errorMsg);
> > +      throw errorMsg;
> 
> throw new Error(errorMsg);

I have a problem with this (the v6 patch I auto-rejected and this one only differ in this in fact). If I use 

throw new Error(errorMsg); 

then the Error is launched but the message is lost:

E/GeckoConsole(  411): [JavaScript Error: "Error: "]

And if I capture it on user-space I get:

ex.name = "Error"
ex.message = undefined.

In the end the only way I found to get the message correctly on the calling function was this.
(In reply to Antonio Manuel Amaya Calvo from comment #71)
> 
> _isAValidDatabaseName: function (aPermName) {
> return ((PermissionsTable[aPermName] &&
> !PermissionsTable[aPermName][access]) ||
>        ((!PermissionsTable[aPermName] &&
> PermissionsReverseTable[aPermName]));
> }
> 

Bar that, that won't work for cases with alternative names but no access modifier. So the condition will have to be even more complex than that. As I said, I can dig at this and separate the method on two if you wish, but I don't really see any advantage of doing so.

Let me know if you want me to do it that way.
The changes here are basically just the review comments plus a couple of typos.
Attachment #692456 - Flags: review?(jonas)
Attachment #692330 - Attachment is obsolete: true
I'm told you can try:

throw new Components.Exception("message");


or possibly


e = new Error("message");
e.__exposedProps__ = { name: "r", message: "r" };
throw e;


The former is preferable if it works.
Comment on attachment 692456 [details] [diff] [review]
patch v8. Addressed v7 review comments

Review of attachment 692456 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if you try the exception throwing methods from comment 76.
Attachment #692456 - Flags: review?(jonas) → review+
The first method (throw new Components.Exception(errorMsg);) worked like a charm. It's the only change over the reviewed version
Attachment #692456 - Attachment is obsolete: true
Attachment #692521 - Flags: review+

        Attachment #692457 -
        Attachment is obsolete: true

    
  

        Attachment #692521 -
        Flags: review+ → review?(jonas)

    
    

    
  
(In reply to Jonas Sicking (:sicking) from comment #77)
> Comment on attachment 692456 [details] [diff] [review]
> patch v8. Addressed v7 review comments
> 
> Review of attachment 692456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if you try the exception throwing methods from comment 76.

The first method worked correctly, uploaded a modified patch with the corresponding interdiff (only that changes). 

Let me know if there's something else I can do to get this on the way to landing.

    
    

    
  
Comment on attachment 692521 [details] [diff] [review]
patch v9. Reviewed version with fixed exception throwing

Review of attachment 692521 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

        Attachment #692521 -
        Flags: review?(jonas)

        Attachment #692521 -
        Flags: review+

        Attachment #692521 -
        Flags: approval-mozilla-b2g18+

        Attachment #692521 -
        Flags: approval-mozilla-aurora+

    
  
Blocks: 821961
No longer blocks: 817031, 817034

    
    

    
  
One thing that would be nice to do, either here or in a followup, is to make the "desktop-notification" be listed as an explicit permission for non-certified apps.

It's quite intentional that it's listed as ALLOW_ACTION in the permission table, but I think there are good reasons for people to modify that to DENY_ACTION or PROMPT_ACTION.


I think we can just hardcode this rule into PermissionsTable.isExplicitInPermissionsTable for now and figure out a more generic way of flagging this in the future.

    
    

    
  
Can we land this as is so we can unblock the settings app? You can file a separate bug for this other change and assign it to me, I can take that too.

    
    

    
  
What's needed to do here to land this?

    
    

    
  
You just have to find someone with checkin permission and the time to do it. Ask around on IRC. Simply adding the checkin-needed keyword will help someone find it.
Keywords: checkin-needed

    
    

    
  
Sorry, that was my bad. Since the logic of the permissions API has changed, the tests are no longer valid. I'll upload a patch for the tests shortly.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Test patches (obsolete)
        — 
          — Splinter Review
      

    


  
This fixes the test breakage. Since the API functionality changed, tests must be changed also.

        Attachment #692963 -
        Flags: review?(fabrice)

    
    

    
  


  
Since the API behavior changed, the old tests broke. This fixes that API test breakage.

        Attachment #692963 -
        Attachment is obsolete: true

        Attachment #692963 -
        Flags: review?(fabrice)

        Attachment #692971 -
        Flags: review?(fabrice)

    
    

    
  
Comment on attachment 692971 [details] [diff] [review]
Patch for the API tests

Review of attachment 692971 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good to me, but let's wait for the try results to be green before landing:
https://tbpl.mozilla.org/?tree=Try&rev=661c5dc40ce2

        Attachment #692971 -
        Flags: review?(fabrice) → review+

    
    

    
  
Hmm try results aren't exactly green... but what fails isn't even remotely related to what's been touched in this patch. The tests included with the patch are working correctly, and they don't generate any warnings (neither the XPCShell test nor the mochitest test).

Can we land this? And does the second patch need the approval-mozilla-b2g18+ and approval-mozilla-aurora+ flags too?
Flags: needinfo?(fabrice)

    
    

    
  
Fabrice said he's waiting for a try build (that he kicked off) to test. Jonas and Fabrice also confirmed that this should be marked as a soft blocker.
blocking-basecamp: ? → +

          tracking-b2g18:
          --- → +
Flags: needinfo?(fabrice)
Priority: P1 → P4
Target Milestone: --- → B2G C3 (12dec-1jan)

    
    

    
  
(In reply to Lawrence Mandel [:lmandel] from comment #93)
> Fabrice said he's waiting for a try build (that he kicked off) to test.
> Jonas and Fabrice also confirmed that this should be marked as a soft
> blocker.

That would imply a minus, not a plus.
blocking-basecamp: + → ?

    
    

    
  
Good catch Jason. I did indeed mean to select minus.
blocking-basecamp: ? → -

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ec4945922e62
https://hg.mozilla.org/mozilla-central/rev/ba17dfaa86e9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

    
  
Blocks: 827294

    
    

    
  
(In reply to Antonio Manuel Amaya Calvo from comment #36)
> Build seems to have failed with a... tar error?
[...]
> DEBUG: pigz abort: corrupted input -- invalid deflate data: <stdin>

(Just to follow up on this -- for reference, this is a known sporadic build issue -- I don't know the backstory, but releng has a magic incantation that fixes it. See e.g. bug 829186 & bug 811614 )

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: