diff mbox series

[net] icmp: randomize the global rate limiter

Message ID 20201015184200.2179938-1-eric.dumazet@gmail.com
State New
Headers show
Series [net] icmp: randomize the global rate limiter | expand

Commit Message

Eric Dumazet Oct. 15, 2020, 6:42 p.m. UTC
From: Eric Dumazet <edumazet@google.com>

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

Comments

Jakub Kicinski Oct. 16, 2020, 11:49 p.m. UTC | #1
On Thu, 15 Oct 2020 11:42:00 -0700 Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>

> 

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


Applied, queued up, thank you!
patchwork-bot+netdevbpf@kernel.org Oct. 17, 2020, midnight UTC | #2
Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Thu, 15 Oct 2020 11:42:00 -0700 you wrote:
> From: Eric Dumazet <edumazet@google.com>

> 

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

> 

> [...]


Here is the summary with links:
  - [net] icmp: randomize the global rate limiter
    https://git.kernel.org/netdev/net/c/b38e7819cae9

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
diff mbox series

Patch

diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
index 837d51f9e1fab7c0999a51184f95971fb43c1b9b..25e6673a085a0f55f1f23bd3974e806ed2706f68 100644
--- a/Documentation/networking/ip-sysctl.rst
+++ b/Documentation/networking/ip-sysctl.rst
@@ -1142,13 +1142,15 @@  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
 
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 9ea66d903c41f560093b5cf21814b494c71f669b..1e8fd77d85037f8c7b5a64fc54630ccffc3a48b1 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);