1234571 - UAF in MutexAutoLock::MutexAutoLock on frame-encoded callback when closing active webrtc call

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

This bug was filed from the Socorro interface and is 
report bp-7676704e-729d-432a-81ef-731c02151222.
=============================================================

Hit this testing openh264 1.5.3 on nightly, when closing an active call (pc_test.html, h264 required).  I'd made ~5 calls, and the last one had run for a while.  Other tabs were open. Device was nexus 10 (tablet).  Was unable to repeat in several attempts.

Bug would be in the gecko side since 'late' callbacks should be ignored

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Moving to webrtc since this appears to be an upstream bug in webrtc.org

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: unregister encoded-frame callback when releasing codec databases

The issue here appears to be an encoded-frame callback during destruction of the VideoSender.  The VideoSender includes a CodecDatabase object, which has a running Generic encoder (for the GMP OpenH264 encoder).  It has registered a callback on that, but hasn't un-registered it, and starts tearing things down.  The critical section comes from above, and gets deleted before the codec database has been, leaving a window where the callback can try to grab the critical section and find it's been deleted.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8702953 [details] [diff] [review]
unregister encoded-frame callback when releasing codec databases

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  
Rather hard.  While it's a UAF, the timing hole is very hard to hit.  And then it would be (much) harder to force something else to be reallocated into it in the hole.  Also it requires tearing down the peerconnection with an active H264 call.  The patch doesn't make it clear why we're unregistering the callback, though with enough poking around someone could infer it.

I was very lucky (or unlucky) to hit it once while testing OpenH264 on Android.  No crashstats entries match.

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 I believe.

If not all supported branches, which bug introduced the flaw?
Upstream bug.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivially apply to older branches

How likely is this patch to cause regressions; how much testing does it need?  Unlikely to cause regressions.  The callback ptr is protected by a critical section/mutex both in webrtc.org code, and in the GMP video encoder code.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

It also appears that in later upstream revs (which we hope to land in our tree in Firefox ~47) they've revised this to not pass a critical section in, and always have the callback registered, removing this timing hole.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

[Tracking Requested - why for this release]:

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ for trunk.
Please nominate branch patches (including ESR38) after it is checked in.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8702953 [details] [diff] [review]
unregister encoded-frame callback when releasing codec databases

Never mind. I see nominations were done. Approving.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec9b53392a6

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ec9b53392a6

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b07f254acc0f

Comment 11

[Carsten Book [:Tomcat]]

this has problems uplifting to beta:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r b07f254acc0f
grafting 321580:b07f254acc0f "Bug 1234571 - unregister encoded-frame callback when releasing codec databases. r=pkerr, a=al" (tip)
merging media/webrtc/trunk/webrtc/modules/video_coding/main/source/generic_encoder.cc
warning: conflicts while merging media/webrtc/trunk/webrtc/modules/video_coding/main/source/generic_encoder.cc! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

the webrtc.org update 43 isn't on beta, so merged it by hand:
https://hg.mozilla.org/releases/mozilla-beta/rev/0b7ba8736906

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0b7ba8736906

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/debff255c08e