Closed Bug 1180684 Opened 4 years ago Closed 3 years ago

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

Categories

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

42 Branch
x86_64
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox41 --- affected
firefox42 --- affected
firefox51 --- fixed

People

(Reporter: cbadau, Assigned: m_kato)

References

Details

(Whiteboard: sbwn1)

Attachments

(2 files)

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.
Flags: needinfo?(bobowen.code)
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.
Flags: needinfo?(bobowen.code) → needinfo?(camelia.badau)
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
Flags: needinfo?(camelia.badau)
Summary: Keyboard interaction for some games don't work with the low integrity NPAPI plug-in sandbox enabled → Keyboard interaction for some games doesn't work with the low integrity NPAPI plug-in sandbox enabled
Duplicate of this bug: 1180645
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?
Flags: needinfo?(aklotz)
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.
I don't really have anything to add about what to do here, sorry. :-(
Flags: needinfo?(aklotz)
(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.
(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. :-(
See Also: → 1201904
Duplicate of this bug: 1239406
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
Whiteboard: [sb?]
Whiteboard: [sb?] → sb+
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?
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin) → needinfo?(bobowen.code)
(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.
Flags: needinfo?(bobowen.code)
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.
Whiteboard: sb+ → sbwn1
This Flash sandbox bug should probably block wider testing of 64-bit Firefox because they break existing Flash content.
Priority: -- → P2
Assignee: nobody → m_kato
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 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.
Attachment #8784239 - Flags: review?(aklotz) → review+
Comment on attachment 8784240 [details]
Bug 1180684 - Part 2. Hook GetKeyState on plugin process.

https://reviewboard.mozilla.org/r/73772/#review74224
Attachment #8784240 - Flags: review?(aklotz) → review+
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.
Pushed by m_kato@ga2.so-net.ne.jp:
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
https://hg.mozilla.org/mozilla-central/rev/f32b355726da
https://hg.mozilla.org/mozilla-central/rev/40854f9eaeb5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1303755
You need to log in before you can comment on or make changes to this bug.