Hello,
below is a final version of patch for NAT issue discussed at bugs@ [1].
Patch below is updated according to feedback I got from Chris, claudio@
and hrvoje@.
The summary of changes is as follows:
- prevent infinite loop when packet hits NAT rule as follows:
pass out on em0 from 172.16.0.0/16 to any nat-to { 49/27 }
the issue has been introduced by my earlier commit [2]. The earlier
change makes pf(4) to interpret 49/27 as single IP address (POOL_NONE)
this is wrong, because pool 49/27 actually contains 32 addresses.
- while investigating the issue I've realized 'random' pool should
rather be using arc4_uniform() with upper limit derived from mask.
also the random number should be turned to netorder.
- also while I was debugging my change I've noticed we should be using
pf_poolmask() to obtain address as a combination of pool address
and result of generator (round-robin all random).
OK to commit?
thanks and
regards
sashan
[1] https://marc.info/?t=165813368200001&r=1&w=2
https://marc.info/?t=165732546500001&r=1&w=2
https://marc.info/?l=openbsd-bugs&m=165817500514813&w=2
[2] https://marc.info/?l=openbsd-cvs&m=164500117319660&w=2
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf_lb.c b/sys/net/pf_lb.c
index d106073d372..dff2349cde7 100644
--- a/sys/net/pf_lb.c
+++ b/sys/net/pf_lb.c
@@ -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:
