1493347 - DataChannelConnection can be freed on wrong thread

Status: RESOLVED FIXED

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

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

I found this when I pushed a patch for bug 1493045 to Try: DataChannelConnection can be freed on a socket transport thread, which violates thread-safety on its WeakPtr member.  This can potentially cause use-after-free: if a WeakPtr to the same object is being constructed/copied concurrently on the main thread, a lost update to the refcount can cause the WeakReference to be prematurely freed.

Here's an example: https://treeherder.mozilla.org/logviewer.html#?job_id=200909614&repo=try&lineNumber=31344

Unfortunately that log is public; see also bug 1136954.  Hopefully nobody is trawling developers' Try pushes and looking closely at all the failures, but if anyone is then they know about this.

Comment 1

[Nils Ohlmeier [:drno]]

Nico, can I ask you to take a look at this one please?

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Sure thing.

Comment 3

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

From DataChannel.cpp:
  LOG(("Deleting DataChannelConnection %p", (void *) this));
  // This may die on the MainThread, or on the STS thread

Comment 4

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

Attachment: Drop DataChannelListener on Destroy()

Comment 5

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

jld: care to retry with this patch?

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Thanks Jesup, I was testing a patch that would make sure the DataChannelConnection was destructed on the main thread. This is much cleaner.

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

I tested Jesup's patch locally on top of jld's, running the test_dataChannel* and test_peerConnection* mochitests, and the RTCPeerConnection-iceGatheringState.html wpt. The tests that crashed with jld's patch alone, pass with jesup's applied. I did not run the reftest suite.

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 9012686 [details] [diff] [review]
Drop DataChannelListener on Destroy()

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

LGTM

Comment 9

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

Comment on attachment 9012686 [details] [diff] [review]
Drop DataChannelListener on Destroy()

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Perhaps a security risk - and blocks landing WeakPtr safety assertions

Fix Landed on Version:
will land on 64

Risk to taking this patch (and alternatives if risky): 
Minimal

String or UUID changes made by this patch: 
none
See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]:
N/A

[User impact if declined]:
Security risk (perhaps)

[Is this code covered by automated tests?]:
It will be (jld's patch)

[Has the fix been verified in Nightly?]:
Not yet

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
no

[Why is the change risky/not risky?]:
Drops a ref while on the correct thread.

[String changes made/needed]:
none

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very hard, more likely impossible. you'd have to have the connection be destroyed on STS thread (instead of mainthread), while something is modifying the usecount of the weakptr on mainthread - and I don't think that can happen given what owns the listener - .

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.  The comment merely states what's obvious from the code.  

Which older supported branches are affected by this flaw?
All

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

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

Comment 10

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

sec-approval+ for trunk. I'll give beta approval as well.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0ca51d2c0ceb064db11e06782fb415b0ed2641

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/4d0ca51d2c0c

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9012686 [details] [diff] [review]
Drop DataChannelListener on Destroy()

Fixes a sec-high, approved for ESR 60.3.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/20c59797e994