1936749 - Crash in [@ CxxThrowException | MpDetoursHookAll]

Status: REOPENED

(External Software Affecting Firefox :: Other, defect)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/37858fa9-0525-4fae-b5ac-8ad400241210

Reason:

Unhandled C++ Exception

Top 8 frames:

0  KERNELBASE.dll  RaiseException
1  MpDetours.dll  CxxThrowException
2  MpDetours.dll  MpDetoursHookAll(bool)
3  MpDetours.dll  MpDetoursHookThreadProc(void*)
4  kernel32.dll  BaseThreadInitThunk
5  mozglue.dll  mozilla::interceptor::FuncHook<mozilla::interceptor::WindowsDllInterceptor<mo...  toolkit/xre/dllservices/mozglue/nsWindowsDllInterceptor.h:150
5  mozglue.dll  patched_BaseThreadInitThunk(int, void*, void*)  toolkit/xre/dllservices/mozglue/WindowsDllBlocklist.cpp:562
6  ntdll.dll  RtlUserThreadStart

This innocuous-looking crash on Socorro is the highest volume non-main process crash in the crash ping data, with 1951 crashes affecting 1287 users over a week on versions 133-135 of Firefox.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:gstoll, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Last error was ERROR_DYNAMIC_CODE_BLOCKED

We only have a handful of socorro crashes here, but there are currently ~3700 crashes listed with ~2100 clients in the last 7 days -- the #2 crasher after OOM small! This also implies people hit it more than once (and may well switch browsers afterwards). What can we do to look into this?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

MpDetours appears to be a Microsoft Windows Defender dll

Comment 4

[Gian-Carlo Pascutto [:gcp]]

The high crash volume was already identified in the first comment. I'm not sure why it was marked S3, Greg?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Pouring through the crash ping data, this is socket process only, >50% of the crash volume for that process.
1 56.7% CxxThrowException | MpDetoursHookAll

the #2 crasher after OOM small! This also implies people hit it more than once (and may well switch browsers afterwards)

See above - not sure how disruptive this actually is! Yay for separating things out into processes.

Comment 6

[Greg Stoll :gstoll]

Sorry, I must have missed that comment. I'll take a look.

Comment 7

[Yannis Juglaret [:yannis] (Away until April 13)]

Analyzing MpDetoursHook.dll's MpDetoursHookAll shows two conditions under which we can end up in this CxxThrowException code path:
(1) reaching MpDetoursHookAll while another thread is already running it;
(2) if a call to VirtualProtect(..., PAGE_EXECUTE_READWRITE, ...) fails.

(1) does not match with what we see in the crash reports. For example we don't have multiple threads running MpDetoursHook.dll in the crash dumps. However (2) makes a lot of sense.

In security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, we can see that MITIGATION_DYNAMIC_CODE_DISABLE is used in socket, RDD and utility processes. This matches precisely with the process types we find on crash-stats. The mitigation will make the call in (2) fail, by design. Also matches with comment 2.

I'll write to our contacts our Microsoft to ask if they could check if the mitigation is active and fail silently rather than raise an exception in this case.

Comment 8

[Greg Stoll :gstoll]

I'm not sure this is related to Windows Defender - all the crash reports I see now on crash-stats have QIPCAP64.dll loaded, whose publisher is Forcepoint LLC.

Comment 9

[Yannis Juglaret [:yannis] (Away until April 13)]

Looking at the 32 crashes that reached crash-stats on this signature in the last six months gives these numbers of occurences for third-party DLLs compared to MpDetours.dll:

amapphook.dll: 1 (Ivanti, Inc.)
amldrappinit.dll: 1 (Ivanti, Inc.)
igd10iumd64.dll: 1 (IntelVPGSigning2021)
igdgmm64.dll: 1 (IntelVPGSigning2021)
pghook.dll: 2 (BeyondTrust Corporation)
qipcap64.dll: 4 (Forcepoint LLC)
cyinjct.dll: 5 (Palo Alto Networks (Netherlands) B.V.)
cyvera.dll: 5 (Palo Alto Networks (Netherlands) B.V.)
cyvrtrap.dll: 5 (Palo Alto Networks (Netherlands) B.V.)
ntnativeapi.dll: 5 (Palo Alto Networks (Netherlands) B.V.)
mpdetours.dll: 32

Comment 10

[Yannis Juglaret [:yannis] (Away until April 13)]

Attachment: Bug 1936749 - Disable ACG if we detect MpDetours.dll injection. r=bobowen

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/391773dafbd6
Disable ACG if we detect MpDetours.dll injection. r=bobowen

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/391773dafbd6

Comment 13

[Yannis Juglaret [:yannis] (Away until April 13)]

Increasing severity based on comment 0 and comment 2.

Comment 14

[Yannis Juglaret [:yannis] (Away until April 13)]

Attachment: Bug 1936749 - Disable ACG if we detect MpDetours.dll injection.

Original Revision: https://phabricator.services.mozilla.com/D236890

Comment 15

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Repeated crashes of socket/rdd/utility processes for some Windows Defender users
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Unclear
• Risk associated with taking this patch: Low
• Explanation of risk level: The patch disables a security mitigation if we detect injection of the faulty DLL. This should only impact the affected users and can only improve the situation.
• String changes made/needed: None
• Is Android affected?: no

Comment 16

[Ryan VanderMeulen [:RyanVM]]

I'm thinking this probably doesn't need to be rushed into next week's Fx135 planned dot release, but LMK if you feel strongly otherwise. Also, I'm guessing we'll probably want to take this on ESR128 also?

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/013d20a459d5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

Comment 19

[Yannis Juglaret [:yannis] (Away until April 13)]

Beta uplift should be enough. I'll push a request for ESR.

Comment 20

[Yannis Juglaret [:yannis] (Away until April 13)]

Attachment: Bug 1936749 - Disable ACG if we detect MpDetours.dll injection.

Original Revision: https://phabricator.services.mozilla.com/D236890

Comment 21

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: Repeated crashes of socket/rdd/utility processes for some Windows Defender users
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Unclear
• Risk associated with taking this patch: Low
• Explanation of risk level: The patch disables a security mitigation if we detect injection of the faulty DLL. This should only impact the affected users and can only improve the situation.
• String changes made/needed: None
• Is Android affected?: no

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/e20039006c7a

Comment 23

[Yannis Juglaret [:yannis] (Away until April 13)]

Early crash ping telemetry shows this crash is now only <1% of the socket process crash volume, suggesting that our patch has almost completely mitigated the issue. Reopening with lower severity still for tracking purposes, since it is present despite low volume and we are hoping that Microsoft will acknowledge and fix this issue.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit BugBot documentation.