1211389 - Crash in nr_ice_candidate_mark_done when setting up WebRTC calls

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

This bug was filed from the Socorro interface and is 
report bp-39a18db1-14c9-45c9-8430-609602151005.
=============================================================

STR:

1) Using latest nightly for both ends, set up for a conversation via Firefox Hello or Talky.io
2) Join the rooms, granting permissions

Expect Results

- Call joins successfully

Actual Results

- Firefox either crashes or the content process crashes if in e10s.

Given the stack location, this seems to be a regression from bug 1208096.

Comment 1

[Mark Banner (:standard8)]

Stack from https://crash-stats.mozilla.com/report/index/39a18db1-14c9-45c9-8430-609602151005

0 	XUL 	nr_ice_candidate_mark_done 	media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
1 	XUL 	nr_ice_candidate_destroy 	media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
2 	XUL 	nr_ice_component_destroy 	media/mtransport/third_party/nICEr/src/ice/ice_component.c
3 	XUL 	nr_ice_media_stream_destroy 	media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
4 	XUL 	nr_ice_remove_media_stream 	media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
5 	XUL 	mozilla::NrIceMediaStream::Close() 	media/mtransport/nricemediastream.cpp
6 	XUL 	mozilla::NrIceCtx::SetStream(unsigned long, mozilla::NrIceMediaStream*) 	media/mtransport/nricectx.cpp
7 	XUL 	mozilla::PeerConnectionMedia::ActivateOrRemoveTransport_s(unsigned long, unsigned long, std::string const&, std::string const&, std::vector<std::string, std::allocator<std::string> > const&) 	media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
8 	XUL 	mozilla::runnable_args_memfn<mozilla::RefPtr<mozilla::PeerConnectionMedia>, void (mozilla::PeerConnectionMedia::*)(unsigned long, unsigned long, const std::basic_string<char>&, const std::basic_string<char>&, const std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > >&), unsigned long, unsigned long, std::basic_string<char>, std::basic_string<char>, std::vector<std::basic_string<char>, std::allocator<std::basic_string<char> > > >::Run 	media/mtransport/runnable_utils.h
9 	XUL 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp

Comment 2

[Byron Campen [:bwc]]

I am having no luck reproducing this bug. Do the two ends need to be on different networks? Looking at the stack, I'm not seeing how this can happen unless undefined behavior has somehow been introduced, or the debug symbols are wrong, since we null-check everything being used on that line.

Comment 3

[Byron Campen [:bwc]]

This is being caused by the teardown logic for candidates destroying the "piggyback" srflx candidate before the relay candidate it is piggybacking on. This is an extremely narrow window UAF. Working on a fix.

Comment 4

[Mark Banner (:standard8)]

Attachment: Log with NSPR_LOG_MODULES=signaling:4,mtransport:4

Comment 5

[Mark Banner (:standard8)]

FWIW, I'm working on Mac with both ends of the call on the same machine. Its affecting me 100% of the time that I've tried it so far.

Comment 6

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ada875e0805f

Comment 7

[Byron Campen [:bwc]]

Attachment: Remove unnecessary call to nr_ice_candidate_mark_done

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8669664 [details] [diff] [review]
Remove unnecessary call to nr_ice_candidate_mark_done

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

I've double checked every callsite of nr_ice_candidate_destroy, and the only one that looks like it might be called when the candidate is in INITIALIZING is the one causing this crash. So, I've just removed the call to nr_ice_candidate_mark_done from nr_ice_candidate_destroy.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfb196fd6069

Comment 9

[Byron Campen [:bwc]]

(In reply to Mark Banner (:standard8) from comment #5)
> FWIW, I'm working on Mac with both ends of the call on the same machine. Its
> affecting me 100% of the time that I've tried it so far.

I'm guessing you're far enough from the TURN servers that the relay candidates aren't initialized by the time offer/answer concludes. I'm able to repro about 1 time out of 10 if I'm running on ASAN.

Comment 10

[Mark Banner (:standard8)]

I just tested the patch locally a few times and it seems to fix the issue :-)

Comment 11

[Byron Campen [:bwc]]

Good to hear, although try is not happy. I do know why though, should not be hard to fix.

Comment 12

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fc5acf4f0ce

Comment 13

[Byron Campen [:bwc]]

Attachment: Make absolutely sure the relay->srflx pointer doesn't dangle

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8669873 [details] [diff] [review]
Make absolutely sure the relay->srflx pointer doesn't dangle

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=306ce0f3b9de

Comment 15

[Nils Ohlmeier [:drno]]

Comment on attachment 8669873 [details] [diff] [review]
Make absolutely sure the relay->srflx pointer doesn't dangle

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

LGTM

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 8669873 [details] [diff] [review]
Make absolutely sure the relay->srflx pointer doesn't dangle

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

   It would be pretty hard. The window of opportunity between the free and the UAF is extremely narrow, but since we're using a function pointer on the freed object we have the potential for executing arbitrary code.

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 it does hint strongly that there exists a UAF bug with relay candidates and their piggybacked srflx candidates.

Which older supported branches are affected by this flaw?

   None.

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

   Bug 1208096

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

   Any time we mess with nICEr can be dicey. Fortunately, this only needs to land on m-c.

Comment 17

[Byron Campen [:bwc]]

Can you verify that the current patch (or binary from the try run) works for you?

Comment 18

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

Adding Valentin who can also reproduce this

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The second crash signature [@ nr_ice_media_stream_destroy ] is gone with the Try build, it doesn't crash anymore when initiating a session between two participants.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

(In reply to Byron Campen [:bwc] from comment #16)

> Which older supported branches are affected by this flaw?
> 
>    None.

If this is trunk only, you don't need approval, though I'm happy to give it. 

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 21

[Mark Banner (:standard8)]

The new patch seems to work fine as well - I tested before applying it, got crashes, I then applied it and did a number of runs and it worked fine. I also then unapplied it and got the crashes again.

So this seems fine as far as I can tell.

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Byron Campen [:bwc]]

(In reply to Al Billings [:abillings] from comment #20)
> (In reply to Byron Campen [:bwc] from comment #16)
> 
> > Which older supported branches are affected by this flaw?
> > 
> >    None.
> 
> If this is trunk only, you don't need approval, though I'm happy to give it. 
> 
> https://wiki.mozilla.org/Security/Bug_Approval_Process

   The changeset that causes this bug needs to be uplifted, so this one will need to tag along.

Comment 24

[Daniel Holbert [:dholbert]]

FWIW: I'm hitting a crash with this bug's signature when trying to use BrowserStack [which uses WebRTC for sending a remote view of another browser over the wire].

The crash happens after I've selected a browser for BrowserStack to show me, while it's preparing for a session. Sample crash report: bp-358b3e22-8a18-4e31-83b5-cc5e92151008

I'm hoping the patch that just landed fixes this; I'll double-check after this fix makes it into a Nightly.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #24)
> FWIW: I'm hitting a crash with this bug's signature when trying to use BrowserStack
> [...] I'm hoping the patch that just landed fixes this; I'll double-check

Yup, no longer crashing when logging into BrowserStack, using current Nightly 44.0a1 (2015-10-19). Hooray!