+guidou, +hta. Can you help investigate and triage this security report?

Adding pthatcher. I know we've been experimenting with the max rate (see discussions in ICE WG), but there should be an overall per-process rate limiter; if there isn't, we need to put one in.

We currently have a max rate per-PeerConnection that we are considering changing.  But that's irrelevant to a cross-PeerConnection rate (since one can just make more and more PeerConnections).

I agree we should make it harder to do this kind of thing, but to make it impossible will be hard.  

For example, even if we fix the ICE check rate, similar techniques can be used, such as creating a large number of PeerConnections and pointing them all at the same STUN/TURN server and triggering ICE restarts over and over (causing STUN traffic to the server each time).  Or, create multiple processes with one PeerConnection each.

How far do we take it?

In previous discussions, we have more or less admitted that preventing someone from mounting a 60 kbps attack from a host is impossible, so we won't bother with defending against that. (Voice hammer attacks with non-RTCP-supporting senders, for instance).

Cullen reports this as a 500 kbps attack. If we can mitigate it by a factor of 6 or so, I'd say we're done here.

A single per-process rate limiter (ugh. another global.) should do it.

500kbps per tab or cross tabs?

Do we have to cover the "attack the STUN/TURN server" as well as "attack the remote peer"?

Tentatively assigning security impact here as low.

We actually do have a global rate limiter: https://cs.chromium.org/chromium/src/content/browser/renderer_host/p2p/socket_host_throttler.cc?type=cs&sq=package:chromium&rcl=1475695348&l=28

But it has some limitations:

- It only limits to 256kbps, not 60kbps (which could be easily changed)
- The limit only applies to packets sent to an endpoint that hasn't sent a STUN packet. I assume the entity managing the DDOS could simply spoof STUN packets from its victim to get around this.

To top it off, I just found that due to some refactoring, the limit was accidentally changed to 256 *Mbps*. Oops. I was able to get up to about 10Mbps before my tab started crashing. Should that up the severity of this issue?

CL to fix above problem: https://codereview.chromium.org/2398603004/

That definitely ups the priority.  

How long ago did this refactoring cause this problem.  In other words, for how many versions of Chrome has it been like this?

Only since Sep 13. So that regression hasn't made it into a stable release yet. Unfortunately we're just barely missing the branch point (it's today) so I'll request a merge.

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e9e3f35d8eba2a4f0d88df274fc4eca48e811083

commit e9e3f35d8eba2a4f0d88df274fc4eca48e811083
Author: deadbeef <deadbeef@chromium.org>
Date: Fri Oct 07 17:50:31 2016

Fixing issue that causes STUN throttle rate to be off by a factor of 1000.

The limit is supposed to be 256kbps, but it was actually effectively
256Mbps due to dividing a microsecond count by "microseconds per
millisecond" instead of "per second".

BUG= chromium:622323 

Review-Url: https://codereview.chromium.org/2398603004
Cr-Commit-Position: refs/heads/master@{#423902}

[modify] https://crrev.com/e9e3f35d8eba2a4f0d88df274fc4eca48e811083/content/browser/renderer_host/p2p/socket_host_throttler.cc
[modify] https://crrev.com/e9e3f35d8eba2a4f0d88df274fc4eca48e811083/content/browser/renderer_host/p2p/socket_host_udp_unittest.cc

+awhalley@ for review/approval (Security TPM)

[Automated comment] There appears to be on-going work (i.e. bugroid changes), needs manual review.

Good to merge to M55

Approving merge to M55 branch 2883 based on comment #17. Please merge ASAP. Thank you.

Merged to 2883, so removing merge labels. 

https://codereview.chromium.org/2410663002/

I'm afraid the panel declined to reward for this bug.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af0c4500d7199fc3bb4fdba844a14b58a3218d7a

commit af0c4500d7199fc3bb4fdba844a14b58a3218d7a
Author: deadbeef <deadbeef@chromium.org>
Date: Tue Oct 11 17:38:50 2016

Fixing issue that causes STUN throttle rate to be off by a factor of 1000.

The limit is supposed to be 256kbps, but it was actually effectively
256Mbps due to dividing a microsecond count by "microseconds per
millisecond" instead of "per second".

BUG= chromium:622323 
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2398603004
Cr-Commit-Position: refs/heads/master@{#423902}
(cherry picked from commit e9e3f35d8eba2a4f0d88df274fc4eca48e811083)

Review-Url: https://codereview.chromium.org/2410663002
Cr-Commit-Position: refs/branch-heads/2883@{#34}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/af0c4500d7199fc3bb4fdba844a14b58a3218d7a/content/browser/renderer_host/p2p/socket_host_throttler.cc
[modify] https://crrev.com/af0c4500d7199fc3bb4fdba844a14b58a3218d7a/content/browser/renderer_host/p2p/socket_host_udp_unittest.cc

[Automated comment] removing mislabelled merge-merged-2840

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot