751009 - [Security Review] Keyboard/IME API

Status: RESOLVED FIXED

(mozilla.org :: Security Assurance, task, P4)

Description

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

Enables implementing virtual keyboards.

Comment 1

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

Probably too early for this review. No bug yet AFAIK. Only this discusion:
https://groups.google.com/d/topic/mozilla.dev.webapi/Vs3-HGv9NNw/discussion

Comment 2

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

AFAIK this is not basecamp (keyboard is controlled by the settings app only). Need to review exactly how this happens as part of the settings app review.

Comment 3

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

Nothing to review until if/when this APi is designed/implemented. Open a new bug then.

Comment 4

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

So there is actually an API (navigator.mozKeyboard), which at the moment doesn't have a permission (but should be certified only- see bug 838308).

I am reviewing this API at the moment in conjunction with reviewing the Keyboard App. (bug 838389)

Comment 5

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

Rebooting this review. Based on the work rlu is doing.

Comment 6

[David Chan [:dchan]]

taking from stefan with target of 9/6

Comment 7

[David Chan [:dchan]]

I took a look at the Keyboard/IME API which was implemented in bug 899999. The code has landed in mozilla-central and I based my review off rev fff320870b20
http://hg.mozilla.org/mozilla-central/rev/fff320870b20

There are 4 main files which make up the IME API
b2g/chrome/content/forms.js
b2g/components/b2g.idl
b2g/components/MozKeyboard.js
b2g/components/Keyboard.jsm

b2g.idl added an interface for the Keyboard with 2 eventlisteners and some functions. Nothing terribly interesting in the file.

Keyboard.jsm
This is the file loaded into the chrome process. It contains a nsIMessageBroadcaster ppmm, which is used for communicating with all the Keyboard / Input applications. There are a bunch of message which the ppmm listens for as well as certain events to handle a child process crash. The important code resides in receiveMessage which checks that the msg.target's messagemanager has the keyboard permission. This check prevents arbitrary child processes from generating Keyboard events. The child app must have the keyboard permisison.

Certain "Form:" messages are converted to their equivalent "Keyboard:" messages. Overall the code is straightforward. There is one outstanding issue that is covered in bug 842436 which is multiple keyboards. If multiple "active" keyboards is allowed, then there are race conditions and the keyboards can interfere with each other. Since the current ppmm broadcasts its events, all the keyboards would receive them.

MozKeyboard.js
This is the code run in the child process. The important tidbits are in MozInputMethod and MozKeyboard init(). Both of these objects take in a window as an argument. The window is checked for the Keyboard permission and the appropriate object returned on permission allow. The receiveMessages() here don't perform permissions checks as they are unneeded. Child processes can only communicate with the parent and not with each other. A permission check in the child receiveMessage doesn't achieve anything since a compromised chrome process is game over.

Messsages are generated by the keyboard and sent to the parent for handling.

forms.js
This file implements the get/set of text/inputelements. This file exists in the chrome process. It uses the requestId idiom to pass results back to the correct keyboard.


Summary:
The Keyboard/IME API looks sound from a security perspective. The bug on limiting IME to the active input needs to be fixed before 3rd party keyboards are allowed. This goes hand in hand with the move of keyboard/input permission to privileged apps.