Prevent SSRC collisions in local tracks

RESOLVED FIXED in Firefox 37

Status

()

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

People

(Reporter: bwc, Assigned: bwc)

Tracking

({sec-high})

Trunk
mozilla39
Points:
---

Firefox Tracking Flags

(firefox36 unaffected, firefox37+ fixed, firefox38+ fixed, firefox39+ fixed, firefox-esr31 unaffected, firefox-esr38 fixed, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage])

Attachments

(2 attachments, 3 obsolete attachments)

No description provided.
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Comment on attachment 8575557 [details] [diff] [review]
Prevent collisions in local SSRCs

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

This should prevent an SSRC from being reused during the lifetime of the JsepSession.
Attachment #8575557 - Flags: review?(ekr)
The namespace for SSRCs includes all SSRCs currently used by either side in the RTP session, not just ones we've generated ourselves.  We should avoid all known (other-side) SSRCs *in use*, however old ones we used an hour ago are reusable (for example), though perhaps it isn't wise or needful to do so (so this is ok on that particular point).  And we have to handle SSRC collision (incoming SSRC with the same as what we're using).
Comment on attachment 8575627 [details] [diff] [review]
Prevent collisions in local SSRCs

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

Prevents generated local SSRCs from colliding with either local or remote SSRCs, and errors out when a remote SSRC collides with a remote one. Checking for duplicates in remote SDP can be a separate bug.
Attachment #8575627 - Flags: review?(ekr)
Comment on attachment 8575627 [details] [diff] [review]
Prevent collisions in local SSRCs

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

I'm not sure this patch is correct. My understanding is that in this setting, each sender is a
single RTP session (See RFC 3550 Section) and so it is not necessary to ensure that sending
and receiving SSRCs do not collide (though it seems nice to do so).

Please confirm with Jesup or MT.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ +1597,5 @@
> +                         << remote.GetLevel() << " collided with a local ssrc");
> +          return NS_ERROR_INVALID_ARG;
> +        }
> +
> +        mRemoteSsrcs.insert(i->ssrc);

Why are you not checking that the remote sources don't collide with each other.
Attachment #8575627 - Flags: review?(ekr)
Ah, I see you suggest checking for remote collisions in a separate bug. Why?
Flags: needinfo?(rjesup)
Jesup: see the question in c8
(In reply to Eric Rescorla (:ekr) from comment #9)
> Ah, I see you suggest checking for remote collisions in a separate bug. Why?

(In reply to Eric Rescorla (:ekr) from comment #8)
> Comment on attachment 8575627 [details] [diff] [review]
> Prevent collisions in local SSRCs
> 
> Review of attachment 8575627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure this patch is correct. My understanding is that in this
> setting, each sender is a
> single RTP session (See RFC 3550 Section) and so it is not necessary to
> ensure that sending
> and receiving SSRCs do not collide (though it seems nice to do so).
> 
> Please confirm with Jesup or MT.
> 
> ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
> @@ +1597,5 @@
> > +                         << remote.GetLevel() << " collided with a local ssrc");
> > +          return NS_ERROR_INVALID_ARG;
> > +        }
> > +
> > +        mRemoteSsrcs.insert(i->ssrc);
> 
> Why are you not checking that the remote sources don't collide with each
> other.

   Because we cannot check that here; we have no way to distinguish between an ssrc going away and then coming back, and a duplicate. The best we can do is check each given remote SDP for duplicates. Also, would this checking be something that belongs in this (security) bug?
I'm confused why this is a sec bug to start...  There was no description of the bug, or why it's a sec bug, just a patch.  Thus I can't evaluate what's up here, nor assign a sec level, nor decide if sec-approval is needed.  (I kinda doubt this is actually a sec bug...)

To ekr's question/NI:
In unicast (which we are) you may be sending one (or more) SSRC's on an RTP session (see RFC 3550 section 8).  Note that all senders in the session share the same SSRC namespace (from either end, so from our perspective, it's all SSRCs we're receiving plus all we're sending).

