1832306 - Crash [@ nsIContent::GetEventTargetParent] through [@ mozilla::dom::BrowserParent::RecvAccessKeyNotHandled]

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Christian Holler (:decoder)]

In experimental IPC fuzzing, we found the following crash on mozilla-central revision f14ed3bab724+ (fuzzing-asan-nyx-opt build):

==2451113==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000098 (pc 0x7f433b175402 bp 0x7ffe3c6bd910 sp 0x7ffe3c6bd8f0 T0)
    #0 0x7f433b175402 in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_RelocateUsingMemutils>::Length() const objdir/dist/include/nsTArray.h:410:37
    #1 0x7f43406812fc in nsIContent::GetEventTargetParent(mozilla::EventChainPreVisitor&) dom/base/FragmentOrElement.cpp:998:40
    #2 0x7f4348f242be in nsXULElement::GetEventTargetParent(mozilla::EventChainPreVisitor&) dom/xul/nsXULElement.cpp:997:20
    #3 0x7f4344fda9fb in mozilla::EventTargetChainItem::GetEventTargetParent(mozilla::EventChainPreVisitor&) dom/events/EventDispatcher.cpp:415:12
    #4 0x7f4344fdf1a4 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) dom/events/EventDispatcher.cpp:910:15
    #5 0x7f43482e5a56 in mozilla::dom::BrowserParent::RecvAccessKeyNotHandled(mozilla::WidgetKeyboardEvent const&) dom/ipc/BrowserParent.cpp:2762:5
    #6 0x7f434855c6e2 in mozilla::dom::PBrowserParent::OnMessageReceived(IPC::Message const&) objdir/ipc/ipdl/PBrowserParent.cpp:5511:81
    #7 0x7f4348656274 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) objdir/ipc/ipdl/PContentParent.cpp:6655:32
    #8 0x7f433d9c33c1 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) ipc/glue/MessageChannel.cpp:1806:25
    [...]

I think what is going on here is that BrowserParent::RecvAccessKeyNotHandled receives a WidgetKeyboardEvent& aEvent but doesn't check certain properties about this event.

The first check is a MOZ_ASSERT checking aEvent.mMessage == eKeyPress and it is unclear if this should be a real check instead (hence also marking this as s-s).

Finally, we end up in nsIContent::GetEventTargetParent here: https://searchfox.org/mozilla-central/rev/445a5ab684b73eb56d807d0f3b2fabcc85a7c3dd/dom/base/FragmentOrElement.cpp#990

My guess here is that the aVisitor.mEvent->AsTouchEvent(); fails because this is actually not a touch event and we end up with a null deref. It would be great if we could investigate if the event here is really properly checked (as it is completely untrusted) and not just fix the null deref.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Masayuki, can you take a look at what's up here?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sure, of course, but maybe next week.

I think that BrowserParent should store dispatched eKeyPress events and ignore (or return IPC_FAIL) if RecvAccessKeyNotHandled receives unexpected event.

Comment 5

[Andrew McCreight [:mccr8]]

I'm going to mark this as sec-high, because it looks like it will just dispatch whatever event. It does set MarkAsHandledInRemoteProcess(), which kind of feels like it makes it less concerning, but I only see that flag being checked in a substantial way in one place, in EventStateManager::WalkESMTreeToHandleAccessKey(), so it feels like that won't always protect us against a hostile content process? We can lower the rating if I'm wrong, as I don't really know anything about these events.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1832306 - Make `BrowserParent` stop handling access keys with delayed events r=smaug!,NeilDeakin!

EventStateManager::WalkESMTreeToHandleAccessKey() requests all BrowserParent
instances to handle access key in content processes:
https://searchfox.org/mozilla-central/rev/a4fd6daad3a4123d995478467c1274653b283801/dom/events/EventStateManager.cpp#1379,1398,1410-1411

Then, content processes will reply if no element matches with the access key:
https://searchfox.org/mozilla-central/rev/8e1b221afcdae76284b1439c547b032d1f84d236/dom/ipc/BrowserChild.cpp#2304-2305,2310

Finally, the parent process handles the keypress event:
https://searchfox.org/mozilla-central/rev/8e1b221afcdae76284b1439c547b032d1f84d236/dom/ipc/BrowserParent.cpp#2710-2712,2720,2725

However, this is odd because if multiple remote processes calls
BrowserParent::RecvAccessKeyNotHandled(), the parent process will handle
same keypress event multiple times.

The approach of this patch is, BrowserParent should store main data of
sending eKeyPress event, and RecvAccessKeyNotHandled() handles only if
the coming event matches with the stored one and clear the data to avoid
handling multiple times.

