Page MenuHomeFreeBSD

Address issue pointed out in CVE-2020-25705
ClosedPublic

Authored by kp on Nov 24 2020, 6:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 28, 2:17 AM
Unknown Object (File)
Sat, Nov 26, 1:22 PM

Details

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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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?

1103

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

1127

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
1138

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
1138

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

This seems to update V_icmplim, so aren't we going to randomly walk away from the configured limit? If we get unlucky enough we can even end up walking all the way down to 0 or up to BANDLIM_UNLIMITED, which would just disable rate limiting.

Shouldn't we keep the original V_icmplim and add the random offset relative each time, that is, add/remove a random offset from the originally configured V_icmplim value, not on top of a value which has already had a random adjustment applied to it.

sys/netinet/ip_icmp.c
93

I think ICMPCTL_ICMPLIM needs to be OID_AUTO, because we already use ICMPCTL_ICMPLIM for icmplim itself.

Without that change the kernel reports "sysctl: OID number(3) is already in use for 'icmplim_inc'"

kp updated this revision to Diff 103974.
kp added a reviewer: gnn.

So this is what I have in mind. We keep the target icmp packet limit setting, but every time we hit it we update an offset to it, for the real limit.
That still randomises, but we're guaranteed to be within icmplim_inc from the real setting.

philip requested changes to this revision.Mar 18 2022, 3:32 AM

This looks good. I would still like to see the comment "for security" to be expanded with some information about how it actually improves security. Something along the lines of "this closes a side channel permitting an attacker to infer the source port from the rate limited ICMP responses, as in CVE-2020-25705."

This revision now requires changes to proceed.Mar 18 2022, 3:32 AM

Update comment
Ensure we keep returning -1 on rate limit exceeded

sys/netinet/ip_icmp.c
1153

counter_ratecheck returns -1 for each over-limit packet in the interval, no? We'd want to pick a new value only once per I'd think?

1159

what about arc4random_uniform(V_icmplim_inc * 2 +1) - V_icmplim_inc instead avoiding the need for the % 2 special cases? (also adding appropriate limits)

  • only update jitter offset when we've just hit the limit
  • Fully use the jitter range

Ping. Any more remarks, or should we commit this?

I think the logic is sound, but icmplim_inc and the description Increment value for randomizing the number of ICMP responses per second don't seem quite right to me. Maybe icmplim_jitter_limit?

"Increment value" seems to imply we increase icmplim by that value.

sys/netinet/ip_icmp.c
1143–1144

Someone could change the icmplim sysctl after this calculation happens; maybe we should clamp at the counter_ratecheck call itself?

Agreed. I commented, before, that a rate limit that is adjusted by a 16 bit value was a regression. Reducing the randomness from 32 to the original 16 bits will duplicate the same mistake WPA made with WPS. This change would allow any attacker to guess the port number more easily. @rpokala expressed it better than I did. I would be OK with this if the random adjustment remained a 32 bit value.

cy requested changes to this revision.Mar 30 2022, 2:40 PM
This revision now requires changes to proceed.Mar 30 2022, 2:40 PM
In D27354#786629, @cy wrote:

Agreed. I commented, before, that a rate limit that is adjusted by a 16 bit value was a regression. Reducing the randomness from 32 to the original 16 bits will duplicate the same mistake WPA made with WPS. This change would allow any attacker to guess the port number more easily. @rpokala expressed it better than I did. I would be OK with this if the random adjustment remained a 32 bit value.

I don't follow. Where are we using a 16 bits value instead of a 32 bits value here?

  • rename to icmplim_jitter to indicate we're jittering icmplim
  • check for underflow on every evaluation, not just when we get a new icmplim_jitter

I'm fine with this

sys/netinet/ip_icmp.c
97

This seems unusual in the way sysctls are typically documented, maybe "Random icmplim jitter adjustment limit" or something like that?

My mistake. The randomness issue was fixed a while ago.

I'm fine with this revision. Thank you!

This revision is now accepted and ready to land.Mar 31 2022, 3:16 AM
This revision was automatically updated to reflect the committed changes.