Closed Bug 1646220 Opened 4 years ago Closed 4 years ago

Crash in [@ webrtc::videocapturemodule::VideoCaptureImpl::DeliverCapturedFrame]

Categories

(Core :: WebRTC, defect)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox-esr68 - unaffected
firefox-esr78 - unaffected
firefox77 --- wontfix
firefox78 --- wontfix
firefox79 + fixed
firefox80 + fixed

People

(Reporter: kbrosnan, Assigned: dminor)

References

(Regression)

Details

(4 keywords, Whiteboard: [geckoview][sec-survey][adv-main79+r])

Crash Data

Attachments

(1 file)

Several of the reports have a poison address of 0xe5e5e5e5e5e5e5e5 marking security sensitive until the WebRTC team has chance to look at this.

This bug is for crash report bp-26e0f089-dab5-42eb-b37c-eb7f60200616.

Top 10 frames of crashing thread:

0 libxul.so webrtc::videocapturemodule::VideoCaptureImpl::DeliverCapturedFrame media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc:129
1 libxul.so webrtc::videocapturemodule::VideoCaptureAndroid::OnIncomingFrame dom/media/systemservices/android_video_capture/video_capture_android.cc:161
2 libxul.so webrtc::ProvideCameraFrame dom/media/systemservices/android_video_capture/video_capture_android.cc:67
3 base.odex (deleted) base.odex @0x4a114 
4 dalvik-LinearAlloc (deleted) dalvik-LinearAlloc @0x10494 
5 dalvik-main space (region space) (deleted) dalvik-main space @0xb8da84 
6 dalvik-main space (region space) (deleted) dalvik-main space @0xd0b8a4 
7 dalvik-main space (region space) (deleted) dalvik-main space @0x8c011c 
8 dalvik-main space (region space) (deleted) dalvik-main space @0xb8cdf4 
9 dalvik-main space (region space) (deleted) dalvik-main space @0xb8d9b4 

This is does not seem to be bug 1621500 as it is on Android and seen on recent trunk and beta.

Group: core-security → media-core-security

Dan, any thoughts on this?

Flags: needinfo?(dminor)

That has shown up on try as well. I'll have a look.

Assignee: nobody → dminor
Flags: needinfo?(dminor)
Crash Signature: [@ webrtc::videocapturemodule::VideoCaptureImpl::DeliverCapturedFrame] [@ std::__ndk1::__tree_min<T>] → [@ webrtc::videocapturemodule::VideoCaptureImpl::DeliverCapturedFrame] [@ std::__ndk1::__tree_min<T>] [@ <name omitted>] [@ std::__ndk1::__tree_node_base<void*>* std::__ndk1::__tree_min<std::__ndk1::__tree_node_base<void*>*>(std::__ndk1::__tree_node_base…
Crash Signature: [@ webrtc::videocapturemodule::VideoCaptureImpl::DeliverCapturedFrame] [@ std::__ndk1::__tree_min<T>] [@ <name omitted>] [@ std::__ndk1::__tree_node_base<void*>* → [@ webrtc::videocapturemodule::VideoCaptureImpl::DeliverCapturedFrame] [@ std::__ndk1::__tree_min<T>] [@ <name omitted>] [@ std::__ndk1::__tree_node_base<void*>*

I think the problem here is that we're racing the destructor on the C++ thread [1] with delivering a frame from the Java camera thread [2]. As part of the destructor running, it nulls out the pointer held by the Java code that is used for callbacks, so camera callbacks after the destructor has run should be safe. Ideally, we'd call stopCapture and all camera callbacks would stop before the destructor ran. Unfortunately, stopCapture can fail [3] if it is interrupted.

Unlike some of the other video capture implementations, once stopCapture is called, we won't see another call to startCapture on the same instance. So we can tear things down in stopCapture without worrying about hitting something like Bug 1625288. One possible solution would be to null out the callback pointer as part of stopCapture, so that even the call fails, we won't hit UAFs. That pointer is volatile in the Java code, which ensures that reads and writes are atomic, but we might need stronger synchronization for this to be safe. We'd also have to check that stopCapture will always be called prior to the destructor running.

The VideoCaptureAndroid C++ object is held by a refptr, so it's possible we could pass that into Java rather than the raw pointer so that Java could keep it alive until callbacks stop or the Java object is garbage collected. This would require some care because the C++ object holds references to the Java object.

We've already had deadlock problems in this code before in Bug 1452048. We have a critical section in C++, but also locking in the Java code.

[1] https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/dom/media/systemservices/android_video_capture/video_capture_android.cc#192
[2] https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/dom/media/systemservices/android_video_capture/video_capture_android.cc#44
[3] https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/dom/media/systemservices/android_video_capture/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java#130

Comment on attachment 9159766 [details]
Bug 1646220 - Remove unlinkCapturer from VideoCaptureAndroid; r=ng!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Not easily, I tried to structure it as a refactoring.
  • 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?: 77,78
  • If not all supported branches, which bug introduced the flaw?: Bug 1578073
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: The same patch should apply.
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions. No extra testing is required, this is covered by automated tests.
Attachment #9159766 - Flags: sec-approval?

The severity field is not set for this bug.
:dminor, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dminor)
Severity: -- → S3
Status: NEW → ASSIGNED
Flags: needinfo?(dminor)

Comment on attachment 9159766 [details]
Bug 1646220 - Remove unlinkCapturer from VideoCaptureAndroid; r=ng!

Approved to land and uplift - but if this is Android specific I don't know if we need to take it to ESR; doesn't Fenix only track release?

Attachment #9159766 - Flags: sec-approval?
Attachment #9159766 - Flags: sec-approval+
Attachment #9159766 - Flags: approval-mozilla-esr78?
Attachment #9159766 - Flags: approval-mozilla-esr68?
Attachment #9159766 - Flags: approval-mozilla-beta+

Comment on attachment 9159766 [details]
Bug 1646220 - Remove unlinkCapturer from VideoCaptureAndroid; r=ng!

Fennec is still shipping off ESR68. Approved for 68.11 there. Agreed that ESR78 probably isn't necessary since this is Android-only code and we don't ship anything Android-related off that branch.

Attachment #9159766 - Flags: approval-mozilla-esr78?
Attachment #9159766 - Flags: approval-mozilla-esr68?
Attachment #9159766 - Flags: approval-mozilla-esr68+

Comment on attachment 9159766 [details]
Bug 1646220 - Remove unlinkCapturer from VideoCaptureAndroid; r=ng!

That said, this'll need a rebased patch for ESR68.

Flags: needinfo?(dminor)
Attachment #9159766 - Flags: approval-mozilla-esr68+

As far as I know this was regressed by Bug 1578073 which landed in Firefox 75, so ESR68 should not be affected by this. I checked the two crashes for std::__ndk1::__tree_min<T> and they were both graphics related. None of the top ten hits for <name omitted> were this bug either.

Flags: needinfo?(dminor)
Has Regression Range: --- → yes
Group: media-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(dminor)
Whiteboard: [geckoview] → [geckoview][sec-survey]

Done.

Flags: needinfo?(dminor)
Whiteboard: [geckoview][sec-survey] → [geckoview][sec-survey][adv-main79+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: