As we are working on making Firefox OS support 3rd-party keyboard/IME, we need to make the "keyboard" permission down one level from "certified" app to "privileged" app. This will allow all the 3rd parties be able to submit their keyboard apps to marketplace as privileged apps. -- BTW, should we modify the permission from "keyboard" to "input" or "input-method"?

Before downgrading keyboard permission to privileged, we need to evaluate security risks and work out a solution. One important security issue has been raised and discussed in bug 842436.

Add keyword in whiteboard for EPM tracking and wiki site: https://wiki.mozilla.org/FirefoxOS/SprintStatus#Systems-Platform

I'll add a new privileged permission 'input' for the new Keyboard/IME API exposed as navigator.mozInputMethod. The original permission 'keyboard' for navigator.mozKeyboard will remain unchanged.

Fabrice, another bug that need your reviewing. Please reassign to other person if you don't have time.

Comment on attachment 796422 [details] [diff] [review] Add input permission to allow privileged app access keyboard/IME API. Review of attachment 796422 [details] [diff] [review]: ----------------------------------------------------------------- r- because of the asserPermission() issue, and also because I'd really like to avoid adding a new permission if possible. Let's check with Paul what he thinks. ::: b2g/components/Keyboard.jsm @@ +96,5 @@ > dump("!! No message manager found for " + msg.name); > return; > } > > + if (!mm.assertPermission("keyboard") && !mm.assertPermission("input")) { if assertPermission() return false, we kill the child process. That means that any process here that has only one of these permissions will be killed.

I'd like to rename the permission form 'keyboard' to 'input'. Because we may have kind of voice input method without keyboard in the future, 'input' is more appropriate for input method apps. To achieve this, we can add 'input' permission first without breaking existing gaia keyboard. Then we'll drop mozKeyboard and 'keyboard' permission after the 3rd party IME framework lands. Does it make sense?

This patch fixes the asserPermission() issue of Comment 5 and add the 'input' permission. If Fabrice & Paul think we should keep using the 'keyboard' permission, I'll make another patch. Thanks;)

That's only my opinion, but I really dislike having two permissions for that. Changing from 'keyboard' to 'input' (or maybe something a bit less generic than input) is fine for me, but please do that in a single step. Yes, we will need to do gaia changes as well, but this won't be the first time.

Pointer to Github pull-request

Comment on attachment 796797 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11815 Gaia patch to use the new permission name - 'input'.

I was going to say that we an use the |role| attribute to enhance the security a little bit and ensure the app with the permission is actually a |keyboard| but I'm not sure about the new API and what is exposed exactly...

I haven't seen any interface exposed to gecko to get or check the |role| of an App. We need expose the |role| with the way of or similar to the |permission|'s. Then the keyboard/IME API can enhance the security by checking the |role|. Before gecko is able to check the |role|, gaia should prevent installing app without keyboard role but with keyboard permission.

We can easily expose the manifest's role to gecko's mozIApplication if needed. (see https://mxr.mozilla.org/mozilla-central/source/dom/interfaces/apps/mozIApplication.idl) I don't think that relying on gaia is good enough here.

Agree with you. But it is a precaution to restrict keyboard permission to the keyboard app with corresponding role during the installation process. Fabrice, should I fix the |role| issue in this bug or a new bug? If we need a new bug, could you help to create one as you are the initial person to propose |role| and should know more about it? I'd like to work on it :-)

Yuan, I filed bug 912340. Feel free to take it, and let me know if you need any help.

Had a quick skim of this bug, and I think I agree with Fabrice in comment 8. (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #12) > I was going to say that we an use the |role| attribute to enhance the > security a little bit and ensure the app with the permission is actually a > |keyboard| but I'm not sure about the new API and what is exposed exactly... I don't understand what a 'role' is - where does this come from. David Chan is doing the security review at the moment for this API, so he is probably the better person to comment here.

(In reply to Paul Theriault [:pauljt] from comment #17) > > I don't understand what a 'role' is - where does this come from. The role comes from bug 892397.

I agree that determining the active keyboard is an important issue to resolve before moving keyboard to a privileged permission. My understanding is that we would be relaxing restrictions on permissions as we were more confident in the APIs. 'role' as currently implemented in unsuitable for use due to there not being any Gecko checks. However once bug 912340 and bug 842436 lands, everything looks good to me from a security perspective. I'll follow up in the Keyboard security review with comments on the API itself.

Update keyword in white board for the status query more precise

The patch downgrades keyboard permission to privileged and uses the |role| attribute to enhance the security.

Filed another bug to rename the keyboard permission(Bug 915570 - Rename 'keyboard' permission and role).

Hi Vivien, Would you mind help on the review of the patch in the bug. I appreciate if it can be reviewed by today so that Xulei can follow it and hopefully get it done by this week. Thank you very much.

Comment on attachment 803555 [details] [diff] [review] permission.v1.patch Review of attachment 803555 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/components/MozKeyboard.js @@ +20,5 @@ > "@mozilla.org/thread-manager;1", "nsIThreadManager"); > > +XPCOMUtils.defineLazyGetter(this, "DOMApplicationRegistry", function () { > + Cu.import("resource://gre/modules/Webapps.jsm"); > + return DOMApplicationRegistry; MozKeyboard.js is a in the child, but you can load DOMApplicationRegistry only in the parent process. @@ +50,5 @@ > + let app = DOMApplicationRegistry.getAppByLocalId(principal.appId); > + if (!(app && app.role === "system")) { > + dump("No 'system' role to use the keyboard API for " + > + principal.origin + "\n"); > + } I'm quite skeptical as what security the role property adds there... If all keyboards must have the 'keyboard' role, we should enforce that when we install the permissions, but not here. ::: dom/apps/src/PermissionsTable.jsm @@ +283,5 @@ > certified: ALLOW_ACTION > }, > "keyboard": { > app: DENY_ACTION, > + privileged: ALLOW_ACTION, I thought that we were renaming the permission also?

(In reply to Fabrice Desré [:fabrice] from comment #24) > Comment on attachment 803555 [details] [diff] [review] > permission.v1.patch > > @@ +50,5 @@ > > + let app = DOMApplicationRegistry.getAppByLocalId(principal.appId); > > + if (!(app && app.role === "system")) { > > + dump("No 'system' role to use the keyboard API for " + > > + principal.origin + "\n"); > > + } > > I'm quite skeptical as what security the role property adds there... > > If all keyboards must have the 'keyboard' role, we should enforce that when > we install the permissions, but not here. > So we only need check the role in PermissionsInstaller.jsm(http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsInstaller.jsm#57)? > ::: dom/apps/src/PermissionsTable.jsm > @@ +283,5 @@ > > certified: ALLOW_ACTION > > }, > > "keyboard": { > > app: DENY_ACTION, > > + privileged: ALLOW_ACTION, > > I thought that we were renaming the permission also? I filed bug 915570 to rename the permission.

(In reply to Yuan Xulei [:yxl] from comment #25) > > > So we only need check the role in > PermissionsInstaller.jsm(http://mxr.mozilla.org/mozilla-central/source/dom/ > apps/src/PermissionsInstaller.jsm#57)? Yes. But once again, I'm not convinced that this gives us any additional security.

Fabrice, thanks. The patch checks the role before installing permissions.

I don't think we need to be checking the role here. Doing so is not in line with other APIs and risks blurring our permission model. As far as I understand the situation, to be a 3rd party keyboard: - The 'keyboard' (soon to be inputmethod) permission allows access to listen event _iff_ you are the active keyboard - To be the active keybaord, the system app will check that you have both the permission and the role before calling setInputMethodActive So checking the role in gecko is unnecessary, and potentially confusing for the future think? Afaik, any app can put any role in its manifest so I don't think we should be relying on it for anything other than Gaia UI reasons.

Comment on attachment 803673 [details] [diff] [review] permission.v2.patch Review of attachment 803673 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review per comment 28

It is quite a simple patch. Just allow privileged apps to access keyboard permission.

Comment on attachment 804490 [details] [diff] [review] permission.v3.patch Review of attachment 804490 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/PermissionsTable.jsm @@ +285,5 @@ > "keyboard": { > app: DENY_ACTION, > + privileged: ALLOW_ACTION, > + certified: ALLOW_ACTION, > + roles: ["system", "keyboard"] remove this "roles" property.

Fabrice, thank you!!