Use after free and undefined behavior in MediaType
Categories
(Core :: WebRTC: Audio/Video, defect, P2)
Tracking
()
People
(Reporter: mozillabugs, Assigned: dminor)
Details
(Keywords: csectype-uaf, reporter-external, sec-moderate, Whiteboard: [adv-main67+][adv-esr60.7+])
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-esr60+
|
Details | Review |
MediaType::Assign()
(media/webrtc/trunk/webrtc/video_capture/windows/MediaType.cpp) has use-after-free bugs and undefined behavior.
The use-after-free is in the constructors
22: MediaType::MediaType(const MediaType& aMediaType)
23: {
24: memset(this, 0, sizeof(MediaType));
25: Assign(aMediaType);
26: }
27:
28:
29: MediaType::MediaType(const AM_MEDIA_TYPE* aMediaType)
30: {
31: memset(this, 0, sizeof(MediaType));
32: Assign(aMediaType);
33: }
because both functions don't check the return value from MediaType::Assign()
. If Assign()
fails to allocate memory, it skips adding a reference to the destination object's pUnk
member (lines 89-91; 95-6):
72: HRESULT
73: MediaType::Assign(const AM_MEDIA_TYPE* aMediaType)
74: {
75: if (!aMediaType)
76: return E_POINTER;
77:
78: if (aMediaType == this)
79: return S_OK;
80:
81: // Release old data...
82: Clear();
83:
84: // Shallow copy.
85: memcpy(this, aMediaType, sizeof(AM_MEDIA_TYPE));
86:
87: // Create deep copy of incoming data...
88: if (cbFormat) {
89: pbFormat = (BYTE*)CoTaskMemAlloc(cbFormat);
90: if (!pbFormat)
91: return E_OUTOFMEMORY;
92: memcpy(pbFormat, aMediaType->pbFormat, cbFormat);
93: }
94:
95: if (pUnk)
96: pUnk->AddRef();
97:
98: return S_OK;
99: }
This means that the destination object's pUnk
object will be freed when the source object aMediaType
is deleted.
There is also a similar bug in media/webrtc/trunk/webrtc/modules/video_capture/windows/BasePin.cpp line 647:
638: STDMETHODIMP
639: BasePin::ConnectionMediaType(AM_MEDIA_TYPE *aMediaType)
640: {
641: if (!aMediaType)
642: return E_POINTER;
643:
644: CriticalSectionAutoEnter monitor(*mLock);
645:
646: if (IsConnected()) {
647: reinterpret_cast<MediaType*>(aMediaType)->Assign(&mMediaType);
648: return S_OK;
649: } else {
650: memset(aMediaType, 0, sizeof(AM_MEDIA_TYPE));
651: return VFW_E_NOT_CONNECTED;
652: }
653: }
I will try to produce a POC; gotta get a webcam.
MediaType::Assign() also invokes undefined behavior on
85: memcpy(this, aMediaType, sizeof(AM_MEDIA_TYPE));
because the location of the base-class subobject AM_MEDIA_TYPE
inside the derived-class object MediaType
is implementation-defined. Per C++17 s.13(5) "The order in which the base class subobjects are allocated in the most derived object (4.5) is unspecified."
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 1•6 years ago
|
||
What happened to the indentation?
Updated•6 years ago
|
Comment 2•6 years ago
|
||
(In reply to mozillabugs from comment #1)
What happened to the indentation?
Bugzilla uses markdown now. I've taken the liberty of editing your comment to fix up some of the indentation/readability of C++ references, hope that's OK. Thanks for the report. (I don't actually work on this part of our codebase so I'll leave it for other people who do work on it to come along and actually investigate this. :-) )
Reporter | ||
Comment 3•6 years ago
|
||
(In reply to :Gijs (he/him) from comment #2)
(In reply to mozillabugs from comment #1)
What happened to the indentation?
Bugzilla uses markdown now. I've taken the liberty of editing your comment to fix up some of the indentation/readability of C++ references, hope that's OK. Thanks for the report. (I don't actually work on this part of our codebase so I'll leave it for other people who do work on it to come along and actually investigate this. :-) )
Hmm, thanks! I'll check out markdown.
Reporter | ||
Comment 4•6 years ago
|
||
I'll report this upstream shortly.
Comment 5•6 years ago
|
||
This is just allocating for the type, it's not an arbitrary user-supplied huge chunk of data, right? What's the likelyhood you'd OOM in just the right place?
Comment 6•6 years ago
|
||
Probably real low. As far as I can tell AM_MEDIA_TYPE is defined here.
I don't know this code super-well. Perhaps dminor can contrive a way to create a lot of these on the stack from JS?
Comment 7•6 years ago
|
||
From link in comment 6:
"The AM_MEDIA_TYPE structure is followed by a variable-length block of data that contains format-specific information. The pbFormat member points to this block, called the format block. The layout of the format block depends on the type of data in the stream, and is specified by the formattype member. The format block might be NULL. Check the cbFormat member to determine the size. Cast the pbFormat member to access the format block."
Example given is some kind of VIDEOINFOHEADER.
Reporter | ||
Comment 8•6 years ago
|
||
This code does not appear upstream AFAIKT.
Assignee | ||
Comment 9•6 years ago
|
||
I'll have a look at fixing this.
Offhand, I think we end up caching the VideoCaptureImpl module so that we can only create one from JS which would in turn limit the number of MediaType instances that can be created, but I'm not 100% certain and I'll have a deeper look on Monday.
Assignee | ||
Comment 10•6 years ago
|
||
Checking with a debugger and the sample at https://mozilla.github.io/webrtc-landing/gum_test.html, this code is only accessed from MediaManager::GetUserMedia. We do cache the capture engine at [1], so the number of calls to MediaType::MediaType would be limited to some small multiple of the number of cameras attached to system. This runs after the gUM permission prompt, so I don't see how we would end up creating a large number of these without the user being prompted many times.
Reporter | ||
Comment 11•6 years ago
|
||
FF calls MediaType::Assign()
when using, e.g., the "Basic getUserMedia demo" at https://webrtc.github.io/samples . However, pUnk
is always nullptr
using that demo and others on that page, e.g., "Choose camera, microphone and speaker". So perhaps this bug is latent. I'll try some more tests.
If pUnk
were non-nullptr
, and something else were using enough CoTaskMemAlloc()-allocated heap, it would be possible manifest the bug. There are some other users of CoTaskMemAlloc() in the FF codebase that might potentially allocate lots of memory (e.g, GetInputScopes() and various stuff in accessible
), and some Windows APIs implicitly allocate from that heap (search "CoTaskMemFree" on microsoft.com), so running out of that heap doesn't necessarily require the user to have an unrealistic/inpossible number of devices on her system.
Updated•6 years ago
|
Assignee | ||
Comment 12•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 13•6 years ago
|
||
![]() |
||
Comment 14•6 years ago
|
||
Assignee | ||
Comment 15•6 years ago
|
||
Comment on attachment 9050310 [details]
Bug 1532465 - Ensure we AddRef prior to early return in MediaType::Assign; r=pehrsons
Beta/Release Uplift Approval Request
- Feature/Bug causing the regression: None
- User impact if declined: Potential crashes / security problems on Windows when accessing the camera.
- Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This just ensures that code is initialized properly in case we hit an unlikely error condition. In normal circumstances, this should not make any difference.
- String changes made/needed: None
Updated•6 years ago
|
Comment 16•6 years ago
|
||
Should we consider this for ESR60 too?
Assignee | ||
Comment 17•6 years ago
|
||
Comment on attachment 9050310 [details]
Bug 1532465 - Ensure we AddRef prior to early return in MediaType::Assign; r=pehrsons
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: The security problem this fixes seems unlikely to occur, but since this is a very low risk patch, I think it makes sense to pick it up for ESR.
- User impact if declined: Potential crashes and/or security problems.
- Fix Landed on Version: 68
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This just ensures that code is initialized properly in case we hit an unlikely error condition. In normal circumstances, this should not make any difference.
- String or UUID changes made by this patch: None
Reporter | ||
Updated•6 years ago
|
Comment 18•6 years ago
|
||
Comment on attachment 9050310 [details]
Bug 1532465 - Ensure we AddRef prior to early return in MediaType::Assign; r=pehrsons
Sec-moderate landed on mozilla-central, no reported regression since landing, uplift approved for 67 beta, thanks.
![]() |
||
Comment 19•6 years ago
|
||
uplift |
Updated•6 years ago
|
Comment 20•6 years ago
|
||
Comment on attachment 9050310 [details]
Bug 1532465 - Ensure we AddRef prior to early return in MediaType::Assign; r=pehrsons
Low risk fix for camera related crashes, OK for ESR 60.7 uplift.
Comment 21•6 years ago
|
||
uplift |
Updated•6 years ago
|
Updated•5 years ago
|
Updated•1 year ago
|
Description
•