Crash (UAF) in VideoCaptureImpl::SetCaptureRotation

RESOLVED FIXED in Firefox 41

Status

()

defect
P1
critical
Rank:
10
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jesup, Assigned: gcp)

Tracking

({crash, csectype-uaf, sec-high})

unspecified
mozilla42
Unspecified
Android
Points:
---

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed, firefox-esr31 unaffected, firefox-esr38 unaffected, b2g-master fixed)

Details

(Whiteboard: [adv-main41+], crash signature)

Attachments

(1 attachment)

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

Updated

4 years ago
Assignee: nobody → gpascutto
Flags: needinfo?(gpascutto)
Assignee

Comment 3

4 years ago
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.
Assignee

Comment 4

4 years ago
I didn't see any relevant fixes upstream.
Assignee

Comment 5

4 years ago
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.
Assignee

Comment 6

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

if (!context)
Attachment #8639306 - Flags: review?(nchen) → review+
Assignee

Comment 8

4 years ago
(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.
Assignee

Comment 10

4 years ago
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/4b95e03f5b6b00be1089281fd87558c27bc8431e
changeset:  4b95e03f5b6b00be1089281fd87558c27bc8431e
user:       Gian-Carlo Pascutto <gcp@mozilla.com>
date:       Tue Jul 28 08:55:06 2015 +0200
description:
Bug 1186657. r=jesup,nchen
Assignee

Comment 11

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/4b95e03f5b6b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee

Updated

4 years ago
Duplicate of this bug: 1190841
Assignee

Comment 15

4 years ago
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.

Updated

4 years ago
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
android-only
Flags: needinfo?(rjesup)
Keywords: sec-high
Assignee

Comment 21

4 years ago
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.