1180684 - Keyboard interaction for some games doesn't work with the low integrity NPAPI plug-in sandbox enabled

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Camelia Badau [:cbadau], Desktop Test Engineering]

Reproducible on:
* Nightly 42.0a1 (2015-07-05)
* Aurora 41.0a2 (2015-07-05)

Affected platforms:
* Windows 10 Pro x64 (Insider Preview Build 10158)
* Windows 8.1 Pro x64
* Windows 7 x64

Steps to reproduce:
1. Launch Firefox.
2. Enable the low integrity NPAPI plug-in sandbox for Flash by setting "dom.ipc.plugins.sandbox-level.flash" to "2". Restart the browser to make sure this setting is applied.
3. Visit http://www.addictinggames.com/action-games/cave-of-wonders-game.jsp or http://www.y8.com/games/new_york_shark or http://www.y8.com/games/dark_cut and try to use one of these games. 

Expected result:
The game is responsive and works as expected, without causing any hangs or crashes.

Actual result:
The game's controls are not working - keyboard input is ignored. Mouse events are working. 

Notes:
1. This issue is reproducible on 64-bit Nightly 42, with that pref set to 2. Without having that pref set to 2, it works fine.
2. I've checked each game on Google Chrome 43.0.2357.130 m and it works fine.
3. This issue is NOT reproducible on Release 39.0 32-bit (20150630154324) and Beta 40.0b1 (20150702173756), where the pref is not configured.

Comment 1

[Bob Owen (:bobowen) (PTO until 5th Jan)]

Camelia, thanks for testing these.

In the summary you say keyboard interaction doesn't work for _some_ games.
Have you found any for which it does work?

That would possibly be useful for tracking down a fix.

Comment 2

[Camelia Badau [:cbadau], Desktop Test Engineering]

Here are some examples of games for which keyboard interaction works (with "dom.ipc.plugins.sandbox-level.flash" set to "2"): 
- http://www.addictinggames.com/action-games/double-bros-game.jsp
- http://www.y8.com/games/v8_muscle_cars_2
- http://www.y8.com/games/save_the_pig
- http://www.addictinggames.com/zombie-games/zombie-situation-game.jsp

Comment 4

[Bob Owen (:bobowen) (PTO until 5th Jan)]

OK, I think I've worked out what the problem is here.

I've hooked WH_GETMESSAGE and I can see the key messages hitting the NPAPI process.

However, all the ones that don't work are using Key.isDown (from ActionScript 1 or 2) to detect key presses.
My guess is that this uses GetAsyncKeyState behind the scenes and that is getting blocked when running at low integrity.
I suppose it could be using GetKeyState, my understanding (from just reading the docs :-) ) is that, that should work as it works using the key messages, but maybe that's being disrupted somehow.

The games that work all use KeyboardEvent that replaced Key.isDown in ActionScript 3.

