Closed
Bug 822158
Opened 12 years ago
Closed 12 years ago
Heap-use-after-free in sipcc::PeerConnectionMedia::IceGatheringCompleted
Categories
(Core :: WebRTC, defect, P1)
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)
1.47 KB,
text/html
|
Details | |
5.21 KB,
patch
|
jib
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•12 years ago
|
||
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
Updated•12 years ago
|
Component: General → WebRTC
Product: Firefox → Core
QA Contact: jsmith
Comment 2•12 years ago
|
||
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
Comment 3•12 years ago
|
||
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().
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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 8•12 years ago
|
||
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+
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #694617 -
Attachment is obsolete: true
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #694946 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
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 12•12 years ago
|
||
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+
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
status-firefox18:
--- → disabled
status-firefox19:
--- → disabled
status-firefox-esr17:
--- → unaffected
Assignee | ||
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
Target Milestone: --- → mozilla20
Comment 15•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox20:
--- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Updated•12 years ago
|
Keywords: csec-uaf,
sec-critical
Comment 17•12 years ago
|
||
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.
Updated•12 years ago
|
Flags: sec-bounty-
Comment 18•12 years ago
|
||
Verified on 1/25 - no crash is seen.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Updated•12 years ago
|
Whiteboard: [WebRTC] [blocking-webrtc+] → [WebRTC] [blocking-webrtc+][adv-main20+]
Updated•12 years ago
|
Whiteboard: [WebRTC] [blocking-webrtc+][adv-main20+] → [WebRTC] [blocking-webrtc+][adv-main20-]
Updated•10 years ago
|
Group: core-security
Updated•9 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•