From 207c7cb72a86f621fdc49a39388109fac98d52cf Mon Sep 17 00:00:00 2001 From: Michael Tuexen Date: Mon, 10 Feb 2025 22:16:20 +0100 Subject: [PATCH] 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 Sponsored by: Netflix, Inc. Differential Revision: https://reviews.freebsd.org/D48804 (cherry picked from commit 923c223f27e792e51ca13c476428adbbf6887551) --- 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 24a14c4d487..26ee6e5c124 100644 --- a/sys/netinet/ip_icmp.c +++ b/sys/netinet/ip_icmp.c @@ -90,7 +90,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) @@ -1110,14 +1110,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; } @@ -1146,7 +1148,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); + } } } } @@ -1162,8 +1166,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); @@ -1192,14 +1196,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 138a7ce71bb..39c252e16b7 100644 --- a/sys/netinet6/icmp6.c +++ b/sys/netinet6/icmp6.c @@ -2741,22 +2741,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, @@ -2778,15 +2762,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; } @@ -2815,7 +2817,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); + } } } } @@ -2835,8 +2839,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); @@ -2898,14 +2902,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);