null pointer crashes in PeerConnectionMedia::EnsureIceGathering_s

RESOLVED FIXED in Firefox 48

Status

()

Core
WebRTC: Networking
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

({csectype-nullptr, sec-low})

unspecified
mozilla50
csectype-nullptr, sec-low
Points:
---

Firefox Tracking Flags

(firefox47 wontfix, firefox48 fixed, firefox49 fixed, firefox50 fixed)

Details

(Whiteboard: [adv-main48+])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

2 years ago
From crash-stats:

https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3APeerConnectionMedia%3A%3AEnsureIceGathering_s&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#reports

This suggests to me that the code that cancels the proxy lookup when the PeerConnectionMedia begins tearing down isn't working reliably. We don't seem to have any asserts that would catch this.

Marking as security for now, since a flaw in the proxy lookup cancellation code could imply security bugs elsewhere, and could maybe cause a dangling pointer to a PCM to be used (which is not what is happening here).
(Assignee)

Comment 1

2 years ago
Created attachment 8762841 [details] [diff] [review]
Assert that PCM is not being torn down in SetProxyOnPcm
(Assignee)

Updated

2 years ago
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Comment on attachment 8762841 [details] [diff] [review]
Assert that PCM is not being torn down in SetProxyOnPcm

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

This is a diagnostic patch. Since this is a fairly rare crash, it may be some time before we have confirmation that this is indeed the problem. If this is the problem, we'll have a fun bug to fix...
Attachment #8762841 - Flags: review?(drno)
(Assignee)

Comment 3

2 years ago
Created attachment 8762844 [details] [diff] [review]
Assert that PCM is not being torn down in OnProxyAvailable
(Assignee)

Updated

2 years ago
Attachment #8762841 - Attachment is obsolete: true
Attachment #8762841 - Flags: review?(drno)
(Assignee)

Updated

2 years ago
Attachment #8762844 - Flags: review?(drno)
Comment on attachment 8762844 [details] [diff] [review]
Assert that PCM is not being torn down in OnProxyAvailable

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

LGTM
Attachment #8762844 - Flags: review?(drno) → review+
(Assignee)

Comment 6

2 years ago
Welp, upon reading the code in nsAsyncResolveRequest, calling Cancel(NS_ERROR_ABORT) is no guarantee that the callback will come back with NS_ERROR_ABORT. So, we need to check something on the PCM to see whether it wants a callback anymore.
(Assignee)

Comment 7

2 years ago
Fortunately, we won't UAF since the callback handler holds onto a RefPtr<PeerConnectionMedia>.
(Assignee)

Comment 8

2 years ago
Created attachment 8762877 [details] [diff] [review]
Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested
(Assignee)

Updated

2 years ago
Attachment #8762844 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
(Assignee)

Updated

2 years ago
Attachment #8762877 - Flags: review?(drno)
Comment on attachment 8762877 [details] [diff] [review]
Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

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

Might be worth addressing my one suggestion below. But otherwise LGTM.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +210,2 @@
>      return NS_OK;
>    }

Looking at nsHttpChannel and nsFtpConnectionThread we should probably add here a reset like this
  pcm_->mProxyRequest = nullptr;
here to be on the safe side.
Attachment #8762877 - Flags: review?(drno) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8764310 [details] [diff] [review]
Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

MozReview-Commit-ID: AIZm4VNZJtV
(Assignee)

Updated

2 years ago
Attachment #8762877 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Comment on attachment 8764310 [details] [diff] [review]
Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08f4bc17a195
Attachment #8764310 - Flags: review+
(Assignee)

Comment 13

2 years ago
Can someone mark this as non-security sensitive?
Summary: (null pointer?) crashes in PeerConnectionMedia::EnsureIceGathering_s → null pointer crashes in PeerConnectionMedia::EnsureIceGathering_s
Group: core-security → media-core-security
(Assignee)

Comment 14

2 years ago
Comment on attachment 8764310 [details] [diff] [review]
Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

Approval Request Comment
[Feature/regressing bug #]:

   Bug 949703

[User impact if declined]:

   Occasional null pointer crashes when a peerconnection is closed soon after creation.

[Describe test coverage new/current, TreeHerder]:

   We have some tests that destroy peerconnections soon after they're created, but the timing is sensitive enough that this would be really hard to reproduce.

[Risks and why]: 

   This is a very safe patch, I don't see any risk.

[String/UUID change made/needed]:

   None.
Attachment #8764310 - Flags: approval-mozilla-beta?
Attachment #8764310 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/50f2e8c8491e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
We shipped several versions with this issue. Why this didn't get a sec approval?
status-firefox47: --- → wontfix
status-firefox48: --- → affected
status-firefox49: --- → affected
Comment on attachment 8764310 [details] [diff] [review]
Stop using the nsresult in OnProxyAvailable to determine whether the PCM is still interested

As it landed already in m-c, taking it in the other branches.
Attachment #8764310 - Flags: approval-mozilla-beta?
Attachment #8764310 - Flags: approval-mozilla-beta+
Attachment #8764310 - Flags: approval-mozilla-aurora?
Attachment #8764310 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 21

2 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> We shipped several versions with this issue. Why this didn't get a sec
> approval?

Because it is just a null ptr crash. (see comment 13)
Group: media-core-security → core-security-release
Whiteboard: [adv-main48+]

Updated

2 years ago
Keywords: csectype-nullptr, sec-low
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.