Closed Bug 831617 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: jsmith, Assigned: amac)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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}]
blocking-b2g: --- → tef?
Summary: Request access to device storage videos and remember my choice, change permission from Always Allow --> Ask - Settings App Process Kills Gets Killed → Request access to device storage videos and remember my choice, change permission from Always Allow --> Ask - Settings App Process Gets Killed
Apparently this just generally happens with any old device storage permission in a privileged app. Flip the permission and boom! Settings app is toast.
Summary: Request access to device storage videos and remember my choice, change permission from Always Allow --> Ask - Settings App Process Gets Killed → Changing a device storage permission in the settings app for a privileged app results in the settings app process gets killed
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+?
Flags: needinfo?(anygregor)
permissions-modify-implicit? Where does this come from?
Well, no gaia app has this permission :(
Flags: needinfo?(anygregor)
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?
Antonio: This is looking like something is going wrong in your fix for bug 812289.
Assignee: nobody → amac
I'm guessing based on the comments I'm reading above we should block on this, right?
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
By the way, do you have any test app for this?
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?
Attached patch Fixes error log trace (obsolete) — Splinter Review
This doesn't fix the described problem. It makes the log trace actually work as intended though.
Attachment #703516 - Flags: review?(jonas)
(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.
It dies-a-lot with that app :) I'll check what's happening. Thanks!
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.
Attachment #703516 - Attachment is obsolete: true
Attachment #703516 - Flags: review?(jonas)
Attachment #703579 - Flags: review?(anygregor)
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 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.
Attachment #703579 - Flags: review?(anygregor) → review+
(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
That's fine. If we're catching the exception and moving on then I think that's enough.
Comment on attachment 703579 [details] [diff] [review]
Fixes a problem when calling set with a non granted permission

[Triage Comment]
Attachment #703579 - Flags: approval-mozilla-b2g18+
https://hg.mozilla.org/mozilla-central/rev/c547039267da
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Checkin needed for b2g18
Keywords: checkin-needed
Keywords: verifyme
QA Contact: jsmith
Verified on 1/20 build.
Status: RESOLVED → VERIFIED
Keywords: verifyme
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.
You need to log in before you can comment on or make changes to this bug.