Several signals from PCMedia to PCImpl are unsafe

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

({sec-high})

Trunk
mozilla35
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox32 wontfix, firefox33+ fixed, firefox34+ fixed, firefox35+ fixed, firefox-esr3133+ fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main33+][adv-esr31.2+])

Attachments

(6 attachments, 2 obsolete attachments)

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.
We could also have a data race on the slots data structure itself here, I suppose.
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.
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?
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.
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).
Posted patch Fire ICE signals on main. (obsolete) — Splinter Review
Initial cut.
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
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
Attachment #8494669 - Flags: review?(ekr)
Keywords: sec-high
Attachment #8494669 - Flags: review?(ekr) → review?(martin.thomson)
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
Attachment #8494669 - Flags: review?(martin.thomson) → review+
Incorporate feedback.
Attachment #8494669 - Attachment is obsolete: true
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.
Attachment #8498272 - Flags: approval-mozilla-beta?
Attachment #8498272 - Flags: approval-mozilla-aurora?
Aurora backport.
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.
Attachment #8498272 - Flags: sec-approval?
Posted patch Fire ICE signals on main. (obsolete) — Splinter Review
Beta backport.
Beta backport.
Attachment #8498460 - Attachment is obsolete: true
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?
Attachment #8498272 - Flags: sec-approval?
Attachment #8498272 - Flags: sec-approval+
Attachment #8498272 - Flags: approval-mozilla-beta?
Attachment #8498272 - Flags: approval-mozilla-beta+
Attachment #8498272 - Flags: approval-mozilla-aurora?
Attachment #8498272 - Flags: approval-mozilla-aurora+
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?
I guess I'll land the backports later tonight.
Does this affect esr31? What about b2g32 (v2.0)?
status-b2g-v2.0: --- → ?
Flags: needinfo?(docfaraday)
Probably yes on both. I need to research what it will take to backport those.
Flags: needinfo?(docfaraday)
https://hg.mozilla.org/mozilla-central/rev/3103826177af
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
b2g32 backport
esr31 backport
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.
Attachment #8499057 - Flags: approval-mozilla-b2g32?
Comment on attachment 8499058 [details] [diff] [review]
Fire ICE signals on main. (esr31 backport)

[Approval Request Comment]
See prior approval request.
Attachment #8499058 - Flags: approval-mozilla-esr31?
Attachment #8499057 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Attachment #8499058 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Since there's no tearing hurry on these, I'll just wait for the sheriff to push them.
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?
Flags: needinfo?(docfaraday)
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?
Flags: needinfo?(rjesup)
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
Flags: needinfo?(rjesup)
I'll look into another backport tomorrow.
Flags: needinfo?(docfaraday)
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.
Whiteboard: [adv-main33+][adv-esr31.2+]
Marking [qe-verify] based on comment 12.
Flags: qe-verify-
b2g30 backport
Comment on attachment 8502883 [details] [diff] [review]
Fire ICE signals on main. (b2g30 backport)

See prior approval requests.
Attachment #8502883 - Flags: approval-mozilla-b2g30?
Comment on attachment 8502883 [details] [diff] [review]
Fire ICE signals on main. (b2g30 backport)

sec-high doesn't need approval
Attachment #8502883 - Flags: approval-mozilla-b2g30?
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.