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

RESOLVED FIXED in Firefox 21

Status

()

Core
WebRTC: Networking
P1
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: posidron, Assigned: ekr)

Tracking

(Blocks: 1 bug, {crash, csectype-uaf, sec-critical})

Trunk
mozilla21
x86_64
Mac OS X
crash, csectype-uaf, sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox19 disabled, firefox20 disabled, firefox21+ fixed, firefox-esr17 unaffected, b2g18 disabled)

Details

(Whiteboard: [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-][adv-main21-])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 710199 [details]
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

Updated

5 years ago
Priority: -- → P1
Whiteboard: [WebRTC],[blocking-webrtc+]
(Assignee)

Comment 1

5 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?
status-b2g18: --- → disabled
status-firefox19: --- → disabled
status-firefox20: --- → disabled
status-firefox21: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox21: --- → +
Flags: needinfo?(cdiehl)
(Reporter)

Comment 2

5 years ago
ekr, please run the testcase in bug 837324 with an ASan -O1 build.
Flags: needinfo?(cdiehl)
(Assignee)

Comment 3

5 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

5 years ago
As said with -O1 ;)

ac_add_options --enable-optimize="-O1 -g"
Assignee: nobody → ekr
(Assignee)

Updated

5 years ago
Whiteboard: [WebRTC],[blocking-webrtc+] → [WebRTC],[blocking-webrtc+],[webrtc-asan-confused]
(Assignee)

Comment 5

5 years ago
Created attachment 712332 [details] [diff] [review]
Fix FMR and multiple release in NrIceCtx
(Assignee)

Comment 6

5 years ago
Created attachment 712333 [details] [diff] [review]
Fix FMR and multiple release in NrIceCtx
(Assignee)

Updated

5 years ago
Attachment #712332 - Attachment is obsolete: true
(Assignee)

Comment 7

5 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)
(Reporter)

Comment 8

5 years ago
Looks fixed, can't reproduce it anymore.
Flags: needinfo?(cdiehl)
(Assignee)

Comment 9

5 years ago
Created attachment 712346 [details] [diff] [review]
Fix FMR and multiple release in NrIceCtx
(Assignee)

Updated

5 years ago
Attachment #712333 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #712346 - Flags: review?(adam)
(Assignee)

Comment 10

5 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

5 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.
(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

5 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

5 years ago
Created attachment 712932 [details] [diff] [review]
Fix nricectx cleanup
(Assignee)

Updated

5 years ago
Attachment #712932 - Flags: review?(adam)
(Assignee)

Updated

5 years ago
Attachment #712346 - Attachment is obsolete: true
Attachment #712346 - Flags: review?(adam)

Comment 15

5 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+
https://hg.mozilla.org/mozilla-central/rev/e870235b44ce
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox21: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

5 years ago
Duplicate of this bug: 836931

Updated

5 years ago
Duplicate of this bug: 835835

Updated

5 years ago
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.