828147 - WebRTC use-after-free crash [@nr_ice_candidate_pair_set_state]

Status: VERIFIED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

To reproduce reload the attached testcase a few times but not too fast, wait till you see "Destroying ICE ctx 'PC:'" in the console for the next reload.


alloc - ice_parser.c:128

int
nr_ice_peer_candidate_from_attribute(nr_ice_ctx *ctx,char *orig,nr_ice_media_stream *stream,nr_ice_candidate **candp)
{
[...]
    if(!(cand=RCALLOC(sizeof(nr_ice_candidate))))
[...]

free - ice_candidate.c:196
int nr_ice_candidate_destroy(nr_ice_candidate **candp)
  {
[...]
    RFREE(cand);
[...]


re-use - ice_candidate_pair.c:512

[...]
    if(pair->state==NR_ICE_PAIR_STATE_FAILED){
      if(r=nr_ice_component_failed_pair(pair->remote->component, pair))
        ABORT(r);
    }
[...]


Tested with m-c changeset: 118049:928550157e6e

This has been tested with Clang/ASan r171858 and --enable-optimize="-O2 -g"

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 2

[Christoph Diehl [:posidron]]

Attachment: NSPR_LOG

Comment 3

[Eric Rescorla (:ekr)]

I think I see what the problem is here. If you look at this stack trace, you can see that we are
firing one of the nICEr timers on the main thread rather than the STS thread:

   #0 0x10000ec94 in (anonymous namespace)::mz_free(_malloc_zone_t*, void*) (in firefox) + 52
    #1 0x10000e265 in wrap_free (in firefox) + 85
    #2 0x10af8bac0 in nr_ice_candidate_destroy ice_candidate.c:196
    #3 0x10af92580 in nr_ice_component_destroy ice_component.c:100
    #4 0x10af9a19c in nr_ice_media_stream_destroy ice_media_stream.c:100
    #5 0x10af9f70d in nr_ice_peer_ctx_destroy_cb ice_peer_ctx.c:313
    #6 0x10afe4dfe in mozilla::nrappkitTimerCallback::Notify nr_timer.cpp:88
    #7 0x109f6926f in nsTimerImpl::Fire nsTimerImpl.cpp:485
    #8 0x109f69c62 in nsTimerEvent::Run nsTimerImpl.cpp:565
    #9 0x109f5814e in nsThread::ProcessNextEvent nsThread.cpp:627
    #10 0x109e465f9 in NS_ProcessNextEvent_P nsThreadUtils.cpp:237
    #11 0x109f5516c in nsThread::Dispatch nsThread.cpp:410
    #12 0x1051a07c4 in nsSocketTransportService::Dispatch nsSocketTransportService2.cpp:116
    #13 0x10b0e2c26 in sipcc::PeerConnectionMedia::Init PeerConnectionMedia.cpp:122
    #14 0x10b0c725b in sipcc::PeerConnectionImpl::Initialize PeerConnectionImpl.cpp:331
    #15 0x109fb0f86 in NS_InvokeByIndex_P xptcinvoke_x86_64_unix.cpp:162
    #16 0x1084a4606 in CallMethodHelper::Call XPCWrappedNative.cpp:3085
    #17 0x10849e632 in XPCWrappedNative::CallMethod XPCWrappedNative.cpp:2385
    #18 0x1084bc9de in XPC_WN_CallMethod XPCWrappedNativeJSOps.cpp:1488
    #19 0x10b53e44a in js::CallJSNative, JS::CallArgs const&) jscntxtinlines.h:373
    #20 0x10b52d2a8 in js::InvokeKernel jsinterp.cpp:391
    #21 0x10b450d0a in js_fun_apply jsinterp.h:112
    #22 0x10b53e44a in js::CallJSNative, JS::CallArgs const&) jscntxtinlines.h:373
    #23 0x10b52d2a8 in js::InvokeKernel jsinterp.cpp:391
    #24 0x10b51c913 in js::Interpret jsinterp.cpp:2368
    #25 0x10b505389 in js::RunScript jsinterp.cpp:348
    #26 0x10b52d1bd in js::InvokeKernel jsinterp.cpp:406
    #27 0x10b52ea18 in js::Invoke jsinterp.h:112
    #28 0x10b352a19 in JS_CallFunctionValue jsapi.cpp:5806
    #29 0x107366067 in nsDOMConstructor::Construct nsDOMClassInfo.cpp:5324
previously allocated by thread T0 here:

Presumably this means that nr_ice_peer_ctx_destroy() was called on the
main thread, which we don't want.

I'll add a MOZ_ASSERT for the right thread to see where this is happening, but
I suspect we'll need to modify the NrIceCtx dtor to destroy these objects on
the STS thread.

Comment 4

[Andrew McCreight [:mccr8]]

Marking sec-critical due to UAF.

Comment 5

[Eric Rescorla (:ekr)]

I think this is now fixed. Can you confirm whether you still see it?

Comment 6

[Christoph Diehl [:posidron]]

Yep, that looks fixed can't reproduce it anymore and haven't seen it during the latest fuzzing runs.

Comment 7

[Eric Rescorla (:ekr)]

Marking as fixed. Can reopen if it appears.

Comment 8

[Jason Smith [:jsmith]]

Marking as verified per comment 6.

Comment 9

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 10

[Jason Smith [:jsmith]]

Comment on attachment 731754 [details] [diff] [review]
Crashtest v1

This is probably the closest I can get for a crashtest on this bug. It basically runs the typical handshake & reloads the page a few times.

Comment 11

[Jason Smith [:jsmith]]

Comment on attachment 731754 [details] [diff] [review]
Crashtest v1

Other bugs indicate Ekr is busy. I'm redirecting the review to jib since he was the original assignee.

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 731754 [details] [diff] [review]
Crashtest v1

Review of attachment 731754 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, but since this test wouldn't have found the bug (which was found with Asan and manual interruptions from the UI), does it add value?

812785.html appears to cover the same pattern, so it seems redundant.

Since we believe this bug was fixed by patches in other reports, hopefully those bugs give us coverage. I recommend dropping it. Ping me again if you disagree.

Comment 13

[Jason Smith [:jsmith]]

Sounds fine. Thanks for the input.