Created attachment 232517 [details] Patch that fixes the issue After commit 8c1400b0a1083c9e2bf8f3418eb3e3cfba1a8444 (review D34443 ), the printing of rules in nested anchors broke. The attached patch fixes it. Steps to reproduce: 1) in pf.conf add some nested anchors, eg: anchor test { anchor test2 { block in proto udp from any to any port 3999 } } 2) load the rules 3) pfctl -a "*" -sr Output before the patch: anchor "test" all { anchor "test2" all { pfctl: DIOCGETRULES: Invalid argument } } Output after the patch: anchor "test" all { anchor "test2" all { block drop in proto udp from any to any port = 3999 } } Note that some printing of some nested anchors is still broken (but likely it was broken even before the above commit). See bug #262590 .
The pfctl_show_nat function likely needs a similar fix. It is unclear to me whether the pfctl_get_rule calls in this function should also use path in place of anchorname (pfctl_get_rule is not used in pfctl_show_rules)
Presumably this is the same issue as #262590 ?
(In reply to Kristof Provost from comment #2) Sadly it is not. I'm working on a patch for that, but it is more convoluted. The issue there are the wildcards (*).
(In reply to Matteo Riondato from comment #3) To be more precise, the real issue in bug #262590 are wildcards used in pf.conf to include all anchors nested under another anchor. The bug there also mentions the issue I'm addressing here, but they are two separate issues, not the same one as I previously thought.
Can you try this: diff --git a/lib/libpfctl/libpfctl.c b/lib/libpfctl/libpfctl.c index 8f064594260b..884431aa73fc 100644 --- a/lib/libpfctl/libpfctl.c +++ b/lib/libpfctl/libpfctl.c @@ -894,7 +894,7 @@ pfctl_add_rule(int dev, const struct pfctl_rule *r, const char *anchor, int pfctl_get_rules_info(int dev, struct pfctl_rules_info *rules, uint32_t ruleset, - const char *path) + const char *path, char *anchor_call) { struct pfioc_rule pr; int ret; @@ -911,6 +911,9 @@ pfctl_get_rules_info(int dev, struct pfctl_rules_info *rules, uint32_t ruleset, rules->nr = pr.nr; rules->ticket = pr.ticket; + if (anchor_call) + strlcpy(anchor_call, pr.anchor_call, MAXPATHLEN); + return (0); } diff --git a/lib/libpfctl/libpfctl.h b/lib/libpfctl/libpfctl.h index b7f703b64def..536377c38404 100644 --- a/lib/libpfctl/libpfctl.h +++ b/lib/libpfctl/libpfctl.h @@ -364,7 +364,7 @@ int pfctl_get_eth_rule(int dev, uint32_t nr, uint32_t ticket, int pfctl_add_eth_rule(int dev, const struct pfctl_eth_rule *r, const char *anchor, const char *anchor_call, uint32_t ticket); int pfctl_get_rules_info(int dev, struct pfctl_rules_info *rules, - uint32_t ruleset, const char *path); + uint32_t ruleset, const char *path, char *anchor_call); int pfctl_get_rule(int dev, uint32_t nr, uint32_t ticket, const char *anchor, uint32_t ruleset, struct pfctl_rule *rule, char *anchor_call); diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c index ffd978b304cb..6b0ed98aeb77 100644 --- a/sbin/pfctl/pfctl.c +++ b/sbin/pfctl/pfctl.c @@ -1150,14 +1150,16 @@ pfctl_show_rules(int dev, char *path, int opts, enum pfctl_show format, snprintf(&path[len], MAXPATHLEN - len, "%s", anchorname); if (opts & PF_OPT_SHOWALL) { - ret = pfctl_get_rules_info(dev, &ri, PF_PASS, anchorname); + ret = pfctl_get_rules_info(dev, &ri, PF_PASS, path, + anchor_call); if (ret != 0) { warn("DIOCGETRULES"); goto error; } header++; } - ret = pfctl_get_rules_info(dev, &ri, PF_SCRUB, anchorname); + ret = pfctl_get_rules_info(dev, &ri, PF_SCRUB, path, + anchor_call); if (ret != 0) { warn("DIOCGETRULES"); goto error; @@ -1195,7 +1197,8 @@ pfctl_show_rules(int dev, char *path, int opts, enum pfctl_show format, } pfctl_clear_pool(&rule.rpool); } - ret = pfctl_get_rules_info(dev, &ri, PF_PASS, anchorname); + ret = pfctl_get_rules_info(dev, &ri, PF_PASS, path, + anchor_call); if (ret != 0) { warn("DIOCGETRULES"); goto error; @@ -1294,7 +1297,8 @@ pfctl_show_nat(int dev, char *path, int opts, char *anchorname, int depth) char *p; for (i = 0; i < 3; i++) { - ret = pfctl_get_rules_info(dev, &ri, nattype[i], anchorname); + ret = pfctl_get_rules_info(dev, &ri, nattype[i], anchorname, + anchor_call); if (ret != 0) { warn("DIOCGETRULES"); return (-1); That should restore the previous behaviour, and appears to be working for me.
That is, I suspect it'll fix both this and 262590
(In reply to Kristof Provost from comment #6) This fixes the issue I outline here (but my patch already did that, and it is simpler: I don't think copying anchor_name is needed, as least for this specific issue). The issue in bug #262590 is a little subtler: 1) we don't recursively print rules in anchors included in pf.conf with a wildcard, for example, using "anchor blacklistd/*". Here is the output from "pfctl -a "*" -sr" with a pfctl patched with your patch: anchor "*" in on lagg0 all { pfctl: DIOCGETRULES: Invalid argument } Note that that "*" is wrong, it should be "blacklistd", but the assignment of p around line 1250 of pfctl.c is too naive, as it should take into account the current values of path and of anchor_call before deciding what path/anchorname to use next (indeed the error is due to pfctl_get_rules_info being called with a path of "*") 2) we don't recursively print rules in other cases, but it is a little weird when it happens, as it only happens in some cases. For example (with pfctl patched with you patch): On one hand: # /usr/obj/usr/src/amd64.amd64/sbin/pfctl/pfctl -a "blacklistd/*" -sr # (no output) but there is a nested anchor "2200" under "blacklistd", and its rules should be printed by the above command. Indeed, if we try to print its rules directly it works: # /usr/obj/usr/src/amd64.amd64/sbin/pfctl/pfctl -a "blacklistd/2200" -sr block drop in quick proto tcp from <port2200> to any port = 2200 # Although, it is really weird, because the following works: # /usr/obj/usr/src/amd64.amd64/sbin/pfctl/pfctl -a "test/*" -sr anchor "test2" all { block drop in proto udp from any to any port = 3999 } # In any case, the situation there is more complex than what is addressed with this patch, which is still needed independently.
To give you an idea of what would be necessary, but not sufficient, to solve the issue in bug #262590, you need something like the following patch (to be applied on top of the patch you posted in #5 This patch solves a subset of the issue outlined in bug #262590, in that at least now rules directly inside an anchor included in pf.conf with "anchor myanchor/*" are printed (but nested anchors are still not printed). To solve the real issue there, one likely needs to get the DIOCGETRULESETS ioctl involved, IMHO. diff -u pfctl.c pfctl.c.mine --- pfctl.c 2022-03-17 13:57:58.290678000 -0400 +++ pfctl.c.mine 2022-03-17 13:57:11.836505000 -0400 @@ -1251,10 +1251,20 @@ *(--p) == '/')) || (opts & PF_OPT_RECURSE))) { brace++; if ((p = strrchr(anchor_call, '/')) != - NULL) + NULL && path[0] && + strnstr(anchor_call, path, p - anchor_call)) p++; - else + else { + int aclen = strlen(anchor_call); + if (anchor_call[aclen - 1] == '*') { + int idx = aclen - 2; + if (! (idx >= 0 && + anchor_call[idx] == '/')) + idx = 0; + anchor_call[idx] = '\0'; + } p = &anchor_call[0]; + } } else p = &anchor_call[0];
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=cd8438e5a3a425ea44b261758e17fe62ebb45fce commit cd8438e5a3a425ea44b261758e17fe62ebb45fce Author: Matteo Riondato <matteo@FreeBSD.org> AuthorDate: 2022-03-17 18:48:28 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2022-03-17 21:37:05 +0000 pfctl: fix retrieving nested anchors PR: 262622 MFC after: 1 week Reviewed by: kp sbin/pfctl/pfctl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=d56abf1d16238eeaab74509999a8f9303534f918 commit d56abf1d16238eeaab74509999a8f9303534f918 Author: Matteo Riondato <matteo@FreeBSD.org> AuthorDate: 2022-03-17 18:48:28 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2022-03-24 09:44:40 +0000 pfctl: fix retrieving nested anchors PR: 262622 MFC after: 1 week Reviewed by: kp (cherry picked from commit cd8438e5a3a425ea44b261758e17fe62ebb45fce) sbin/pfctl/pfctl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=5567b132a40f44a87884cb29d14faab39d24d832 commit 5567b132a40f44a87884cb29d14faab39d24d832 Author: Matteo Riondato <matteo@FreeBSD.org> AuthorDate: 2022-03-17 18:48:28 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2022-03-24 09:44:31 +0000 pfctl: fix retrieving nested anchors PR: 262622 MFC after: 1 week Reviewed by: kp (cherry picked from commit cd8438e5a3a425ea44b261758e17fe62ebb45fce) sbin/pfctl/pfctl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Closing as it seems to be done at this point. Thank you Kristof!