1112709 - Flash Protected Mode Hook doesn't work on Windows 8/8.1: CreateFileW isn't hooking properly

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Marcia Knous [:marcia]]

QA testing of Bug 1108035 indicated there is an issue related to 8.1

https://etherpad.mozilla.org/FlashProtectedMode has the specifics from testing. Filing this bug to track the specific fix needed so that Windows 8.1 behaves properly.

Comment 1

[Marcia Knous [:marcia]]

Changing title.

Comment 2

[Benjamin Smedberg]

It appears that WindowsDllInterceptor isn't working in this case. Details:

For the NopSpacePatcher:
`fn` is correct, and points to the following assembly:

_CreateFileA@28:
77457410 FF 25 48 03 4B 77    jmp         dword ptr ds:[774B0348h]  
77457416 CC                   int         3  
77457417 CC                   int         3  
77457418 CC                   int         3  
77457419 CC                   int         3  
7745741A CC                   int         3  
7745741B CC                   int         3  
_CreateFileW@28: this is the kernel32.dll _CreateFileW@28
7745741C FF 25 44 03 4B 77    jmp         dword ptr ds:[774B0344h]   `fn` points here
77457422 CC                   int         3  
77457423 CC                   int         3  
77457424 CC                   int         3  
77457425 CC                   int         3  
77457426 CC                   int         3  
77457427 CC                   int         3  

So the initial 5-byte nop loop succeeds, but the subsequent check for a MOV fails.

The indirection of *0x774b03ff ends us up at an entirely different _CreateFileW@28 function at 0x76DF23DF which is Kernelbase.dll.

So it seems to me that we can either 1) do some kind of version-specific kernel32/kernelbase magic in the client or 2) teach nsWindowsDllInterceptor about this redirection pattern and have it patch the real function, not the redirector.

Ehsan/m_kato/dmajor/aklotz: do you have thoughts about which of these makes more sense?

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

The detours patcher can handle this case when the jmp is opcode 0x39 (near, relative), in which case it just modifies the target address of the jmp to point to the hook function. This case is a bit different since it's a far, absolute indirect jmp; I think we should add this case to the detour patcher's repertoire.

Comment 4

[Makoto Kato [:m_kato]]

We should analyze FF /4 or JMP [disp32].  Also, we should investigate Win10 for this too.

Comment 5

[(no longer active)]

I also think that we should extend the detour patches to understand this case.

Comment 6

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 8539989 [details] [diff] [review]
fix

We need support jmp [disp32] opcode for x86.

Also, Flash's protected mode supports x86 binary only, so we don't need it for x64 flash.  And, to hook CreateFileW on x64 binary with Win8, we need fix of bug 1113546 at first.

Comment 8

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d78f8b811a39

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d78f8b811a39

Comment 10

[(Away)]

Teaching nsWindowsDllInterceptor about this sounds good to me.

Comment 11

[Benjamin Smedberg]

Comment on attachment 8539989 [details] [diff] [review]
fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1108035 was ineffective on Windows 8/8.1 because of this bug.
[User impact if declined]: Bug 1119941 will not be effective on win8/8.1.
[Describe test coverage new/current, TBPL]: Has automated tests and has been in mozilla-central
[Risks and why]: Moderate-to-low risk: it's possible that this change could affect other functions which Firefox was already hooking, but we don't know of any and haven't seen ill effects.
[String/UUID change made/needed]: None

Comment 12

[Andrei Vaida [:avaida]]

I finished testing the fix pushed for this issue on Nightly 37.0a1 (2015-01-11), using Windows 8.1 32-bit and Windows 8 32-bit, but I'm still seeing the bug.

Here's an overview:
(A) Windows 8.1 32-bit (e10s enabled)
  (A.1) dom.ipc.plugins.flash.disable-protected-mode = false (default)
    * 2 x "Adobe Flash Player 16.0 r0" processes active
    * 2 x "Plugin Container for Nightly" processes active
  (A.2) dom.ipc.plugins.flash.disable-protected-mode = true
    * 2 x "Adobe Flash Player 16.0 r0" processes active
    * 2 x "Plugin Container for Nightly" processes active

(B) Windows 8 32-bit (e10s enabled)
  (B.1) dom.ipc.plugins.flash.disable-protected-mode = false (default)
    * 2 x "Adobe Flash Player 16.0 r0" processes active
    * 2 x "Plugin Container for Nightly" processes active
  (B.2) dom.ipc.plugins.flash.disable-protected-mode = true
    * 2 x "Adobe Flash Player 16.0 r0" processes active
    * 2 x "Plugin Container for Nightly" processes active

Similar to Bug 1108035, the tests were performed on various websites with flash content, using Flash 16.0.0.235. Please note that the browser was restarted after each pref change, to make sure that the new value is applied successfully.

Makoto, any thoughts on this?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ecdf4edd0b9

Comment 14

[Makoto Kato [:m_kato]]

