Closed Bug 822158 Opened 12 years ago Closed 12 years ago

Heap-use-after-free in sipcc::PeerConnectionMedia::IceGatheringCompleted

Categories

(Core :: WebRTC, defect, P1)

x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox18 --- disabled
firefox19 --- disabled
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: inferno, Assigned: jib)

References

Details

(4 keywords, Whiteboard: [WebRTC] [blocking-webrtc+][adv-main20-])

Attachments

(2 files, 2 obsolete files)

Attached file Testcase
20121215234951 http://hg.mozilla.org/mozilla-central/rev/5ea1c76e4bb3 Set user_pref("media.peerconnection.enabled", true); Load the testcase and wait 4-5 seconds. ================================================================= ==13418== ERROR: AddressSanitizer: heap-use-after-free on address 0x7f8f35a06c48 at pc 0x7f8f6aae161a bp 0x7f8f52febd50 sp 0x7f8f52febd48 READ of size 8 at 0x7f8f35a06c48 thread T5 #0 0x7f8f6aae1619 in ~lock_block media/webrtc/signaling//../../../media/mtransport/sigslot.h:309 #1 0x7f8f6aae1619 in ~lock_block media/webrtc/signaling//../../../media/mtransport/sigslot.h:308 #2 0x7f8f6aae1619 in operator() media/webrtc/signaling//../../../media/mtransport/sigslot.h:2350 #3 0x7f8f6aae1619 in sipcc::PeerConnectionMedia::IceGatheringCompleted(mozilla::NrIceCtx*) media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:316 0x7f8f35a06c48 is located 72 bytes inside of 256-byte region [0x7f8f35a06c00,0x7f8f35a06d00) freed by thread T0 here: #0 0x4265e0 in __interceptor_free #1 0x7f8f6aae2f35 in Release media/webrtc/signaling//./src/peerconnection/PeerConnectionMedia.h:349 #2 0x7f8f6aae2f35 in sipcc::PeerConnectionMedia::SelfDestruct() media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:250 #3 0x7f8f6aad911e in operator class nsIThread * media/webrtc/signaling//../../../media/mtransport/runnable_utils_generated.h:48 #4 0x7f8f6aad911e in sipcc::PeerConnectionImpl::ShutdownMedia(bool) media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:974 #5 0x7f8f6aad8cdf in CloseInt media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:949 #6 0x7f8f6aad8cdf in sipcc::PeerConnectionImpl::Close(bool) media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:931 previously allocated by thread T0 here: #0 0x4266a0 in malloc #1 0x7f8f6dba2148 in moz_xmalloc memory/mozalloc/mozalloc.cpp:54 Thread T5 created by T0 here: #0 0x4227a4 in __interceptor_pthread_create #1 0x7f8f6f67eecf in _PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:393 #2 0x7f8f6f67e92a in PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:476 #3 0x7f8f69ae5329 in nsThread::Init() xpcom/threads/nsThread.cpp:331 Shadow byte and word: 0x1ff1e6b40d89: fd 0x1ff1e6b40d88: fd fd fd fd fd fd fd fd More shadow bytes: 0x1ff1e6b40d68: fa fa fa fa fa fa fa fa 0x1ff1e6b40d70: fa fa fa fa fa fa fa fa 0x1ff1e6b40d78: fa fa fa fa fa fa fa fa 0x1ff1e6b40d80: fd fd fd fd fd fd fd fd =>0x1ff1e6b40d88: fd fd fd fd fd fd fd fd 0x1ff1e6b40d90: fd fd fd fd fd fd fd fd 0x1ff1e6b40d98: fd fd fd fd fd fd fd fd 0x1ff1e6b40da0: fd fd fd fd fd fd fd fd 0x1ff1e6b40da8: fd fd fd fd fd fd fd fd Stats: 577M malloced (2142M for red zones) by 1156372 calls Stats: 50M realloced by 28904 calls Stats: 511M freed by 899229 calls Stats: 498M really freed by 865181 calls Stats: 750M (192082 full pages) mmaped in 1468 calls mmaps by size class: 11:288150; 12:3968; 13:896; 14:480; 15:208; 16:704; 17:404; 18:34; 19:35; 20:21; 21:3; mallocs by size class: 11:1137529; 12:9674; 13:2909; 14:1894; 15:577; 16:2171; 17:1475; 18:73; 19:43; 20:24; 21:3; frees by size class: 11:885563; 12:6237; 13:2267; 14:1683; 15:422; 16:1480; 17:1456; 18:59; 19:40; 20:22; rfrees by size class: 11:852117; 12:5875; 13:2084; 14:1641; 15:416; 16:1474; 17:1454; 18:58; 19:40; 20:22; Stats: malloc large: 4366 small slow: 74017 ==13418== ABORTING
Some others crash with Write. ==10025== ERROR: AddressSanitizer: heap-use-after-free on address 0x7f07932c2a8c at pc 0x7f07b42c644a bp 0x7ffffab95a70 sp 0x7ffffab95a68 WRITE of size 4 at 0x7f07932c2a8c thread T0 #0 0x7f07b42c6449 in sipcc::PeerConnectionImpl::IceGatheringCompleted_m(mozilla::NrIceCtx*) media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1086 #1 0x7f07b42c7259 in mozilla::runnable_args_m_1<sipcc::PeerConnectionImpl*, void (sipcc::PeerConnectionImpl::*)(mozilla::NrIceCtx*), mozilla::NrIceCtx*>::Run() media/webrtc/signaling//../../../media/mtransport/runnable_utils_generated.h:122 #2 0x7f07b320a572 in NS_ProcessNextEvent_P(nsIThread*, bool) objdir-ff-asan/xpcom/build/nsThreadUtils.cpp:237 #3 0x7f07b2cf3bec in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82 #4 0x7f07b3361368 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:215 #5 0x7f07b29eec1d in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163 #6 0x7f07af977adf in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3891 #7 0x7f07af978aba in XRE_main toolkit/xre/nsAppRunner.cpp:4089 #8 0x4097bd in main browser/app/nsBrowserApp.cpp:174 #9 0x7f07b924376c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 0x7f07932c2a8c is located 108 bytes inside of 240-byte region [0x7f07932c2a20,0x7f07932c2b10) freed by thread T0 here: #0 0x4265e0 in __interceptor_free #1 0x7f07b42bc117 in sipcc::PeerConnectionImpl::Release() media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:207 previously allocated by thread T0 here: #0 0x4266a0 in malloc
Component: General → WebRTC
Product: Firefox → Core
QA Contact: jsmith
Exact crashes will vary - a PeerConnectionMedia object is getting freed while still hooked into stuff - my catch of this in GDB has it crashing in PeerConnectionMedia::IceGatheringCompleted() (which is called from mozilla::NrIceCtx::EmitAllCandidates(), kicked off by a STUN response). #5 0x00007f92b27f885e in sigslot::lock_block<sigslot::single_threaded>::~lock_block (this= 0x7f92a8cfc3d0, __in_chrg=<optimized out>) at ../../../dist/include/mtransport/sigslot.h:309 #6 0x00007f92b4bdd01d in sigslot::signal1<mozilla::NrIceCtx*, sigslot::single_threaded>::operator() ( this=0x7f9287a4e340, a1=0x7f9287a593e0) at ../../../../media/mtransport/sigslot.h:2348 #7 0x00007f92b4c3b17c in sipcc::PeerConnectionMedia::IceGatheringCompleted (this=0x7f9287a4e300, aCtx= 0x7f9287a593e0) at ../../../../../media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:316 *this is all 0xa5's #8 0x00007f92b4c3d439 in sigslot::_connection1<sipcc::PeerConnectionMedia, mozilla::NrIceCtx*, sigslot::single_threaded>::emit (this=0x7f9287a6a1c0, a1=0x7f9287a593e0) at ../../../../../media/mtransport/sigslot.h:1852 #9 0x00007f92b4bdcff2 in sigslot::signal1<mozilla::NrIceCtx*, sigslot::single_threaded>::operator() ( this=0x7f9287a593e8, a1=0x7f9287a593e0) at ../../../../media/mtransport/sigslot.h:2346 #10 0x00007f92b4bdc3fd in mozilla::NrIceCtx::EmitAllCandidates (this=0x7f9287a593e0) at ../../../../media/mtransport/nricectx.cpp:377 #11 0x00007f92b4bdcbae in mozilla::NrIceCtx::initialized_cb (s=0x0, h=0, arg=0x7f9287a593e0) at ../../../../media/mtransport/nricectx.cpp:467 #12 0x00007f92b4bb9fb4 in nr_ice_initialize_finished_cb (s=0x0, h=0, cb_arg=0x7f929b2d85cc) at ../../../../../../media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:345 #13 0x00007f92b4bb4667 in nr_ice_srvrflx_stun_finished_cb (sock=0x0, how=0, cb_arg=0x7f928c7313ec) at ../../../../../../media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:493 #14 0x00007f92b4bc3462 in nr_stun_client_process_response (ctx=0x7f92ab11ec0c, msg= 0x7f92a8cfcb40 "\001\001", len=88, peer_addr=0x7f92a8cfc9d0) at ../../../../../../media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c:658 #15 0x00007f92b4bbe67e in nr_ice_socket_readable_cb (s=0x7f9287a57200, how=0, cb_arg=0x7f9287a652cc) at ../../../../../../media/mtransport/third_party/nICEr/src/ice/ice_socket.c:106
Assignee: nobody → ekr
Keywords: crash
Priority: -- → P1
Whiteboard: [WebRTC] [blocking-webrtc+]
I believe the fix here is that we need to detach from the ICE signals. Because we are operating with muxex-free sigslot, this needs to happen on the ICE thread rather than letting it happen automatically. I.e. call SignalGatheringCompleted.disconnect() and SignalCompleted.disconnect() in PCMedia::ShutdownMediaTransport().
JIB, can you make this fix and test?
Assignee: ekr → jib
Comment on attachment 694617 [details] [diff] [review] Heap-use-after-free in PeerConnectionMedia::IceGatheringCompleted Got a lot of help from ekr on this one. Seems to fix the hang. I'll do some more tests tomorrow, but I'm submitting for review.
Attachment #694617 - Flags: review?(rjesup)
Comment on attachment 694617 [details] [diff] [review] Heap-use-after-free in PeerConnectionMedia::IceGatheringCompleted Review of attachment 694617 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +1108,5 @@ > PeerConnectionImpl::IceCompleted(NrIceCtx *aCtx) > { > + // Doing async calls here to avoid dispatches piling up on the callstack, > + // which don't tear down cleanly on shutdown > + trailing ws
Attachment #694617 - Flags: review?(rjesup) → review+
Comment on attachment 694617 [details] [diff] [review] Heap-use-after-free in PeerConnectionMedia::IceGatheringCompleted Review of attachment 694617 [details] [diff] [review]: ----------------------------------------------------------------- lgtm as long as the tests pass. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +1070,5 @@ > void > PeerConnectionImpl::IceGatheringCompleted(NrIceCtx *aCtx) > { > + // Doing async calls here to avoid dispatches piling up on the callstack, > + // which don't tear down cleanly on shutdown I would just say "Do an async call here to unwind the stack." @@ +1072,5 @@ > { > + // Doing async calls here to avoid dispatches piling up on the callstack, > + // which don't tear down cleanly on shutdown > + > + nsRefPtr<PeerConnectionImpl> pc(this); Add a comment that this refptr keeps the PC alive. @@ +1108,5 @@ > PeerConnectionImpl::IceCompleted(NrIceCtx *aCtx) > { > + // Doing async calls here to avoid dispatches piling up on the callstack, > + // which don't tear down cleanly on shutdown > + Same comments here as above.
Attachment #694617 - Flags: review+
Attachment #694617 - Attachment is obsolete: true
Attachment #694946 - Attachment is obsolete: true
Comment on attachment 694948 [details] [diff] [review] Switched to async dispatch of Ice(Gathering)Completed to unwind stack for clean shutdown. [Security approval request comment] How easily could an exploit be constructed based on the patch? Difficult, though heap-use-after-free is potentially exploitable, in theory, allowing for remote code execution. Queued dispatched code attempted to access a freed PeerConnectionMedia instance during teardown of this sub-system. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? webrtc is under pref. If not all supported branches, which bug introduced the flaw? Unknown. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? How likely is this patch to cause regressions; how much testing does it need? Not likely. Passed signalling + mtransport unit-tests, mochitests and crashtests from https://wiki.mozilla.org/Webrtc/checkin_policy Carrying forward r+ from rjesup and ekr.
Attachment #694948 - Flags: sec-approval?
Attachment #694948 - Flags: review+
Comment on attachment 694948 [details] [diff] [review] Switched to async dispatch of Ice(Gathering)Completed to unwind stack for clean shutdown. sec-approval+. Merry Christmas.
Attachment #694948 - Flags: sec-approval? → sec-approval+
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Keywords: verifyme
Blocks: 820990
Does not qualify for a bounty because the crash was previously reported by cdeihl in bug 820990, but we're also strongly leaning against awarding bounties for these preffed-off features. They landed for partial interop testing, but they are well known to be incomplete and buggy which is why they are preffed off. According to our severity ratings these should actually be "sec-moderate" bugs since users would have to take unusual steps (open about:config, etc) to make themselves vulnerable. We're calling them sec-critical for now because we do intend to turn on these features eventually, but the feature is not ready for prime-time use or bounty testing.
Flags: sec-bounty-
Verified on 1/25 - no crash is seen.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [WebRTC] [blocking-webrtc+] → [WebRTC] [blocking-webrtc+][adv-main20+]
Whiteboard: [WebRTC] [blocking-webrtc+][adv-main20+] → [WebRTC] [blocking-webrtc+][adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: