Closed Bug 1616079 Opened 3 years ago Closed 3 years ago

Crash in [@ RtlAcquireSRWLockExclusive | mozilla::detail::MutexImpl::lock | mozilla::dom::WorkerRunnable::DispatchInternal]

Categories

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

73 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox-esr68 --- unaffected
firefox73 --- disabled
firefox74 + fixed
firefox75 + fixed

People

(Reporter: philipp, Assigned: edenchuang)

Details

(4 keywords, Whiteboard: [post-critsmash-triage])

Crash Data

Attachments

(1 file)

This bug is for crash report bp-6b42f74b-7ac0-4dee-995d-c2e540200217.

Top 10 frames of crashing thread:

0 ntdll.dll RtlAcquireSRWLockExclusive 
1 mozglue.dll mozilla::detail::MutexImpl::lock mozglue/misc/Mutex_windows.cpp:22
2 xul.dll mozilla::dom::WorkerRunnable::DispatchInternal dom/workers/WorkerRunnable.cpp
3 xul.dll mozilla::dom::WorkerRunnable::Dispatch dom/workers/WorkerRunnable.cpp:89
4 xul.dll mozilla::detail::RunnableFunction<`lambda at z:/task_1581648291/build/src/dom/workers/remoteworkers/RemoteWorkerChild.cpp:475:19'>::Run xpcom/threads/nsThreadUtils.h:562
5 xul.dll mozilla::MozPromise<nsTArray<bool>, nsresult, 0>::ThenValue<`lambda at z:/task_1581648291/build/src/extensions/permissions/nsPermissionManager.cpp:3300:29', `lambda at z:/task_1581648291/build/src/extensions/permissions/nsPermissionManager.cpp:3301:11'>::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:727
6 xul.dll mozilla::MozPromise<RefPtr<mozilla::AllocPolicy::Token>, bool, 1>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:403
7 xul.dll mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run xpcom/threads/TaskDispatcher.h:197
8 xul.dll mozilla::EventTargetWrapper::Runner::Run xpcom/threads/AbstractThread.cpp:113
9 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:282

these cross-platform content crash signatures are showing up in increased volume since firefox 73. in firefox 73.0b9 the sw-e10s work got disabled and those crashes stopped, so i assume they are related to this new feature.
the crashing addresses of a number of reports indicate that this is a UAF issue.

It seems, that we fail accessing a WorkerPrivate's mutex, thus a freed WorkerPrivate.

However, as the DispatchInternal function is inlined, we cannot be sure, if it is the access to the WorkerRunnable's mWorkerPrivate or the access to its parent's WorkerPrivate.

The WorkerRunnable contains a raw pointer to WorkerPrivate though. Rather than to try to patch, the better approach might be to start work on: bug 1539508, bug 1350337 and get rid of all those raw pointers on WorkerPrivate.

Eden, can you please have a look?

Flags: needinfo?(echuang)

Yes, I can take a look.
Maybe we can try to replace the raw pointer of WorkerRunnable with RefPtr first.
I guess this might a side-effect of bug 1575185. Will look it in detail.

Flags: needinfo?(echuang)
Assignee: nobody → echuang

This could be a dupe of bug 1604847.

Group: core-security → dom-core-security

According to the stack, it seems that the WorkerPrivate was alive when creating the WorkerRunnable.

https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/workers/remoteworkers/RemoteWorkerChild.cpp#463

But got deleted before executing the WorkerRunnable. And when executing the runnable, we got UAF on WorkerPrivate at

https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/workers/remoteworkers/RemoteWorkerChild.cpp#477

Priority: -- → P1

This is

Keywords: sec-high

(In reply to Eden Chuang[:edenchuang] from comment #4)

According to the stack, it seems that the WorkerPrivate was alive when creating the WorkerRunnable.

https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/workers/remoteworkers/RemoteWorkerChild.cpp#463

But got deleted before executing the WorkerRunnable. And when executing the runnable, we got UAF on WorkerPrivate at

https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/workers/remoteworkers/RemoteWorkerChild.cpp#477

That makes sense. Maybe InitializeWorkerRunnable can have a RefPtr<WorkerPrivate>? Then the dispatch will still fail but there will not be a UAF.

Workaround in comment 1 does not work. Using RefPtr on WorkerRunnable::mWorkerPrivate changes WorkerPrivate's life cycle. And it also makes WorkerPrivate be freed on the unexpected thread. So we meet lots of crashes in ~WorkerPrivate().

I am not sure if using RefPtr holding WorkerPrivate in InitializeWorkerRunnable is suitable or not, since the WorkerPrivate's life cycle still changed.

What my guess is RemoteWorkerChild::mState probably is switched to Terminated/PendingTerminate for some reasons, i.e. shutdown, before dispatching the InitializeWorkerRunnable. The state switching makes the WorkerPrivate strong reference on Pending state be released.

https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/dom/workers/remoteworkers/RemoteWorkerChild.cpp#446

But the following logic doesn't expect the state switching case. It seems to expect the RemoteWorkerChild::mState should be Pending

https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/dom/workers/remoteworkers/RemoteWorkerChild.cpp#452-459

Here is a workaround to check if RemoteWorkerChild::mState is Pending before dispatching InitializeWorkerRunnable. The difference from holding a RefPtr<WorkerPrivate> in InitializeWorkerRunnable is this workaround does not change any object's life cycle.

Comment on attachment 9128764 [details]
Checking if RemoteWorkerChild::mState is Pending before dispatching IninializeWorkerRunnable

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: I think it is not easy to construct the case. It needs a special thread execution sequence to achieve, but the thread execution switching is controlled by OS.
  • 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?: 73
  • 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?:
  • How likely is this patch to cause regressions; how much testing does it need?: Essentially the patch should not cause regressions, it only adds a condition check before dispatching runnable.
Attachment #9128764 - Flags: sec-approval?
Attachment #9128764 - Flags: sec-approval? → sec-approval+

Comment on attachment 9128764 [details]
Checking if RemoteWorkerChild::mState is Pending before dispatching IninializeWorkerRunnable

Beta/Release Uplift Approval Request

  • User impact if declined: Intermittent UAF
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Adds a safety check, overall restricts the possible things that can happen (no new situations would need to be tested for).
  • String changes made/needed:
Attachment #9128764 - Flags: approval-mozilla-beta?

Comment on attachment 9128764 [details]
Checking if RemoteWorkerChild::mState is Pending before dispatching IninializeWorkerRunnable

Approved for our last beta.

Attachment #9128764 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.