Bug 1281482 (CVE-2017-5388)

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

RESOLVED FIXED in Firefox 51

Status

()

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

People

(Reporter: fluffy, Assigned: bwc)

Tracking

({csectype-dos, sec-low})

unspecified
mozilla52
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(e10s+, firefox48- wontfix, firefox49+ wontfix, firefox50+ wontfix, firefox51+ fixed, firefox52 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Posted file 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.
Group: core-security → media-core-security
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
Flags: needinfo?(docfaraday)
(Assignee)

Comment 2

3 years ago
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.
Flags: needinfo?(docfaraday)
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.
Assignee: nobody → docfaraday
Rank: 15
Flags: needinfo?(docfaraday)
Priority: -- → P1
Summary: WebRTC can be used to generate a large amount of UDP traffic for DDOS attacks → WebRTC can be used to generate a large amount of UDP traffic for DDOS attacks (when using e10s)
(Assignee)

Comment 4

3 years ago
This would not be hard.
Flags: needinfo?(docfaraday)
[Tracking Requested - why for this release]: we plan to ship e10s in 48
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.

Updated

3 years ago
tracking-e10s: --- → +
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?
Flags: needinfo?(docfaraday)
(Assignee)

Comment 8

3 years ago
I've got other stuff I'm juggling at the moment. I might be able to later today or next week.
Flags: needinfo?(docfaraday)
Wontfix for 49 as we are now heading into beta 8. We could still potentially take a patch in aurora.
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.
Flags: needinfo?(rjesup)
Flags: needinfo?(docfaraday)
(Assignee)

Comment 11

3 years ago
Nothing in progress yet, but I can toss something together today.
Flags: needinfo?(docfaraday)
(Assignee)

Updated

3 years ago
Attachment #8793812 - Flags: review?(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?
(Assignee)

Comment 15

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

Comment 16

3 years ago
Actually, it is called from STS in both cases, just different processes. So we should be good.
Comment on attachment 8793812 [details] [diff] [review]
Extend STUN rate limit to e10s sockets

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

LGTM
Attachment #8793812 - Flags: review?(drno) → review+
https://hg.mozilla.org/mozilla-central/rev/2f21fdc94dcf
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi Byron, Dan, should we uplift this fix to Beta50?
Flags: needinfo?(dveditz)
Flags: needinfo?(docfaraday)
Hi Byron and Dan,
and 51 aurora?
(Assignee)

Comment 22

3 years ago
It is a pretty low-impact bug, to be honest. I could be convinced otherwise.
Flags: needinfo?(docfaraday)
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.
Flags: needinfo?(rjesup) → needinfo?(docfaraday)
(Assignee)

Comment 24

3 years ago
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.
Flags: needinfo?(docfaraday)
Attachment #8793812 - Flags: approval-mozilla-aurora?
Group: media-core-security → core-security-release
Thanks guys, this is now a wontfix for Fx50.
Comment on attachment 8793812 [details] [diff] [review]
Extend STUN rate limit to e10s sockets

Fix a sec issue. Take it in 51 aurora.
Attachment #8793812 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
wontfix on 50 and taking in 51 LGTM.
Flags: needinfo?(dveditz)
Comment 21 is private: false
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Alias: CVE-2017-5388
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.