Hi,
Looking at some corner-cases I've realised that collapse_redirspec
does a poor job of weeding out incompatibilities in the redirect
pool specifications and hence inferring the rule address family
from it.
An example is a rule like this:
pass out nat-to em0
Let's say that em0 has an IPv4 address 192.168.20.221 and an IPv6
link-local address fe80::221:86ff:fefa:5fc1. What this rule is
supposed to do? Since it cannot create two rules out of this
specification (the parser works), you have to choose whether this
will be an IPv4 rule or an IPv6 rule. Quite some time ago for
some reasons it translated to an IPv4 rule:
pass out inet nat-to 192.168.20.221
Then the behaviour was changed (rather silently) and now it
translates to an IPv6 rule:
pass out inet6 nat-to fe80::221:86ff:fefa:5fc1
I think that both is somewhat unexpected, even less so the
current behaviour.
I propose to avoid the confusion by flagging such situations as
errors, e.g.:
% echo 'pass out nat-to { ::1 1.1.1.1 }' | ./obj/pfctl -o none -vnf -
stdin:1: translation spec contains addresses with different address families
stdin:1: skipping rule due to errors
stdin:1: rule expands to no valid combination
While previously it would pick only one of them (the first one):
% echo 'pass out nat-to { ::1 1.1.1.1 }' | pfctl -o none -vnf -
pass out inet6 all flags S/SA nat-to ::1
You are still able to use "em0" as a source/destination filter
however:
% echo 'pass out from em0' | pfctl -o none -vnf -
pass out on em0 inet6 from fe80::221:86ff:fefa:5fc1 to any flags S/SA
pass out inet from 192.168.20.221 to any flags S/SA
meaning the diff only affects how nat-to/rdr-to pools are processed.
I have carefully tested that and do not expect any unrelated
fallout. And for the reasons stated above I don't believe
anyone is using this since it's largely error prone.
OK?
P.S.
binat bzero'ing is actually necessary since otherwise it
contains garbage and we're now checking for it more carefully.
diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index de9537e..ce80f8e 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -4295,11 +4295,11 @@ collapse_redirspec(struct pf_pool *rpool, struct
pf_rule *r,
struct redirspec *rs, u_int8_t allow_if)
{
struct pf_opt_tbl *tbl = NULL;
struct node_host *h;
struct pf_rule_addr ra;
- int i = 0;
+ int af = 0, i = 0;
if (!rs || !rs->rdr || rs->rdr->host == NULL) {
rpool->addr.type = PF_ADDR_NONE;
return (0);
}
@@ -4307,17 +4307,45 @@ collapse_redirspec(struct pf_pool *rpool, struct
pf_rule *r,
if (r->rule_flag & PFRULE_AFTO)
r->naf = rs->af;
/* count matching addresses */
for (h = rs->rdr->host; h != NULL; h = h->next) {
- if (!r->af || !h->af || rs->af || h->af == r->af) {
+ if (h->af) {
+ /* check that af-to target address has correct af */
+ if (rs->af && rs->af != h->af) {
+ yyerror("af mismatch in %s spec",
+ allow_if ? "routing" : "translation");
+ return (1);
+ }
+ /* skip if the rule af doesn't match redirect af */
+ if ((r->af && r->af != h->af) && /* exclude af-to */
+ !(r->naf && h->af == r->naf))
+ continue;
+ /*
+ * fail if the chosen af is not universal for the
+ * whole supplied redirect address pool
+ */
+ if (!r->af && af && af != h->af) {
+ yyerror("%s spec contains addresses with "
+ "different address families",
+ allow_if ? "routing" : "translation");
+ return (1);
+ }
+ if (!r->af)
+ af = h->af;
i++;
- if (h->af && !r->af)
- r->af = h->af;
- } else if (r->naf && h->af == r->naf)
+ } else {
+ /*
+ * we silently allow any not af-specific host specs,
+ * e.g. (em0) and let the kernel deal with them
+ */
i++;
+ }
}
+ /* set rule af to one chosen above */
+ if (!r->af && af)
+ r->af = af;
if (i == 0) { /* no pool address */
yyerror("af mismatch in %s spec",
allow_if ? "routing" : "translation");
return (1);
@@ -4695,10 +4723,11 @@ expand_rule(struct pf_rule *r, int keeprule, struct
node_if *interfaces,
bcopy(&rb.nat.addr, &dsth->addr, sizeof(dsth->addr));
dsth->ifname = NULL;
dsth->next = NULL;
dsth->tail = NULL;
+ bzero(&binat, sizeof(binat));
if ((binat.rdr =
calloc(1, sizeof(*binat.rdr))) == NULL)
err(1, "expand_rule: calloc");
bcopy(nat->rdr, binat.rdr, sizeof(*binat.rdr));
bcopy(&nat->pool_opts, &binat.pool_opts,
