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

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Byron Campen [:bwc]]

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.

Comment 1

[Byron Campen [:bwc]]

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

Comment 2

[Byron Campen [:bwc]]

Attachment: Simplify how we choose which streams to gather stats from

Comment 3

[Byron Campen [:bwc]]

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

Comment 4

[Martin Thomson [:mt:]]

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.

Comment 5

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

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)).

Comment 6

[Byron Campen [:bwc]]

I don't believe so, since that manifested as the report not being set.

Comment 7

[Byron Campen [:bwc]]

This is sec-high primarily because of comment 1; this is a potential UAF.

Comment 8

[Byron Campen [:bwc]]

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.

Comment 9

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

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?

Comment 10

[Byron Campen [:bwc]]

(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.

Comment 11

[Byron Campen [:bwc]]

Attachment: (beta backport) Simplify how we choose which streams to gather stats from

Beta backport

Comment 12

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

Let's get an ESR31 patch for this, if you would, along with Aurora and Beta.

Comment 13

[Byron Campen [:bwc]]

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.

Comment 14

[Byron Campen [:bwc]]

ESR31 patch is proving to be a little tricky.

Comment 15

[Byron Campen [:bwc]]

Attachment: (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.

Comment 16

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12394290ae58

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Can we get this landed on trunk soon please?

Comment 18

[Byron Campen [:bwc]]

Landed it on m-i yesterday.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

What, you expect me to look one comment up from the bottom??!?!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Anyway, my bad :)
https://hg.mozilla.org/mozilla-central/rev/12394290ae58

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3034a08beebd

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e62ca3da49e1

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/fdf993390ef4

Comment 24

[Ryan VanderMeulen [:RyanVM]]

The backports for b2g30, b2g32, and b2g34 are a bit too hairy for me. Mind looking? :)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

And b2g37.

Comment 26

[Byron Campen [:bwc]]

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.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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).

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3f4b9420cc52

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Byron Campen [:bwc]]

Attachment: (b2g30 backport) Simplify how we choose which streams to gather stats from

b2g30 backport, needs testing

Comment 32

[Byron Campen [:bwc]]

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.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/e379411f1404

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Byron Campen [:bwc]]

Attachment: (b2g30 backport) Simplify how we choose which streams to gather stats from

Fix crash

Comment 37

[Byron Campen [:bwc]]

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.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Thanks :)

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1d41d15f4b9f

Comment 39

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Is there a need for manually testing this fix? If yes, could you please provide some detailed steps on how to do it?

Comment 40

[Byron Campen [:bwc]]

(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.