Closed Bug 838169 Opened 12 years ago Closed 12 years ago

WebRTC use-after-free crash [@mozilla::NrIceCtx::Release]

Categories

(Core :: WebRTC: Networking, defect, P1)

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 --- disabled
firefox20 --- disabled
firefox21 + fixed
firefox-esr17 --- unaffected
b2g18 --- disabled

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)

Attached file callstack
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
Priority: -- → P1
Whiteboard: [WebRTC],[blocking-webrtc+]
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?
Flags: needinfo?(cdiehl)
ekr, please run the testcase in bug 837324 with an ASan -O1 build.
Flags: needinfo?(cdiehl)
I ran it with an ASan and --disable-optimize and no joy. Please provide the set of ASan flags you want me to use.
As said with -O1 ;) ac_add_options --enable-optimize="-O1 -g"
Blocks: 796463
Assignee: nobody → ekr
Whiteboard: [WebRTC],[blocking-webrtc+] → [WebRTC],[blocking-webrtc+],[webrtc-asan-confused]
Attachment #712332 - Attachment is obsolete: true
cdiehl: Can you test with the above patch? It removes the crash on my system with the above repro case.
Flags: needinfo?(cdiehl)
Looks fixed, can't reproduce it anymore.
Flags: needinfo?(cdiehl)
Attachment #712333 - Attachment is obsolete: true
Attachment #712346 - Flags: review?(adam)
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 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.
(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.
(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.
Attachment #712932 - Flags: review?(adam)
Attachment #712346 - Attachment is obsolete: true
Attachment #712346 - Flags: review?(adam)
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+
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Whiteboard: [WebRTC],[blocking-webrtc+],[webrtc-asan-confused] → [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-]
Blocks: 839941
Whiteboard: [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-] → [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-][adv-main21-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: