null pointer crashes in PeerConnectionMedia::EnsureIceGathering_s

RESOLVED FIXED in Firefox 48

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

({csectype-nullptr, sec-low})

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main48+])

Attachments

(1 attachment, 3 obsolete attachments)

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: nobody → docfaraday
Status: NEW → ASSIGNED
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)
Attachment #8762841 - Attachment is obsolete: true
Attachment #8762841 - Flags: review?(drno)
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+
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.
Fortunately, we won't UAF since the callback handler holds onto a RefPtr<PeerConnectionMedia>.
Attachment #8762844 - Attachment is obsolete: true
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
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+
Attachment #8762877 - Attachment is obsolete: true
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+
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
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?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/50f2e8c8491e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
We shipped several versions with this issue. Why this didn't get a sec approval?
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+
(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+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.