From c2364f2ab38e3255986defad064283e6e86862e7 Mon Sep 17 00:00:00 2001 From: sashan Date: Wed, 3 Aug 2022 08:16:04 +0000 Subject: [PATCH] Bug was reported by Chriss Cappucio. It has turned out my earlier change to pf_lb.c was not complete. We must add a test to determine number of addresses defined by pool, so we don't treat pool definition 172.16.0.0/16 as a single IP address in pool. If pool is defined as 172.16.0.0/16, then we don't want to fall back to PF_POOL_NONE. Missing this measure in pf_map_addr() may cause pf_get_sport() to enter infinite loop when source ports translation become depleted for the first address found in pool (like 172.16.0.1), because the bug prevents pf_map_addr() to move to next address in pool (like 172.16.0.2). while investigating issue I've also noticed an oddity for small random pools such as 192.168.1.32/28. One would expect the addresses for nat will be randomly picked from range .32 - .47 in this case. however the random selection yield significantly more (like 20%) addresses ending by .32 In order to fix it we make random pool to use arc4random_uniform(~mask + 1) instead of current arc4random(). feedback by claudio@ tested by hrvoje@ --- sys/net/pf_lb.c | 45 +++++++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/sys/net/pf_lb.c b/sys/net/pf_lb.c index d106073d372..588115cbff7 100644 --- a/sys/net/pf_lb.c +++ b/sys/net/pf_lb.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf_lb.c,v 1.70 2022/02/16 08:46:11 sashan Exp $ */ +/* $OpenBSD: pf_lb.c,v 1.71 2022/08/03 08:16:04 sashan Exp $ */ /* * Copyright (c) 2001 Daniel Hartmeier @@ -344,6 +344,17 @@ pf_map_addr_sticky(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr, return (0); } +uint32_t +pf_rand_addr(uint32_t mask) +{ + uint32_t addr; + + mask = ~ntohl(mask); + addr = arc4random_uniform(mask + 1); + + return (htonl(addr)); +} + int pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr, struct pf_addr *naddr, struct pf_addr *init_addr, struct pf_src_node **sns, @@ -428,24 +439,29 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr, } else if (init_addr != NULL && PF_AZERO(init_addr, af)) { switch (af) { case AF_INET: - rpool->counter.addr32[0] = arc4random(); + rpool->counter.addr32[0] = pf_rand_addr( + rmask->addr32[0]); break; #ifdef INET6 case AF_INET6: if (rmask->addr32[3] != 0xffffffff) - rpool->counter.addr32[3] = arc4random(); + rpool->counter.addr32[3] = pf_rand_addr( + rmask->addr32[3]); else break; if (rmask->addr32[2] != 0xffffffff) - rpool->counter.addr32[2] = arc4random(); + rpool->counter.addr32[2] = pf_rand_addr( + rmask->addr32[2]); else break; if (rmask->addr32[1] != 0xffffffff) - rpool->counter.addr32[1] = arc4random(); + rpool->counter.addr32[1] = pf_rand_addr( + rmask->addr32[1]); else break; if (rmask->addr32[0] != 0xffffffff) - rpool->counter.addr32[0] = arc4random(); + rpool->counter.addr32[0] = pf_rand_addr( + rmask->addr32[0]); break; #endif /* INET6 */ default: @@ -500,11 +516,16 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr, } } else if (PF_AZERO(&rpool->counter, af)) { /* - * fall back to POOL_NONE if there are no addresses in - * pool + * fall back to POOL_NONE if there is a single host + * address in pool. */ - pf_addrcpy(naddr, raddr, af); - break; + if ((af == AF_INET && + rmask->addr32[0] == INADDR_BROADCAST) || + (af == AF_INET6 && + IN6_ARE_ADDR_EQUAL(&rmask->v6, &in6mask128))) { + pf_addrcpy(naddr, raddr, af); + break; + } } else if (pf_match_addr(0, raddr, rmask, &rpool->counter, af)) return (1); @@ -532,9 +553,9 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr, weight = rpool->weight; } - pf_addrcpy(naddr, &rpool->counter, af); + pf_poolmask(naddr, raddr, rmask, &rpool->counter, af); if (init_addr != NULL && PF_AZERO(init_addr, af)) - pf_addrcpy(init_addr, naddr, af); + pf_addrcpy(init_addr, &rpool->counter, af); pf_addr_inc(&rpool->counter, af); break; case PF_POOL_LEASTSTATES: -- 2.20.1