Aaron, I noticed that you were CCed ... sorry for yet another needinfo. :-)
Do you have any ideas as to how we might work around this problem?
Could we hook our own GetAsyncKeyState function (assuming that's the issue) in the NPAPI process and simulate it using the key messages or possibly GetKeyState?

Comment 5

[Bob Owen (:bobowen) (PTO until 5th Jan)]

After hooking GetKeyState and GetAsyncKeyState, I've found it is actually GetKeyState that is being called.

This seems to be blocked by low integrity for some reason.

It is even called for a game that works, but it doesn't seem to affect the game as far as I can tell.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

I don't really have anything to add about what to do here, sorry. :-(

Comment 7

[Bob Owen (:bobowen) (PTO until 5th Jan)]

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #6)
> I don't really have anything to add about what to do here, sorry. :-(

That's OK, as usual I'm a bit out of my depth, so I thought you might have some experience of this sort of thing.
I've actually got something that seems to work by storing key downs and ups from the key messages and then returning the current stored value from the hooked GetKeyState function.

I need to think how this will work when there are multiple plugins calling GetKeyState in the same flash process.

Not sure whether people will think this solution is viable, but at least there is some hope, which is more than I had yesterday.

Comment 8

[Bob Owen (:bobowen) (PTO until 5th Jan)]

(In reply to Bob Owen (:bobowen) from comment #7)

> I've actually got something that seems to work by storing key downs and ups
> from the key messages and then returning the current stored value from the
> hooked GetKeyState function.

Aside from not being able to work out how to handle the multiple plugin case, as we don't know what plugin the GetKeyState came from.

I've also realised this idea doesn't work for windowless plugins as the key messages hit the HWND in the broker process. :-(

Comment 10

[csamborski]

Here are some other examples from the bug 1239406:
Meat Boy (map pack): http://www.newgrounds.com/portal/view/472826
Hammerfest: http://www.hfest.net/try.html

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

This bug seems to have stalled a while back. It came up a few months ago in dup bug 1239406. Do we need to prioritize this fix or is this of low value right now?

Comment 12

[Benjamin Smedberg]

Bob, can we detect this pattern in our sandbox or with a hook, to try and collect telemetry/measure this in the wild?

For win64 this is not high priority. If we start rolling the new sandbox out to win32, it is probably much more important.

Comment 13

[Bob Owen (:bobowen) (PTO until 5th Jan)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> Bob, can we detect this pattern in our sandbox or with a hook, to try and
> collect telemetry/measure this in the wild?

I don't think the sandbox itself can help.

I'm not sure we can by other means either.
We could hook GetKeyState, but if comment 5 is still correct, then that gets called even for games that work, so it probably wouldn't give us useful data.

There may be some other ways of detecting it.
I'll try and take another look at some point, but it won't be near the top of my list for a while.

Comment 14

[moute3]

I've been experiencing this bug too. This is a weird bug though. The keyboard inputs will be taken at one point, the next it will stop working then a few minutes later start working again in http://www.kongregate.com/games/DustinAux/the-enchanted-cave-2. But in www.kongregate.com/games/Tukkun/anti-idle-the-game , it will know I pressed a key, just not which one.

Comment 15

[Chris Peterson [:cpeterson]]

This Flash sandbox bug should probably block wider testing of 64-bit Firefox because they break existing Flash content.

Comment 16

[Makoto Kato [:m_kato]]

Attachment: Bug 1180684 - Part 1. Analyze MOV with GS.

Allow MOV with GS prefix on x64.  Windows 10 uses this opcode on GetKeyState().

Review commit: https://reviewboard.mozilla.org/r/73770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73770/

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Bug 1180684 - Part 2. Hook GetKeyState on plugin process.

Review commit: https://reviewboard.mozilla.org/r/73772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73772/

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8784239 [details]
Bug 1180684 - Part 1. Analyze MOV with GS.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73770/diff/1-2/

Comment 19

[Makoto Kato [:m_kato]]

Comment on attachment 8784240 [details]
Bug 1180684 - Part 2. Hook GetKeyState on plugin process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73772/diff/1-2/

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8784239 [details]
Bug 1180684 - Part 1. Analyze MOV with GS.

https://reviewboard.mozilla.org/r/73770/#review74200

::: xpcom/build/nsWindowsDllInterceptor.h:81
(Diff revision 2)
> +
> +  switch (mod) {
> +  case 0:
> +    if (rm == 4) {
> +      // SIB
> +      if ((*(aOp + 1) & 0xf) == 5) {

I think you want 0x7 instead of 0xf to mask the lowest 3 bits of the SIB byte.

But do we really need to examine the SIB byte here? Based on what I see in the Intel processor manuals, I think we can assume disp32 based on mod == 0 and r/m == 0x5 alone... but I suppose either way produces the same result.

But please fix the mask at the very least.

::: xpcom/build/nsWindowsDllInterceptor.h:88
(Diff revision 2)
> +        return 6;
> +      }
> +      return 2;
> +    } else if (rm == 5) {
> +      // [RIP/EIP + disp32]
> +      // Since we don't modify relative offset, we should mark as improssible

nit: s/improssible/impossible

::: xpcom/build/nsWindowsDllInterceptor.h:701
(Diff revision 2)
>        } else if ((origBytes[nBytes] & 0xf0) == 0x50) {
>          // 1-byte push/pop
>          nBytes++;
> +      } else if (origBytes[nBytes] == 0x65) {
> +        // GS prefix
> +        if (origBytes[nBytes + 1] == 0x48 &&

I guess this is ok for now, but could you add an additional comment to show that you're checking for REX followed by the opcode here?

I'd really like us to refactor this code at some point so that we could properly process prefixes in such a way that they would automatically have an effect on all subsequent bytes, rather than having to special case on this.

I know that would take some work though and is best dealt with in a separate bug. :-)

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8784239 [details]
Bug 1180684 - Part 1. Analyze MOV with GS.

https://reviewboard.mozilla.org/r/73770/#review74222

Oh, and r+ once those issues are fixed.

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8784240 [details]
Bug 1180684 - Part 2. Hook GetKeyState on plugin process.

https://reviewboard.mozilla.org/r/73772/#review74224

Comment 23

[Makoto Kato [:m_kato]]

Comment on attachment 8784239 [details]
Bug 1180684 - Part 1. Analyze MOV with GS.

https://reviewboard.mozilla.org/r/73770/#review74200

> I think you want 0x7 instead of 0xf to mask the lowest 3 bits of the SIB byte.
> 
> But do we really need to examine the SIB byte here? Based on what I see in the Intel processor manuals, I think we can assume disp32 based on mod == 0 and r/m == 0x5 alone... but I suppose either way produces the same result.
> 
> But please fix the mask at the very least.

Thanks.  This is typo.  r/m==5 will need to re-calculate and modify offset that is relative from current IP reg.

> I guess this is ok for now, but could you add an additional comment to show that you're checking for REX followed by the opcode here?
> 
> I'd really like us to refactor this code at some point so that we could properly process prefixes in such a way that they would automatically have an effect on all subsequent bytes, rather than having to special case on this.
> 
> I know that would take some work though and is best dealt with in a separate bug. :-)

I will file new bug for it.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f32b355726da
Part 1. Analyze MOV with GS. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/40854f9eaeb5
Part 2. Hook GetKeyState on plugin process. r=aklotz

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f32b355726da
https://hg.mozilla.org/mozilla-central/rev/40854f9eaeb5