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

[Stefan Bader]

On 23.02.21 14:52, 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 commit fc3d57b1e860f9bdd97bbbf14f1617e718fae7f3 bionic/linux-oracle-5.4)
> CVE-2020-25705
> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>

-- cut --
> 
> I chose bionic/linux-oracle-5.4 for this cherry-pick since the patch had already
> been backported, regression tested, and released; thereby reducing the chances of backporting
> error or regression.
-- cut --
> ---

When this gets applied, lets cut off above verbatim section.

-Stefan
>  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);
>