1072044 - Several signals from PCMedia to PCImpl are unsafe

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Byron Campen [:bwc]]

None of the following signal handlers are torn down in a safe way right now:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#614

The PC is (usually, it is racy) destroyed before the PCMedia, which still has these slots connected when the PC dies. If anything from the ICE layer happens between the PCImpl dying, and the PCMedia dying, the signal results in a UAF.

Comment 1

[Byron Campen [:bwc]]

We could also have a data race on the slots data structure itself here, I suppose.

Comment 2

[Byron Campen [:bwc]]

I can think of a few ways to fix this:

1) Locking in PCMedia. Not attractive.
2) Enable locking in sigslot (see bug 932570). Slightly better, but still locking.
3) Have some sort of delegate object that holds a weak ref to PC and handles the dispatches.

Comment 3

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

Also, we can enable multithread mode in sigslot (which also locks IIRC); bug 782657.  Is locking once on an ice candidate a problem here?  Safety/perf tradeoff?

Comment 4

[Eric Rescorla (:ekr)]

Last time we turned on multithread, we got deadlocks, IIRC. That
might be better with the big SDP refactor.

Byron, what about having the transition from the STS thread to the
main thread be in PCMedia and then doing a detach in PCImpl just
before we do push the self destruct on the STS thread.

Comment 5

[Byron Campen [:bwc]]

Having the signals fire on main would solve that problem, but we'd probably need to rewrite some test code, and allow the thread on which these signals fire to be configured (for tests that have a fake main thread).

Comment 6

[Byron Campen [:bwc]]

Attachment: Fire ICE signals on main.

Initial cut.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8494669 [details] [diff] [review]
Fire ICE signals on main.

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

https://tbpl.mozilla.org/?tree=Try&rev=8a0fdf30be2f

Comment 8

[Martin Thomson [:mt:]]

Comment on attachment 8494669 [details] [diff] [review]
Fire ICE signals on main.

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

