Closed Bug 1361410 Opened 7 years ago Closed 7 years ago

Webroot SecureAnywhere (WRusr.dll) crashes in BaseThreadInitThunk

Categories

(Core :: General, defect)

x86
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: ccorcoran, Assigned: ccorcoran)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Crash Data

Attachments

(2 files)

Per bug 1322554 comment 129, there is an incompatibility between Webroot SecureAnywhere and an upcoming Firefox patch which hooks BaseThreadInitThunk.

Steps to reproduce:
- on windows 10 32-bit
- install Webroot SecureAnywhere
- install Firefox. One of the few builds with the relevant patch is: https://ftp.mozilla.org/pub/firefox/nightly/2017/04/2017-04-11-00-40-09-mozilla-aurora/firefox-54.0a2.en-US.win32.installer.exe

Firefox will crash on startup. 1322554 has plenty of further background details.

Currently proposed solution: Add WRusr.dll to blocklist.
Dees, do you have any contact at this company? It could be useful contacting them.
Flags: needinfo?(dchinniah)
(In reply to Marco Castelluccio [:marco] from comment #1)
> Dees, do you have any contact at this company? It could be useful contacting
> them.

No immediate contacts, no. Pinging :adamopenweb to also tackle this...
Flags: needinfo?(dchinniah) → needinfo?(astevenson)
I've now connected Marco via email with the Senior Director of Engineering at the SecureAnywhere product.
Flags: needinfo?(astevenson) → needinfo?(mcastelluccio)
Bit more detail on this... WRusr.dll is loaded very early at startup. Here's output from windbg by just loading firefox.exe:

> Microsoft (R) Windows Debugger Version 10.0.15063.137 X86
> Copyright (c) Microsoft Corporation. All rights reserved.
> 
> CommandLine: "C:\Program Files\Mozilla Developer Preview\firefox.exe"
> Symbol search path is: srv*
> Executable search path is: 
> ModLoad: 01340000 01405000   firefox.exe
> ModLoad: 77350000 774dd000   ntdll.dll
> ModLoad: 77220000 772b5000   C:\Windows\System32\KERNEL32.DLL
> ModLoad: 74a40000 74c05000   C:\Windows\System32\KERNELBASE.dll
> ModLoad: 758f0000 75967000   C:\Windows\System32\ADVAPI32.dll
> ModLoad: 67540000 67574000   C:\Windows\SYSTEM32\WRusr.dll
> ModLoad: 76fc0000 7707d000   C:\Windows\System32\msvcrt.dll
> ModLoad: 00db0000 00e6d000   C:\Windows\System32\msvcrt.dll
> ModLoad: 758a0000 758e1000   C:\Windows\System32\sechost.dll
> ModLoad: 75560000 75690000   C:\Windows\System32\USER32.dll
> ModLoad: 77150000 77217000   C:\Windows\System32\RPCRT4.dll
> ModLoad: 74000000 7401a000   C:\Windows\System32\win32u.dll
> ModLoad: 74770000 74888000   C:\Windows\System32\ucrtbase.dll
> ModLoad: 75100000 75121000   C:\Windows\System32\GDI32.dll
> ModLoad: 748e0000 74a3e000   C:\Windows\System32\gdi32full.dll
> ModLoad: 6af30000 6af6e000   C:\Program Files\Mozilla Developer Preview\mozglue.dll
> ModLoad: 73f80000 73ff9000   C:\Windows\System32\msvcp_win.dll
> ModLoad: 759d0000 76d18000   C:\Windows\System32\SHELL32.dll
> ModLoad: 6ad30000 6ad9d000   C:\Windows\SYSTEM32\MSVCP140.dll
> ModLoad: 745b0000 745e9000   C:\Windows\System32\cfgmgr32.dll
> ModLoad: 772c0000 7734c000   C:\Windows\System32\shcore.dll
> ModLoad: 62f40000 62f55000   C:\Windows\SYSTEM32\VCRUNTIME140.dll
> ModLoad: 75130000 75368000   C:\Windows\System32\combase.dll
> ModLoad: 74c10000 74c67000   C:\Windows\System32\bcryptPrimitives.dll
> ModLoad: 6fcf0000 6fe4d000   C:\Windows\SYSTEM32\dbghelp.dll
> ModLoad: 74020000 745a4000   C:\Windows\System32\windows.storage.dll
> ModLoad: 74c70000 74cb5000   C:\Windows\System32\shlwapi.dll
> ModLoad: 6fb30000 6fb38000   C:\Windows\SYSTEM32\VERSION.dll
> ModLoad: 73e70000 73e7e000   C:\Windows\System32\kernel.appcore.dll
> ModLoad: 73e80000 73ec5000   C:\Windows\System32\powrprof.dll
> ModLoad: 739c0000 739ca000   C:\Windows\SYSTEM32\CRYPTBASE.DLL
> ModLoad: 73ee0000 73ef0000   C:\Windows\System32\profapi.dll
> ModLoad: 007d0000 007da000   C:\Windows\SYSTEM32\CRYPTBASE.DLL
> ModLoad: 75370000 75462000   C:\Windows\System32\ole32.dll
> ModLoad: 750f0000 750f6000   C:\Windows\System32\PSAPI.DLL
> ModLoad: 76de0000 76e47000   C:\Windows\System32\WS2_32.dll
> ModLoad: 76d20000 76db6000   C:\Windows\System32\OLEAUT32.dll
> ModLoad: 6dec0000 6e05c000   C:\Windows\SYSTEM32\urlmon.dll
> ModLoad: 6f7b0000 6fa71000   C:\Windows\SYSTEM32\WININET.dll
> ModLoad: 675e0000 67636000   C:\Windows\SYSTEM32\OLEACC.dll
> ModLoad: 6fb40000 6fb4a000   C:\Windows\SYSTEM32\Secur32.dll
> ModLoad: 6fb70000 6fb76000   C:\Windows\SYSTEM32\MSIMG32.dll
> ModLoad: 6e200000 6e411000   C:\Windows\SYSTEM32\iertutil.dll
> ModLoad: 73db0000 73dd6000   C:\Windows\SYSTEM32\SSPICLI.DLL
> (1e90.1e24): Break instruction exception - code 80000003 (first chance)
> eax=00000000 ebx=009d0000 ecx=00aff918 edx=773e43a0 esi=00c71590 edi=7735739c
> eip=7741edf4 esp=00aff934 ebp=00aff960 iopl=0         nv up ei pl zr na pe nc
> cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000246
> ntdll!LdrpDoDebuggerBreak+0x2b:

Which means our basic DLL blocking mechanism won't be sufficient. Fingers crossed that we can get cooperation, otherwise we'll have to get sophisticated.
Hi Johnny, here is a sort of play-by-play of a repro on a test machine.

Env:
- Windows 10 32-bit
- Nightly FF build here: https://ftp.mozilla.org/pub/firefox/nightly/2017/04/2017-04-11-00-40-09-mozilla-aurora/firefox-54.0a2.en-US.win32.installer.exe
- WRusr.dll is 193072 bytes, version 9.0.15.40


Steps to repro:
1. Launch Firefox. I set a breakpoint just after we add a detour to BaseThreadInitThunk. The minidump is attached (m01_after_our_patch.dmp). A short jump has been written as expected:

> 0:000> u kernel32!basethreadinitthunk
> KERNEL32!BaseThreadInitThunk:
> 77239b80 ebf9            jmp     KERNEL32!TermsrvConvertSysRootToUserDir+0x3b (77239b7b)
> 77239b82 55              push    ebp

2. Set a breakpoint on write to BaseThreadInitThunk.

> ba w4 KERNEL32!BaseThreadInitThunk

3. Breakpoint is hit. Minidump attached as m02_memory_overwrite.dmp. Stack trace follows:

> 0:005> kv
>  # ChildEBP RetAddr  Args to Child              
> WARNING: Stack unwind information not available. Following frames may be wrong.
> 00 0103f5e4 67557785 00000040 0261bba0 0000000c WRusr!SynProc+0x526d
> 01 0103f5fc 675556b3 0267c487 77239b80 0261bba0 WRusr!SynProc+0x7515
> 02 0103f624 67555867 0267c487 77239b80 00000002 WRusr!SynProc+0x5443
> 03 0103f66c 6756071c 005dc5e8 005dcb98 67557710 WRusr!SynProc+0x55f7
> 04 0103f790 67562939 67557710 00000000 00000001 WRusr!SynProc+0x104ac
> 05 0103f9bc 77239ba4 00000000 77239b80 76e72253 WRusr!SynProc2+0x1ff9
> 06 0103f9d0 773bac9b 00000000 31cedd74 00000000 KERNEL32!BaseThreadInitThunk+0x24 (FPO: [Non-Fpo])

4. Breakpoint hits a few more times; I continue.

5. Crash in Kernel32!BaseThreadInitThunk. Minidump attached as m03_crash.dmp

> 0:031> u kernel32!basethreadinitthunk
> KERNEL32!BaseThreadInitThunk:
> 77239b80 8bff            mov     edi,edi
> 77239b82 55              push    ebp

As noted previously, the patch here has been overwritten.
Flags: needinfo?(johnnyshaw02)
Figured I'd move the discussion over here as this bug item is directly related to WRusr. Here is my latest response from the other thread (https://bugzilla.mozilla.org/show_bug.cgi?id=1322554):

I've found the root of the problem on our end. I'm 90% sure I have a solution in place to resolve this. I'm doing some further validation on my end. Once that is done I'll update again. I might be able to send someone a one-off build to do some validation against on your end. If that is something someone would be interested in let me know.
Flags: needinfo?(johnnyshaw02)
Flags: needinfo?(mcastelluccio)
To conclude, bug 1322554 comment 156 shows that changing the method of hooking BaseThreadInitThunk suppresses this crash. That patch has since landed for 55
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
in 55.0b there are still a couple of those crashes around with wusr.dll hooking into the process:
* bp-21be1b1a-e15d-4250-89dc-ccb550170620 (WRusr.dll, 9.0.15.65)
* bp-9e297d98-08df-4b30-a580-180b00170619 (WRusr.dll, 9.0.17.24)
* bp-f5f75d26-c62b-430f-b981-ad7100170619 (WRusr.dll, 9.0.17.24)
* bp-f2f4f576-7478-4186-9fb4-eda660170619 (WRusr.dll, 9.0.17.24)

what's the version on your end that's supposed to include the fix?
Flags: needinfo?(johnnyshaw02)
The product team hasn't scheduled the fix for a release yet. Is this still a crucial problem or is the change to the hooking approach in Mozglue sufficient?
Flags: needinfo?(johnnyshaw02)
so far the mitigation on our part doesn't seem to work fully - it's a bit early to judge the impact, since we haven't fully rolled-out 55.0b versions yet and only a small fraction of beta users are already using it. the picture will become clearer towards the end of the week & i'll follow up again then.
Flags: needinfo?(madperson)
it's currently around 100 installations per week o the firefox 55 beta channel crashing with this pattern in our crash stats data. most of them repeatedly and on startup. this may probably get worse once firefox 55 is released generally, which is scheduled for 2017-08-08.

Correlations for Firefox Beta
(93.93% in signature vs 00.24% overall) Module "WRusr.dll" = true [99.53% vs 04.61% if startup_crash = 1]
(95.14% in signature vs 07.77% overall) Module "oleacc.dll" = true
(97.98% in signature vs 18.46% overall) Module "secur32.dll" = true
(100.0% in signature vs 24.24% overall) plugin_filename = null
(80.57% in signature vs 07.41% overall) Module "dbgcore.dll" = true
(02.43% in signature vs 69.97% overall) e10s_enabled = 1
(76.11% in signature vs 19.05% overall) Module "gdi32full.dll" = true [82.63% vs 12.29% if startup_crash = 1]
(84.21% in signature vs 22.16% overall) platform_pretty_version = Windows 10
(03.64% in signature vs 63.42% overall) platform_pretty_version = Windows 7
(12.96% in signature vs 70.22% overall) startup_crash = 0

also need-infoing carl to see if something else is necessary on our part to mitigate this...
Flags: needinfo?(madperson) → needinfo?(ccorcoran)
They are using an injection technique that circumvents our DLL block list. It's a technique we should probably dedicate some time to understanding.

Otherwise we'll have to make a one-off workaround specifically for old versions of WRusr.
Flags: needinfo?(ccorcoran)
Crash Signature: [@ BaseThreadInitThunk ]
Status: RESOLVED → REOPENED
Keywords: regression
Resolution: FIXED → ---
Johnny, we're still seeing a steady trickle of crashes from WRusr.dll. We had seen previously in bug 1322554 that your suggestion of using a different hooking technique did indeed resolve the issue on my machine. What we're seeing in the wild though is that on the current beta, with the suggested workaround, we're still seeing crashes.

I hope you have some moments to take a look into this so we can resolve these crashes.

Here's a graph showing the current trend for BaseThreadInitThunk crashes in Beta: https://crash-stats.mozilla.com/signature/?product=Firefox&release_channel=beta&signature=BaseThreadInitThunk&date=%3E%3D2017-07-19T17%3A18%3A34.000Z&date=%3C2017-08-02T17%3A18%3A34.000Z#graphs

Some points from the Correlations tab there...
- 92% of these crash instances had WRusr.dll loaded in process. 
- 42% have crash address = 0x6b8a0110
- 26% have crash address = 0x6b8b00c4

I am currently unable to reproduce a crash, but from deeper investigation, it does appear to be from WRusr.dll reverting our BaseThreadInitThunk hook.

Disassembly of BaseThreadInitThunk normally looks like this:

> KERNEL32!BaseThreadInitThunk:
> 74ab9b80 8bff            mov     edi,edi
> 74ab9b82 55              push    ebp
> 74ab9b83 8bec            mov     ebp,esp
> 74ab9b85 51              push    ecx
> 74ab9b86 a10841b274      mov     eax,dword ptr [KERNEL32!__security_cookie (74b24108)]

Here's the disassembly after we hook it:

> KERNEL32!BaseThreadInitThunk:
> 74ab9b80 e9c3b0fef7      jmp     mozglue!patched_BaseThreadInitThunk (6caa4c48)
> 74ab9b85 51              push    ecx
> 74ab9b86 a10841b274      mov     eax,dword ptr [KERNEL32!__security_cookie (74b24108)]

And here's the disassembly after the patch is overwritten (in a crashing scenario):

> kernel32!BaseThreadInitThunk:
> 76c08720 8bff            mov     edi,edi
> 76c08722 55              push    ebp
> 76c08723 8bec            mov     ebp,esp
> 76c08725 51              push    ecx
> 76c08726 a110018a6b      mov     eax,dword ptr ds:[6B8A0110h]

I am not able to repro this locally; this is from investigating various crash dumps with the same failing address. As you can see, the crash address appears here in place of the address of KERNEL32!__security_cookie.

Could it be that WRusr's patch-remover is calculating that address properly somehow? Do you need any more information from me?

Feel free to contact me via email or IRC to discuss also.
Flags: needinfo?(johnnyshaw02)
(resetting 55 to affected, since this we might take a fix for this in a dot release)
Carl, I'll try to reproduce myself. is the available in nightly or only in beta? Feel free to email me with a dump if you have one.
Flags: needinfo?(johnnyshaw02)
I wasn't able to reproduce this with 57.0a1 (I think that's the build number nightly is on). Would you please send me a dump or point me to the right version?
Flags: needinfo?(ccorcoran)
For privacy reasons I cannot share the dump files unfortunately. But I can answer questions you have about them. On the crash-stats link I posted above, click on the "reports" tab and you can see more details about the individual crash reports, including Firefox version, crashing address, and call stacks.
Flags: needinfo?(ccorcoran)
Have you guys been able to reproduce it? I'm going to put in some code to step aside for you guys. I can't say exactly when it will get released. But, it should hopefully help. Do you have a way of verifying a fix?
Flags: needinfo?(ccorcoran)
I am going to do a bit more testing to try and repro. Otherwise, no we don't have a way to verify a fix at the moment.

