Closed Bug 876878 Opened 8 years ago Closed 7 years ago

crash in webrtc::videocapturemodule::GetMaxOfFrameArray

Categories

(Core :: WebRTC, defect)

22 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
firefox25 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected

People

(Reporter: jsmith, Assigned: jesup)

Details

(Keywords: crash, sec-moderate, sec-vector, Whiteboard: [getUserMedia], [blocking-gum-][leave-open][webrtc-topcrash][adv-main24+])

Crash Data

Attachments

(1 file, 1 obsolete file)

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