mus: Don't allow SetEventObserver to monitor key events

mus: Don't allow SetEventObserver to monitor key events

It should only be allowed to monitor mouse/touch events. Otherwise a malicious
or compromised app could spy on keystrokes for another app.

BUG=614792
TEST=added to mus_ws_unittests

Committed: https://crrev.com/ac96551d22ac7ff8389760422dd9641e72e878f0
Cr-Commit-Position: refs/heads/master@{#396032}

[James Cook, 2016-05-25 20:59:40]

jamescook@chromium.org changed reviewers:
+ erg@chromium.org

[James Cook, 2016-05-25 20:59:41]

erg, please take a look.

FYI - WindowTree::SetEventObserver() exists to implement the PointerWatcher
feature I built that allows the ash system tray bubble, shelf overflow bubble,
etc. to close on clicks outside their window bounds.

https://codereview.chromium.org/2016443003/diff/1/components/mus/ws/window_tr...
File components/mus/ws/window_tree_unittest.cc (left):

https://codereview.chromium.org/2016443003/diff/1/components/mus/ws/window_tr...
components/mus/ws/window_tree_unittest.cc:388: TEST_F(WindowTreeTest,
SetEventObserverNoTarget) {
I removed this test because the remaining event types supported by
SetEventObserver always have a target -- they are all mouse or touch events, and
if they don't hit a client window they are dispatched to the root window.

[Elliot Glaysher, 2016-05-25 21:26:40]

lgtm

Maybe off topic: I do wonder about the accelerator stuff which also uses the
EventMatcher interface. Maybe we should ensure that accelerator EventMatchers
have to have the control or alt or super flags set?

[rjkroege, 2016-05-25 22:26:58]

On 2016/05/25 21:26:40, Elliot Glaysher wrote:
> lgtm
> 
> Maybe off topic: I do wonder about the accelerator stuff which also uses the
> EventMatcher interface. Maybe we should ensure that accelerator EventMatchers
> have to have the control or alt or super flags set?

Might be worth asking tsepez@?

[James Cook, 2016-05-25 23:00:55]

On 2016/05/25 22:26:58, rjkroege wrote:
> On 2016/05/25 21:26:40, Elliot Glaysher wrote:
> > lgtm
> > 
> > Maybe off topic: I do wonder about the accelerator stuff which also uses the
> > EventMatcher interface. Maybe we should ensure that accelerator
EventMatchers
> > have to have the control or alt or super flags set?
> 
> Might be worth asking tsepez@?

I just talked to ben@ and sky@. I can't say I understand all the details, but
the WindowTree AddAccelerator code can only be invoked by the window manager
(via the WindowManagerClient interface, which is an associated interface on the
window manager's connection to the WS WindowTree interface). Arbitrary apps
can't bind to that interface.

The window manager exports an AcceleratorRegistrar interface, but this can only
be accessed by apps requesting the capability "mus:window_manager" in their
manifest.json files. app_driver is the only app that does this, for things like
making Ctrl-N open a browser when none is running. Apps cannot request new
capabilities at runtime, so a compromised renderer can't get that interface and
install accelerators to spy on keys.

[James Cook, 2016-05-25 23:01:04]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-05-25 23:02:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2016443003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2016443003/1

[commit-bot: I haz the power, 2016-05-25 23:10:10]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-25 23:12:36]

Description was changed from

==========
mus: Don't allow SetEventObserver to monitor key events

It should only be allowed to monitor mouse/touch events. Otherwise a malicious
or compromised app could spy on keystrokes for another app.

BUG=614792
TEST=added to mus_ws_unittests
==========

to

==========
mus: Don't allow SetEventObserver to monitor key events

It should only be allowed to monitor mouse/touch events. Otherwise a malicious
or compromised app could spy on keystrokes for another app.

BUG=614792
TEST=added to mus_ws_unittests

Committed: https://crrev.com/ac96551d22ac7ff8389760422dd9641e72e878f0
Cr-Commit-Position: refs/heads/master@{#396032}
==========

[commit-bot: I haz the power, 2016-05-25 23:12:37]

Patchset 1 (id:??) landed as
https://crrev.com/ac96551d22ac7ff8389760422dd9641e72e878f0
Cr-Commit-Position: refs/heads/master@{#396032}