Note that the rtp_rtcp module in webrtc/trunk/webrtc detects collisions in SetRemoteSSRC() and will take correct actions (send BYE, change SSRC).

To byron's comment:
An SSRC going away should involve an RTCP BYE.  On BYE, we should be able to remove it from the list (or to be really pedantic, remove it after we're sure we won't see a late packet and think it's a new SSRC).  Also, a completed negotiation that removes an SSRC should mean it's gone, and would be available for reuse.  

I don't think we have to do much if anything about far-end errors here, except maybe check for within-the-same-SDP-and-session duplication and flag it.
Flags: needinfo?(rjesup)
Attachment #8575557 - Flags: review?(ekr)
(In reply to Randell Jesup [:jesup] from comment #12)
> I'm confused why this is a sec bug to start...  There was no description of
> the bug, or why it's a sec bug, just a patch.  Thus I can't evaluate what's
> up here, nor assign a sec level, nor decide if sec-approval is needed.  (I
> kinda doubt this is actually a sec bug...)
> 
> To ekr's question/NI:
> In unicast (which we are) you may be sending one (or more) SSRC's on an RTP
> session (see RFC 3550 section 8).  Note that all senders in the session
> share the same SSRC namespace (from either end, so from our perspective,
> it's all SSRCs we're receiving plus all we're sending).
> 
> Note that the rtp_rtcp module in webrtc/trunk/webrtc detects collisions in
> SetRemoteSSRC() and will take correct actions (send BYE, change SSRC).
> 
> To byron's comment:
> An SSRC going away should involve an RTCP BYE.  On BYE, we should be able to
> remove it from the list (or to be really pedantic, remove it after we're
> sure we won't see a late packet and think it's a new SSRC).  Also, a
> completed negotiation that removes an SSRC should mean it's gone, and would
> be available for reuse.  
> 
> I don't think we have to do much if anything about far-end errors here,
> except maybe check for within-the-same-SDP-and-session duplication and flag
> it.

   The sec bug here is that the SSRC is part of the keying material for SRTP. So, renegotiating away an SSRC doesn't make it safe to reuse later, I would guess.
(In reply to Byron Campen [:bwc] from comment #13)
> (In reply to Randell Jesup [:jesup] from comment #12)
> > I'm confused why this is a sec bug to start...  There was no description of
> > the bug, or why it's a sec bug, just a patch.  Thus I can't evaluate what's
> > up here, nor assign a sec level, nor decide if sec-approval is needed.  (I
> > kinda doubt this is actually a sec bug...)
...
>    The sec bug here is that the SSRC is part of the keying material for
> SRTP. So, renegotiating away an SSRC doesn't make it safe to reuse later, I
> would guess.

Ekr?  you're the DTLS-SRTP person here.  Severity, or analysis?
Flags: needinfo?(ekr)
Attachment #8575557 - Attachment is obsolete: true
Attachment #8575627 - Flags: review?(martin.thomson)
Comment on attachment 8575627 [details] [diff] [review]
Prevent collisions in local SSRCs

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

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
@@ +1597,5 @@
> +                         << remote.GetLevel() << " collided with a local ssrc");
> +          return NS_ERROR_INVALID_ARG;
> +        }
> +
> +        mRemoteSsrcs.insert(i->ssrc);

We're OK if we don't even check at all.  We might want to check somewhere else, but I'd dump this section out.  We don't need to worry about collisions between our keys and theirs.  That is, until we do EKT.
Attachment #8575627 - Flags: review?(martin.thomson) → review+
One final request; I would put a note over the mSsrcs declaration that mentions what it protects (i.e., our SSRCs) and what it does not (i.e., EKT).  We do plan to do EKT, and we stand a serious risk of screwing up there if we fail to get that part right.
(In reply to Martin Thomson [:mt] from comment #16)
> One final request; I would put a note over the mSsrcs declaration that
> mentions what it protects (i.e., our SSRCs) and what it does not (i.e.,
> EKT).  We do plan to do EKT, and we stand a serious risk of screwing up
> there if we fail to get that part right.

   Do we have a bug for EKT yet? It would be better if such a comment had that bug number in it.
(In reply to Martin Thomson [:mt] from comment #16)
> One final request; I would put a note over the mSsrcs declaration that
> mentions what it protects (i.e., our SSRCs) and what it does not (i.e.,
> EKT).  We do plan to do EKT, and we stand a serious risk of screwing up
> there if we fail to get that part right.

   Also, how careful do we need to be about painting that kind of bullseye on this problem?
(In reply to Byron Campen [:bwc] from comment #18)
>    Do we have a bug for EKT yet? It would be better if such a comment had
> that bug number in it.

No bugs yet.

>    Also, how careful do we need to be about painting that kind of bullseye
> on this problem?

I'm not concerned about that.  The EKT RFC should already cover these sorts of attacks.
(In reply to Martin Thomson [:mt] from comment #20)
> (In reply to Byron Campen [:bwc] from comment #18)
> >    Also, how careful do we need to be about painting that kind of bullseye
> > on this problem?
> 
> I'm not concerned about that.  The EKT RFC should already cover these sorts
> of attacks.

   I'm mainly talking about someone looking at that comment in this patch, then asking "why does this matter for EKT?", then figuring out the specific vulnerability we're closing here.
Attachment #8578316 - Attachment is obsolete: true
Attachment #8575627 - Attachment is obsolete: true
Comment on attachment 8578322 [details] [diff] [review]
Prevent collisions in local SSRCs

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

Carry forward r=mt

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b7c4d0ea82c
Attachment #8578322 - Flags: review+
Flags: needinfo?(ekr) → needinfo?(docfaraday)
Rank: 15
Priority: -- → P1
Flags: needinfo?(docfaraday)
Keywords: sec-high
Comment on attachment 8578322 [details] [diff] [review]
Prevent collisions in local SSRCs

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

   A great deal of luck would be required, since there would need to be a collision in the randomly generated ssrc. Since the encryption key for DTLS-SRTP is based on ssrc, we could be exposed to a two time pad.

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

   Not really.

Which older supported branches are affected by this flaw?

   37 and up.

If not all supported branches, which bug introduced the flaw?

   Bug 1016476

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

   The backports should be very easy.

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

   Pretty unlikely. The usual unit-testing should suffice.
Attachment #8578322 - Flags: sec-approval?
Comment on attachment 8578322 [details] [diff] [review]
Prevent collisions in local SSRCs

sec-approval+ for trunk.

I'd like to get aurora and beta patches in this week if possible if we can get them made and nominated.
Attachment #8578322 - Flags: sec-approval? → sec-approval+
Comment on attachment 8578322 [details] [diff] [review]
Prevent collisions in local SSRCs

This applies cleanly to aurora, beta patch coming soon.

Approval Request Comment
[Feature/regressing bug #]:

   Bug 1016476

[User impact if declined]:

   See sec-approval comment.

[Describe test coverage new/current, TreeHerder]:

   Odds are extremely low that this bug would happen in CI, and there isn't any way to cause it at will.

[Risks and why]: 

   Very low.

[String/UUID change made/needed]:

   None.
Attachment #8578322 - Flags: approval-mozilla-aurora?
Attachment #8578322 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8579507 [details] [diff] [review]
(beta backport) Prevent collisions in local SSRCs

See aurora request.
Attachment #8579507 - Flags: approval-mozilla-beta?
Attachment #8579507 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Marking checkin-needed for aurora and beta, once the sheriff is satisfied that the central landing is ok.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/35515400af36
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Duplicate of this bug: 793398
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.