812785 - WebRTC use-after-free crash [@nr_ice_component_stun_server_cb]

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

alloc: ./media/mtransport/third_party/nICEr/src/ice/ice_component.c:54

   if(!(comp=RCALLOC(sizeof(nr_ice_component))))


free: ./media/mtransport/third_party/nICEr/src/ice/ice_component.c:98

    RFREE(component);


re-use: ./media/mtransport/third_party/nICEr/src/ice/ice_component.c:341

    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)(%d): received request from %s",comp->stream->pctx->label,comp->stream->label,comp->component_id,req->src_addr.


Tested with m-c changeset: 113558:9a6d708faf3f

Comment 1

[Eric Rescorla (:ekr)]

Please describe what you did to make this happen.

Comment 2

[Christoph Diehl [:posidron]]

It happened during fuzzing the SDP, I am looking to get a testcase for this.

Comment 3

[Christoph Diehl [:posidron]]

Attachment: testcase

If the testcase does not crash onload, reload the testcase after you see "Closed pc2". For me it was enough to reload 3 times to reproduce the same crash constantly.

Comment 4

[Robert Kaiser]

So, given this is in WebRTC, does this mean it affects 18 and higher? Is this serious enough that we should track it?

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Reproduced; uninitialized comp->stream (0xa5...); looking into why.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is a regression which has been started to happen lately.

PASS: http://hg.mozilla.org/mozilla-central/rev/58ebb638a7ea
FAIL: http://hg.mozilla.org/mozilla-central/rev/9a6d708faf3f

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58ebb638a7ea&tochange=9a6d708faf3f

Crash report: bp-01dc0cdf-da76-4276-8b7b-3c0c32121119

0 	XUL 	nr_ice_component_stun_server_cb 	ice_component.c:341
1 	XUL 	nr_stun_server_process_request 	stun_server_ctx.c:271
2 	XUL 	nr_ice_socket_readable_cb 	ice_socket.c:112
3 	XUL 	mozilla::NrSocket::OnSocketReady 	nr_socket_prsock.cpp:190
4 	XUL 	nsSocketTransportService::DoPollIteration 	nsSocketTransportService2.cpp:807
5 	XUL 	nsSocketTransportService::Run 	nsSocketTransportService2.cpp:646
6 	XUL 	_ZThn24_N24nsSocketTransportService3RunEv 	nsSocketTransportService2.cpp:690
7 	XUL 	nsThread::ProcessNextEvent 	nsThread.cpp:627
8 	XUL 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:221
9 	XUL 	nsThread::ThreadFunc 	nsThread.cpp:265
10 	libnspr4.dylib 	_pt_root 	ptthread.c:156
11 	libsystem_c.dylib 	libsystem_c.dylib@0x4e8be 	
12 	libsystem_c.dylib 	libsystem_c.dylib@0x51b74 	
13 	libnspr4.dylib 	libnspr4.dylib@0x1dacf 	

Regression from bug 811695? I will check tinderbox builds for a better range.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Indeed a regression from bug 811695.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Odd, though not 100% impossible if it's a timing change.  The code for mtransport/stun isn't related to the webrtc channel code, and the null conduit for gum shouldn't have any affect on nICEr.

Tim, do you see any way bug 811695 can be related to this crash?

I'm tracking the cause, and basically the streams (and components) are getting destroyed when a STUN callback happens because of a response/request from the STUN server.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

tim, see last comment

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: gdb traceback with local vars, etc

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Randell Jesup [:jesup] from comment #8)
> Tim, do you see any way bug 811695 can be related to this crash?

It's related because the build before your patch landed doesn't crash.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: crashtest v1

Crashtest which exercises this code path five times and always crashes the browser.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: crashtest v1.1

Missed to qref the temporary crashlist changes. I will wait with the review once the bug has been fixed.

Comment 14

[Eric Rescorla (:ekr)]

Attachment: Use after free issues in ICE

Comment 15

[Eric Rescorla (:ekr)]

Attachment: Use after free issues in ICE

Comment 16

[Eric Rescorla (:ekr)]

Attachment: Use after free issues in ICE

Comment 17

[Eric Rescorla (:ekr)]

Comment on attachment 683559 [details] [diff] [review]
Use after free issues in ICE

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

Jesup,

Can you verify that this fixes the problem in your test case and if soreview
the patch?

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 683559 [details] [diff] [review]
Use after free issues in ICE

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

Looks good, makes sense

::: media/mtransport/nricectx.cpp
@@ +488,5 @@
>  }
>  }  // close namespace
>  
>  
> +

spurious whitespace

