Currently mozKeyboard doesn't require a permission to use. That means any web app can inject text into any other app just by using navigator.mozKeyboard.setValue. I haven't been able to make sendKeys work that way, yet. For an example: 1. Host the attached file somewhere. It's on http://sigsegv.es/testK.html also for the time being. 2. Open the page with B2G web browser. 3. When it loads, touch the browser address bar. Or just open other app (without killing the browser) and select any input text bar. 4. See the input bar autofill. While this could be worse if it allowed to *read* the content of the text boxes instead of just writing them, this kind of interaction between apps/content should not be allowed.

I don't believe a permission would provide sufficient isolation between apps. It appears that two apps with the new mozKeyboard permission would still be able to set each others' textfields. I think there needs to be additional restrictions on the sending/receiving of the messages. Perhaps enforcing that only the foreground app can send mozKeyboard events or adding an identifier for mozKeyboard events similar to requestID. There is still an issue with multiple tabs within a browser app being able to interact if we use a per-app solution.

True but... only keyboard apps should be able to get that permission, and keyboard apps should, by definition, be able to interact with any other app :). Furthermore, if that permission restricts mozKeyboard to certified apps then for V1 it would fix the problem, since we can give that permission only to the keyboard app and that's it. For v2 and beyond, and if/when the extensible keyboard API is developed, a simple permission won't probably cut it.

I'm going to report this bug to the thread about new Keyboard API. Anyway, I would address the problem by enforcing CSR. INMO, the problem is the sender of the `setValue` message must to be compared with the receiver of the message. If they don't belong to the same domain, avoid the communications. I think we should keep mozKeyboard open without permission because you are free to access the keyboard on your domain, not others.

(In reply to Salvador de la Puente González [:salva] from comment #3) > I'm going to report this bug to the thread about new Keyboard API. Anyway, I > would address the problem by enforcing CSR. INMO, the problem is the sender > of the `setValue` message must to be compared with the receiver of the > message. If they don't belong to the same domain, avoid the communications. Salva, that would not allow the use case of the keyboard app, which I think is the main use case for this API. > > I think we should keep mozKeyboard open without permission because you are > free to access the keyboard on your domain, not others. IMO anything that simulates user actions should be kept privileged, since there are some mitigation measures that depend on the user having taken some action. For example, what happens with the actions that can only be executed on event handlers as a mitigation measure (setfullscreen?) if we allow events to be simulated? This should be a certified (for this version anyway)/privileged (after a thorough review/rewrite/rethink) API.

(In reply to Antonio Manuel Amaya Calvo from comment #4) > (In reply to Salvador de la Puente González [:salva] from comment #3) > > I'm going to report this bug to the thread about new Keyboard API. Anyway, I > > would address the problem by enforcing CSR. INMO, the problem is the sender > > of the `setValue` message must to be compared with the receiver of the > > message. If they don't belong to the same domain, avoid the communications. > > Salva, that would not allow the use case of the keyboard app, which I think > is the main use case for this API. I noticed this in the mail I sent to the list. The (trusted) keyboard application is an exception, it has a special permission to inject text anywhere. > > > > > I think we should keep mozKeyboard open without permission because you are > > free to access the keyboard on your domain, not others. > > IMO anything that simulates user actions should be kept privileged. I see the point here. If you want to alter another DOM element, use the DOM, do not impersonate the user. If you want to simulate a user action, ask for the privilege.

IMO, the mozKeyboard API could be used only by the active keyboard app. To be able to interact with other apps through mozKeyboard, the app should meet two requirements: 1) It is a keyboard app, declared in the manifest file with a special keyboard permission. 2) It is launched as the active keyboard receiving user's input. Then for v1 as there is only one keyboard app, which is always active, we should restrict the usage of mozKeyboard to that app only.

CC'ing Jonas. Do you think we should add the permission?

Just to add some insight into this, in case it isn't obvious.... let's assume that I buy the domain imashamelesspirate.com, and buy a certificate from any good CA, say Verisign, with that domain. Then I set a page on https://imashamelesspirate.com where I put a phishing page spoofing www.yourchosenbank.com (adding a <title>YourChosenBank(tm)</title>). And I add to my page a little code like the one I shown on my example that whenever the browser URL bar is focused writes https://www.yourchosenbank.com. The way the browser app works is: when the browser URL bar isn't focused, it shows the title (YourChosenBank(tm)) and when the browser URL bar is focused, it shows the URL (should show https://imashamelesspirate.com, will show https://www.yourchosenbank.com). And there it is, we've given phishers an almost bulletproof way to fool security conscious users. BTW, I didn't mark this as a private security bug since there isn't a commercial/public release yet. Otherwise it would most definitely qualify.

