1240760 - (CVE-2016-1962) Second datachannel with id crashes in PR_Unlock | mozilla::DataChannelConnection::Close after navigation

Status: VERIFIED FIXED

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

Description

[Dominique Hazaël-Massieux]

Attachment: ff-crash-datachannel.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20160106231438

Steps to reproduce:

Load the attached page


Actual results:

The page loads, redirects to mozilla.org, and after ~20 seconds, Firefox crashes.

This is reproducible with any other navigation action, including a simple reload.

from my quick tests, it only happens when 2 data channels are open, with the 2nd set to use the same id as the one the first one automatically gets; it's probably somewhat related to https://bugzilla.mozilla.org/show_bug.cgi?id=1017462


Expected results:

Firefox should not have crashed

Comment 1

[Dominique Hazaël-Massieux]

Crash log at https://crash-stats.mozilla.com/report/index/db9fd3fa-dd6e-4575-8793-630df2160119

Comment 2

[Loic]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d3a2e391df48f1c9389bdc132eb72065442dc2db&tochange=9a44575026505c955bdda462d6ad346d197ab107

I guess the crash was already present and surfaced after bug 1205249 which has disabled jemalloc.

Comment 3

[Nils Ohlmeier [:drno]]

Forgot to mention that I actually tried to reproduce the problem with the provided test page. But it looks like because of the timing of adding the ICE candidates (before calling setRemoteDescription) on my machine ICE fails to establish any connection. So I was not able to reproduce the problem.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 46+ since this sounds like a reproducible crash.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

I see only two crashes over the last 28 days. Not sure I care enough to track it.

Comment 6

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

Attachment: MozReview Request: Bug 1240760: keep a grip on the owning DataChannelConnection DataChannel::Close() r?mcmanus

Hold onto mConnection since inside Close() it may call through the mConnection and
then null it out inside.

Review commit: https://reviewboard.mozilla.org/r/35499/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35499/

Comment 7

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

Note: should be trivially upliftable

Comment 8

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

Note: this is a UAF crash - we're freeing the DataChannelConnection from within a call through it.  Minimum sec-high, but it's a UAF-on-GC basically, so that seems worth sec-crit

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 8720898 [details]
MozReview Request: Bug 1240760: keep a grip on the owning DataChannelConnection DataChannel::Close() r?mcmanus

https://reviewboard.mozilla.org/r/35499/#review32155

Comment 10

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

Attachment: keep a grip on the owning DataChannelConnection DataChannel::Close()

mcmanus r+'d the mozreview version; re-uploading for approvals

Comment 11

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

Comment on attachment 8720910 [details] [diff] [review]
keep a grip on the owning DataChannelConnection DataChannel::Close()

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not easily, though may point to a possible UAF, especially the comment

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Somewhat (the comment).  Someone who knows gecko may intuit that adding a ref like this implies possible UAF.

Which older supported branches are affected by this flaw? All

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Trivial, should apply with little or no change

How likely is this patch to cause regressions; how much testing does it need?  Very unlikely to regress; holds a ref for a little longer.

Comment 12

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

Comment on attachment 8720910 [details] [diff] [review]
keep a grip on the owning DataChannelConnection DataChannel::Close()

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: 47 (will land)

Risk to taking this patch (and alternatives if risky): Very low risk

String or UUID changes made by this patch:  none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 13

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> I see only two crashes over the last 28 days. Not sure I care enough to
> track it.

Because it is a sec-critical rated security issue. Please reconsider your minusing of it everywhere.

sec-approval+.

Comment 14

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

(In reply to Al Billings [:abillings] from comment #13)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> > I see only two crashes over the last 28 days. Not sure I care enough to
> > track it.
> 
> Because it is a sec-critical rated security issue. Please reconsider your
> minusing of it everywhere.
> 
> sec-approval+.

I'll note he minused it before I made it a sec issue (I noted the crash address was a 5e5e...)

Comment 15

[Daniel Veditz [:dveditz]]

This was not a security bug until after it was tracking-minused

Comment 16

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

Attachment: Update DataChannel::Close()

Version of patch with comment removed and simplified log comment for checkin

Comment 17

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

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

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8720910 [details] [diff] [review]
keep a grip on the owning DataChannelConnection DataChannel::Close()

Sec-crit issue, taking it in beta, aurora, esr38.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0582397dafe3

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f84789c8b08

Comment 22

[Sylvestre Ledru [:Sylvestre]]

(In reply to Al Billings [:abillings] from comment #13) 
> Because it is a sec-critical rated security issue. Please reconsider your
> minusing of it everywhere.
Is it just for tracking and as Randell mentioned, it wasn't a security issue at this time.
I would have been happy to take the uplift (and happy that Ritu did it)

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Carsten, Wes: Could you please land this on esr38 branch too? Thanks!

Comment 24

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

Looks like it was already done, just not marked:
https://hg.mozilla.org/releases/mozilla-esr38/rev/221de852fda3

Comment 27

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this crash on Firefox 46.0a1 (2016-01-19) using Windows 10 64-bit.

Verified fixed on Firefox 47.0a1 (2016-02-29), Firefox 46.0a2 (2016-02-29), Firefox 45 beta 10  	(20160225145837), Firefox  45 ESR tinderbox-build (20160229180409) and Firefox 38.6.1esrpre tinderbox-build (20160229210832) under Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11.