1186657 - Crash (UAF) in VideoCaptureImpl::SetCaptureRotation

Status: RESOLVED FIXED

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

Description

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

Crashes seen in crashstats in webrtc::videocapturemodule::VideoCaptureImpl::SetCaptureRotation(webrtc::VideoCaptureRotation)
with fairly apparent UAF addresses

Comment 1

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

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

Comment 2

[Maire Reavy [:mreavy]]

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.)

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Code historically has issues. e.g. See https://bugzilla.mozilla.org/show_bug.cgi?id=915671
and comments like http://hg.mozilla.org/releases/mozilla-release/annotate/3a9878cc0371/media/webrtc/trunk/webrtc/modules/video_capture/android/video_capture_android.cc#l238

The locking here is tricky. However the UAF/null deref is on the *actual lock*, ouch.

http://hg.mozilla.org/releases/mozilla-release/annotate/3a9878cc0371/media/webrtc/trunk/webrtc/modules/video_capture/android/video_capture_android.cc#l165
http://hg.mozilla.org/releases/mozilla-release/annotate/3a9878cc0371/media/webrtc/trunk/webrtc/modules/video_capture/android/video_capture_android.cc#l215
^
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.

http://hg.mozilla.org/releases/mozilla-release/annotate/3a9878cc0371/media/webrtc/trunk/webrtc/modules/video_capture/android/video_capture_android.cc#l113

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

http://hg.mozilla.org/releases/mozilla-release/annotate/3a9878cc0371/media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc#l344

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.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

I didn't see any relevant fixes upstream.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. v1 fix-capture-null

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.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

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/video_capture_android.cc
@@ +40,5 @@
>      jint length,
>      jint rotation,
>      jlong timeStamp,
>      jlong context) {
> +  if (context == 0)

if (!context)

Comment 8

[Gian-Carlo Pascutto [:gcp]]

(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.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6212584a8804

Comment 10

[Gian-Carlo Pascutto [:gcp]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/4b95e03f5b6b00be1089281fd87558c27bc8431e
changeset:  4b95e03f5b6b00be1089281fd87558c27bc8431e
user:       Gian-Carlo Pascutto <[e-mail]>
date:       Tue Jul 28 08:55:06 2015 +0200
description:
Bug 1186657. r=jesup,nchen

Comment 11

[Gian-Carlo Pascutto [:gcp]]

FWIW I don't see any way to file private bugs vs the WebRTC repos.

Comment 12

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

I suspect they can make them private, but I'll check

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b95e03f5b6b

Comment 15

[Gian-Carlo Pascutto [:gcp]]

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

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8639306 [details] [diff] [review]
Patch 1. v1 fix-capture-null

Fix a crash, should be low risk, taking it.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f6fab6fdde2

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 19

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

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.

Comment 20

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

From gcp:
[User impact if declined]: Potential security issue, hard to exploit
android-only

Comment 21

[Gian-Carlo Pascutto [:gcp]]

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?

Comment 22

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

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".