1109552 - (CVE-2015-0808) Mismatched free() / delete / delete [] in webrtc::VPMContentAnalysis::Release()

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[mitchwharper]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141126041045



Actual results:

==6262== Mismatched free() / delete / delete []
==6262==    at 0x4C2C2BC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6262==    by 0x9714343: webrtc::VPMContentAnalysis::Release() (scoped_ptr.h:154)
==6262==    by 0x97153DF: webrtc::VPMFramePreprocessor::Reset() (frame_preprocessor.cc:41)
==6262==    by 0x971567D: webrtc::VPMFramePreprocessor::~VPMFramePreprocessor() (frame_preprocessor.cc:29)
==6262==    by 0x9715DE0: webrtc::VideoProcessingModuleImpl::~VideoProcessingModuleImpl() (video_processing_impl.cc:75)
==6262==    by 0x9715E16: webrtc::VideoProcessingModuleImpl::~VideoProcessingModuleImpl() (video_processing_impl.cc:79)
==6262==    by 0x9716198: webrtc::VideoProcessingModule::Destroy(webrtc::VideoProcessingModule*) (video_processing_impl.cc:46)
==6262==    by 0x96D967B: webrtc::ViEEncoder::~ViEEncoder() (vie_encoder.cc:309)
==6262==    by 0x96D9792: webrtc::ViEEncoder::~ViEEncoder() (vie_encoder.cc:311)
==6262==    by 0x96D335E: webrtc::ViEChannelManager::DeleteChannel(int) (vie_channel_manager.cc:293)
==6262==    by 0x96C69B3: webrtc::ViEBaseImpl::DeleteChannel(int) (vie_base_impl.cc:216)
==6262==    by 0x845460D: mozilla::WebrtcVideoConduit::~WebrtcVideoConduit() (VideoConduit.cpp:140)
==6262==  Address 0x850860d0 is 0 bytes inside a block of size 307,200 alloc'd
==6262==    at 0x4C2B800: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6262==    by 0x9714552: webrtc::VPMContentAnalysis::Initialize(int, int) (content_analysis.cc:129)
==6262==    by 0x971469B: webrtc::VPMContentAnalysis::ComputeContentMetrics(webrtc::I420VideoFrame const&) (content_analysis.cc:58)
==6262==    by 0x9715662: webrtc::VPMFramePreprocessor::PreprocessFrame(webrtc::I420VideoFrame const&, webrtc::I420VideoFrame**) (frame_preprocessor.cc:135)
==6262==    by 0x9716103: webrtc::VideoProcessingModuleImpl::PreprocessFrame(webrtc::I420VideoFrame const&, webrtc::I420VideoFrame**) (video_processing_impl.cc:207)
==6262==    by 0x96D80D9: webrtc::ViEEncoder::DeliverFrame(int, webrtc::I420VideoFrame*, int, unsigned int const*) (vie_encoder.cc:657)
==6262==    by 0x96DB02B: webrtc::ViEFrameProviderBase::DeliverFrame(webrtc::I420VideoFrame*, int, unsigned int const*) (vie_frame_provider_base.cc:61)
==6262==    by 0x96C9EF5: webrtc::ViECapturer::DeliverI420Frame(webrtc::I420VideoFrame*) (vie_capturer.cc:630)
==6262==    by 0x96CACBB: webrtc::ViECapturer::ViECaptureProcess() (vie_capturer.cc:562)
==6262==    by 0x96CACCA: webrtc::ViECapturer::ViECaptureThreadFunction(void*) (vie_capturer.cc:552)
==6262==    by 0x974217A: webrtc::ThreadPosix::Run() (thread_posix.cc:379)
==6262==    by 0x974227E: StartThread (thread_posix.cc:106)

Comment 1

[Andrew McCreight [:mccr8]]

Do you have test cases for these various bugs you filed today?

Comment 2

[mitchwharper]

Yep, I found these while investigating a separate bug, here's the comment on the attachment with the trace that these all came out of (note I've ran this a few times and all of these are easily reproducible)

I found these while investigating bug 1108455, here's the comment to the attached trace that these all came out of:

I've had some trouble building with valgrind enabled on Windows, so I instead downloaded Firefox 34.0.5 source from the main FTP server and built with the suggestions at https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Valgrind.

