1827144 - Crash in [@ mozilla::MediaTransportHandlerIPC::MediaTransportHandlerIPC]

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Andrew Osmond [:aosmond] (he/him)]

Crash report: https://crash-stats.mozilla.org/report/index/076979c1-e1c7-4fa5-8982-92ded0230407

Reason: SIGSEGV / SI_KERNEL

Top 10 frames of crashing thread:

0  libxul.so  std::__atomic_base<unsigned long>::fetch_sub  /builds/worker/fetches/sysroot-x86_64-linux-gnu/usr/include/c++/7/bits/atomic_base.h:524
0  libxul.so  mozilla::ThreadSafeAutoRefCnt::operator--  xpcom/base/nsISupportsImpl.h:361
0  libxul.so  mozilla::MozPromiseRefcountable::Release  xpcom/threads/MozPromise.h:151
0  libxul.so  mozilla::RefPtrTraits<mozilla::MozPromise<bool, nsTString<char>, false> >::Release  mfbt/RefPtr.h:50
0  libxul.so  RefPtr<mozilla::MozPromise<bool, nsTString<char>, false> >::ConstRemovingRefPtrTraits<mozilla::MozPromise<bool, nsTString<char>, false> >::Release  mfbt/RefPtr.h:381
0  libxul.so  RefPtr<mozilla::MozPromise<bool, nsTString<char>, false> >::assign_assuming_AddRef  mfbt/RefPtr.h:69
0  libxul.so  RefPtr<mozilla::MozPromise<bool, nsTString<char>, false> >::assign_with_AddRef  mfbt/RefPtr.h:62
0  libxul.so  RefPtr<mozilla::MozPromise<bool, nsTString<char>, false> >::operator=  mfbt/RefPtr.h:175
0  libxul.so  mozilla::MediaTransportHandlerIPC::MediaTransportHandlerIPC  dom/media/webrtc/jsapi/MediaTransportHandlerIPC.cpp:25
1  libxul.so  mozilla::MediaTransportHandler::Create  dom/media/webrtc/jsapi/MediaTransportHandler.cpp:202

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Similar root cause as bug 1827142. We should not take RefPtr to ourselves in constructors:
https://searchfox.org/mozilla-central/rev/409f8ca0a306ba44953ef294ba99b5155c10a3d3/dom/media/webrtc/jsapi/MediaTransportHandlerIPC.cpp#27

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1827144.

Comment 3

[Andrew McCreight [:mccr8]]

I'll mark this as sec-high because it is a UAF crash.

Comment 4

[Andrew McCreight [:mccr8]]

It sounds like the issue here is that something gets allocated in the constructor, then another thread frees it before we take a strong reference on the current thread. To make this exploitable, you'd presumably need some third thread to allocate in the same location before the constructor returns, which sounds tricky, so I'll mark it sec-moderate. Feel free to change to sec-high if you want.

Comment 6

[Andrew McCreight [:mccr8]]

Is this ready to land? It would be nice to be able to uplift this to beta. Thanks.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9327726 [details]
Bug 1827144.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It would be obvious that we release the object during the constructor and we fixed that. Achieving the timing to exploit this may be tricky.
• 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
• 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?: It should uplift easily.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely, very simple restructuring.
• Is Android affected?: No

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9327726 [details]
Bug 1827144.

This doesn't need sec-approval since it's only sec-moderate. Given the low risk, go ahead and land it and request Beta & ESR approval. If grafts cleanly to both.

Comment 9

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

r=webrtc-reviewers
https://hg.mozilla.org/integration/autoland/rev/2a075ce8475de6276df09cca14869b2227d9ae89
https://hg.mozilla.org/mozilla-central/rev/2a075ce8475d

Comment 10

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

The patch landed in nightly and beta is affected.
:aosmond, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox113 to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9327726 [details]
Bug 1827144.

Beta/Release Uplift Approval Request

• User impact if declined: Low volume crash, sec issue
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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): Makes a minor adjustment to the constructor to avoid a race on the ref counting.
• String changes made/needed:
• Is Android affected?: Unknown

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9327726 [details]
Bug 1827144.

Approved for 113.0b9 and 102.11esr.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/701fb01baa92

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/4803a8de9f8e