Page MenuHomeFreeBSD

Address issue pointed out in CVE-2020-25705
Needs ReviewPublic

Authored by gnn on Nov 24 2020, 6:34 PM.

Details

Reviewers
emaste
philip
cy
Summary

Add jitter to the ICMP bandwidth limit

Test Plan

Use ping -f to show the limit being jittered in the log.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 35235
Build 32175: arc lint + arc unit

Event Timeline

gnn requested review of this revision.Nov 24 2020, 6:34 PM
philip requested changes to this revision.Nov 25 2020, 6:07 AM
philip added a subscriber: philip.

Minor nits in commentary. Functionally looks good!

sys/netinet/ip_icmp.c
95

Maybe "number of responses per second" rather than "bandwidth limit" to be consistent with icmplim, above?

1105

We should remember to adjust this comment when the paper is published. :)

1134

Spurious whitespace here.

This revision now requires changes to proceed.Nov 25 2020, 6:07 AM

Address issues pointed out by philip@

gnn marked 3 inline comments as done.Nov 25 2020, 3:54 PM

I'm not sure recalculating the jitter value on each badport_bandlim call gives a desirable distribution. If my math is right it seems we'd transmit packets with 100% probability to (V_icmplim - V_icmplim_jitter), declining to 0% at (V_icmplim + V_icmplim_jitter).

cem added inline comments.
sys/netinet/ip_icmp.c
1145

How frequently is badport_bandlim() called, and do the jitter numbers need to be unpredictable? If a non-cryptographic PRNG would suffice, prng32_bounded() might be a suitable replacement with less CPU use.

Updates based on conversations with cperciva and emaste

cperciva added inline comments.
sys/kern/subr_counter.c
130 ↗(On Diff #79996)

I would call this inc rather than jitter, since it's the increment being added.

sys/netinet/ip_icmp.c
91

We probably want this to be 3 (so ICMPs are counted as 0, 1, or 2 units towards the limit). Making it 16 would dramatically cut the number of packets which get out before we hit the limit.

sys/netinet/ip_icmp.c
91

We could multiply the limit by (V_icmplim_jitter / 2) to keep the average number of packets approximately the same as the limit, regardless of how icmplim_jitter is adjusted?

Clean up various bits for hopeful commit.

gnn marked 3 inline comments as done.Nov 26 2020, 3:26 PM

Forgive my (carefully cultivated) ignorance of the network stack, but I'd like to understand this:

  1. We already have a bandwidth limit for outgoing ICMP packets
  2. We already have a packets-per-second limit for outgoing ICMP packets
  3. Outgoing ICMP packets which exceed either the bandwidth or packets-per-second limit are dropped and never sent
  4. This change jitters the packets-per-second limit

Is that correct?

cy requested changes to this revision.Nov 30 2020, 3:45 PM
cy added inline comments.
sys/netinet/ip_icmp.c
1145

counter_ratecheck() in ys/kern/subr_counter.c takes only two arguments.

sys/sys/counter.h
75 ↗(On Diff #80002)

counter_ratecheck() in ys/kern/subr_counter.c takes only two arguments.

This revision now requires changes to proceed.Nov 30 2020, 3:45 PM

Forgive my (carefully cultivated) ignorance of the network stack, but I'd like to understand this:

  1. We already have a bandwidth limit for outgoing ICMP packets
  2. We already have a packets-per-second limit for outgoing ICMP packets
  3. Outgoing ICMP packets which exceed either the bandwidth or packets-per-second limit are dropped and never sent
  4. This change jitters the packets-per-second limit

Is that correct?

Correct. A constant rate limit allows the attackers to infer which source port the request is coming from reducing its randomness from 32 bits to the original 16 bits. (Same mistake WPA made with WPS.)

sys/kern/subr_counter.c
130 ↗(On Diff #80002)

See here (counter_ratecheck() in sys/kern/subr_counter.c).

sys/sys/counter.h
75 ↗(On Diff #80002)

This commit is changing that API and file to take 3 arguments. That's why it shows up in the diff.

sys/sys/counter.h
75 ↗(On Diff #80002)

My mistake. I missed that.

gnn marked 3 inline comments as done.Nov 30 2020, 8:39 PM
gnn marked 3 inline comments as done.Dec 1 2020, 1:55 AM
In D27354#612664, @cy wrote:

Correct. A constant rate limit allows the attackers to infer which source port the request is coming from reducing its randomness from 32 bits to the original 16 bits. (Same mistake WPA made with WPS.)

Thanks for the clarification!

Will this not make the reported message incorrect? In the worst case (assume inc was 2 every time and modulo off-by-one) we could start limiting at 100 packets sent, but report limiting from 200 to 200.

Update with suggestions from emaste