This output contains some traces with errors that are closer to the failing method.

Valgrind command: `G_SLICE=always-malloc valgrind --tool=memcheck --vex-iropt-register-updates=allregs-at-mem-access --smc-check=all-non-file ./firefox`

Steps taken:
1. Start the browser
2. Open a new tab (note the use of unitialized memory, this may be a separate bug, and I couldn't find a similar bug in Bugzilla but my Bugzilla-fu isn't great)
3. Visit https://www.webrtc-experiment.com/RTCMultiConnection/MultiRTC/ in two separate tabs
4. Input the same room ID for both instances
5. Enable video and audio on the second tab, and allow access
6. Share my microphone and camera
7. Switch to other tab
8. Enable video and audio on first tab
9. Share camera and microphone
10. Preview camera from second user (this is where the first jump on uninitialized memory occured)
11. Preview microphone from second user
12. Switch tabs
13. Preview camera and mic from first user
14. Exit browser

Comment 3

[mitchwharper]

Seems to be a valid complaint:

The allocation (http://mxr.mozilla.org/mozilla-release/source/media/webrtc/trunk/webrtc/modules/video_processing/main/source/content_analysis.cc#129):

129   prev_frame_.reset(new uint8_t[width_ * height_]);

And the deallocation (http://mxr.mozilla.org/mozilla-release/source/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h#154):

152   inline void operator()(T* ptr) const {
153     enum { type_must_be_complete = sizeof(T) };
154     delete ptr;
155   }
156 };

Comment 5

[Julian Seward [:jseward]]

Yeah, looks valid to me.

Comment 6

[mitchwharper]

At first glance, it's at least plausible this could be exploitable. The size of the array is very easily controlled, and most of its contents are deterministicaly controllable according to the given input stream.

Comment 7

[mitchwharper]

For tracking purposes, looks like this was introduced in Bug 1043808, and is present back to 33.

Taking a look at the code, it seems like there's some sort of bug in http://mxr.mozilla.org/mozilla-release/source/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h--there's a different deleter defined for array types that's not being used, but I'm not familiar with C++ generics.

Comment 8

[mitchwharper]

Proper link (also, I ran diff and this file hasn't changed since 34): http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h

Comment 9

[mitchwharper]

Perhaps this is the issue?

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/system_wrappers/interface/scoped_ptr.h#178

178 template <class T, int n>
179 struct DefaultDeleter<T[n]> {
180   // Never allow someone to declare something like scoped_ptr<int[10]>.
181   COMPILE_ASSERT(sizeof(T) == -1, do_not_use_array_with_size_as_type);
182 };

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

I suspect this is fixed by just changing
scoped_ptr<uint8_t> prev_frame_;
to
scoped_ptr<uint8_t[]> prev_frame_;

I believe while officially undefined behavior, in practice for our supported platforms/compilers, "delete new uint8_t[n];" is safe (at least for base types like uint8_t).  On linux, at least, the delete appears to work correctly (there's no length word preceding the allocation).

(Glandium/jseward/ted - just looking for a double-check on that, especially for windows - thanks).

Comment 11

[(not currently active) Ted Mielczarek]

Sorry, I don't know the answer to that question.

Comment 12

[Julian Seward [:jseward]]

Me neither, sorry.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Benjamin - do you know about windows allocators in the current supported version?

Comment 14

[Benjamin Smedberg]

I don't understand the question, but I'm also going to defer all allocator questions to our new allocator overlord glandium

Comment 15

[Mike Hommey [:glandium]]

Yes, in practice, in Gecko, delete new T[n], where T is a type without a destructor, is safe. In practice, it does free(malloc(n * sizeof(T))).

Obviously, when T is a type with a destructor, that's a different story.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix incorrect scoped_ptr type (uint8_t array vs ptr)

this should fix it

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

-> sec-low per my and glandium's analysis - delete/delete[] mismatches for simple-type arrays while theoretically undefined are safe in practice (and don't leak either)

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8540965 [details] [diff] [review]
Fix incorrect scoped_ptr type (uint8_t array vs ptr)

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

Note this is only a correctness issue. It's not a security issue.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 21

[Phil Ringnalda (:philor)]

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