Closed Bug 1532465 Opened 6 years ago Closed 6 years ago

Use after free and undefined behavior in MediaType

Categories

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

65 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 67+ fixed
firefox66 --- wontfix
firefox67 + fixed
firefox68 + fixed

People

(Reporter: mozillabugs, Assigned: dminor)

Details

(Keywords: csectype-uaf, reporter-external, sec-moderate, Whiteboard: [adv-main67+][adv-esr60.7+])

Attachments

(1 file)

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

Flags: needinfo?(mozillabugs)

What happened to the indentation?

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

Group: core-security → media-core-security

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

I'll report this upstream shortly.

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?

Flags: needinfo?(jib)

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?

Flags: needinfo?(jib) → needinfo?(dminor)

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.

This code does not appear upstream AFAIKT.

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: nobody → dminor
Flags: needinfo?(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

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.

Priority: -- → P2
Group: media-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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
Attachment #9050310 - Flags: approval-mozilla-beta?
Flags: sec-bounty? → sec-bounty+

Should we consider this for ESR60 too?

Flags: needinfo?(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
Flags: needinfo?(dminor)
Attachment #9050310 - Flags: approval-mozilla-esr60?
Flags: needinfo?(mozillabugs)

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.

Attachment #9050310 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-

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.

Attachment #9050310 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [adv-main67+][adv-esr60.7+]
Group: core-security-release
No longer depends on: 1646904
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: