Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 10 Feb 2025 21:21:12 GMT
From:      Michael Tuexen <tuexen@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org
Subject:   git: 923c223f27e7 - main - icmp: use per rate limit randomized jitter
Message-ID:  <202502102121.51ALLCuS056570@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch main has been updated by tuexen:

URL: https://cgit.FreeBSD.org/src/commit/?id=923c223f27e792e51ca13c476428adbbf6887551

commit 923c223f27e792e51ca13c476428adbbf6887551
Author:     Michael Tuexen <tuexen@FreeBSD.org>
AuthorDate: 2025-02-10 21:16:20 +0000
Commit:     Michael Tuexen <tuexen@FreeBSD.org>
CommitDate: 2025-02-10 21:16:20 +0000

    icmp: use per rate limit randomized jitter
    
    Using the same random jitter for multiple rate limits allows an
    attacker to use one rate limiter to figure out the current jitter
    and then use this knowledge to de-randomize the other rate limiters.
    This can be mitigated by using a separate randomized jitter for each
    rate limiter.
    This issue was reported as issue number 10 in Keyu Man et al.:
    SCAD: Towards a Universal and Automated Network Side-Channel
    Vulnerability Detection
    
    Reviewed by:            rrs, Peter Lei, glebius
    MFC after:              3 days
    Sponsored by:           Netflix, Inc.
    Differential Revision:  https://reviews.freebsd.org/D48804
---
 sys/netinet/ip_icmp.c | 20 ++++++++++++--------
 sys/netinet6/icmp6.c  | 50 +++++++++++++++++++++++++++-----------------------
 2 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/sys/netinet/ip_icmp.c b/sys/netinet/ip_icmp.c
index 58484e532fef..2b61820cdaa8 100644
--- a/sys/netinet/ip_icmp.c
+++ b/sys/netinet/ip_icmp.c
@@ -88,7 +88,7 @@ SYSCTL_PROC(_net_inet_icmp, ICMPCTL_ICMPLIM, icmplim, CTLTYPE_UINT |
     &sysctl_icmplim_and_jitter, "IU",
     "Maximum number of ICMP responses per second");
 
-VNET_DEFINE_STATIC(int, icmplim_curr_jitter) = 0;
+VNET_DEFINE_STATIC(int, icmplim_curr_jitter[BANDLIM_MAX]) = {0};
 #define V_icmplim_curr_jitter		VNET(icmplim_curr_jitter)
 VNET_DEFINE_STATIC(u_int, icmplim_jitter) = 16;
 #define	V_icmplim_jitter		VNET(icmplim_jitter)
@@ -1108,14 +1108,16 @@ static const char *icmp_rate_descrs[BANDLIM_MAX] = {
 };
 
 static void
-icmplim_new_jitter(void)
+icmplim_new_jitter(int which)
 {
 	/*
 	 * Adjust limit +/- to jitter the measurement to deny a side-channel
 	 * port scan as in https://dl.acm.org/doi/10.1145/3372297.3417280
 	 */
+	KASSERT(which >= 0 && which < BANDLIM_MAX,
+	    ("%s: which %d", __func__, which));
 	if (V_icmplim_jitter > 0)
-		V_icmplim_curr_jitter =
+		V_icmplim_curr_jitter[which] =
 		    arc4random_uniform(V_icmplim_jitter * 2 + 1) -
 		    V_icmplim_jitter;
 }
@@ -1144,7 +1146,9 @@ sysctl_icmplim_and_jitter(SYSCTL_HANDLER_ARGS)
 				error = EINVAL;
 			else {
 				V_icmplim_jitter = new;
-				icmplim_new_jitter();
+				for (int i = 0; i < BANDLIM_MAX; i++) {
+					icmplim_new_jitter(i);
+				}
 			}
 		}
 	}
@@ -1160,8 +1164,8 @@ icmp_bandlimit_init(void)
 	for (int i = 0; i < BANDLIM_MAX; i++) {
 		V_icmp_rates[i].cr_rate = counter_u64_alloc(M_WAITOK);
 		V_icmp_rates[i].cr_ticks = ticks;
+		icmplim_new_jitter(i);
 	}
-	icmplim_new_jitter();
 }
 VNET_SYSINIT(icmp_bandlimit, SI_SUB_PROTO_DOMAIN, SI_ORDER_ANY,
     icmp_bandlimit_init, NULL);
@@ -1190,14 +1194,14 @@ badport_bandlim(int which)
 	    ("%s: which %d", __func__, which));
 
 	pps = counter_ratecheck(&V_icmp_rates[which], V_icmplim +
-	    V_icmplim_curr_jitter);
+	    V_icmplim_curr_jitter[which]);
 	if (pps > 0) {
 		if (V_icmplim_output)
 			log(LOG_NOTICE,
 			    "Limiting %s response from %jd to %d packets/sec\n",
 			    icmp_rate_descrs[which], (intmax_t )pps,
-			    V_icmplim + V_icmplim_curr_jitter);
-		icmplim_new_jitter();
+			    V_icmplim + V_icmplim_curr_jitter[which]);
+		icmplim_new_jitter(which);
 	}
 	if (pps == -1)
 		return (-1);
diff --git a/sys/netinet6/icmp6.c b/sys/netinet6/icmp6.c
index b962d39151e9..3740a3eaec0d 100644
--- a/sys/netinet6/icmp6.c
+++ b/sys/netinet6/icmp6.c
@@ -2750,22 +2750,6 @@ SYSCTL_PROC(_net_inet6_icmp6, ICMPV6CTL_ERRPPSLIMIT, errppslimit,
     &sysctl_icmp6lim_and_jitter, "IU",
     "Maximum number of ICMPv6 error/reply messages per second");
 
