Closed Bug 1186657 Opened 9 years ago Closed 9 years ago

Crash (UAF) in VideoCaptureImpl::SetCaptureRotation


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




Tracking Status
firefox41 --- fixed
firefox42 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-master --- fixed


(Reporter: jesup, Assigned: gcp)



(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [adv-main41+])

Crash Data


(1 file)

Crashes seen in crashstats in webrtc::videocapturemodule::VideoCaptureImpl::SetCaptureRotation(webrtc::VideoCaptureRotation)
with fairly apparent UAF addresses
backlog: --- → webRTC+
Rank: 10
See reports e329af44-8eaf-4417-a9c4-4f59a2150716 and b47488a9-a80a-4f77-b759-a34222150717 (which is a null-deref in the same location); 15 and 8 crashes (all android) in the last week
Gian-Carlo -- Can I assign this bug to you?  I think you're the best choice to take this.  (It appears to be an Android-only crash.)
Flags: needinfo?(gpascutto)
Assignee: nobody → gpascutto
Flags: needinfo?(gpascutto)
Code historically has issues. e.g. See
and comments like

The locking here is tricky. However the UAF/null deref is on the *actual lock*, ouch.
Those comments are pretty suspicious. Why would blocking the camera thread there be bad? Why is that lock/unlock cycle so special? Let's look further.

So, umm, we're checking that variable outside the lock. But that actually makes sense:

Now we try to take the lock. What guarantees are there it's still alive? I don't think there are any.

Need to think a bit on how to fix this. To avoid the race on the variable, we need the lock. But we can't lock before we check the variable to see if it's alive.
I didn't see any relevant fixes upstream.
So, what seems to be happening here is that the Java code is still trying to call the callbacks when the relevant C++ stuff is already destructed.

The destructor calls stopCapture in the Java code. Although that's dispatching the actual stop to another thread, it's joining the same thread before exiting, so this is synchronous from our perspective. There's a number of things that can go wrong there, but I don't see any way to tell what just from the report here.

Maybe I can add a separate call that unlinks the native capturer from the Java code, and does so in a way that can't fail. This is a bit ugly as opposed to fixing the actual bug, but I have nothing more to go by and this *is* a UAF.
I'm open to other ideas. This takes the fangs out of this bug, even without fully understanding the underlying issue. Unfortunately the wonky threading makes even this fix less clean than I'd like. We null out the pointer to the native code and avoid calling into dead objects.
Attachment #8639306 - Flags: review?(rjesup)
Attachment #8639306 - Flags: review?(nchen)
Attachment #8639306 - Flags: review?(rjesup) → review+
Comment on attachment 8639306 [details] [diff] [review]
Patch 1. v1 fix-capture-null

Review of attachment 8639306 [details] [diff] [review]:

Looks okay to me. What manages the lifetime of VideoCaptureAndroid? And is this going to be upstreamed?

::: media/webrtc/trunk/webrtc/modules/video_capture/android/
@@ +40,5 @@
>      jint length,
>      jint rotation,
>      jlong timeStamp,
>      jlong context) {
> +  if (context == 0)

if (!context)
Attachment #8639306 - Flags: review?(nchen) → review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> Looks okay to me. What manages the lifetime of VideoCaptureAndroid? 

I think that's webrtc vie_capture. Which means for us it'll be (indirectly) MediaEngineWebRTCVideoSource.

> And is this going to be upstreamed?

We have some modifications here and the function that we saw the actual UAF is (IIRC) specific to our patches. So the underlying bug might exist upstream but it's not clear to me it would be as serious there.
changeset:  4b95e03f5b6b00be1089281fd87558c27bc8431e
user:       Gian-Carlo Pascutto <>
date:       Tue Jul 28 08:55:06 2015 +0200
Bug 1186657. r=jesup,nchen
FWIW I don't see any way to file private bugs vs the WebRTC repos.
I suspect they can make them private, but I'll check
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment on attachment 8639306 [details] [diff] [review]
Patch 1. v1 fix-capture-null

Approval Request Comment
[Feature/regressing bug #]: Long time WebRTC bug
[User impact if declined]: Potential security issue, hard to exploit
[Describe test coverage new/current, TreeHerder]: basic gUM tests, though many are disabled on treeherder/Android
[Risks and why]: gum breakage, low risk
[String/UUID change made/needed]: N/A
Attachment #8639306 - Flags: approval-mozilla-beta?
Attachment #8639306 - Flags: approval-mozilla-aurora?
Comment on attachment 8639306 [details] [diff] [review]
Patch 1. v1 fix-capture-null

Fix a crash, should be low risk, taking it.
Attachment #8639306 - Flags: approval-mozilla-beta?
Attachment #8639306 - Flags: approval-mozilla-beta+
Attachment #8639306 - Flags: approval-mozilla-aurora?
Attachment #8639306 - Flags: approval-mozilla-aurora+
Given that this sec issue is in webrtc, I am assuming this does not affect esr31/38. Please let me know if that is not the case.
Group: core-security → core-security-release
This should have had a security rating and sec-approval+ before checkin. I need someone to set sec-approval? on the patch and fill out the template so we have information needed for advisories and other purposes. 

How exploitable is this crash? It needs a rating.
Flags: needinfo?(gpascutto)
Flags: needinfo?(rjesup)
From gcp:
[User impact if declined]: Potential security issue, hard to exploit
Flags: needinfo?(rjesup)
Keywords: sec-high
To exploit this you need to wedge the camera so it refuses to stop, which due to the "quality" of many Android camera drivers may be quite feasible. Then you need to get the VideoCaptureAndroid subsystem to destruct. I think that requires a MediaEngineWebRTCVideo shutdown which only happens at Shutdown itself right now (jesup, is that right?). If you achieve that, you then get Java code calling into an already destructed C++ object. I'm not clear how exploitable the latter is due to the it needing to happen when we're already shutting down, but I presume call-into-destructed-object is an understood exploit vector?
Flags: needinfo?(gpascutto) → needinfo?(rjesup)
call-into-destroyed-object is generally exploitable, but is especially hard to exploit deep in shutdown since you really need it to be reallocated to something semi-controllable.  Currently, VideoSources are shut down only on MediaManager shutdown (in MediaEngineWebRTC::Shutdown()).  This will change at some point, since we want to release MediaManager when it's idle "long enough".
Flags: needinfo?(rjesup)
Whiteboard: [adv-main41+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.