Closed Bug 1151139 Opened 5 years ago Closed 5 years ago

Racy call to PeerConnectionMedia::num_ice_media_streams from PeerConnectionImpl::BuildStatsQuery_m

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox37 --- wontfix
firefox38 + fixed
firefox39 + fixed
firefox40 + fixed
firefox-esr31 38+ fixed
firefox-esr38 38+ fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: bwc, Assigned: bwc)

Details

(Keywords: sec-high, Whiteboard: [adv-main38+][adv-esr31.7+])

Attachments

(4 files, 1 obsolete file)

Marking as security until further analysis can be carried out.

The data structure whose size we're querying in BuildStatsQuery_m is written on STS, meaning we could get literally anything as the return:

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp?from=BuildStatsQuery_m&case=true#2905

We need to encode "all streams" in some other way.
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Comment on attachment 8588240 [details] [diff] [review]
Simplify how we choose which streams to gather stats from

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

I need to give this one more look next week, but no sense in waiting until then to r?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac451fbbdbb0
Attachment #8588240 - Flags: review?(martin.thomson)
Comment on attachment 8588240 [details] [diff] [review]
Simplify how we choose which streams to gather stats from

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

I can't see anything wrong here.  The code duplication is unfortunate, but I can't see a simple way to address that without passing more higher level state to RecordIceStats_s.
Attachment #8588240 - Flags: review?(martin.thomson) → review+
Is there any way this could be responsible for the Android top-crasher?  (I think not, but it's late and I'm too tired to review it for that (and I'm on PTO)).
Flags: needinfo?(docfaraday)
I don't believe so, since that manifested as the report not being set.
Flags: needinfo?(docfaraday)
This is sec-high primarily because of comment 1; this is a potential UAF.
Keywords: sec-high
Comment on attachment 8588240 [details] [diff] [review]
Simplify how we choose which streams to gather stats from

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

   It would probably be pretty hard to actually exploit this bug, although getting the browser to crash might not be too difficult (a well-timed renegotiation with an SDP with lots of m-sections might do the trick).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   No, it just looks like cleanup/simplification.

Which older supported branches are affected by this flaw?

   This flaw would be pretty hard to hit in 37 or lower, easier in 38, and still easier in 39 and 40. Prior supported versions are probably also affected.

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

   I do not have backports at the moment, but they should not be too hard to create, and should be relatively low risk.

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

   There is some risk that it could cause the stats API to break, but this is covered well by mochitests, and I've done some hand-testing as well.
Attachment #8588240 - Flags: sec-approval?
Comment on attachment 8588240 [details] [diff] [review]
Simplify how we choose which streams to gather stats from

sec-approval+.

Should we take this in ESR31?
Attachment #8588240 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #9)
> Comment on attachment 8588240 [details] [diff] [review]
> Simplify how we choose which streams to gather stats from
> 
> sec-approval+.
> 
> Should we take this in ESR31?

   Yeah, probably.
Let's get an ESR31 patch for this, if you would, along with Aurora and Beta.
Comment on attachment 8588240 [details] [diff] [review]
Simplify how we choose which streams to gather stats from

This applies cleanly to aurora, and seems to work fine.

Approval Request Comment
[Feature/regressing bug #]:

   Bug 906990

[User impact if declined]:

   See sec-approval

[Describe test coverage new/current, TreeHerder]:

   Mochitests covers this stuff reasonably well.

[Risks and why]: 

   Fairly low, on functionality that is reasonably well-covered by CI.

[String/UUID change made/needed]:

   None.
Attachment #8588240 - Flags: approval-mozilla-aurora?
Attachment #8588712 - Flags: approval-mozilla-beta?
ESR31 patch is proving to be a little tricky.
Attachment #8588240 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8588712 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
esr31 backport. Needed to disable some mochitest checks, since this change exposes a very minor bug in esr31.
Attachment #8588817 - Flags: approval-mozilla-esr31?
Attachment #8588817 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Can we get this landed on trunk soon please?
Flags: needinfo?(docfaraday)
Landed it on m-i yesterday.
Flags: needinfo?(docfaraday)
What, you expect me to look one comment up from the bottom??!?!
Anyway, my bad :)
https://hg.mozilla.org/mozilla-central/rev/12394290ae58
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
The backports for b2g30, b2g32, and b2g34 are a bit too hairy for me. Mind looking? :)
Flags: needinfo?(docfaraday)
And b2g37.
Sorry, someone else will have to give this a try, since I'm going to be out. The esr31 patch would be a good starting place.
Flags: needinfo?(docfaraday)
Randell or Jan-Ivar, can one of you help here? This needs to be rebased on to all 4 active B2G release branches AFAICT (b2g30/32/34/37).
Flags: needinfo?(rjesup)
Flags: needinfo?(jib)
b2g30 backport, needs testing
Comment on attachment 8594961 [details] [diff] [review]
(b2g30 backport) Simplify how we choose which streams to gather stats from

Testing seems to check out. See other approval requests.
Attachment #8594961 - Flags: review+
Attachment #8594961 - Flags: approval-mozilla-b2g30?
Comment on attachment 8594961 [details] [diff] [review]
(b2g30 backport) Simplify how we choose which streams to gather stats from

sec-high doesn't need approval. Thanks for doing the rebase.
Attachment #8594961 - Flags: approval-mozilla-b2g30?
Attachment #8594961 - Attachment is obsolete: true
Comment on attachment 8596633 [details] [diff] [review]
(b2g30 backport) Simplify how we choose which streams to gather stats from

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

Ok, this should be working now.
Attachment #8596633 - Flags: review+
Attachment #8596633 - Flags: approval-mozilla-b2g30?
Flags: needinfo?(docfaraday)
Attachment #8596633 - Flags: approval-mozilla-b2g30?
Is there a need for manually testing this fix? If yes, could you please provide some detailed steps on how to do it?
Flags: needinfo?(docfaraday)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #39)
> Is there a need for manually testing this fix? If yes, could you please
> provide some detailed steps on how to do it?

I would say no, since it was a really hard race to hit.
Flags: needinfo?(docfaraday)
Whiteboard: [adv-main38+][adv-esr31.7+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.