Closed Bug 1109552 (CVE-2015-0808) Opened 5 years ago Closed 5 years ago

Mismatched free() / delete / delete [] in webrtc::VPMContentAnalysis::Release()

Categories

(Core :: WebRTC, defect)

34 Branch
x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: mitchwharper, Assigned: jesup)

References

Details

(Keywords: sec-low, valgrind, Whiteboard: [adv-main37+])

Attachments

(1 file)

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)
Component: Untriaged → WebRTC
Product: Firefox → Core
Do you have test cases for these various bugs you filed today?
Flags: needinfo?(mitchwharper)
Keywords: valgrind
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
Flags: needinfo?(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 };
Duplicate of this bug: 1109556
Yeah, looks valid to me.
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.
Flags: needinfo?(rjesup)
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.
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
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 };
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).
Flags: needinfo?(ted)
Flags: needinfo?(rjesup)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(jseward)
Assignee: nobody → rjesup
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Sorry, I don't know the answer to that question.
Flags: needinfo?(ted)
Me neither, sorry.
Flags: needinfo?(jseward)
Benjamin - do you know about windows allocators in the current supported version?
Flags: needinfo?(benjamin)
Keywords: sec-high
I don't understand the question, but I'm also going to defer all allocator questions to our new allocator overlord glandium
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(mh+mozilla)
Attachment #8540965 - Flags: review?(mh+mozilla)
-> 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)
Keywords: sec-highsec-low
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.
Attachment #8540965 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/f42c4aa524f6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Whiteboard: [adv-main37+]
Alias: CVE-2015-0808
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.