Great example Antonio. This is a big concern. Btw. The same pishing attack can be done in a much simpler way. What URL do you think I have open on http://dl.dropbox.com/u/2988854/Screenshots/Screen%20Shot%202013-02-07%20at%203.22.43%20PM.png ? I can tell you it is not http://www.royalbank.com It is http://www.royalbank.com.imapirate.com Because the URL is so long, the user will never see that it is an attack.

I'll let Jonas have the final say, but back when first discussed this API, it was decided to be certified only for V1. This is what is documented on the web API page. (https://wiki.mozilla.org/WebAPI)

I think we should use a certified-only permission. That's reasonably non-risky for v1.

Manifests changes needed on the gaia side.

ladamski considers this a security blocker.

Comment on attachment 711696 [details] [diff] [review] gecko patch Review of attachment 711696 [details] [diff] [review]: ----------------------------------------------------------------- The good news is the vulnerability is plugged. The bad news is that selects don't work anymore. I can click on it, the selection choices appear, but the selection isn't saved. You can check that on the FTU (can't choose the continent and country for the timezone). ::: b2g/components/Keyboard.jsm @@ +64,5 @@ > + dump("Keyboard message " + msg.name + > + " from a content process with no 'keyboard' privileges."); > + return null; > + } > + For some reason this doesn't apply cleanly neither on a freshly pulled b2g18 nor on m-c. My Keyboard.jsm doesn't have the previous lines (the try catch block)

Now with select fields working. I played around and everything looks fine.

This shouldn't really be a permission. We should only expose the keyboard object in the app which is instantiated as the keyboard app. And in the parent process we should only be listening to messages from that app. Having a permission as well isn't a bad idea, though. So if that's easier to implement then we can start there.

(In reply to Jonas Sicking (:sicking) from comment #17) > This shouldn't really be a permission. We should only expose the keyboard > object in the app which is instantiated as the keyboard app. And in the > parent process we should only be listening to messages from that app. That is doable, but with more changes than this patch. The system app launches the keyboard app (from a hardcoded URL for now) and then both the keyboard app and the system app use the keyboard api. The keyboard app uses sendKey() but also onfocuschange(). The system app uses onfocuschange(), setSelectedOption(), removeFocus() and setValue(). onfocuschange() is triggered by a message from the frame script forms.js when we focus or blur an input field. So we could fix that the way you want it by: - knowing which app is the keyboard app with a setting. - let the keyboard app and the system app (we know the manifestURL for this one already) access onfocuschange() - let the system app access all the mozkeyboard methods. I think that we can implement that properly by returning different objects QIed() from nsIDOMGlobalPropertyInitializer.init(). > Having a permission as well isn't a bad idea, though. So if that's easier to > implement then we can start there. If the previous idea is sane, we should rather do it at not add a new permission imho.

(In reply to Jonas Sicking (:sicking) from comment #17) > This shouldn't really be a permission. We should only expose the keyboard > object in the app which is instantiated as the keyboard app. And in the > parent process we should only be listening to messages from that app. This sounds a lot like what having a keyboard permission on the keyboard (and system as per Fabrice explanation) app would do: Only the app insantiated as the keyboard app (which has the permission) would have visibility of the keyboard object/can use the keyboard object. And the parent process only accepts messages from apps that have that permission. And using the permission has the advantages that all the mechanisms are in place already (and that makes the patch/change smaller) and that it's more homogeneous with how the rest of the system works. We don't have a special case for the dial app to use the ability to dial, or for the SMS app to use the SMS capability. We just have permissions for capabilities. And acting on behalf of the user (which is what a virtual keyboard does) is one of such capabilities.

Comment on attachment 712060 [details] [diff] [review] gecko patch v2 Review of attachment 712060 [details] [diff] [review]: ----------------------------------------------------------------- I tried this and it seems to work correctly. The exploit doesn't work anymore and the selects work correctly.

This patch removes the need to add explicit permissions to the system and keyboard app, but checks the appIds instead. To protect from compromised content processes, we check in the parent for the "keyboard" permission that we only set for the system and keyboard apps.

Gaia patch needed for the gecko patch v3 to work.

Comment on attachment 713309 [details] [diff] [review] Gecko patch v3 Review of attachment 713309 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty sure this will work... but it's plain ugly. Frankly, once we have a permission framework in place I don't understand why we're using hardcoded exceptions. The 'permissions' permission is only used by the Settings app currently, but we didn't add a hardcoded check for that, we just added a new permission as it should be. In any case, that's my opinion. Going to need-info Lucas on this, since he's the responsible of all things security to get his input.

Please, Lucas, can you pipe in about the need or not to have an explicit permission for the keyboard API instead of hardcoding the apps that can use it?

Comment on attachment 713311 [details] [diff] [review] Gaia Patch v2 Review of attachment 713311 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the value applied to keyboard_manager.js. ::: build/preferences.js @@ +13,5 @@ > homescreen += "/index.html"; > } > prefs.push(["browser.homescreenURL", homescreen]); > +prefs.push(["keyboard.manifestURL", KEYBOARD + (GAIA_PORT ? GAIA_PORT : '') + > + "/manifest.webapp"]); You also want to use that in apps/system/js/keyboard_manager.js

Comment on attachment 713309 [details] [diff] [review] Gecko patch v3 Review of attachment 713309 [details] [diff] [review]: ----------------------------------------------------------------- Are the changes to Keyboard.jsm really needed? ::: b2g/components/MozKeyboard.js @@ +62,5 @@ > + dump("No permission to use the keyboard API for " + > + principal.origin + "\n"); > + return null; > + } > + Another way would have been to return null if we are in a content process. System, Browser and Keyboard are in-process and they should be the only one. ::: dom/apps/src/PermissionsTable.jsm @@ +259,5 @@ > "open-remote-window": { > app: DENY_ACTION, > privileged: DENY_ACTION, > certified: ALLOW_ACTION > + } You don't need the change to this file.

I agree with Fabrice that this really should be a permission, but given where we are for 1.0 the hardcoded approach is tolerable.

(In reply to Lucas Adamski from comment #27) > I agree with Fabrice that this really should be a permission, but given > where we are for 1.0 the hardcoded approach is tolerable. Er the permission solution was *already* implemented (V2) and in fact required less changes (at least on the Gaia side).

Even though I still think that the permission solution was better (and in fact Lucas agreed with that) and even if that patch was already implemented, what is true is that we need to land something to fix this before the branching. So... if you think that the hardcode solution is better then land it, or land the permissions one, but land one :) We should not ship the next version with the keyboard API as it is right now.

Will this potentially affect Marionette, and its sendKeys() functionality? http://dxr.mozilla.org/mozilla-central/testing/marionette/marionette-sendkeys.js.html

That does not work (we always go in the default CheckOnlyManifestURL from nsFrameMessageManager.h) yet. Halp!

I think the perfect solution is to use a permission as well as a smartness in the system app to make sure we only listen to events from the "current keyboad app". To support installable keyboards we don't want to let any app which has the keyboard permission to inject text arbitrarily. Only the app which the user is currently using as keyboard is the one we should be able to inject text. So we still need something like the approach used in the current patch to enforce that. But we still should require that any app which is installable as a keyboard app has been reviewed for this. So that it doesn't do something like keyboard logging to sniff passwords. Using a permission is a good solution for that. So ultimately we need to do both things. For now, since we don't have installable keyboards yet, either approach will solve the immediate need. So I think we should just go with whatever is simplest (which, if we have a working patch, is whatever that patch does :) )

Ok, so reverting review to the patches that implement the permission based control. We'll add the other checks when we'll support installable keyboards.

(In reply to Fabrice Desré [:fabrice] from comment #34) > https://github.com/mozilla-b2g/gaia/commit/ > 91a25a586f0f445f807f2ce5c4878173a97374d6 v1-train: f043cb83fd8ff9341ef449d8ab4d2adc98ac4d11 v1.0.1: e16d66293430043f9c0824875256c69e85b62c7b I'll hold off setting the status-b2g18* flags on the gecko portion landing.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e46320645d1d https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/4c8036d2efed

(In reply to Jonas Sicking (:sicking) from comment #32) > I think the perfect solution is to use a permission as well as a smartness > in the system app to make sure we only listen to events from the "current > keyboad app". > > > To support installable keyboards we don't want to let any app which has the > keyboard permission to inject text arbitrarily. Only the app which the user > is currently using as keyboard is the one we should be able to inject text. > So we still need something like the approach used in the current patch to > enforce that. > > But we still should require that any app which is installable as a keyboard > app has been reviewed for this. So that it doesn't do something like > keyboard logging to sniff passwords. Using a permission is a good solution > for that. > > So ultimately we need to do both things. CC'ing ppl working on 3rd keyboard app support (bug 816869) I think Gecko can tell which app is currently active by it's visibility state.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #39) > > CC'ing ppl working on 3rd keyboard app support (bug 816869) > > I think Gecko can tell which app is currently active by it's visibility > state. Filed bug 842436 for this.

No need to create a TC in Moztrap for this defect.

Cannot verify, need steps to blackbox test this issue.

(In reply to Darren from comment #42) > Cannot verify, need steps to blackbox test this issue. If the "keyboard" permission is set, you can check the navigator.mozKeyboard is an instanceof nsIB2GKeyboard . This is what I do for my test in bug 815105 Here is the code that performs the permission check http://mxr.mozilla.org/mozilla-central/source/b2g/components/MozKeyboard.js#40

Antonio - can you please test this to verify that it's now fixed? If so, move status from Resolved=>Verified. Thanks!

(In reply to John Hammink from comment #44) > Antonio - can you please test this to verify that it's now fixed? If so, > move status from Resolved=>Verified. Thanks! Done :) My test page behaves correctly now (that is, doesn't inject text anywhere).

Per comment 45,I clear "Verifyme".