I'd like to see a few more thread asserts scattered around here.  Not because this is needed to ensure integrity, but because that's a far better way to document thread invariants than comments.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +590,5 @@
>  }
>  
>  
>  void
>  PeerConnectionMedia::IceGatheringStateChange(NrIceCtx* ctx,

_s

@@ +593,5 @@
>  void
>  PeerConnectionMedia::IceGatheringStateChange(NrIceCtx* ctx,
>                                               NrIceCtx::GatheringState state)
>  {
> +  // We will still be around because we have not started teardown yet

// We will still be around to service this call because teardown completes on the main thread based on a dispatch from this thread (i.e., STS).  If we're here, this runs ahead of SelfDestruct_m.

@@ +604,4 @@
>  }
>  
>  void
>  PeerConnectionMedia::IceConnectionStateChange(NrIceCtx* ctx,

_s

Comment 9

[Byron Campen [:bwc]]

Attachment: Fire ICE signals on main.

Incorporate feedback.

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8498272 [details] [diff] [review]
Fire ICE signals on main.

I will probably have to cook up backports of this patch, more in a bit.

Approval Request Comment
[Feature/regressing bug #]:

   There have been problems with these dispatches in one form or another since the initial webrtc landing, but bugs 935723 and 991037 have made it much easier to hit these bugs.

[User impact if declined]:

   Rare crashes on end of call, which can possibly be induced by content with no permission prompts.

[Describe test coverage new/current, TBPL]:

   None for this specific scenario, and the crash is rare enough that it is actually fairly difficult to hit.

[Risks and why]: 

   This patch alters some cross-thread dispatch logic slightly, and so there is a small risk that it introduces a new problem.

[String/UUID change made/needed]:

   None.

Comment 11

[Byron Campen [:bwc]]

Attachment: Fire ICE signals on main. (aurora backport)

Aurora backport.

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8498272 [details] [diff] [review]
Fire ICE signals on main.

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

   I've tried writing a crashtest that hits this bug, and it is pretty difficult. You have to time the destruction of the PCImpl (which is controlled by the garbage collector) to happen at the same time as some sort of ICE event, and you cannot just try to saturate with lots of ICE events by supplying lots of STUN servers, since each dispatch acquires a RefPtr to the PCImpl, preventing it from being destroyed until the event has been processed. An ICE event has to happen after ~PeerConnectionImpl starts, and before the PeerConnectionMedia starts tearing down on STS, which is a fairly narrow window.

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

   Not really, it looks like a minor refactor/threading simplification.

Which older supported branches are affected by this flaw?

   There has been a lot of shakeup around these signals since 32, but a similar flaw has existed in at least one ICE signal since webrtc initially landed.

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

   I do for aurora and beta, they weren't terribly difficult/risky.

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

   Not likely, the usual CI testing, plus some hand-running of unit-tests that don't run on CI, should be fine.

Comment 13

[Byron Campen [:bwc]]

Attachment: Fire ICE signals on main.

Beta backport.

Comment 14

[Byron Campen [:bwc]]

Attachment: Fire ICE signals on main. (beta backport)

Beta backport.

Comment 15

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

Comment on attachment 8498272 [details] [diff] [review]
Fire ICE signals on main.

Approve all things!

Please get this in today down to Beta, if possible, as last Beta build is tomorrow before release.

Also, do we need this on ESR31?

Comment 16

[Byron Campen [:bwc]]

Just waiting for the trees to open to land on inbound. Am I supposed to land aurora/beta, or is someone else supposed to take that?

Comment 17

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3103826177af

Comment 18

[Byron Campen [:bwc]]

I guess I'll land the backports later tonight.

Comment 19

[Byron Campen [:bwc]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0aee9433884

Comment 20

[Byron Campen [:bwc]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e97e1abb8395

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Does this affect esr31? What about b2g32 (v2.0)?

Comment 22

[Byron Campen [:bwc]]

Probably yes on both. I need to research what it will take to backport those.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3103826177af

Comment 24

[Byron Campen [:bwc]]

Attachment: Fire ICE signals on main. (b2g32 backport)

b2g32 backport

Comment 25

[Byron Campen [:bwc]]

Attachment: Fire ICE signals on main. (esr31 backport)

esr31 backport

Comment 26

[Byron Campen [:bwc]]

Comment on attachment 8499057 [details] [diff] [review]
Fire ICE signals on main. (b2g32 backport)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
See prior approval.

Comment 27

[Byron Campen [:bwc]]

Comment on attachment 8499058 [details] [diff] [review]
Fire ICE signals on main. (esr31 backport)

[Approval Request Comment]
See prior approval request.

Comment 28

[Byron Campen [:bwc]]

Since there's no tearing hurry on these, I'll just wait for the sheriff to push them.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/786c5942c99f
https://hg.mozilla.org/releases/mozilla-esr31/rev/dbcb72bb6615

I don't recall what parts of WebRTC are enabled for v1.4 (b2g30). I don't think this needs uplifting there, though?

Comment 30

[Byron Campen [:bwc]]

If it is enabled at all, we may need to uplift there as well. I don't recall when we enabled webrtc for b2g. jesup?

Comment 31

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

Audio-only webrtc was enabled for 1.3, and IIRC in theory video was in 1.4 but was largely unsupported.  But yes, 1.4 has PeerConnections

Comment 32

[Byron Campen [:bwc]]

I'll look into another backport tomorrow.

Comment 33

[Maire Reavy [:mreavy]]

WebRTC is all there for v1.4 (except for H.264 H/W support), but it's not a supported release.  v2.0 is where it's supported, and we are telling everyone to use v2.0 for WebRTC.

Byron -- If the backport for v1.4 starts to get complicated or time consuming, please ping me.  Thanks.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/786c5942c99f

Comment 35

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qe-verify] based on comment 12.

Comment 36

[Byron Campen [:bwc]]

Attachment: Fire ICE signals on main. (b2g30 backport)

b2g30 backport

Comment 37

[Byron Campen [:bwc]]

Comment on attachment 8502883 [details] [diff] [review]
Fire ICE signals on main. (b2g30 backport)

See prior approval requests.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8502883 [details] [diff] [review]
Fire ICE signals on main. (b2g30 backport)

sec-high doesn't need approval

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/2bb390891df9