If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 38, Firefox OS v1.4

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

({sec-high})

Trunk
mozilla40
sec-high
Points:
---

Firefox Tracking Flags

(firefox37 wontfix, firefox38+ fixed, firefox39+ fixed, firefox40+ fixed, firefox-esr3138+ fixed, firefox-esr3838+ 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)

Details

(Whiteboard: [adv-main38+][adv-esr31.7+])

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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)

Comment 1

3 years ago
Also, this line is unsafe:

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

Comment 2

3 years ago
Created attachment 8588240 [details] [diff] [review]
Simplify how we choose which streams to gather stats from
(Assignee)

Updated

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

Updated

3 years ago
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox39: --- → affected
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Comment 6

3 years ago
I don't believe so, since that manifested as the report not being set.
Flags: needinfo?(docfaraday)
(Assignee)

Comment 7

3 years ago
This is sec-high primarily because of comment 1; this is a potential UAF.
Keywords: sec-high
(Assignee)

Comment 8

3 years ago
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+
status-firefox37: affected → wontfix
status-firefox-esr31: --- → ?
tracking-firefox38: --- → +
tracking-firefox39: --- → +
tracking-firefox40: --- → +
(Assignee)

Comment 10

3 years ago
(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.
status-firefox-esr31: ? → affected
(Assignee)

Comment 11

3 years ago
Created attachment 8588712 [details] [diff] [review]
(beta backport) Simplify how we choose which streams to gather stats from

Beta backport
Let's get an ESR31 patch for this, if you would, along with Aurora and Beta.
tracking-firefox-esr38: --- → 38+
(Assignee)

Comment 13

3 years ago
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?
(Assignee)

Updated

3 years ago
Attachment #8588712 - Flags: approval-mozilla-beta?
(Assignee)

Comment 14

3 years ago
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+
(Assignee)

Comment 15

3 years ago
Created attachment 8588817 [details] [diff] [review]
(esr31 backport) Simplify how we choose which streams to gather stats from

esr31 backport. Needed to disable some mochitest checks, since this change exposes a very minor bug in esr31.
(Assignee)

Updated

3 years ago
Attachment #8588817 - Flags: approval-mozilla-esr31?
Attachment #8588817 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox-esr31: --- → ?
(Assignee)

Comment 16

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/12394290ae58
tracking-firefox-esr31: ? → 38+
Can we get this landed on trunk soon please?
Flags: needinfo?(docfaraday)
(Assignee)

Comment 18

3 years ago
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
Last Resolved: 3 years ago
status-b2g-master: affected → fixed
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
https://hg.mozilla.org/releases/mozilla-aurora/rev/3034a08beebd
status-firefox39: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/e62ca3da49e1
status-firefox38: affected → fixed
https://hg.mozilla.org/releases/mozilla-esr31/rev/fdf993390ef4
status-firefox-esr31: affected → fixed
status-firefox-esr38: affected → fixed
The backports for b2g30, b2g32, and b2g34 are a bit too hairy for me. Mind looking? :)
Flags: needinfo?(docfaraday)
And b2g37.
(Assignee)

Comment 26

3 years ago
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)
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3f4b9420cc52
status-b2g-v2.2: affected → fixed
Or not:
https://treeherder.mozilla.org/logviewer.html#?job_id=102231&repo=mozilla-b2g37_v2_2

Backed out:
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a648cc800a13
status-b2g-v2.2: fixed → affected
I figured it out. Thanks for all the pointers.
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1e4241dee898
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/c54aa1be51d6
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/c54aa1be51d6
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/bcbd45a97031
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/bcbd45a97031

Byron, you're going to have to take a look at b2g30 and decide if it's really worth uplifting there or not. The esr31 patch mostly-applies, but there's one hunk that was sufficiently different that I didn't want to touch it. Wouldn't break my heart if you decide on a wontfix.
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/file/4fff6bfb4963/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#l1965
status-b2g-v2.0: affected → fixed
status-b2g-v2.0M: affected → fixed
status-b2g-v2.1: affected → fixed
status-b2g-v2.1S: affected → fixed
status-b2g-v2.2: affected → fixed
Flags: needinfo?(rjesup)
Flags: needinfo?(jib)
(Assignee)

Comment 31

3 years ago
Created attachment 8594961 [details] [diff] [review]
(b2g30 backport) Simplify how we choose which streams to gather stats from

b2g30 backport, needs testing
(Assignee)

Comment 32

3 years ago
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?
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/e379411f1404
status-b2g-v1.4: affected → fixed
Backed out for mochitest crashes.
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/25ceaaf19c0a

https://treeherder.mozilla.org/logviewer.html#?job_id=131482&repo=mozilla-b2g30_v1_4
status-b2g-v1.4: fixed → affected
Flags: needinfo?(docfaraday)
(Assignee)

Comment 36

3 years ago
Created attachment 8596633 [details] [diff] [review]
(b2g30 backport) Simplify how we choose which streams to gather stats from

Fix crash
(Assignee)

Updated

3 years ago
Attachment #8594961 - Attachment is obsolete: true
(Assignee)

Comment 37

3 years ago
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?
(Assignee)

Updated

3 years ago
Flags: needinfo?(docfaraday)
Attachment #8596633 - Flags: approval-mozilla-b2g30?
Thanks :)

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1d41d15f4b9f
status-b2g-v1.4: affected → fixed
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)
(Assignee)

Comment 40

2 years ago
(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+]

Updated

2 years ago
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.