Closed Bug 838169 Opened 7 years ago Closed 7 years ago

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

Categories

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

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

(Blocks 1 open bug)

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"
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+
https://hg.mozilla.org/mozilla-central/rev/e870235b44ce
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Duplicate of this bug: 836931
Duplicate of this bug: 835835
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.