906384 - ICE pacing needs to be global

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

Martin Thomson pointed out to me that the Firefox ICE implementation doesn't do global
pacing. This means that an attacker can cause Firefox to generate a lot of attack traffic to a victim by making a lot of peerconnections with a lot of ICE candidates pointing to the victim.

We need to make ICE pacing Firefox-global, thus limiting the total amount of
traffic Firefox generates in total.

Comment 1

[Martin Thomson [:mt:]]

Attachment: draft on algorithm changes

I've written a draft on the subject, text version attached.  See also http://martinthomson.github.io/drafts/draft-thomson-mmusic-ice-webrtc.html

Comment 2

[Timothy B. Terriberry (:derf)]

(In reply to Martin Thomson from comment #1)
> http://martinthomson.github.io/drafts/draft-thomson-mmusic-ice-webrtc.html

404

Comment 3

[Eric Rescorla (:ekr)]

I asked Martin to remove it, since this is a security bug.

Comment 4

[Eric Rescorla (:ekr)]

The fix for this issue will make the issue pretty obvious, and to complicate matters, there is a similar issue in Chrome. Who at Mozilla would be the best person to coordinate with Chrome security on coordinated fixing?

Comment 5

[Eric Rescorla (:ekr)]

Notes from today's call with Justin and Martin. Instead of doing global pacing, the proposal is to instead put a global "circuit breaker" cap on the maximum amount of STUN traffic you can generate and then (as needed) adjust pacing to the point where legitimate applications should stay under the cap.

Comment 6

[Andrew McCreight [:mccr8]]

Dan probably has a better idea than me.

Comment 7

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

-> ekr for now; if implementation needs to go elsewhere we'll reassign

Comment 8

[Maire Reavy [:mreavy]]

We'd like to resolve this in the code within the next 2 weeks.  Adding an internal tracking deadline of Sept 6.

Comment 9

[Eric Rescorla (:ekr)]

The problem isn't implementing it, it's when we can commit it without implicitly disclosing a problem in both ffox and Chrome.

Comment 10

[Maire Reavy [:mreavy]]

(In reply to Eric Rescorla (:ekr) from comment #9)
> The problem isn't implementing it, it's when we can commit it without
> implicitly disclosing a problem in both ffox and Chrome.

I understand. I'm just asking everyone (everyone who needs to help make this happen) if we can target resolving this issue within the next 2 weeks.  I realize there are lots of dependencies.

Comment 11

[Maire Reavy [:mreavy]]

> 
> I understand. I'm just asking everyone (everyone who needs to help make this
> happen) if we can target resolving this issue within the next 2 weeks.  I
> realize there are lots of dependencies.

I also realize that it won't be truly "resolved" until we release updates to the field.  So perhaps the better and more precise question to ask everyone is: can we target having a release plan with Chrome to resolve this issue within 2 weeks?

Comment 12

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

Per discussion with dveditz, sec-low

Comment 13

[Daniel Veditz [:dveditz]]

(In reply to Eric Rescorla (:ekr) from comment #9)
> The problem isn't implementing it, it's when we can commit it without
> implicitly disclosing a problem in both ffox and Chrome.

When asked about landing this the Google security team said "Please go right ahead. We never get very excited about DOS class issues."

Comment 14

[Byron Campen [:bwc]]

Attachment: (WIP) Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 1.8K/sec over 20 sec.

Strawman patch.

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 827067 [details] [diff] [review]
(WIP) Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 1.8K/sec over 20 sec.

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

Is this roughly what we're looking for?

Comment 16

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=5f5888d855b7

Comment 17

[Byron Campen [:bwc]]

Attachment: (WIP) Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 1.8K/sec over 20 sec.

Add a comment that more clearly explains why the rate limiting is needed.

Comment 18

[Eric Rescorla (:ekr)]

Comment on attachment 827486 [details] [diff] [review]
(WIP) Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 1.8K/sec over 20 sec.

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

::: media/mtransport/nr_socket_prsock.cpp
@@ +395,5 @@
> +    // (see http://tools.ietf.org/html/draft-thomson-mmusic-ice-webrtc)
> +    // Tolerate rate of 8k/sec, for one second.
> +    static SimpleTokenBucket burst(8192*1, 8192);
> +    // Tolerate rate of 1.8k/sec over twenty seconds.
> +    static SimpleTokenBucket sustained(1843*20, 1843);

I think Martin may have changed his recommendation here to just be 100kbps overall.

Martin?

@@ +399,5 @@
> +    static SimpleTokenBucket sustained(1843*20, 1843);
> +    if (burst.getTokens(UINT32_MAX) < len ||
> +        sustained.getTokens(UINT32_MAX) < len) {
> +      // TODO([e-mail]): Better error code for this?
> +      ABORT(R_NOT_PERMITTED);

IIRC, R_NOT_PERMITTED causes an error higher up.

I recommend R_WOULDBLOCK or a silent drop.

@@ +403,5 @@
> +      ABORT(R_NOT_PERMITTED);
> +    }
> +
> +    burst.getTokens(len);
> +    sustained.getTokens(len);

You should comment this so it's clear what's going on.

Also, a note that this isn't thread safe would probably be useful.

Comment 19

[Martin Thomson [:mt:]]

Byron has used the numbers that I recommended in the draft. However, I've talked to some people and a higher limit is probably better for the longer term limit to prevent some valid cases with multiple agents from bumping the cap. Doubling the rate for the slower token bucket would probably be ok.

Comment 20

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #18)
> Comment on attachment 827486 [details] [diff] [review]
> (WIP) Very simple global rate-limiting based on SimpleTokenBucket. Will
> tolerate a maximum of 8K/sec over 1 sec, and 1.8K/sec over 20 sec.
> 
> Review of attachment 827486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +399,5 @@
> > +    static SimpleTokenBucket sustained(1843*20, 1843);
> > +    if (burst.getTokens(UINT32_MAX) < len ||
> > +        sustained.getTokens(UINT32_MAX) < len) {
> > +      // TODO([e-mail]): Better error code for this?
> > +      ABORT(R_NOT_PERMITTED);
> 
> IIRC, R_NOT_PERMITTED causes an error higher up.
> 
> I recommend R_WOULDBLOCK or a silent drop.
> 

   I was worried that at some point R_WOULDBLOCK would prompt a retry. I guess it wouldn't be too awful if that happened.

> @@ +403,5 @@
> > +      ABORT(R_NOT_PERMITTED);
> > +    }
> > +
> > +    burst.getTokens(len);
> > +    sustained.getTokens(len);
> 
> You should comment this so it's clear what's going on.
> 
> Also, a note that this isn't thread safe would probably be useful.

Ok.

Comment 21

[Byron Campen [:bwc]]

(In reply to Martin Thomson from comment #19)
> Byron has used the numbers that I recommended in the draft. However, I've
> talked to some people and a higher limit is probably better for the longer
> term limit to prevent some valid cases with multiple agents from bumping the
> cap. Doubling the rate for the slower token bucket would probably be ok.

I'll do that.

Comment 22

[Byron Campen [:bwc]]

Attachment: Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

Incorporate feedback.

Comment 23

[Eric Rescorla (:ekr)]

Comment on attachment 831077 [details] [diff] [review]
Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

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

lgtm

::: media/mtransport/nr_socket_prsock.cpp
@@ +401,5 @@
> +
> +    // Check number of tokens in each bucket.
> +    if (burst.getTokens(UINT32_MAX) < len ||
> +        sustained.getTokens(UINT32_MAX) < len) {
> +      ABORT(R_WOULDBLOCK);

Suggest adding a high level (ERR) log here and an assert. That way if this happens
in the field we will know earlier.

Comment 24

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #23)
> Comment on attachment 831077 [details] [diff] [review]
> Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a
> maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.
> 
> Review of attachment 831077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm
> 
> ::: media/mtransport/nr_socket_prsock.cpp
> @@ +401,5 @@
> > +
> > +    // Check number of tokens in each bucket.
> > +    if (burst.getTokens(UINT32_MAX) < len ||
> > +        sustained.getTokens(UINT32_MAX) < len) {
> > +      ABORT(R_WOULDBLOCK);
> 
> Suggest adding a high level (ERR) log here and an assert. That way if this
> happens
> in the field we will know earlier.

Hmm. This would allow someone to crash my browser by spamming candidates; we don't require any user interaction to allow a PeerConnection to spin up (at least, not in the DataChannel-only case).

Comment 25

[Byron Campen [:bwc]]

Attachment: Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

Unbitrot

Comment 26

[Byron Campen [:bwc]]

Attachment: Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

Add some logging when we drop STUN requests due to the rate limit.

Comment 27

[Byron Campen [:bwc]]

Attachment: Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

Use MOZ_ASSERT, since that won't crash on release builds.

Comment 28

[Byron Campen [:bwc]]

Comment on attachment 832497 [details] [diff] [review]
Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

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

This look about right to you?

https://bugzilla.mozilla.org/attachment.cgi?oldid=832368&action=interdiff&newid=832497&headers=1

Comment 29

[Eric Rescorla (:ekr)]

Comment on attachment 832497 [details] [diff] [review]
Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

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

Does MOZ_ASSERT generate a log message if it's not in debug mode?

Anyway, feel free to land as-is without rereview.

Comment 30

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

> Does MOZ_ASSERT generate a log message if it's not in debug mode?

No

Comment 31

[Eric Rescorla (:ekr)]

Then let's add a separate log to go along with the MOZ_ASSERT.

Byron, please land.

Comment 32

[Byron Campen [:bwc]]

Attachment: Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

Add a log message.

Comment 33

[Byron Campen [:bwc]]

Comment on attachment 8333497 [details] [diff] [review]
Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

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

Carry forward r+ ekr.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8333497 [details] [diff] [review]
Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a906959d7743

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a906959d7743

Comment 36

[u279076]

Does this need any testing from QA? If so, please advise.

Comment 37

[Byron Campen [:bwc]]

Hmm. Probably not. ekr?

Comment 38

[Martin Thomson [:mt:]]

I can provide a test case that someone should run, but I think that Byron is amply qualified to do so.  I don't know if you want to commit that test case to mochitest though.

Comment 39

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

How far back does this issue go?

Comment 40

[Eric Rescorla (:ekr)]

Back to the original landing for WebRTC.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

This applies cleanly to beta and b2g26. Please request approval for uplift :)

Comment 42

[Byron Campen [:bwc]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #41)
> This applies cleanly to beta and b2g26. Please request approval for uplift :)

What about aurora?

Comment 43

[Byron Campen [:bwc]]

Comment on attachment 8333497 [details] [diff] [review]
Very simple global rate-limiting based on SimpleTokenBucket. Will tolerate a maximum of 8K/sec over 1 sec, and 3.6K/sec over 20 sec.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

   Bug 790517

User impact if declined: 
 
   A user's browser could be induced to perform a DoS attack.

Testing completed (on m-c, etc.):

   The usual CI testing, plus some manual testing.

Risk to taking this patch (and alternatives if risky):

   Fairly low.
 
String or IDL/UUID changes made by this patch:

   None.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

It landed on trunk when it was version 28, so Aurora already has the fix.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a58670048f99
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ac3d130edf30

Comment 46

[u279076]

(In reply to Martin Thomson [:mt] from comment #38)
> I can provide a test case that someone should run, but I think that Byron is
> amply qualified to do so.  I don't know if you want to commit that test case
> to mochitest though.

Eric, please advise.

Comment 47

[Eric Rescorla (:ekr)]

I think it's fine to commit it as long as it doesn't do something bad as a side effect.