1730048 - Crash in [@ nsXULPopupManager::ExecuteMenu]

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/764a6bd4-1e30-4f54-b5d1-551f40210909

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll nsXULPopupManager::ExecuteMenu layout/xul/nsXULPopupManager.cpp:1499
1 xul.dll nsMenuFrame::PassMenuCommandEventToPopupManager layout/xul/nsMenuFrame.cpp:970
2 xul.dll nsMenuFrame::StartBlinking layout/xul/nsMenuFrame.cpp:912
3 xul.dll nsMenuFrame::Execute layout/xul/nsMenuFrame.cpp:893
4 xul.dll nsMenuFrame::HandleEvent layout/xul/nsMenuFrame.cpp:414
5 xul.dll static mozilla::EventTargetChainItem::HandleEventTargetChain dom/events/EventDispatcher.cpp:615
6 xul.dll static mozilla::EventDispatcher::Dispatch dom/events/EventDispatcher.cpp:1082
7 xul.dll mozilla::PresShell::EventHandler::DispatchEventToDOM layout/base/PresShell.cpp:8646
8 xul.dll mozilla::PresShell::EventHandler::DispatchEvent layout/base/PresShell.cpp:8218
9 xul.dll mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo layout/base/PresShell.cpp:8150

I don't know if this is actionable at all, or how common this is, but I came across a UAF crash in Nightly.

Comment 1

[Andrew McCreight [:mccr8]]

I'll mark this sec-high, but it may end up getting marked stalled, too.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

The caller is an nsIFrame and that may go away because of HideOpenMenusBeforeExecutingMenu. Better to keep the params alive.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1730048, reorder the code in nsMenuFrame::PassMenuCommandEventToPopupManager a bit, r=mstange

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

the patch is based on code inspection

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9244437 [details]
Bug 1730048, reorder the code in nsMenuFrame::PassMenuCommandEventToPopupManager a bit, r=mstange

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This code runs in the parent process only, so it isn't obvious how to trigger the crash from a child process. Some other security issue is needed for that.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: The relevant code is old. Code in esr78 looks exactly the same as in Nightly.
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely. Basically adding two kungfuDeathGrips based on code inspection.

Comment 6

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9244437 [details]
Bug 1730048, reorder the code in nsMenuFrame::PassMenuCommandEventToPopupManager a bit, r=mstange

Approved to land

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

reorder the code in nsMenuFrame::PassMenuCommandEventToPopupManager a bit, r=mstange
https://hg.mozilla.org/integration/autoland/rev/426254fe00c3eceef012bd95090382d148c95747
https://hg.mozilla.org/mozilla-central/rev/426254fe00c3

Comment 8

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

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please request Beta/ESR91 approval on this when you get a chance.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9244437 [details]
Bug 1730048, reorder the code in nsMenuFrame::PassMenuCommandEventToPopupManager a bit, r=mstange

Beta/Release Uplift Approval Request

• User impact if declined: crashes
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: (The fix is based on the stack trace)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Basically two kungfuDeathGrips
• String changes made/needed: NA

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9244437 [details]
Bug 1730048, reorder the code in nsMenuFrame::PassMenuCommandEventToPopupManager a bit, r=mstange

Approved for 94.0b9 and 91.3esr.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fbe2fe84012a

Comment 13

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/89303e627ad8