1281482 - (CVE-2017-5388) WebRTC can be used to generate a large amount of UDP traffic for DDOS attacks (when using e10s)

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Cullen Jennings]

Attachment: stunTraffic.html

1. Create a large number of webkitRTCPeerConnection objects
2. Point the STUN server for ICE at the a the target to DDOS
3. Set them up with to start gathering stun address

A STUN packet is sent every 1 ms. This allows an attacker doing perhaps an add buy and using to have each browser display the add send about 500 kbps of UDP traffic to an IP and port of the attackers choosing and might contribute to a DDOS attack on say DNS servers. 

Currently plan to talk about this at upcoming IETF which has a a draft deadline of July 8 but am happy to work with FF team to change that timing if needed. I am strongly committed to what is good for the WebRTC community do not want to do anything that would have any negative impact on FF.

Comment 1

[Martin Thomson [:mt:]]

Phew, good thing we don't implement webkitRTCPeerConnection.  I managed to get 3.5Mbps out of Firefox when I did this last.

Byron, we should be limiting to 8k bytes per second (64 kbps) [1], per content process (worth fixing? probably not).  Does Cullen's report sound correct?  Or did we neglect to rate limit gathering checks when we added the rate limit in bug 906384?

[1] http://searchfox.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#770-773

Comment 2

[Byron Campen [:bwc]]

STUN requests for gathering are rate limited just like ICE checks are. The problem is this rate limit does not apply for e10s. I doubt it makes sense to try to make the rate limit apply across all content processes right now, although if/when we start using more than one we might want to look into moving the rate limit into the parent process somehow.

Comment 3

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

Byron - How much work would it be to implement/fix the limit in e10s? (ignoring the per-content-process thing, which I don't think I care about much if at all even if we turn on multiple content processes).  

I think we do want to fix this very soon given the plans to ship e10s to production; and if the fix is simple enough uplift to Aurora.

Comment 4

[Byron Campen [:bwc]]

This would not be hard.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

[Tracking Requested - why for this release]: we plan to ship e10s in 48

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Not tracking as it is a sec low, we are at the end of the cycle and the reporter is ok to delay the announce of the issue.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 49, while it's sec-low it sounds like it would be nice to fix in 49. We could still take a patch in beta. Byron are you able to take a look?

Comment 8

[Byron Campen [:bwc]]

I've got other stuff I'm juggling at the moment. I might be able to later today or next week.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix for 49 as we are now heading into beta 8. We could still potentially take a patch in aurora.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jesup, Byron, I noticed that this bug is marked as a P1 issue. Do we have a fix in the works? I'd be happy to take an uplift in Beta50 for it if deemed safe.

Comment 11

[Byron Campen [:bwc]]

Nothing in progress yet, but I can toss something together today.

Comment 12

[Byron Campen [:bwc]]

Attachment: Extend STUN rate limit to e10s sockets

.

Comment 13

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2224925f398a

Comment 14

[Nils Ohlmeier [:drno]]

Comment on attachment 8793812 [details] [diff] [review]
Extend STUN rate limit to e10s sockets

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

::: media/mtransport/nr_socket_prsock.cpp
@@ +788,5 @@
> +#endif
> +  }
> +
> +  // Take len tokens from both buckets.
> +  // (not threadsafe, but no problem since this is only called from STS)

In e10s case this is not going to be called on STS. Is that a problem?

Comment 15

[Byron Campen [:bwc]]

(In reply to Nils Ohlmeier [:drno] from comment #14)
> Comment on attachment 8793812 [details] [diff] [review]
> Extend STUN rate limit to e10s sockets
> 
> Review of attachment 8793812 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/mtransport/nr_socket_prsock.cpp
> @@ +788,5 @@
> > +#endif
> > +  }
> > +
> > +  // Take len tokens from both buckets.
> > +  // (not threadsafe, but no problem since this is only called from STS)
> 
> In e10s case this is not going to be called on STS. Is that a problem?

Hmm. I think in the e10s case, this code will be called only from one thread, on the content process. So I think we're ok, although the comment should probably be corrected.

Comment 16

[Byron Campen [:bwc]]

Actually, it is called from STS in both cases, just different processes. So we should be good.

Comment 17

[Nils Ohlmeier [:drno]]

Comment on attachment 8793812 [details] [diff] [review]
Extend STUN rate limit to e10s sockets

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

LGTM

Comment 18

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f21fdc94dcfb4635ed5218ea3d2dd43fb332d07
Bug 1281482: Extend STUN rate limit to e10s sockets. r=drno

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f21fdc94dcf

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Byron, Dan, should we uplift this fix to Beta50?

Comment 21

[Gerry Chang [:gchang]]

Hi Byron and Dan,
and 51 aurora?

Comment 22

[Byron Campen [:bwc]]

It is a pretty low-impact bug, to be honest. I could be convinced otherwise.

Comment 23

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

It's mostly low-impact (and hard for people to find!), but it's also quite low-risk (mostly just encapsulating a bit of code in a function, and calling it from one additional place).
Should go for at least 51, and 52 seems reasonable.  I might rate it higher than low, since e10s is on by default.  Not crit, but medium at least; perhaps high.

Comment 24

[Byron Campen [:bwc]]

Comment on attachment 8793812 [details] [diff] [review]
Extend STUN rate limit to e10s sockets

Approval Request Comment
[Feature/regressing bug #]:

   Present since webrtc landed.

[User impact if declined]:

   Content will be able to cause the browser to saturate the upstream at an arbitrary IP:port over UDP.

[Describe test coverage new/current, TreeHerder]:

   None.

[Risks and why]:

   Pretty low. Just moving some code around, and calling it in one additional place.

[String/UUID change made/needed]:

   None.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks guys, this is now a wontfix for Fx50.

Comment 26

[Gerry Chang [:gchang]]

Comment on attachment 8793812 [details] [diff] [review]
Extend STUN rate limit to e10s sockets

Fix a sec issue. Take it in 51 aurora.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f55da68af640

Comment 28

[Daniel Veditz [:dveditz]]

wontfix on 50 and taking in 51 LGTM.