Add jitter to the ICMP bandwidth limit
Details
Details
Use ping -f to show the limit being jittered in the log.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 35041 Build 32021: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
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).
| 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. | |
| sys/kern/subr_counter.c | ||
|---|---|---|
| 130 | 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. | |
| 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? | |
Comment Actions
Forgive my (carefully cultivated) ignorance of the network stack, but I'd like to understand this:
- We already have a bandwidth limit for outgoing ICMP packets
- We already have a packets-per-second limit for outgoing ICMP packets
- Outgoing ICMP packets which exceed either the bandwidth or packets-per-second limit are dropped and never sent
- This change jitters the packets-per-second limit
Is that correct?
Comment Actions
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/sys/counter.h | ||
|---|---|---|
| 75 | My mistake. I missed that. | |
Comment Actions
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.
Comment Actions
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'" | |
Comment Actions
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.
Comment Actions
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."
| sys/netinet/ip_icmp.c | ||
|---|---|---|
| 1151 | 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? | |
| 1157 | 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) | |
Comment Actions
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? | |
Comment Actions
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.
Comment Actions
- 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
Comment Actions
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? | |