I think it's a good idea if WRusr would have some exceptions for us. We install a number of hooks that are a part of normal browser operation and can cause problems if they're removed.
Flags: needinfo?(ccorcoran)
We should leave your hooks alone already. For whatever reason, we're detecting an anomaly after initially seeing your hooks that is prompting us to unhook them.
Hi Johnny, I have been doing more testing and still wasn't able to reproduce the issue. But I found that in all cases, our hook was being removed; that is 100% reproducible. Do you have an ETA on a fix from your side? Or need anything from me?

thanks!
Flags: needinfo?(johnnyshaw02)
Pretty high volume on release (55 included), we should probably fix it before 56 goes out.
Or take it as ride along in a potential dot release of 55 if the patch is safe.
My fix will be in the next release. I don't have a time frame for you.
Flags: needinfo?(johnnyshaw02)
Comment on attachment 8898767 [details]
Bug 1361410: Don't hook BaseThreadInitThunk when WRusr.dll is loaded, mitigating a crash;

https://reviewboard.mozilla.org/r/170154/#review175390

::: mozglue/build/WindowsDllBlocklist.cpp:873
(Diff revision 1)
>  
> -#ifdef _M_IX86 // Minimize impact; crashes in BaseThreadInitThunk are vastly more frequent on x86
> +#ifdef _M_IX86 // Minimize impact. Crashes in BaseThreadInitThunk are more frequent on x86
> +
> +  // Bug 1361410: WRusr.dll will overwrite our hook and cause a crash.
> +  // Workaround: If we detect WRusr.dll, don't hook.
> +  if (NULL == GetModuleHandleW(L"WRusr.dll")) {

Nit: I think our more common style is `if (!...)`
Attachment #8898767 - Flags: review?(dmajor) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/89585e8d8a5b
Don't hook BaseThreadInitThunk when WRusr.dll is loaded, mitigating a crash; r=dmajor
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/89585e8d8a5b
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Hi :ccorcoran,
Do you think we should uplift this workaround to Beta56 to mitigate this issue?
Flags: needinfo?(ccorcoran)
Comment on attachment 8898767 [details]
Bug 1361410: Don't hook BaseThreadInitThunk when WRusr.dll is loaded, mitigating a crash;

Approval Request Comment

[Feature/Bug causing the regression]:
3rd-party software

[User impact if declined]:
This patch mitigates top startup crash signature 'BaseThreadInitThunk' by disabling crashy code for users running Webroot SecureAnywhere. If declined, these users will continue to experience intermittent startup crashes.

[Is this code covered by automated tests?]:
No. This patch depends on intermittent behavior of 3rd-party software for which automated testing would be impractical. But the mechanism has been verified locally to work.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
It's a very small tweak, targeted to a very specific case. It will be completely transparent to most users.

[String changes made/needed]:
None
Flags: needinfo?(ccorcoran)
Attachment #8898767 - Flags: approval-mozilla-beta?
Comment on attachment 8898767 [details]
Bug 1361410: Don't hook BaseThreadInitThunk when WRusr.dll is loaded, mitigating a crash;

Workaround for a top crash, let's uplift this for beta 6. 
We should check back on Friday to see if the crash rate improves. 
Marcia, would you mind checking up on this one once we have beta 6 out?
Flags: needinfo?(mozillamarcia.knous)
Attachment #8898767 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Carl Corcoran [:ccorcoran] from comment #30)
> [Is this code covered by automated tests?]:
> No. This patch depends on intermittent behavior of 3rd-party software for
> which automated testing would be impractical. But the mechanism has been
> verified locally to work.
> 
> [Has the fix been verified in Nightly?]:
> Yes
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> No

Setting qe-verify-  based on Car's assessment on manual testing needs.
Flags: qe-verify-
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #31)
> Comment on attachment 8898767 [details]
> Bug 1361410: Don't hook BaseThreadInitThunk when WRusr.dll is loaded,
> mitigating a crash;
> 
> Workaround for a top crash, let's uplift this for beta 6. 
> We should check back on Friday to see if the crash rate improves. 
> Marcia, would you mind checking up on this one once we have beta 6 out?

So far, I don't see any crashes in Beta 6: https://crash-stats.mozilla.com/signature/?signature=BaseThreadInitThunk
Flags: needinfo?(mozillamarcia.knous)
That's amazingly good news. 
David, do you think we can be certain this worked (rather than the signature migrating to something else?)
Flags: needinfo?(dmajor)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #35)
> That's amazingly good news. 
> David, do you think we can be certain this worked (rather than the signature
> migrating to something else?)

I've paged out a lot of the details about these crashes, but from glancing over the previous comments here, I don't think these crashes would show up as anything else.
Flags: needinfo?(dmajor)
See Also: → 1424217
You need to log in before you can comment on or make changes to this bug.