::: media/mtransport/test/nrappkit_unittest.cpp
@@ +73,5 @@
> +  }
> +
> +  int Schedule_w() {
> +    NR_ASYNC_SCHEDULE(cb, this);
> +    

trailing spaces

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

FYI, I'm unable to make it crash now; I had 100% reproducibility before.

Comment 20

[Eric Rescorla (:ekr)]

Comment on attachment 683559 [details] [diff] [review]
Use after free issues in ICE

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

We don't actually *know* this is a security issue, though free memory
reads sometimes are. In this case we're trying to printf memory which
has been freed, so that may turn out to be safe.

* How easily can the security issue be deduced from the patch? 

Seems medium hard. It's basically a failure to cleanup, so someone would
have to deduce what's not getting cleaned up.


* Do comments in the patch, the check-in comment, or tests included
  in the patch paint a bulls-eye on the security problem?

Not exactly, but the tests will reproduce the crash if the patch is not
applies.


 * Which older supported branches are affected by this flaw?

FF 18.


*  If not all supported branches, which bug introduced the flaw?

It was introduced in FF 18.


 * Do you have backports for the affected branches? If not, how different,
  hard to create, and risky will they be?

Same patch for all branches.


*  How likely is this patch to cause regressions; how much testing does
it need?

It's a sensible patch and shouldn't cause crashes. I don't think it will
cause failures, but some testing is definitely needed.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Eric Rescorla from comment #20)
> Comment on attachment 683559 [details] [diff] [review]
> Use after free issues in ICE
> 
> Review of attachment 683559 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We don't actually *know* this is a security issue, though free memory
> reads sometimes are. In this case we're trying to printf memory which
> has been freed, so that may turn out to be safe.

Yes, but immediately after that (if logging isn't on) it tries to deref pointers from the freed block.  Barring strong evidence otherwise, I'd consider use-after-free a real and possibly exploitable sec-critical bug.

> 
> * How easily can the security issue be deduced from the patch? 
> 
> Seems medium hard. It's basically a failure to cleanup, so someone would
> have to deduce what's not getting cleaned up.
> 
> 
> * Do comments in the patch, the check-in comment, or tests included
>   in the patch paint a bulls-eye on the security problem?
> 
> Not exactly, but the tests will reproduce the crash if the patch is not
> applies.

Running the tests on an unpatched browser will quickly show the use-after-free.

> 
> 
>  * Which older supported branches are affected by this flaw?
> 
> FF 18.

And now FF19 since we've uplifted.

> 
> 
> *  If not all supported branches, which bug introduced the flaw?
> 
> It was introduced in FF 18.
> 
> 
>  * Do you have backports for the affected branches? If not, how different,
>   hard to create, and risky will they be?
> 
> Same patch for all branches.
> 
> 
> *  How likely is this patch to cause regressions; how much testing does
> it need?
> 
> It's a sensible patch and shouldn't cause crashes. I don't think it will
> cause failures, but some testing is definitely needed.

I agree; it's a pretty clean patch.

Comment 22

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #21)
> (In reply to Eric Rescorla from comment #20)
> > Comment on attachment 683559 [details] [diff] [review]
> > Use after free issues in ICE
> > 
> > Review of attachment 683559 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > We don't actually *know* this is a security issue, though free memory
> > reads sometimes are. In this case we're trying to printf memory which
> > has been freed, so that may turn out to be safe.
> 
> Yes, but immediately after that (if logging isn't on) it tries to deref
> pointers from the freed block.  Barring strong evidence otherwise, I'd
> consider use-after-free a real and possibly exploitable sec-critical bug.

Not that this isn't a potential security issue, but since you mention logging:
In nICEr, r_log() is a function, not a macro, so whether or not logging is on
we always try to format the data.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 683559 [details] [diff] [review]
Use after free issues in ICE

sec-approval+

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Eric Rescorla from comment #17)
> Comment on attachment 683559 [details] [diff] [review]
> Use after free issues in ICE
>
> Can you verify that this fixes the problem in your test case and if soreview
> the patch?

Eric, can you push the patch including my crashtest to try (from the alder branch) so we can get results for all platforms?

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Alder push of patch and crashtest:
https://tbpl.mozilla.org/?tree=Try&rev=75e8e29956a4

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Randell Jesup [:jesup] from comment #25)
> Alder push of patch and crashtest:
> https://tbpl.mozilla.org/?tree=Try&rev=75e8e29956a4

Looks greenish! So I think we should be fine across platforms. Yay.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Can we get this landed? It's waiting nearly a week now.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b49f84c2c111

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b49f84c2c111

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 683173 [details] [diff] [review]
crashtest v1.1

Pushed crashtest to try:
https://tbpl.mozilla.org/?tree=Try&rev=2dc29b39736d

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given my crashtest and the following tryserver run this issue is not fixed:

https://tbpl.mozilla.org/?tree=Try&rev=2dc29b39736d

Crash for Windows opt:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17485084&tree=Try

Comment 32

[Christoph Diehl [:posidron]]

It does crash at random locations:

#0 0x108aee15c in mozilla::RefPtr<mozilla::NrIceCtx>::RefPtr RefPtr.h:102
#1 0x108b3d3d4 in sipcc::PeerConnectionMedia::ice_ctx const PeerConnectionMedia.h:259

#0 0x10501398c in nsTArray_base<nsTArrayDefaultAllocator>::Length const nsTArray.h:204
#1 0x108b8497b in sipcc::PeerConnectionMedia::AddRemoteStream PeerConnectionMedia.cpp:304

See bug: https://bugzilla.mozilla.org/show_bug.cgi?id=808546

Comment 33

[Eric Rescorla (:ekr)]

Folks, please do not push security fixes to nICEr to m-i or m-c without checking with me first.

I need to coordinate upstreaming them with the resiprocate nICEr repository.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

There are other variations of the crash with the attached testcase and tryserver run:

https://tbpl.mozilla.org/php/getParsedLog.php?id=17484747&tree=Try
 0  XUL!nr_socket_local_sendto [nr_socket_prsock.cpp : 356 + 0x0]
 1  XUL!nr_stun_client_send_request [stun_client_ctx.c : 365 + 0x22]
 2  XUL!nr_stun_client_timer_expired_cb [stun_client_ctx.c : 228 + 0x7]

https://tbpl.mozilla.org/php/getParsedLog.php?id=17493401&tree=Try
 0  libxul.so!mozilla::NrSocket::sendto(void const*, unsigned int, int, nr_transport_addr_*) [nr_socket_prsock.cpp : 356 + 0x3]
 1  libxul.so!nr_socket_sendto [nr_socket.c : 78 + 0x13]
 2  libxul.so!nr_stun_client_send_request [stun_client_ctx.c : 365 + 0x23]
 3  libxul.so!nr_stun_client_timer_expired_cb [stun_client_ctx.c : 228 + 0x6]

https://tbpl.mozilla.org/php/getParsedLog.php?id=17485338&tree=Try
 0  xul.dll!nr_ice_component_stun_server_cb [ice_component.c:2dc29b39736d : 341 + 0xd]
 1  xul.dll!nr_stun_server_process_request [stun_server_ctx.c:2dc29b39736d : 271 + 0x20]
 2  xul.dll!nr_ice_socket_readable_cb [ice_socket.c:2dc29b39736d : 112 + 0x21]

https://tbpl.mozilla.org/php/getParsedLog.php?id=17485112&tree=Try
 0  xul.dll!nr_ice_ctx_remember_id [ice_ctx.c:2dc29b39736d : 502 + 0xc]
 1  xul.dll!nr_ice_candidate_pair_start [ice_candidate_pair.c:2dc29b39736d : 369 + 0x12]
 2  xul.dll!nr_ice_candidate_pair_do_triggered_check [ice_candidate_pair.c:2dc29b39736d : 399 + 0x8]
 3  xul.dll!nr_ice_component_stun_server_cb [ice_component.c:2dc29b39736d : 518 + 0xb]

https://tbpl.mozilla.org/php/getParsedLog.php?id=17484759&tree=Try
 0  XUL!nr_ice_candidate_pair_insert [ice_candidate_pair.c : 518 + 0x0]
 1  XUL!nr_ice_component_stun_server_cb [ice_component.c : 471 + 0x10]
 2  XUL!nr_stun_server_process_request [stun_server_ctx.c : 271 + 0x1e]

https://tbpl.mozilla.org/php/getParsedLog.php?id=17484749&tree=Try
 0  XUL!nr_ice_peer_ctx_fire_done [ice_peer_ctx.c : 398 + 0x0]
 1  XUL!mozilla::nrappkitTimerCallback::Notify(nsITimer*) [nr_timer.cpp : 50 + 0xa]
 2  XUL!nsTimerImpl::Fire() [nsTimerImpl.cpp : 485 + 0xb]

Comment 35

[Daniel Veditz [:dveditz]]

Although there was a fix checked in the bug got reopened so I'm setting Firefox 20 status back to "affected". But maybe the crashes whimboo is seeing are the other bugs Ekr hinted at.

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

I've been rerunning the testcase in a loop.  15 minutes with no crashes.  I'll try the crashtest instead and see.

Ok, 15 min running that too, no crashes (both tests I modified to reload themselves a second or two after they ended).

Comment 37

[Randell Jesup [:jesup] (needinfo me)]

And finally I made it crash by stopping via navigating away ('Home').  Crashed at 
sipcc::PeerConnectionMedia::AddRemoteStream()

pc->impl()->media() is null, so calling ->AddRemoteStream() from that crashes.

This is a *different* bug than the one reported and originally closed here.  Probably both reports from Christoph are media() being NULL.  This should be a separate bug.

The reports from Henrik above seem to be separate from both, and in the ice code.

I'm going to re-close this bug as this one appears fixed.  We need two new bugs: one for the null media() issue, the other for Henrik's issues, which I have not been able to duplicate.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: crashtest v1.2

Updated crashtest due to latest crashtest.list changes.

> I need to coordinate upstreaming them with the resiprocate nICEr repository.

Erik, can we land the crashtest or is it still an issue you have to coordinate. That would mean I will wait with the check-in. Thanks.

Comment 39

[Eric Rescorla (:ekr)]

Yes, we can land it. The coordination as to land the patch to nICEr contemporaneously with landing hte patch on m-i but since we already landed on m-i and I've notified the resiprocate people it's too late now.

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 690085 [details] [diff] [review]
crashtest v1.2

https://hg.mozilla.org/integration/mozilla-inbound/rev/36ac53618236

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36ac53618236