Closed Bug 1493347 Opened 11 months ago Closed 11 months ago

DataChannelConnection can be freed on wrong thread

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ fixed
firefox62 --- wontfix
firefox63 + fixed
firefox64 + fixed

People

(Reporter: jld, Assigned: jesup)

Details

(Keywords: csectype-race, sec-high, Whiteboard: [adv-main63+][adv-esr60.3+])

Attachments

(1 file)

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.
Group: core-security → network-core-security
Component: Networking → WebRTC: Networking
Priority: -- → P2
Assignee: nobody → na-g
Nico, can I ask you to take a look at this one please?
Flags: needinfo?(na-g)
Sure thing.
From DataChannel.cpp:
  LOG(("Deleting DataChannelConnection %p", (void *) this));
  // This may die on the MainThread, or on the STS thread
Assignee: na-g → rjesup
Status: NEW → ASSIGNED
jld: care to retry with this patch?
Flags: needinfo?(jld)
Thanks Jesup, I was testing a patch that would make sure the DataChannelConnection was destructed on the main thread. This is much cleaner.
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.
Flags: needinfo?(na-g)
Comment on attachment 9012686 [details] [diff] [review]
Drop DataChannelListener on Destroy()

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

LGTM
Attachment #9012686 - Flags: review?(drno) → review+
Flags: needinfo?(jld)
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.
Attachment #9012686 - Flags: sec-approval?
Attachment #9012686 - Flags: approval-mozilla-esr60?
Attachment #9012686 - Flags: approval-mozilla-beta?
sec-approval+ for trunk. I'll give beta approval as well.
Attachment #9012686 - Flags: sec-approval?
Attachment #9012686 - Flags: sec-approval+
Attachment #9012686 - Flags: approval-mozilla-beta?
Attachment #9012686 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/4d0ca51d2c0c
Group: network-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9012686 [details] [diff] [review]
Drop DataChannelListener on Destroy()

Fixes a sec-high, approved for ESR 60.3.
Attachment #9012686 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [adv-main63+][adv-esr60.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.