831617 - Changing a device storage permission in the settings app for a privileged app results in the settings app process gets killed

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 1/16/2013
Device: Unagi

Steps:

1. Install a privileged packaged app requesting access for geolocation, contacts, device storage videos/sdcard/music/pictures
2. Launch the app
3. Request and remember permissions for device storage videos
4. Go to settings --> app perms UI --> app you installed
5. Change the device storage videos perm from Always Allow --> Ask

Expected:

No error should occur. The next time I request device storage videos, I should be prompted.

Actual:

The settings app abnormally terminates. The logcat appears to show something blew up:

01-16 17:35:36.485: I/Gecko(109): Security problem: Content process does not have `permissions-modify-implicit'.  It will be killed.
01-16 17:35:36.485: E/GeckoConsole(109): [JavaScript Error: "PermissionSettings message undefined had an implicit permission change. Child process killed." {file: "resource://gre/modules/PermissionSettings.jsm" line: 162}]

Comment 1

[Jason Smith [:jsmith]]

Apparently this just generally happens with any old device storage permission in a privileged app. Flip the permission and boom! Settings app is toast.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Gregor - can you look at this and comment on the possible fix/LOE here so we can determine the feasibility of blocking tef+ for the issue or tracking for b2g18+?

Comment 3

[Gregor Wagner [:gwagner]]

permissions-modify-implicit? Where does this come from?
Well, no gaia app has this permission :(

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No app should have that permission. That call is there to kill the app when it has failed the security checks. We're basically calling assertPermission with a permission name that we know no app has.

The question is, why does _internalAddPermission return false?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Antonio: This is looking like something is going wrong in your fix for bug 812289.

Comment 6

[Jason Smith [:jsmith]]

I'm guessing based on the comments I'm reading above we should block on this, right?

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

I'm on it. The permissions-modify-implicit is, as Jonas said, something I used to kill the process that try to modify an implicit permission (which is forbidden). But that should not be launched unless the child process is behaving incorrectly since it throws there also. 

And yes, this should be a blocker... it has to work

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

By the way, do you have any test app for this?

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

Hmm... I can't reproduce this. I've:

1. Installed a privileged application that requests 
    "device-storage:pictures":{ "access": "readwrite" },
    "device-storage:music":{ "access": "readwrite" },
    "device-storage:videos":{ "access": "readwrite" }

2. Open the settings app.
3. Change the permissions from ask to allow. Or from allow to deny. Or whatever

and it doesn't fail.

There's a bug on the error log though. msg.name should be msg.type on line 162 of PermissionSettings.jsm. 

Which build are you using? Can you provide me with the test app you're using?

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Fixes error log trace

This doesn't fix the described problem. It makes the log trace actually work as intended though.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Antonio Manuel Amaya Calvo from comment #8)
> By the way, do you have any test app for this?

https://marketplace.firefox.com/app/test-webapi-permissions-3

Still reproduces for me on a 1/17 build. Originally tested on 1/16 build.

Let me know if you can reproduce it with that app.

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

It dies-a-lot with that app :) I'll check what's happening. Thanks!

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Fixes a problem when calling set with a non granted permission

The problem was that the API doesn't allow setting a non existant (on the database) permission. And the Settings app tries to set/reset the permission for all access modes, regardless if they were asked for or not.

Comment 14

[Antonio Manuel Amaya Calvo (:amac)]

Ok, I fixed it, I think. 

The problem was a combination of factors:

* The settings app tries to change the permission for all possible access values (read, 
  write, create), regardless if they were asked for or not by the app.
* The parent process was checking (correctly) if the permission that was being changed 
  was already in the database. If the permission wasn't on the db then it killed the 
  child process.
* And the child process wasn't (incorrectly) checking that the permission was in the 
  database.

So the child process was sending a request that the parent process considered invalid and thus it was killing the process.

Anyway, the patch fixes that by checking if the permission is on the DB before calling the parent process.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 703579 [details] [diff] [review]
Fixes a problem when calling set with a non granted permission

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

This looks good. However we need a gaia fix as well so that we don't attempt to set permissions that aren't there which will cause an exception to be thrown.

Comment 16

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #15)
> Comment on attachment 703579 [details] [diff] [review]
> Fixes a problem when calling set with a non granted permission
> 
> Review of attachment 703579 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. However we need a gaia fix as well so that we don't attempt
> to set permissions that aren't there which will cause an exception to be
> thrown.

Actually (I checked it yesterday while fixing this) the settings app, while not being very orthodox, works correctly since it catches the exception on a permission by permission basis and doesn't break the loop (so even when exceptions are launched it sets all the permissions correctly). Changing it to call get before set would just mean that get would be called twice (one by the settings app, and one internally by set) so it would be slower (well, it would be slower if calling get is more expensive than throwing/catching an exception, which I don't know if it is).

Anyway, your call. If you still think we need to fix gaia I can make that change too, just let me know and I'll make the change

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That's fine. If we're catching the exception and moving on then I think that's enough.

Comment 18

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c547039267da

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 703579 [details] [diff] [review]
Fixes a problem when calling set with a non granted permission

[Triage Comment]

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/c547039267da

Comment 21

[Jason Smith [:jsmith]]

Checkin needed for b2g18

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1521398ee4a8

Comment 23

[Jason Smith [:jsmith]]

Verified on 1/20 build.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.