NACK/Cmnt: [PATCH] icmp: randomize the global rate limiter

[Stefan Bader]

On 22.02.21 22:07, Thadeu Lima de Souza Cascardo wrote:
> On Mon, Feb 22, 2021 at 10:55:00AM -0700, Tim Gardner wrote:
>> From: Eric Dumazet <edumazet at google.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1902115
>>
>> [ Upstream commit b38e7819cae946e2edf869e604af1e65a5d241c5 ]
>>
>> Keyu Man reported that the ICMP rate limiter could be used
>> by attackers to get useful signal. Details will be provided
>> in an upcoming academic publication.
>>
>> Our solution is to add some noise, so that the attackers
>> no longer can get help from the predictable token bucket limiter.
>>
>> Fixes: 4cdf507d5452 ("icmp: add a global rate limitation")
>> Signed-off-by: Eric Dumazet <edumazet at google.com>
>> Reported-by: Keyu Man <kman001 at ucr.edu>
>> Signed-off-by: Jakub Kicinski <kuba at kernel.org>
>> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>> Signed-off-by: Kamal Mostafa <kamal at canonical.com>
>> Signed-off-by: Ian May <ian.may at canonical.com>
>> (cherry picked from focal/oracle-5.4 commit fc3d57b1e860f9bdd97bbbf14f1617e718fae7f3)
>> CVE-2020-25705
>> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
>>
>> back port notes: This was a clean cherry pick of the Focal oracle-5.4 backport.

When bringing up backport notes I was saying that we document what we did *if*
we do backports. And I gave an exact example of how this is done by everybody else.

But regardless of that, once a thread is NACKed once it is dead. Re-submissions
go into a new thread. Having one thread with a mix of ok and rejected patches
mixed is just too dangerous.

-Stefan

> 
> As for the fix for CVE-2020-25668, I find it unusual to document the backport
> like this. I do take backports from different series sometimes, making sure
> they make sense for the different series I am applying them to.
> 
> However, the line I care about is present here and this is better documenting
> the source of the backport than when only the code is taken from the different
> series.
> 
> Acked-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>
> 
>> ---
>>  Documentation/networking/ip-sysctl.txt | 4 +++-
>>  net/ipv4/icmp.c                        | 7 +++++--
>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
>> index 5f53faff4e25..151d6ad0e0a7 100644
>> --- a/Documentation/networking/ip-sysctl.txt
>> +++ b/Documentation/networking/ip-sysctl.txt
>> @@ -1005,12 +1005,14 @@ icmp_ratelimit - INTEGER
>>  icmp_msgs_per_sec - INTEGER
>>  	Limit maximal number of ICMP packets sent per second from this host.
>>  	Only messages whose type matches icmp_ratemask (see below) are
>> -	controlled by this limit.
>> +	controlled by this limit. For security reasons, the precise count
>> +	of messages per second is randomized.
>>  	Default: 1000
>>  
>>  icmp_msgs_burst - INTEGER
>>  	icmp_msgs_per_sec controls number of ICMP packets sent per second,
>>  	while icmp_msgs_burst controls the burst size of these packets.
>> +	For security reasons, the precise burst size is randomized.
>>  	Default: 50
>>  
>>  icmp_ratemask - INTEGER
>> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
>> index f369e7ce685b..dcffda472585 100644
>> --- a/net/ipv4/icmp.c
>> +++ b/net/ipv4/icmp.c
>> @@ -239,7 +239,7 @@ static struct {
>>  /**
>>   * icmp_global_allow - Are we allowed to send one more ICMP message ?
>>   *
>> - * Uses a token bucket to limit our ICMP messages to sysctl_icmp_msgs_per_sec.
>> + * Uses a token bucket to limit our ICMP messages to ~sysctl_icmp_msgs_per_sec.
>>   * Returns false if we reached the limit and can not send another packet.
>>   * Note: called with BH disabled
>>   */
>> @@ -267,7 +267,10 @@ bool icmp_global_allow(void)
>>  	}
>>  	credit = min_t(u32, icmp_global.credit + incr, sysctl_icmp_msgs_burst);
>>  	if (credit) {
>> -		credit--;
>> +		/* We want to use a credit of one in average, but need to randomize
>> +		 * it for security reasons.
>> +		 */
>> +		credit = max_t(int, credit - prandom_u32_max(3), 0);
>>  		rc = true;
>>  	}
>>  	WRITE_ONCE(icmp_global.credit, credit);
>> -- 
>> 2.17.1
>>
>>
>> -- 
>> kernel-team mailing list
>> kernel-team at lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210223/b94b597e/attachment.sig>