crash in webrtc::videocapturemodule::GetMaxOfFrameArray

RESOLVED FIXED in Firefox 24

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jsmith, Assigned: jesup)

Tracking

({crash, sec-moderate, sec-vector})

22 Branch
mozilla25
x86
Windows NT
Points:
---

Firefox Tracking Flags

(firefox21 wontfix, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, firefox25 fixed, firefox-esr17 unaffected, firefox-esr24 unaffected, b2g18 unaffected)

Details

(Whiteboard: [getUserMedia], [blocking-gum-][leave-open][webrtc-topcrash][adv-main24+], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

This bug was filed from the Socorro interface and is 
report bp-0a4f8b73-e624-48f2-bbf5-dc62d2130527 .
============================================================= 

Frame 	Module 	Signature 	Source
0 	xul.dll 	webrtc::videocapturemodule::GetMaxOfFrameArray 	media/webrtc/trunk/webrtc/modules/video_capture/windows/help_functions_ds.cc:26
1 	xul.dll 	webrtc::videocapturemodule::DeviceInfoDS::CreateCapabilityMap 	media/webrtc/trunk/webrtc/modules/video_capture/windows/device_info_ds.cc:609
2 	xul.dll 	webrtc::videocapturemodule::DeviceInfoImpl::NumberOfCapabilities 	media/webrtc/trunk/webrtc/modules/video_capture/device_info_impl.cc:76
3 	xul.dll 	webrtc::ViECaptureImpl::NumberOfCapabilities 	media/webrtc/trunk/webrtc/video_engine/vie_capture_impl.cc:442
4 	xul.dll 	mozilla::MediaEngineWebRTCVideoSource::ChooseCapability 	content/media/webrtc/MediaEngineWebRTCVideo.cpp:157
5 	xul.dll 	mozilla::MediaEngineWebRTCVideoSource::Allocate 	content/media/webrtc/MediaEngineWebRTCVideo.cpp:224
6 	xul.dll 	mozilla::GetUserMediaRunnable::ProcessGetUserMedia 	dom/media/MediaManager.cpp:739
7 	xul.dll 	mozilla::GetUserMediaRunnable::Run 	dom/media/MediaManager.cpp:600
8 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
9 	xul.dll 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:265
10 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:395
11 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:90
12 	msvcr100.dll 	_callthreadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314
13 	msvcr100.dll 	_threadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292
14 	kernel32.dll 	BaseThreadInitThunk 	
15 	ntdll.dll 	__RtlUserThreadStart 	
16 	ntdll.dll 	_RtlUserThreadStart
Looks like a bug in GetFrameRateList() in the directx/camera-driver....  it would help a lot to know what camera/driver the reports were for.

Bas, any ideas?
Comment on attachment 755485 [details] [diff] [review]
null out pointer for GetFrameRateList

Worth a try...
Attachment #755485 - Flags: review?(bas)
Whiteboard: [getUserMedia], [blocking-gum-][webrtc-uplift]
Comment on attachment 755485 [details] [diff] [review]
null out pointer for GetFrameRateList

Review of attachment 755485 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good for me but I'm afraid I'm not really qualified to review this code as I'm not a WebRTC peer.
Attachment #755485 - Flags: review?(bas)
Comment on attachment 755485 [details] [diff] [review]
null out pointer for GetFrameRateList

As module owner, I'll interpret that as r+ ;-)
Attachment #755485 - Flags: review+
https://hg.mozilla.org/projects/cypress/rev/758527363f29

We should watch crash stats once this gets to the field, and also investigate what broken cameras/drivers might be at fault.
Whiteboard: [getUserMedia], [blocking-gum-][webrtc-uplift] → [getUserMedia], [blocking-gum-][webrtc-uplift][leave-open]
Comment on attachment 755485 [details] [diff] [review]
null out pointer for GetFrameRateList

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Patch might not get a chance to fix/wallpaper over camera driver bug

Testing completed (on m-c, etc.): Still waiting for m-c merge - trivial fix


Risk to taking this patch (and alternatives if risky): No risk to the patch.  Just initializing a pointer to NULL in case the broken driver is leaving it unset.

String or IDL/UUID changes made by this patch: none
Attachment #755485 - Flags: approval-mozilla-beta?
Attachment #755485 - Flags: approval-mozilla-aurora?
Assignee: nobody → rjesup
Attachment #755485 - Flags: approval-mozilla-beta?
Attachment #755485 - Flags: approval-mozilla-beta+
Attachment #755485 - Flags: approval-mozilla-aurora?
Attachment #755485 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-beta/rev/f7ddec3461a7
Aurora is waiting on closure
Target Milestone: --- → mozilla24
https://hg.mozilla.org/releases/mozilla-aurora/rev/34fdc5b5710d
Whiteboard: [getUserMedia], [blocking-gum-][webrtc-uplift][leave-open] → [getUserMedia], [blocking-gum-][leave-open]
Considering the time the patch was submitted, I'll verify the fix on Firefox 22 beta 5. (I'll check in Socorro if until then new crash reports will appear)
Flags: needinfo?
Flags: needinfo?
While checking in Socorro for crash reports from last month, I found these 4 reports, which seem to be related to Firefox 22 beta 4 build (they have 20130605070403 as build ID). They only appear under the "Reports" tab in Socorro, and not also under the "Signature Summary" tab (Fx 22 beta 4 isn't mentioned).

https://crash-stats.mozilla.com/report/index/01ce5409-f812-4938-8d8a-1d09d2130613

https://crash-stats.mozilla.com/report/index/7426a742-4d0a-45b0-b176-5ea472130613

https://crash-stats.mozilla.com/report/index/62634ceb-b37c-4ec5-b48c-25e592130613


https://crash-stats.mozilla.com/report/index/5bb257a5-6f27-401a-abbf-5c57b2130613


Does anyone have any suggestions?
Flags: needinfo?
Suggestions specifically on what?
Flags: needinfo?
This would indicate that the problem was indeed caused by a bug in the driver.  All signs still point to a bug in the camera driver returning success without setting the pointer to a value.  This also proves that this was a potential security bug (though limited to machines with the bad camera driver - and webrtc isn't going to work on those cameras, so it may be hard to engineer users into enabling their cameras, since everytime they try for any site it will crash (in 22 beta-3 and before, and 23/24 builds up to when we landed the ptr-nulling patch).

The followon patch needed will be to check for a null array returned, and cleanly fail instead of null-derefing.

Needinfo to dveditz to decide if this makes sense to keep closed.
Group: core-security
Flags: needinfo?(dveditz)
(In reply to Jason Smith [:jsmith] from comment #13)
> Suggestions specifically on what?

I was wondering about the strange behavior that I experienced in comment 12. It seems weird to me. Is it expected?
By far the worst crash on the list for WebRTC-related crashes.
Whiteboard: [getUserMedia], [blocking-gum-][leave-open] → [getUserMedia], [blocking-gum-][leave-open][webrtc-topcrash]
Note that all the crashes in B5 and later are null derefs (i.e. the safety patch works).

We need to land an "error and don't crash patch".  Also, we need to find out what camera and what driver is causing this failure.  This is a bug in some camera driver on windows.
Flags: needinfo?(dveditz)
Grr.  Thanks keyboard.  I was going to say, clearing needinfo to dveditz since we should keep this closed, as it applies to FF20 and FF21 (hard to test, but we see the crashes).
I'll call this sec-moderate rather than worse mostly because of the mitigations mentioned in comment 14
Final 'don't crash with null deref if the driver does something stupid' patch
Attachment #755485 - Attachment is obsolete: true
Comment on attachment 780534 [details] [diff] [review]
Avoid null deref if camera doesn't update framelist ptr

bas: you can review this one. :-) Just adds a null-check, and treats a broken driver as having failed the GetMaxOfFrameArray() call.  I'll propose this to the upstream codebase.
Attachment #780534 - Flags: review?(bas)
Attachment #780534 - Flags: review?(bas) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/19d9842325b2

Will nominate for Aurora shortly.  Not sure we need to nominate it for beta at this late date.
Target Milestone: mozilla24 → mozilla25
https://hg.mozilla.org/mozilla-central/rev/19d9842325b2

Assuming this won't make it onto Fx23 since it's only sec-moderate.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 780534 [details] [diff] [review]
Avoid null deref if camera doesn't update framelist ptr

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Crashes (null-deref) if you have a particular-but-unknown broken camera driver.

Testing completed (on m-c, etc.): on m-c.

Risk to taking this patch (and alternatives if risky): virtually none
 
String or IDL/UUID changes made by this patch: none
Attachment #780534 - Flags: approval-mozilla-aurora?
Attachment #780534 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [getUserMedia], [blocking-gum-][leave-open][webrtc-topcrash] → [getUserMedia], [blocking-gum-][leave-open][webrtc-topcrash][adv-main24+]
getUserMedia wasn't enabled-by-default until FF 20
Group: core-security
You need to log in before you can comment on or make changes to this bug.