Address issue pointed out in CVE-2020-25705

[kp]

Summary

Add jitter to the ICMP bandwidth limit

Test Plan

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

[philip]

Minor nits in commentary. Functionally looks good!

sys/netinet/ip_icmp.c

96

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

1106

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

1136

Spurious whitespace here.

[gnn]

Address issues pointed out by philip@

[emaste]

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]

sys/netinet/ip_icmp.c

1147

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.

[gnn]

Updates based on conversations with cperciva and emaste

[cperciva]

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

92

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.

[cem]

sys/netinet/ip_icmp.c

92

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?

[gnn]

Clean up various bits for hopeful commit.

[rpokala]

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]

sys/netinet/ip_icmp.c

1147

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.

[cy]

In D27354#612569, @rpokala wrote:

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.)

[cem]

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.

[cy]

sys/sys/counter.h

75 ↗

(On Diff #80002)

My mistake. I missed that.

[rpokala]

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!

[emaste]

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.

[gnn]

Update with suggestions from emaste

[kp]

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

94

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]

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]

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."

[kp]

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

[emaste]

sys/netinet/ip_icmp.c

1158

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?

1164

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)

[kp]

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

[kp]

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

[emaste]

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

1152–1153

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

[cy]

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.

[kp]

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?

[kp]

• 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

[emaste]

I'm fine with this

sys/netinet/ip_icmp.c

98

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

[kp]

• sysctl description

[cy]

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

[philip]

I'm fine with this revision. Thank you!