1552571 - MediaManager::GetUserMediaWindowListener API is a bit of a footgun

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I'm opening a separate bug for what I think is a vulnerable design pattern in MediaManager::GetUserMediaWindowListener. So far it's responsible for at least bug 1547381 and bug 1550955.

The pattern carries comments like "callers need to make sure to hold a strong reference". Members of mActiveListeners remove themselves from this owning list (through a weak back-pointer) inside methods called while iterating over this list. This is a footgun. See bug 1547381 comment 4 for an example.

I have some ideas on avoiding these surprise properties.

Marking as a sec bug until the bugs mentioned have been merged, or in case there are other bugs from this yet to be discovered.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1552571: Optimize GetUserMediaWindowListener's source listener iteration and ownership.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

So, my patches compile locally, but on try I get

/include/nsTArray.h:664:34: error: Cannot instantiate
'nsTArray_CopyChooser<std::__1::reference_wrapper<RefPtr<mozilla::SourceListener> > >'
with non-memmovable template argument 'std::__1::reference_wrapper<RefPtr<mozilla::SourceListener> >'

Looks like std::reference_wrapper is not moveable? I guess I'll just have to drop it, and either use pointers or take the refcount hit.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Found a workaround for that, but now I'm getting this crash:

23:51:49     INFO - Thread 0 (crashed)
23:51:49     INFO -  0  XUL!mozilla::layers::ImageContainer::~ImageContainer() [ImageContainer.cpp:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 224 + 0x11]
23:51:49     INFO -     rax = 0x00000001150a4b03   rdx = 0x0000000000000000
23:51:49     INFO -     rcx = 0x0000000119bb7788   rbx = 0x0000000116405860
23:51:49     INFO -     rsi = 0x0000000120f0b598   rdi = 0x0000000120f0b588
23:51:49     INFO -     rbp = 0x00007fff511d9d20   rsp = 0x00007fff511d9cd0
23:51:49     INFO -      r8 = 0x0000000119d000b8    r9 = 0x0000000000000017
23:51:49     INFO -     r10 = 0x0000000000000016   r11 = 0x000000012c0a0dc0
23:51:49     INFO -     r12 = 0x0000000116405860   r13 = 0x0000000000000000
23:51:49     INFO -     r14 = 0x0000000120f0b570   r15 = 0x0000000120f0b5c0
23:51:49     INFO -     rip = 0x000000011080d865
23:51:49     INFO -     Found by: given as instruction pointer in context
23:51:49     INFO -  1  XUL!mozilla::MediaEngineDefaultVideoSource::~MediaEngineDefaultVideoSource() [MediaEngineDefault.cpp:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 68 + 0xad]
23:51:49     INFO -     rbp = 0x00007fff511d9d40   rsp = 0x00007fff511d9d30
23:51:49     INFO -     rip = 0x00000001123f7fdd
23:51:49     INFO -     Found by: previous frame's frame pointer
23:51:49     INFO -  2  XUL!mozilla::MediaEngineDefaultVideoSource::~MediaEngineDefaultVideoSource() [MediaEngineDefault.cpp:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 68 + 0xe]
23:51:49     INFO -     rbp = 0x00007fff511d9d60   rsp = 0x00007fff511d9d50
23:51:49     INFO -     rip = 0x00000001123f802e
23:51:49     INFO -     Found by: previous frame's frame pointer
23:51:49     INFO -  3  XUL!mozilla::MediaDevice::~MediaDevice() [MediaManager.h:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 107 + 0xe]
23:51:49     INFO -     rbp = 0x00007fff511d9d80   rsp = 0x00007fff511d9d70
23:51:49     INFO -     rip = 0x0000000112131ece
23:51:49     INFO -     Found by: previous frame's frame pointer
23:51:49     INFO -  4  XUL!mozilla::MediaDevice::Release() [MediaManager.cpp:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 712 + 0x2d]
23:51:49     INFO -     rbp = 0x00007fff511d9da0   rsp = 0x00007fff511d9d90
23:51:49     INFO -     rip = 0x00000001121244bd
23:51:49     INFO -     Found by: previous frame's frame pointer
23:51:49     INFO -  5  XUL!mozilla::SourceListener::~SourceListener() [MediaManager.cpp:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 399 + 0x4d]
23:51:49     INFO -     rbp = 0x00007fff511d9dd0   rsp = 0x00007fff511d9db0
23:51:49     INFO -     rip = 0x000000011217492d
23:51:49     INFO -     Found by: previous frame's frame pointer
23:51:49     INFO -  6  XUL!mozilla::SourceListener::~SourceListener() [MediaManager.cpp:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 399 + 0xe]
23:51:49     INFO -     rbp = 0x00007fff511d9df0   rsp = 0x00007fff511d9de0
23:51:49     INFO -     rip = 0x0000000112132f2e
23:51:49     INFO -     Found by: previous frame's frame pointer
23:51:49     INFO -  7  XUL!mozilla::GetUserMediaWindowListener::~GetUserMediaWindowListener() [MediaManager.cpp:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 693 + 0x39]
23:51:49     INFO -     rbp = 0x00007fff511d9e30   rsp = 0x00007fff511d9e00
23:51:49     INFO -     rip = 0x000000011213591a
23:51:49     INFO -     Found by: previous frame's frame pointer
23:51:49     INFO -  8  XUL!mozilla::detail::RunnableMethodImpl<mozilla::GetUserMediaWindowListener*, void (mozilla::GetUserMediaWindowListener::*)(), true, (mozilla::RunnableKind)0>::~RunnableMethodImpl() [nsThreadUtils.h:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 1149 + 0x49]
23:51:49     INFO -     rbp = 0x00007fff511d9e50   rsp = 0x00007fff511d9e40
23:51:49     INFO -     rip = 0x00000001121724b9
23:51:49     INFO -     Found by: previous frame's frame pointer
23:51:49     INFO -  9  XUL!mozilla::Runnable::Release() [nsThreadUtils.cpp:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 54 + 0x75]
23:51:49     INFO -     rbp = 0x00007fff511d9e80   rsp = 0x00007fff511d9e60
23:51:49     INFO -     rip = 0x000000010f7c1db5
23:51:49     INFO -     Found by: previous frame's frame pointer
23:51:49     INFO - 10  XUL!mozilla::CycleCollectedJSContext::ProcessStableStateQueue() [CycleCollectedJSContext.cpp:2db7731cd9b30d77b9a0fcff86c5545c1db20fd0 : 431 + 0xe]
23:51:49     INFO -     rbp = 0x00007fff511d9ec0   rsp = 0x00007fff511d9e90
23:51:49     INFO -     rip = 0x000000010f6f815e
23:51:49     INFO -     Found by: previous frame's frame pointer

