1577430 - Crash in [@ mozilla::dom::MessagePortParent::Close]

Status: RESOLVED DUPLICATE of bug 1597481

(Core :: DOM: Workers, defect, P2)

Description

[[:philipp]]

This bug is for crash report bp-842666d6-444e-4a60-b6c2-621120190829.

Top 10 frames of crashing thread:

0 xul.dll void mozilla::dom::MessagePortParent::Close dom/messagechannel/MessagePortParent.cpp:143
1 xul.dll void mozilla::dom::MessagePortService::CloseAll dom/messagechannel/MessagePortService.cpp:267
2 xul.dll void mozilla::dom::MessagePortService::CloseAll dom/messagechannel/MessagePortService.cpp:291
3 xul.dll bool mozilla::dom::MessagePortService::ForceClose dom/messagechannel/MessagePortService.cpp:390
4 xul.dll mozilla::dom::MessagePortParent::ForceClose dom/messagechannel/MessagePortParent.cpp:158
5 xul.dll void mozilla::UniquePtr<mozilla::dom::RemoteWorkerController::Op, mozilla::DefaultDelete<mozilla::dom::RemoteWorkerController::Op> >::reset mfbt/UniquePtr.h:325
6 xul.dll void nsTArray_Impl<mozilla::UniquePtr<mozilla::dom::RemoteWorkerController::Op, mozilla::DefaultDelete<mozilla::dom::RemoteWorkerController::Op> >, nsTArrayInfallibleAllocator>::Clear xpcom/ds/nsTArray.h:1766
7 xul.dll void mozilla::dom::RemoteWorkerController::Shutdown dom/workers/remoteworkers/RemoteWorkerController.cpp:159
8 xul.dll void mozilla::dom::RemoteWorkerParent::ActorDestroy dom/workers/remoteworkers/RemoteWorkerParent.cpp:87
9 xul.dll mozilla::ipc::IProtocol::DestroySubtree ipc/glue/ProtocolUtils.cpp:659

this crash signature is starting to show up in low volume since firefox 69 and looking security sensitive.

Comment 1

[Daniel Veditz [:dveditz]]

asuth: can you help figure out where this might be going wrong or point at which MessagePort user is misusing it?

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

This looks suspiciously familiar, but my initial efforts to find the bug I'm thinking of are failing.

The general situation here is:

• SharedWorkers are e10s and explicitly reuse our MessagePort implementation (compared with Dedicated Workers which have a custom postMessage implementation). Our MessagePort implementation is built on top of PBackground. The SharedWorker's MessagePort will travel over PBackground from the SharedWorker thread.
• The SharedWorker in a content process creates a PSharedWorker actor up to PBackground from the main thread.
• The SharedWorkerManager in PBackground creates a PRemoteWorker down from PBackground to the "Remote Worker" thread in whatever content process.
• In the event of abrupt content process termination without clean shutdown, the channel gets torn down and this results in sequential-ish termination of the 3 different PBackground actors (because they come from different content process threads), which may create unexpected orderings.
  • Although our IPC implementation tries to reduce scenarios like this by performing sub-tree destruction before sub-tree deallocation, that's done a per-top-level-actor basis, and we're dealing with 3 different top-level actors, so those mitigations don't help here.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Note that because the crashing on PBackground on freed memory that gets poisoned, it's going to be very hard to exploit the crash. We'd just expect the parent process to crash.

Comment 4

[Jens Stutte [:jstutte]]

I digged a bit into the code starting from the stack trace. As far as I can see, we have called two times MessagePortService::CloseAll on the same instance of MessagePortService. As I can see only one call on the call stack of the crashed stack, I must assume, that there is another thread trying to close the same instance and probably freed already the memory behind the MessagePortParent instance still in use here.

I deduce this from here: https://hg.mozilla.org/releases/mozilla-beta/annotate/bb9376d19d797edfc469a59d7398bdb0e891c81a/dom/messagechannel/MessagePortService.cpp#l155 on the call stack:
CloseAll can be called here only if mNextStepCloseAll is set. And mNextStepCloseAll is set only here: https://hg.mozilla.org/releases/mozilla-beta/annotate/bb9376d19d797edfc469a59d7398bdb0e891c81a/dom/messagechannel/MessagePortService.cpp#l285 to true, that is in the same CloseAll routine.

If this double-closure is an expected thing to happen, we should ensure to check at the latest here: https://hg.mozilla.org/releases/mozilla-beta/annotate/bb9376d19d797edfc469a59d7398bdb0e891c81a/dom/messagechannel/MessagePortService.cpp#l267 if the mParent still exists. But even here https://hg.mozilla.org/releases/mozilla-beta/annotate/bb9376d19d797edfc469a59d7398bdb0e891c81a/ipc/glue/BackgroundParentImpl.cpp#l986 it might already have been freed (only read access) and should not be used without caution?

Anyway, if Andrew is right for the exploitability, is this still a sec-high / P1?

Comment 5

[Jens Stutte [:jstutte]]

Checked back with asuth, it is probably exploitable only as DoS attack. So stepping down criticality and priority.

Comment 6

[Jens Stutte [:jstutte]]

:perry, I see changes in the pointer handling in the patch for bug 1597481 that might help here, too?

Comment 7

[Perry Jiang [:perry] [no longer employee, use ni?]]

(In reply to Jens Stutte [:jstutte] from comment #6)

:perry, I see changes in the pointer handling in the patch for bug 1597481 that might help here, too?

I think this bug and that one are the same thing. Both bugs' backtraces reference the same line of MessagePortService::CloseAll. Bug 1597481's patch also only made it into 72+, and all the current crashers for this one are 70/71, so I want to say this bug is resolved/duplicate.