Even with this approach, we cannot avoid one eKeyPress event handled in
multiple content processes, or in both the main process and a content process.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hi, could you review it which related to the access key handling?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9336025 [details]
Bug 1832306 - Make BrowserParent stop handling access keys with delayed events r=smaug!,NeilDeakin!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I'm not sure how it's easy/difficult to control a content process.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All branches after e10s support
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This can be grafted to 115 (and probably 114 too), I guess that uplifting to 102 requires to rebase it by hands without not so difficult things. Feel free to request it if we should uplift this to 102.
• How likely is this patch to cause regressions; how much testing does it need?: Access keys may stop working if something is wrong (the path is meaningful only when chrome access key modifier and content access key modifiers are same). Simple cases can be tested simply, but testing with various race conditions is impossible.
• Is Android affected?: No

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9336025 [details]
Bug 1832306 - Make BrowserParent stop handling access keys with delayed events r=smaug!,NeilDeakin!

Approved to request uplift and land.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Tom Ritter [:tjr] from comment #9)

Comment on attachment 9336025 [details]
Bug 1832306 - Make BrowserParent stop handling access keys with delayed events r=smaug!,NeilDeakin!

Approved to request uplift and land.

Thank you! Do you think that this should be fixed in 102 ESR branch?

Comment 11

[Tom Ritter [:tjr]]

If the backport is relatively simple, then yes. If it's very complex I think we could hold given the shorter lifespan of ESR 102 and the complexity (and user visibility) it seems a code-execution-based exploit for this would require.

Comment 12

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

Make BrowserParent stop handling access keys with delayed events r=smaug,NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/15a57a8cf9c1a82df222dfec76e5f167ea53107b
https://hg.mozilla.org/mozilla-central/rev/15a57a8cf9c1

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9336025 [details]
Bug 1832306 - Make BrowserParent stop handling access keys with delayed events r=smaug!,NeilDeakin!

Beta/Release Uplift Approval Request

• User impact if declined: Poisoned content process may execute specific menu items via access key.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: For checking access key works:

1. Set ui.key.chromeAccess and ui.key.contentAccess to 4 in about:config (on Windows and/or Linux)
2. Restart Firefox
3. Load a page which do not use access key (e.g., data:text/html,blankpage)
4. Press Alt-F

Then, the file menu should be activated.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch records the last access key press event at being sent from BrowserParent and reply IPC message is ignored if the coming event is different from the stored last access key press. Therefore, the access key may not work if this had regressions, but it can be checked above steps to check the behavior. (Additionally, the path actually runs only when the above preferences are set to same value).
• String changes made/needed:
• Is Android affected?: No

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for ESR102

ESR Approval Request Comment
[Feature/Bug causing the regression]: Access key implementation for e10s
[User impact if declined]: Poisoned content process may execute specific menu items via access key
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: For checking access key works:

1. Set ui.key.chromeAccess and ui.key.contentAccess to 4 in about:config (on Windows and/or Linux)
2. Restart Firefox
3. Load a page which do not use access key (e.g., data:text/html,blankpage)
4. Press Alt-F

[Why is the change risky/not risky?]: This patch records the last access key press event at being sent from BrowserParent and reply IPC message is ignored if the coming event is different from the stored last access key press. Therefore, the access key may not work if this had regressions, but it can be checked above steps to check the behavior. (Additionally, the path actually runs only when the above preferences are set to same value).
[String changes made/needed]: No

If you use kdiff3 for the merging tool, you can graft the changeset 15a57a8cf9c1. This is the rebased patch.

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9336025 [details]
Bug 1832306 - Make BrowserParent stop handling access keys with delayed events r=smaug!,NeilDeakin!

Approved for 115.0b7.

Comment 16

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c261dccd96a

Comment 17

[Donal Meehan [:dmeehan]]

Comment on attachment 9339456 [details] [diff] [review]
Patch for ESR102

Approved for 102.13esr.

Comment 18

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/7b5e63e739fa

Comment 19

[Ciprian Georgiu, Desktop QA]

I did not reproduce the crash mentioned in comment 0, on an affected asan Nightly build (2023-05-10) with Ubuntu 20.04 x64 or Win 10 x64, but per my conversation with Masayuki, I understood that will be enough to make sure that no regression were introduced by this patch.

Testing with the latest asan builds, Esr 102.13, Beta 115.0b7 and Nightly 116.0a1, with STR from comment 14, I can confirm that access key works as expected in File Menu on Win 11 x64.