Closed Bug 1234571 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: WebRTC, defect, P1)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox43 --- wontfix
firefox44 + fixed
firefox45 + fixed
firefox46 + fixed
firefox-esr38 44+ fixed
firefox-esr45 --- fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- affected
b2g-v2.5 --- fixed
b2g-v2.2r --- affected
b2g-master --- fixed

People

(Reporter: jesup, Assigned: jesup)

Details

(Keywords: crash, csectype-uaf, sec-critical, Whiteboard: [post-critsmash-triage][adv-main44+][adv-esr38.6+])

Crash Data

Attachments

(1 file)

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
Group: media-core-security
Rank: 10
Moving to webrtc since this appears to be an upstream bug in webrtc.org
Component: Audio/Video: GMP → WebRTC
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.
Attachment #8702953 - Flags: review?(pkerr)
Attachment #8702953 - Flags: review?(pkerr) → review+
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.
Attachment #8702953 - Flags: sec-approval?
Attachment #8702953 - Flags: approval-mozilla-esr38?
Attachment #8702953 - Flags: approval-mozilla-beta?
Attachment #8702953 - Flags: approval-mozilla-aurora?
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.
[Tracking Requested - why for this release]:
Sec-approval+ for trunk.
Please nominate branch patches (including ESR38) after it is checked in.
Comment on attachment 8702953 [details] [diff] [review]
unregister encoded-frame callback when releasing codec databases

Never mind. I see nominations were done. Approving.
Attachment #8702953 - Flags: sec-approval?
Attachment #8702953 - Flags: sec-approval+
Attachment #8702953 - Flags: approval-mozilla-esr38?
Attachment #8702953 - Flags: approval-mozilla-esr38+
Attachment #8702953 - Flags: approval-mozilla-beta?
Attachment #8702953 - Flags: approval-mozilla-beta+
Attachment #8702953 - Flags: approval-mozilla-aurora?
Attachment #8702953 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/6ec9b53392a6
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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)
Flags: needinfo?(rjesup)
the webrtc.org update 43 isn't on beta, so merged it by hand:
https://hg.mozilla.org/releases/mozilla-beta/rev/0b7ba8736906
Flags: needinfo?(rjesup)
Group: media-core-security, core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main44+][adv-esr38.6+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.