1532465 - Use after free and undefined behavior in MediaType

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

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

Comment 1

[mozillabugs]

What happened to the indentation?

Comment 2

[:Gijs (he/him)]

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

Comment 3

[mozillabugs]

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

Comment 4

[mozillabugs]

I'll report this upstream shortly.

Comment 5

[Daniel Veditz [:dveditz]]

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

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 8

[mozillabugs]

This code does not appear upstream AFAIKT.

Comment 9

[Dan Minor [:dminor]]

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.

Comment 10

[Dan Minor [:dminor]]

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.

[1] https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/dom/media/systemservices/CamerasParent.cpp#746

Comment 11

[mozillabugs]

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.

Comment 12

[Dan Minor [:dminor]]

Attachment: Bug 1532465 - Ensure we AddRef prior to early return in MediaType::Assign; r=pehrsons

Comment 13

[Dan Minor [:dminor]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af3e08c3b74522f184719a3c61ce1ff1bb73b161

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/af3e08c3b745

Comment 15

[Dan Minor [:dminor]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Should we consider this for ESR60 too?

Comment 17

[Dan Minor [:dminor]]

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

Comment 18

[Pascal Chevrel:pascalc]

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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/97b3d94e492d

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/5e6590420e9c