diff mbox series

icmp: randomize the global rate limiter

Message ID 20210223135209.3936-2-tim.gardner@canonical.com
State New
Headers show
Series icmp: randomize the global rate limiter | expand

Commit Message

Tim Gardner Feb. 23, 2021, 1:52 p.m. UTC
From: Eric Dumazet <edumazet@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@google.com>
Reported-by: Keyu Man <kman001@ucr.edu>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
Signed-off-by: Ian May <ian.may@canonical.com>
(cherry picked from commit fc3d57b1e860f9bdd97bbbf14f1617e718fae7f3 bionic/linux-oracle-5.4)
CVE-2020-25705
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

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.
---
 Documentation/networking/ip-sysctl.txt | 4 +++-
 net/ipv4/icmp.c                        | 7 +++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

Comments

Stefan Bader Feb. 24, 2021, 8:54 a.m. UTC | #1
On 23.02.21 14:52, Tim Gardner wrote:
> From: Eric Dumazet <edumazet@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@google.com>
> Reported-by: Keyu Man <kman001@ucr.edu>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Kamal Mostafa <kamal@canonical.com>
> Signed-off-by: Ian May <ian.may@canonical.com>
> (cherry picked from commit fc3d57b1e860f9bdd97bbbf14f1617e718fae7f3 bionic/linux-oracle-5.4)
> CVE-2020-25705
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
Acked-by: Stefan Bader <stefan.bader@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);
>
Kleber Souza Feb. 24, 2021, 9:48 a.m. UTC | #2
On 23.02.21 14:52, Tim Gardner wrote:
> From: Eric Dumazet <edumazet@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@google.com>
> Reported-by: Keyu Man <kman001@ucr.edu>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Kamal Mostafa <kamal@canonical.com>
> Signed-off-by: Ian May <ian.may@canonical.com>
> (cherry picked from commit fc3d57b1e860f9bdd97bbbf14f1617e718fae7f3 bionic/linux-oracle-5.4)
> CVE-2020-25705
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> 
> 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.

Acked-by: Kleber Sacilotto de Souza <kleber.souza@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);
>
diff mbox series

Patch

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