58.56 KB, patch
Jim Straus: review-
|Details | Diff | Splinter Review|
9.10 KB, patch
|Details | Diff | Splinter Review|
39.08 KB, patch
|Details | Diff | Splinter Review|
Lots of our new Web APIs should not be available to all pages. But we need a centralized piece of code which manages these permissions.
cjones said in bug 702256 comment 48 that the current nsPermissionManager is dying. I don't know why -- it seems pretty straightforward...
This is the bug I filed a while back on the new permissions API. Jonas, you said you had some thoughts about how this should work?
(In reply to Justin Lebar [:jlebar] from comment #1) > cjones said in bug 702256 comment 48 that the current nsPermissionManager is > dying. I don't know why -- it seems pretty straightforward... That's what I heard. I don't have any primary-source information.
Some desiderata - needs to work across processes, both in that all gecko processes can do permissions checks, and in that a parent process can do a permission check of a child. (The latter part needs to be designed very carefully, and could maybe be a followup.) - probably needs to be fast. - (obviously), populated from web-app manifests. User force-overrides would be a nice-to-have.
As discussed previously there are UX issues to be decided upon for this, mainly whether permissions are requested during "install" time (like Android) or at first use (e.g. like the geolocation API in Firefox)
(In reply to Ben Francis from comment #5) > As discussed previously there are UX issues to be decided upon for this, > mainly whether permissions are requested during "install" time (like > Android) or at first use (e.g. like the geolocation API in Firefox) I think this is a separate issue. I just want to track the code which stores the permissions here. How the permissions get granted we can work out separately.
Yes. The primary model under plan is to have a "root set" of trusted stores, configured on shipped devices by vendors. The applications installed by those stores will be automatically granted the permissions they need. The trusted stores are responsible for review, and there'll be an audit trail from the app to store. The secondary model, which we'll likely use for self-hosted apps and apps from untrusted stores, is to present a UI with checkboxes (unchecked by default!) for each requested permission, to be granted by the user at a fine grain. The first model is a higher priority, and it doesn't need UI. The second does.
On another note, I'd like for us to consider whether we want this API to have the notion of a page/origin with "root" permissions. A page with "root" permissions is never denied permission for anything. I'm concerned that if we don't have this, then we'll end up in basically the same place, but via a more painful road. We'll have many separate APIs and need to give certain apps (e.g., the home screen) explicit permission to use almost all of them. Then when we add a new API, we'll have to go through and give all these apps those same permissions again. And when we add a new privileged page, we'll have to figure out what are the right permissions, and modify them as the page's capabilities change. The advantage of fine-grained control over permissions is that a misbehaving page (say, hijacked via xss) can't take down the phone. But a lot of these APIs are mission-critical anyway. If a page can turn off the screen at will, or reboot the device, or turn the radio off, it can render the phone basically unusable. At the point that the page can touch one of these critical APIs, there isn't a lot of point, as I see it, in further restricting what the page can do.
I think root permissions is not a good idea. The homescreen has more touch points than most, but there's a huge set of APIs it has no business touching, like bluetooth etc. Not just wrt malicious attacks, but bugginess etc. As time goes on the number of touchpoints should go down, not up. Unless you can prove to me that there's an application that needs to touch every API we will ever possibly add, r- from me ;). POLA says that "since this app can hurt the phone's usability in one way, let's give it access to all possible ways to hurt the phone" is a bad argument.
I don't think this has anything to do with the principle of least astonishment, considering how chrome JS and the Unix security models work. That doesn't mean these are good models or the right models for B2G, but applying them here certainly doesn't surprise me. Are you sure the homescreen doesn't need to touch bluetooth? If we want an Android-style popover menu to appear when you hold down the power button, and we want one of those options to be airplane mode, then presumably the homescreen will need to be able to turn all the radios on and off. (Or we're going to have to implement non-full-screen apps, so the homescreen can launch a special power-button-down dialog app.) Or what if we want an iOS-style dialog for "open wifi network available". Again, the homescreen is the only app which can pop over every app. I understand that it's appealing to explicitly lay out the permissions that each app has. But it's precisely because of surprising requirements like these that I think maintaining this list will be a pain. The last thing I want is for someone implementing that popover to have to argue with a reviewer as for why we should allow the homescreen to access wifi rather than moving the code into a separate app, where the only motivation for using a separate app is the vague notion of restricting the permissions we hand out. Anyway, it's a small point. I'll be happy when we have the permissions manager, either way.
(In reply to Justin Lebar [:jlebar] from comment #10) > I don't think this has anything to do with the principle of least > astonishment, Principle of least authority. > Are you sure the homescreen doesn't need to touch bluetooth? If we want an > Android-style popover menu to appear when you hold down the power button, > and we want one of those options to be airplane mode, then presumably the > homescreen will need to be able to turn all the radios on and off. Bluetooth can be disabled through the settings API. > Or what if we want an iOS-style dialog for "open wifi network available". > Again, the homescreen is the only app which can pop over every app. > Popover for wifi dialog doesn't require access to wifi APIs. > Anyway, it's a small point. I'll be happy when we have the permissions > manager, either way. Yup, that's the #1 goal.
Created attachment 606280 [details] [diff] [review] Sketch of code for Permissions This is just a sketch of code to provide permissions, and permissions ipc. Note that it doesn't do anything yet and isn't fully hooked up. It also needs to be attached to JS, figure out how the master Permissions Manager process gets exec'ed and used by other processes, hook it in to replace the existing permission system (geolocation, cookies, etc), etc.
Jim, Why can't this be done as an incremental improvement of the current permission manager?
Created attachment 607553 [details] [diff] [review] Changes to allow for prompting for more than geolocation, change to camera to use Extended ContentPermissionPrompt to allow for more than just geolocation check. Specifically added information for content-camera, but it is re-organized to allow for easy addition of new permissions. Also modified CameraContent to use the prompt, showing how other services can use the changes.
Comment on attachment 607553 [details] [diff] [review] Changes to allow for prompting for more than geolocation, change to camera to use Fabrice is a much better reviewer for this. Fabrice, if you're too overloaded lmk and I'll pick this back up.
assigned to me for sec action to schedule a meeting
Re-setting blocking and dependency list, assuming it was removed by mistake by Michael.
Created attachment 632858 [details] WIP PermissionManager This is a work in progress of the actual permission manager. I'm not sure if the pattern I used for asynchronously responding to permission tests is the best, so feedback there would be appreciated. It doesn't interface into any UI yet (so it doesn't even try to prompt), nor is it tied to the electroysization (though the interfaces are quite similar). Mostly what this is here for is to show how the ApiTestPermission looks (asynchronously, with associated event being sent back), the tagging of permissions with app type, how the manifest permissions would be stored into the permission manager, and how the Settings would get and store changes by the user. Note that the uri is expected to be something different for apps. One thought is to use app://rest.of/uri and apps://rest.of/uri. This would keep app permissions from leaking to web permissions, and would allow mapping back to http and https for locking web access to the originating domain.
Jim, I don't quite understand what your patch is trying to solve. It appears that you're adding a few functions to add/remove/check permissions, but we already have functions to do that on nsIPermissionManager. You're also adding a new 'appType' argument here and there, but that won't really do much to help with the current shortcommings of the current API since we still can't key on which app we're talking about. I.e. we can't give facebook.com different access when installed as an app, compared to when browsed to using the web browser. In general the main thing that we need to change on nsIPermissionManager is to change to from keying on nsIURIs to keying on nsIPrincipal, and then make it pull out all the relevant information from the principal. Have you talked with Mounir who has started work on sticking the appropriate information on principals.
Seems like this is the best place to note it, but we need to ensure that only permissions explicitly enumerated in the manifest can be granted. This holds true for implicit and explicit permissions.
Created attachment 637242 [details] [diff] [review] WIP PermissionManager Here is a much more complete version, though not done yet. The functionality that is added is to 1) mass set defaults for permissions (like whatever parses the manifest will need). The defaults are held in the PermissionManager (and can be seen in the Init function). This function also tests if the requesting uri is allowed to set the permissions. 2) Two other mass permissions functions to get and set are there for the Settings app. 3) ApiTest(Exact)Permission that is asynchronous. Other changes are to validate that the permissions being requested are known, that for ApiTestPermission a permission that wasn't from the manifest is always denied. As for distinguishing between an installed app and web content, there was discussions previously (mailing lists and work week) and it looks like somewhat covered by bug 758258 that maybe the URI for installed apps might be different. I've inquired with Mounir but haven't gotten a response back. Whatever is selected can be used by the new functions once we know what they are. Also, this still needs a secure way to prompt the user to grant permissions when needed.
Hmm.. still not understanding all aspects of this patch, but it's looking closer. I'd really like to keep the permission manager API simple, so I don't think we should add support for mass setting/getting permissions. That's just as easy to do as a simple loop in the app.install() implementation. There's also the missing manifest uri etc which will determine with "data jar" to check the permission for. However Mounir is already working on that part in the "data jar" bug, so I think we should focus on the other pieces in this bug. Specifically what would be great to do here would be to add the infrastructure which calls the permission manager, and if needed calls into the gaia front-end code to bring up a prompt and asynchronously return a yes/no decision and write that into the permission manager if the decision is a permanent one.
We're going through and cleaning up the dependency tree for the security model bugs for basecamp. I'm not entirely clear on what work we're hoping to do in this bug, but since there's both discussions and patches here I don't want to close it without making sure that nothing is lost. We currently have the following, more narrowly scoped bugs: Bug 770731: API exposed to JS for managing app permissions. Useful for for example the settings app. Bug 764189: Tracking the overall security model Bug 758269: Making the install code put data into nsIPermissionManager based on information from the app manifest Is there anything else that we should do? If so, should we perhaps file separate bugs on that that more narrowly scoped. It's always hard to follow bugs which contain a lot of general discussions like this one does.
I'll take the silence as agreement that we have coverage on the topics discovered here. :-) Jokes aside, I *think* the topics discussed here has coverage in other more narrowly scoped bugs as per previous comment. However if you disagree, don't hesitate to reopen this bug, or file new bugs.