838308 - mozKeyboard should require a permission to use

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Example of naive attack

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.

Comment 1

[David Chan [:dchan]]

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.

Comment 2

[Antonio Manuel Amaya Calvo (:amac)]

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.

Comment 3

[Salvador de la Puente González [:salva]]

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.

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

(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.

Comment 5

[Salvador de la Puente González [:salva]]

(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.

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

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.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

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.

Comment 9

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

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.

Comment 10

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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)

Comment 11

[[:fabrice] Fabrice Desré]

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

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: gecko patch

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: gaia patch

Manifests changes needed on the gaia side.

Comment 14

[Alex Keybl [:akeybl]]

ladamski considers this a security blocker.

Comment 15

[Antonio Manuel Amaya Calvo (:amac)]

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)

Comment 16

[[:fabrice] Fabrice Desré]

Attachment: gecko patch v2

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

Comment 17

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

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.

Comment 18

[[:fabrice] Fabrice Desré]

(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.

Comment 19

[Antonio Manuel Amaya Calvo (:amac)]

(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 20

[Antonio Manuel Amaya Calvo (:amac)]

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.

Comment 21

[[:fabrice] Fabrice Desré]

Attachment: Gecko patch v3

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.

Comment 22

[[:fabrice] Fabrice Desré]

Attachment: Gaia Patch v2

Gaia patch needed for the gecko patch v3 to work.

Comment 23

[Antonio Manuel Amaya Calvo (:amac)]

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.

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

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 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 27

[Lucas Adamski [:ladamski]]

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

Comment 28

[Antonio Manuel Amaya Calvo (:amac)]

(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).

Comment 29

[Antonio Manuel Amaya Calvo (:amac)]

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.

Comment 30

[Stephen Donner [:stephend] Not actively reading bugmail]

Will this potentially affect Marionette, and its sendKeys() functionality?

http://dxr.mozilla.org/mozilla-central/testing/marionette/marionette-sendkeys.js.html

Comment 31

[[:fabrice] Fabrice Desré]

Attachment: add a checkContainApp() that does not kill the process when failing.

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

Comment 32

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

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 :) )

Comment 33

[[:fabrice] Fabrice Desré]

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.

Comment 34

[[:fabrice] Fabrice Desré]

https://github.com/mozilla-b2g/gaia/commit/91a25a586f0f445f807f2ce5c4878173a97374d6

Comment 35

[John Ford [:jhford] CET/CEST Berlin Time]

(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.

Comment 36

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef0e08b4530

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ef0e08b4530

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

Comment 39

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 40

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 41

[amiller]

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

Comment 42

[Darren]

Cannot verify, need steps to blackbox test this issue.

Comment 43

[David Chan [:dchan]]

(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

Comment 44

[John Hammink]

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

Comment 45

[Antonio Manuel Amaya Calvo (:amac)]

(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).

Comment 46

[Shally Li（leave from Mozilla）]

Per comment 45,I clear "Verifyme".