Andreas, any idea why this is happening?

Comment 4

[Andreas Pehrson [:pehrsons]]

There's a race with thread 34?

Was this SourceListener not Stop()ed and Deallocate()d properly before being released?

I did some retriggers on debug to see if there's anything somewhat-high-frequency giving this away earlier.

Comment 5

[Andreas Pehrson [:pehrsons]]

I suppose the window listener was destroyed without going through Remove() for some SourceListener. It should be caught in debug at [1]. If so, there's no Runnable scheduled for SourceListener::Stop() keeping the SourceListener alive. If this is true it's basically breaking the contract for GetUserMediaWindowListener in that it must Remove() all SourceListeners before being destroyed. It's on purpose that we don't have a catch-all in the dtor because we want to catch these rather than be forgiving (and possibly keep devices alive for too long).

Looking at your patch I don't see anything that changes this flow. Perhaps MediaManager:5 logs could reveal something.

[1] https://hg.mozilla.org/try/file/29b9d3eb8761d20bda9906f7e2be4c982d926bfe/dom/media/MediaManager.cpp#l686

Comment 6

[Andreas Pehrson [:pehrsons]]

Indeed. RemoveAll() are not actually removing the SourceListeners. Hmm?!

Does SafeWalk work?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2db7731cd9b30d77b9a0fcff86c5545c1db20fd0&selectedJob=247511747

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

It looks like it's intermittent.

Does SafeWalk work?

It should. I assume copy-semantics of a std::reference_wrapper copies the reference as if it were a pointer and not the object itself? I'll try a patch without it using regular refcounting next to see if it matters.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Much greener now with regular ref-counting. Will probably go with that.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9065809 [details]
Bug 1552571: Optimize GetUserMediaWindowListener's source listener iteration and ownership.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: We don't know of any remaining exploits not fixed by bug 1547381 and bug 1550955. Code observation would suggest there are none. This is a code cleanup follow-up to those issues.

It's not clear this needs to remain a sec bug now that the other two have landed in 68. This approval request is mostly because 68 went to beta in the meantime.

However, it's not fully clear why this problem only manifested in 68 since the code was the same in earlier versions. My guess is other changes/optimizations nearby caused ref-counts to drop to 1 ahead of this code call in 68—causing this code to free the memory—whereas in 67 and earlier they never dropped below 2 for some reason. But this disclaimer also applies to bug 1547381 and bug 1550955 which have already landed.

• 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?: 68
• If not all supported branches, which bug introduced the flaw?: Unclear
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Should apply cleanly I suspect, since 68 went to beta recently.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It increases ref-counting of MediaManager's listener nodes marginally when iterated. No new testing needed.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #9)

Comment on attachment 9065809 [details]

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No

Actually, yes the comments on the SafeWalk function kinda do, though hopefully we caught all the cases in the previous two issues.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Actually, let me inline that SafeWalk() thing.

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Now ready.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 9065809 [details]
Bug 1552571: Optimize GetUserMediaWindowListener's source listener iteration and ownership.

Don't really need sec-approval here since we never shipped this regression. Also it's sec-audit, but since the specific instance bugs were sec-high I appreciate the caution.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This bug looks to be a bit stalled over the last couple weeks. Is this going to be close to landing soon? 68 goes to RC in a couple weeks.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks, I've picked it up again. Patch up for review. To be clear, this is more of a code cleanup thing after a sec bug fix, not a sec bug itself.

Comment 16

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

https://hg.mozilla.org/integration/autoland/rev/64070cd015ca9edb956da99d0241f7a6cf5cc4c0
https://hg.mozilla.org/mozilla-central/rev/64070cd015ca