Closed
Bug 838169
Opened 11 years ago
Closed 10 years ago
WebRTC use-after-free crash [@mozilla::NrIceCtx::Release]
Categories
(Core :: WebRTC: Networking, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: posidron, Assigned: ekr)
References
Details
(Keywords: crash, csectype-uaf, sec-critical, Whiteboard: [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-][adv-main21-])
Attachments
(2 files, 3 obsolete files)
11.53 KB,
text/plain
|
Details | |
12.07 KB,
patch
|
abr
:
review+
|
Details | Diff | Splinter Review |
This happened while fuzzing the JSEP and might relate to bug 835835. alloc: nricectx.cpp RefPtr<NrIceCtx> ctx = new NrIceCtx(name, offerer); free: nricectx.h:189 NS_INLINE_DECL_THREADSAFE_REFCOUNTING(NrIceCtx) stun_client_ctx.c:658 [..] /* Fire the callback */ if (ctx->finished_cb) { NR_async_cb finished_cb = ctx->finished_cb; ctx->finished_cb = 0; /* prevent 2nd call */ /* finished_cb call must be absolutely last thing in function * because as a side effect this ctx may be operated on in the * callback */ finished_cb(0,0,ctx->cb_arg); [..] re-use: nricectx.h:189 NS_INLINE_DECL_THREADSAFE_REFCOUNTING(NrIceCtx) Tested with m-i changeset: 120856:66efdc5f9355 -O1
Updated•11 years ago
|
Priority: -- → P1
Whiteboard: [WebRTC],[blocking-webrtc+]
Assignee | ||
Comment 1•11 years ago
|
||
Something is seriously wrong with this callstack. The relevant line of C code where this is allegedly called is: finished_cb(0,0,ctx->cb_arg); And this comes from ctx->finished_cb. In any case, this is C code, so it's pretty implausible that it's calling C++ member functions from NrIceCtx. Can you get this in the debugger?
Updated•10 years ago
|
status-b2g18:
--- → disabled
status-firefox19:
--- → disabled
status-firefox20:
--- → disabled
status-firefox21:
--- → affected
status-firefox-esr17:
--- → unaffected
tracking-firefox21:
--- → +
Updated•10 years ago
|
Flags: needinfo?(cdiehl)
Reporter | ||
Comment 2•10 years ago
|
||
ekr, please run the testcase in bug 837324 with an ASan -O1 build.
Flags: needinfo?(cdiehl)
Assignee | ||
Comment 3•10 years ago
|
||
I ran it with an ASan and --disable-optimize and no joy. Please provide the set of ASan flags you want me to use.
Reporter | ||
Comment 4•10 years ago
|
||
As said with -O1 ;) ac_add_options --enable-optimize="-O1 -g"
Updated•10 years ago
|
Assignee: nobody → ekr
Assignee | ||
Updated•10 years ago
|
Whiteboard: [WebRTC],[blocking-webrtc+] → [WebRTC],[blocking-webrtc+],[webrtc-asan-confused]
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #712332 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
cdiehl: Can you test with the above patch? It removes the crash on my system with the above repro case.
Flags: needinfo?(cdiehl)
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #712333 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #712346 -
Flags: review?(adam)
Assignee | ||
Comment 10•10 years ago
|
||
Adam, There are a number of changes here, some defects, some not: 1. Make sure that PCImpl::mSTSThread is set even if PCMedia::Init fails. 2. Have nr_stun_client_ctx_destroy() zero the pointer it was asked to destroy so that you can call it twice in a row safely (this now can happen). 3. Have nr_ice_candidate_destroy() call nr_stun_client_ctx_destroy) 4. Remove weird refcting hacks in NrIceCtx because they aren't needed with change #3.
Comment 11•10 years ago
|
||
Comment on attachment 712346 [details] [diff] [review] Fix FMR and multiple release in NrIceCtx Review of attachment 712346 [details] [diff] [review]: ----------------------------------------------------------------- This looks okay to me, modulo the callback issue we discussed on IRC. I'm leaving it as 'r?' pending the resolution of that issue (detailed below). ::: media/mtransport/nricectx.cpp @@ -388,2 @@ > int r = nr_ice_initialize(ctx_, &NrIceCtx::initialized_cb, > this); Now that we're not using a reference increment to keep the NrIceCtx alive, I think we can run into situations where the underlying nr_ice_ctx still has callbacks pending on it after we've been destroyed. I think we need nr_ice_ctx_destroy to zero out its callback pointer (and check for zero when invoking it) so that any event that happens between nr_ice_ctx_destroy and nr_ice_ctx_destroy_cb doesn't try to call into (the now destroyed) NrIceCtx. ::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c @@ +669,5 @@ > if(!ctxp || !*ctxp) > return(0); > > ctx=*ctxp; > + *ctxp=0; Is there (expected to be) any consistency with how the nICEr dtors handle the passed-in pointer WRT whether it gets zeroed out? It seems to be somewhat arbitrary at the moment.
Comment 12•10 years ago
|
||
(In reply to Adam Roach [:abr] from comment #11) > ::: media/mtransport/nricectx.cpp > @@ -388,2 @@ > > int r = nr_ice_initialize(ctx_, &NrIceCtx::initialized_cb, > > this); > > Now that we're not using a reference increment to keep the NrIceCtx alive, I > think we can run into situations where the underlying nr_ice_ctx still has > callbacks pending on it after we've been destroyed. I think we need > nr_ice_ctx_destroy to zero out its callback pointer (and check for zero when > invoking it) so that any event that happens between nr_ice_ctx_destroy and > nr_ice_ctx_destroy_cb doesn't try to call into (the now destroyed) NrIceCtx. Do these callbacks run on the same thread that would zero out the callback pointer (in ns_ice_ctx_destroy()? If so, please add asserts for that. If not, then locking will be needed.
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #12) > (In reply to Adam Roach [:abr] from comment #11) > > > ::: media/mtransport/nricectx.cpp > > @@ -388,2 @@ > > > int r = nr_ice_initialize(ctx_, &NrIceCtx::initialized_cb, > > > this); > > > > Now that we're not using a reference increment to keep the NrIceCtx alive, I > > think we can run into situations where the underlying nr_ice_ctx still has > > callbacks pending on it after we've been destroyed. I think we need > > nr_ice_ctx_destroy to zero out its callback pointer (and check for zero when > > invoking it) so that any event that happens between nr_ice_ctx_destroy and > > nr_ice_ctx_destroy_cb doesn't try to call into (the now destroyed) NrIceCtx. > > Do these callbacks run on the same thread that would zero out the callback > pointer (in ns_ice_ctx_destroy()? Of course. > If so, please add asserts for that. If not, then locking will be needed. Not really practical, since this is all in nICEr and it has no real notion of threads.
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #712932 -
Flags: review?(adam)
Assignee | ||
Updated•10 years ago
|
Attachment #712346 -
Attachment is obsolete: true
Attachment #712346 -
Flags: review?(adam)
Comment 15•10 years ago
|
||
Comment on attachment 712932 [details] [diff] [review] Fix nricectx cleanup Review of attachment 712932 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me.
Attachment #712932 -
Flags: review?(adam) → review+
Assignee | ||
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e870235b44ce
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e870235b44ce
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•10 years ago
|
Whiteboard: [WebRTC],[blocking-webrtc+],[webrtc-asan-confused] → [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-]
Updated•10 years ago
|
Whiteboard: [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-] → [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-][adv-main21-]
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•