1495512 - Users report Firefox 64 is no longer opening/starting up when Dr.Web is installed

Status: VERIFIED FIXED

(Core :: mozglue, defect, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1495114#c2

Comment 1

[Jim Mathies [:jimm]]

Hey Adam, can you assist with outreach? We're expecting this 3rd party to address whatever the issue is here.

https://company.drweb.com/contacts/moscow/?lng=en

Comment 2

[Adam Stevenson [:adamopenweb]]

Reaching out to Dr.Web by email.

Comment 3

[Alexander Nosov]

Hi All,

My name is Alex and I present Dr.Web. We find some problems in FireFox patcher.

Our hook looks like

ntdll!NtMapViewOfSection:
00007ffd`24d3a3e0 b81089743e      mov     eax,3E748910h
00007ffd`24d3a3e5 4863c0          movsxd  rax,eax
00007ffd`24d3a3e8 ffe0            jmp     rax
00007ffd`24d3a3ea 250803fe7f      and     eax,offset SharedUserData+0x308 (00000000`7ffe0308)
00007ffd`24d3a3ef 017503          add     dword ptr [rbp+3],esi
00007ffd`24d3a3f2 0f05            syscall
00007ffd`24d3a3f4 c3              ret
00007ffd`24d3a3f5 cd2e            int     2Eh

Instruction movsxd  rax,eax used to reduce size of patch.

Function WindowsDllPatcherBase::ResolveRedirectedAddress(FARPROC aOriginalFunction) (firefox \mozglue\misc\interceptor\PatcherBase.h) don't detect redirection to 0x3E748910 that looks like

kd> u 3E748910
00000000`3e748910 4c894c2420      mov     qword ptr [rsp+20h],r9
00000000`3e748915 4c89442418      mov     qword ptr [rsp+18h],r8
00000000`3e74891a 4889542410      mov     qword ptr [rsp+10h],rdx
00000000`3e74891f 48894c2408      mov     qword ptr [rsp+8],rcx
00000000`3e748924 4883ec78        sub     rsp,78h
00000000`3e748928 488b05f9080100  mov     rax,qword ptr [00000000`3e759228]
00000000`3e74892f 4889442458      mov     qword ptr [rsp+58h],rax
00000000`3e748934 488b442458      mov     rax,qword ptr [rsp+58h]

In result AddHook decide to patch ntdll!NtMapViewOfSection instead of 0x3E748910. But function CreateTrampoline don't know jmp rax opcode (ffe0) and return error. Distance from NtMapViewOfSection begin to end of jump opcode is only 10 bytes, but CreateTrampoline expected 13 so return error in any case.

I suggest to add pattern for our stub to ResolveRedirectedAddress and hook target function.

Thanks,
Alex

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

OK, so here's an update:

As pointed out in Comment 3, there are some incompatibilities between the way the Dr. Web's detour patcher works compared to ours. Theirs does a 10-byte patch by virtue of ensuring that their hook lies in the first 2GB of virtual address space, thus allowing the use of movsxd to expand that address to 64-bits in rax.

Our disassembler does not track the values of registers, so ResolveRedirectedAddress cannot follow their hook; instead, our interceptor code attempts to disassemble enough of the patched NtMapViewOfSection to place its own hook.

Unfortunately the Dr. Web hook does not write out nop or int 3 instructions to the remaining bytes of the final instruction that they patched, so it throws our disassembler for a loop (since that instruction is now corrupted).

The easiest way for us to modify our interceptor code is to add this 10-byte patching capability to our own code.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19ad9610ebf2fcb740bf34e3bc08c865491d44b1

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

The above try build works with Dr. Web.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1495512: Part 1 - DLL Interceptor - Add capability to do 10-byte detour patches against ntdll; r?handyman!

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1495512: Part 2 - Add test for 10-byte DLL interception; r?handyman!

Depends on D10285

Comment 9

[copyliu]

confirmed can work with Dr.Web

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17212e7dfe29
Part 1 - DLL Interceptor - Add capability to do 10-byte detour patches against ntdll; r=handyman
https://hg.mozilla.org/integration/autoland/rev/78154ca1e2ac
Part 2 - Add test for 10-byte DLL interception; r=handyman

Comment 11

[Andreea Pavel [:apavel]]

Backed out for failing win MinGW builds

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=210961699&revision=78154ca1e2ac4b5f07276ab9969883d517940d9d

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=210961699&repo=autoland&lineNumber=15693

Backout: https://hg.mozilla.org/integration/autoland/rev/bb6f1073c5ce8beb459b69df723ee812ab94a7e8

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

I still don't understand why we're backing out tier-2 failures.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=140aa296fd4db70b5415663dd09c9f1551049e5b

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ae82cae37d9
Part 1 - DLL Interceptor - Add capability to do 10-byte detour patches against ntdll; r=handyman
https://hg.mozilla.org/integration/autoland/rev/0cd30c3d9b30
Part 2 - Add test for 10-byte DLL interception; r=handyman

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac4d133f8fb8
Backed out 2 changesets for Windows build bustages CLOSED TREE

Comment 16

[Andreea Pavel [:apavel]]

(In reply to Aaron Klotz [:aklotz] from comment #12)
> I still don't understand why we're backing out tier-2 failures.

Depending on the occurrence rate, we backout tier2 failures as well

Comment 17

[Andreea Pavel [:apavel]]

(In reply to Pulsebot from comment #15)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/ac4d133f8fb8
> Backed out 2 changesets for Windows build bustages CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=211523703&revision=0cd30c3d9b303429f3fe2b72a5b1312ab4f53a23

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=211523703&repo=autoland&lineNumber=43558

Backout: https://hg.mozilla.org/integration/autoland/rev/ac4d133f8fb8

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c62bb991f8b7955856ab40e703056a5b799a86c2

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

Naturally PGO works just fine on my local machine...

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b2b8721c92b2fdb9d6c786b02bde4434d412648

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09573684485b
Part 1 - DLL Interceptor - Add capability to do 10-byte detour patches against ntdll; r=handyman
https://hg.mozilla.org/integration/autoland/rev/a31b6b37d11c
Part 2 - Add test for 10-byte DLL interception; r=handyman

Comment 22

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/09573684485b
https://hg.mozilla.org/mozilla-central/rev/a31b6b37d11c

Comment 23

[Rares Doghi, Desktop QA]

Hi, I managed to reproduce this issue in older versions of Firefox but I can no longer reproduce this issue using the latest Nightly 65.0a1 (2018-11-28).

This issue was tested on:

Windows 10 x86 - x64
Windows 7 x86 - x64
Windows 8.1 x86 - x64

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

Beta approval is not necessary. This issue only showed up with the launcher process, which is only enabled on Nightly.

Comment 26

[Rares Doghi, Desktop QA]

Based on Comment 25 I will mark this issue accordingly.