-VNET_DEFINE_STATIC(int, icmp6lim_curr_jitter) = 0;
-#define	V_icmp6lim_curr_jitter	VNET(icmp6lim_curr_jitter)
-
-VNET_DEFINE_STATIC(u_int, icmp6lim_jitter) = 8;
-#define	V_icmp6lim_jitter	VNET(icmp6lim_jitter)
-SYSCTL_PROC(_net_inet6_icmp6, OID_AUTO, icmp6lim_jitter, CTLTYPE_UINT |
-    CTLFLAG_VNET | CTLFLAG_RW, &VNET_NAME(icmp6lim_jitter), 0,
-    &sysctl_icmp6lim_and_jitter, "IU",
-    "Random errppslimit jitter adjustment limit");
-
-VNET_DEFINE_STATIC(int, icmp6lim_output) = 1;
-#define	V_icmp6lim_output	VNET(icmp6lim_output)
-SYSCTL_INT(_net_inet6_icmp6, OID_AUTO, icmp6lim_output,
-    CTLFLAG_VNET | CTLFLAG_RW, &VNET_NAME(icmp6lim_output), 0,
-    "Enable logging of ICMPv6 response rate limiting");
-
 typedef enum {
 	RATELIM_PARAM_PROB = 0,
 	RATELIM_TOO_BIG,
@@ -2787,15 +2771,33 @@ static const char *icmp6_rate_descrs[RATELIM_MAX] = {
 	[RATELIM_OTHER] = "(other)",
 };
 
+VNET_DEFINE_STATIC(int, icmp6lim_curr_jitter[RATELIM_MAX]) = {0};
+#define	V_icmp6lim_curr_jitter	VNET(icmp6lim_curr_jitter)
+
+VNET_DEFINE_STATIC(u_int, icmp6lim_jitter) = 8;
+#define	V_icmp6lim_jitter	VNET(icmp6lim_jitter)
+SYSCTL_PROC(_net_inet6_icmp6, OID_AUTO, icmp6lim_jitter, CTLTYPE_UINT |
+    CTLFLAG_VNET | CTLFLAG_RW, &VNET_NAME(icmp6lim_jitter), 0,
+    &sysctl_icmp6lim_and_jitter, "IU",
+    "Random errppslimit jitter adjustment limit");
+
+VNET_DEFINE_STATIC(int, icmp6lim_output) = 1;
+#define	V_icmp6lim_output	VNET(icmp6lim_output)
+SYSCTL_INT(_net_inet6_icmp6, OID_AUTO, icmp6lim_output,
+    CTLFLAG_VNET | CTLFLAG_RW, &VNET_NAME(icmp6lim_output), 0,
+    "Enable logging of ICMPv6 response rate limiting");
+
 static void
-icmp6lim_new_jitter(void)
+icmp6lim_new_jitter(int which)
 {
 	/*
 	 * Adjust limit +/- to jitter the measurement to deny a side-channel
 	 * port scan as in https://dl.acm.org/doi/10.1145/3372297.3417280
 	 */
+	KASSERT(which >= 0 && which < RATELIM_MAX,
+	    ("%s: which %d", __func__, which));
 	if (V_icmp6lim_jitter > 0)
-		V_icmp6lim_curr_jitter =
+		V_icmp6lim_curr_jitter[which] =
 		    arc4random_uniform(V_icmp6lim_jitter * 2 + 1) -
 		    V_icmp6lim_jitter;
 }
@@ -2824,7 +2826,9 @@ sysctl_icmp6lim_and_jitter(SYSCTL_HANDLER_ARGS)
 				error = EINVAL;
 			else {
 				V_icmp6lim_jitter = new;
-				icmp6lim_new_jitter();
+				for (int i = 0; i < RATELIM_MAX; i++) {
+					icmp6lim_new_jitter(i);
+				}
 			}
 		}
 	}
@@ -2844,8 +2848,8 @@ icmp6_ratelimit_init(void)
 	for (int i = 0; i < RATELIM_MAX; i++) {
 		V_icmp6_rates[i].cr_rate = counter_u64_alloc(M_WAITOK);
 		V_icmp6_rates[i].cr_ticks = ticks;
+		icmp6lim_new_jitter(i);
 	}
-	icmp6lim_new_jitter();
 }
 VNET_SYSINIT(icmp6_ratelimit, SI_SUB_PROTO_DOMAIN, SI_ORDER_ANY,
     icmp6_ratelimit_init, NULL);
@@ -2907,14 +2911,14 @@ icmp6_ratelimit(const struct in6_addr *dst, const int type, const int code)
 	};
 
 	pps = counter_ratecheck(&V_icmp6_rates[which], V_icmp6errppslim +
-	    V_icmp6lim_curr_jitter);
+	    V_icmp6lim_curr_jitter[which]);
 	if (pps > 0) {
 		if (V_icmp6lim_output)
 			log(LOG_NOTICE, "Limiting ICMPv6 %s output from %jd "
 			    "to %d packets/sec\n", icmp6_rate_descrs[which],
 			    (intmax_t )pps, V_icmp6errppslim +
-			    V_icmp6lim_curr_jitter);
-		icmp6lim_new_jitter();
+			    V_icmp6lim_curr_jitter[which]);
+		icmp6lim_new_jitter(which);
 	}
 	if (pps == -1) {
 		ICMP6STAT_INC(icp6s_toofreq);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202502102121.51ALLCuS056570>