(In reply to Andrei Vaida, QA [:avaida] from comment #12)
> I finished testing the fix pushed for this issue on Nightly 37.0a1
> (2015-01-11), using Windows 8.1 32-bit and Windows 8 32-bit, but I'm still
> seeing the bug.
> 
> Here's an overview:
> (A) Windows 8.1 32-bit (e10s enabled)
>   (A.1) dom.ipc.plugins.flash.disable-protected-mode = false (default)
>     * 2 x "Adobe Flash Player 16.0 r0" processes active
>     * 2 x "Plugin Container for Nightly" processes active
>   (A.2) dom.ipc.plugins.flash.disable-protected-mode = true
>     * 2 x "Adobe Flash Player 16.0 r0" processes active
>     * 2 x "Plugin Container for Nightly" processes active
> 
> (B) Windows 8 32-bit (e10s enabled)
>   (B.1) dom.ipc.plugins.flash.disable-protected-mode = false (default)
>     * 2 x "Adobe Flash Player 16.0 r0" processes active
>     * 2 x "Plugin Container for Nightly" processes active
>   (B.2) dom.ipc.plugins.flash.disable-protected-mode = true
>     * 2 x "Adobe Flash Player 16.0 r0" processes active
>     * 2 x "Plugin Container for Nightly" processes active
> 
> Similar to Bug 1108035, the tests were performed on various websites with
> flash content, using Flash 16.0.0.235. Please note that the browser was
> restarted after each pref change, to make sure that the new value is applied
> successfully.
> 
> Makoto, any thoughts on this?

Although I test on Windows 7 SP1 with Nightly 2014-01-12, dom.ipc.plugins.flash.disable-protected-mode doesn't work.  Could you test on Windows 7 too?

Parent process doesn't seem to call SendDisableFlashProtectedMode.

Comment 15

[Makoto Kato [:m_kato]]

I will continue as bug 1120747.  It doesn't depended on this.

Comment 16

[Makoto Kato [:m_kato]]

Windows 8.1 issue is both bug 1120747 and the following.

1. Flash 16 call CreateFileA("...\mmc.cfg"), not CreateFileW
2. Windows 8.1 UP1 is the following.  So we cannot hook reading mmc.cfg 

0:009> u kernel32!CreateFileA
KERNEL32!CreateFileA:
76af8920 ff259403b576    jmp     dword ptr [KERNEL32!_imp__CreateFileA (76b50394)]

0:007> u poi(KERNEL32!_imp__CreateFileA)
KERNELBASE!CreateFileA:
74fd7190 8bff            mov     edi,edi
74fd7192 55              push    ebp
74fd7193 8bec            mov     ebp,esp
...

74fd71b9 ff75fc          push    dword ptr [ebp-4]
74fd71bc e8dfcdffff      call    KERNELBASE!CreateFileW (74fd3fa0)
74fd71c1 8bf0            mov     esi,eax

So we need support redirection from kenrel32 to kernelbase or should hook CreteFileA instead.

Comment 17

[Andrei Vaida [:avaida]]

(In reply to Makoto Kato (:m_kato) from comment #14)
> Although I test on Windows 7 SP1 with Nightly 2014-01-12,
> dom.ipc.plugins.flash.disable-protected-mode doesn't work.  Could you test
> on Windows 7 too?

I'm seeing the same behavior on Windows 7 32-bit (SP1) as well, using Nightly 38.0a1 (2015-01-12) with e10s enabled:
 (A) dom.ipc.plugins.flash.disable-protected-mode = false (default)
    * 2 x "plugin-container.exe" processes active
    * 2 x "FlashPlayerPlugin_16_0_0_235.exe" processes active
 (B) dom.ipc.plugins.flash.disable-protected-mode = true
    * 2 x "plugin-container.exe" processes active
    * 2 x "FlashPlayerPlugin_16_0_0_235.exe" processes active

I used the following websites in my tests: 
 * http://www.albinoblacksheep.com/flash/wafflesloop
 * https://games.yahoo.com/
 * https://www.youtube.com/

Comment 18

[Makoto Kato [:m_kato]]

try run for new fix
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3e3d8584d06

Comment 19

[Benjamin Smedberg]

Attachment: BUG1112709

I think your fix is better, but for the record here's a patch which hooks CreateFileA in addition to CreateFileW.

Comment 20

[Makoto Kato [:m_kato]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #19)
> Created attachment 8549164 [details] [diff] [review]
> BUG1112709
> 
> I think your fix is better, but for the record here's a patch which hooks
> CreateFileA in addition to CreateFileW.


Your fix hooks both functions.  It is OK for Windows 8.1, but if on Windows 7, CreateFileA may call both hooked functions.

On Windows 7, kenrel32!CreateFileA will call kernel32!CreateFileW.
On Windows 8.1, kerenl32!CreateFileA will call kernelbase!CreateFileA, then it will call kenrelbase!CreateFileW.

Disassembling Windows 7's CreateFileA.

0:000> u kernel32!CreateFileA
kernel32!CreateFileA:
7652eb11 8bff            mov     edi,edi
...
7652eb3f ff75fc          push    dword ptr [ebp-4]
7652eb42 e80efeffff      call    kernel32!CreateFileWImplementation
...

0:000> u 7652e955
kernel32!CreateFileWImplementation:
7652e955 8bff            mov     edi,edi
...
7652e997 ff7508          push    dword ptr [ebp+8]
7652e99a e809000000      call    kernel32!CreateFileW (7652e9a8)
...

Comment 21

[Makoto Kato [:m_kato]]

benjamin creates new fix for hooking CreateFileA, so redirection issue is moved to bug 1121829

Comment 22

[Benjamin Smedberg]

To be clear, I don't want to take my patch, I just want to take bug 1121829. I'm going to mark this bug FIXED again since the patch here landed and got uplifted, and set tracking flags on your other one.

Comment 23

[Andrei Vaida [:avaida]]

Makoto, given that Bug 1120747 is now verified fixed and Windows 7 is no longer affected, is there anything else to be done by manual QA here?

Comment 24

[Makoto Kato [:m_kato]]

(In reply to Andrei Vaida, QA [:avaida] from comment #23)
> Makoto, given that Bug 1120747 is now verified fixed and Windows 7 is no
> longer affected, is there anything else to be done by manual QA here?

I think you should verify protected mode issue on Windows 8.1 with bug 1123966.

Comment 25

[Andrei Vaida [:avaida]]

This is verified fixed on Aurora 37.0a2 (2015-02-02) and Beta 36.0b5 (20150129200438), using Windows 7 32 bit.