Bug 262590
| Summary: | [pf][patch] Anchor "blacklistd/*" not correctly shown in pfctl -a \* -s rules | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Matteo Riondato <matteo> | ||||
| Component: | bin | Assignee: | freebsd-bugs (Nobody) <bugs> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | CC: | jlduran, kp, matteo | ||||
| Priority: | --- | ||||||
| Version: | CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
I should mention this is on FreeBSD 14.0-CURRENT #6 main-fdc418f15-dirty from yesterday (20220315) I believe the problem is in commit 8c1400b0a1083c9e2bf8f3418eb3e3cfba1a8444 (here is the review: https://reviews.freebsd.org/D34443). Specifically, there seems to be some issue with anchor_call: before the change it is updated by the ioctl call (I assume), but after the change, it is never set to anything. Perhaps the pfctl_rules_info should also have a field char *anchor_call, and get_rules_info should populate it? (In reply to Matteo Riondato from comment #2) anchor_call gets populated by pfctl_get_clear_rule(). Perhaps it also needs to be populated by get_rules_info(), but it's not true that it's never set. Can you re-test post 3c3a19d1f42af049e798b193d4fd2a872c7c8fec ? This appears to be working now.
At least, with a test case where I set these rules:
anchor "blacklist" {
anchor "foo" {
pass in from 1.2.3.4
}
}
pfctl -sA -a "blacklist" produces
blacklist/foo
pfctl -sr -a "blacklist/*" produces
anchor "foo" all {
pass in inet from 1.2.3.4 to any flags S/SA keep state
}
pfctl -sr -a "blacklist/foo" produces
pass in inet from 1.2.3.4 to any flags S/SA keep state
and pfctl -sr -a "*" produces
anchor "blacklist" all {
anchor "foo" all {
pass in inet from 1.2.3.4 to any flags S/SA keep state
}
}
That all appears to be correct.
Yes, all of these work now, but the following still doesn't work, which is what I meant to highlight in this issue, but I did not explain myself well.
1) Create the following pf.conf:
pass from any to any
anchor "parent/*"
block in proto tcp from any to any port 12345
2) Load the rules in this pf.conf:
# pfctl -f pf.conf
3) Notice how the rules are already not printed correctly:
# pfctl -a \* -s rules
pass all flags S/SA keep state
anchor "*" all {
pfctl: DIOCGETRULES: Invalid argument
}
block drop in proto tcp from any to any port = 12345
#
4) Now create the following file child.pf:
block in proto udp from any to any port 23456
anchor child {
block in proto icmp
}
block in proto tcp from any to any port 34567
5) Load the rules of child.pf into the "parent" rule:
# pfctl -a parent -f child.pf
6) Notice how the rules are not printed correctly, with the "child" anchor not even being mentioned, but also none of the rules inside the "parent" anchor but outside of "child"
pass all flags S/SA keep state
anchor "*" all {
pfctl: DIOCGETRULES: Invalid argument
}
block drop in proto tcp from any to any port = 12345
There are multiple issues here, but one of them is that the way the pointer p is updatde around line 1250 of pfctl.c is not correct, and it is what is causing the use of "*" instead of "parent" as anchorname in the next recursive call to pfctl_show_rules, and it is such use that causes the "DIOGETRULES: Invalid argument" error. Note that even fixing this specific issue about p would not be sufficient, because the next call would still not be able to print "child" and the rules in it.
(In reply to Matteo Riondato from comment #5) The output in point 6 is again obtained with # pfctl -a \* -s rules Created attachment 232549 [details]
Patch that fixes the issue for me
The attached patch fixes the above case for me, but it may break other cases that I am not aware of, as I cannot understand why the previous use of strrchar would ever be the right thing to do. If you explain it to me, I can update the patch accordingly (there are other things I don't understand here, e.g., why if anchor_call contains '_' then we should do something special).
I assumed that anchor_call will never be shorter than 2 if it has a '*' as last character (because then it must have "/*", but I may be wrong about it, and one should do a dance similar to that done for the parsing of the argument to -a on the command line. Please let me know what you think, and I can update the patch.
(In reply to Matteo Riondato from comment #7) That doesn't look right, no. It strips the trailing '/*', so we end up with 'anchor "parent"' rather than 'anchor "parent/*"', which doesn't seem like what we want. I'm also very confused by the test scenario, given that we have 'parent/*', but then set extra rules in the 'parent' anchor, and not in say 'parent/foo' like I'd expect. (In reply to Kristof Provost from comment #8) We don't end up with "anchor parent", we end up with "parent", rather than with "parent/*": anchor_call does not include the "anchor " part, as far as I can tell. Why wouldn't "parent" be what we want (notice that if you pass something with '/*" to the next recursive call, you get the error). The stripping of "/*" is exactly what happens also when parsing the command line arguments and one gives "-a parent/*". As for the test scenario, please notice that there is a rule inside child too. If you don't like the rules that are inside parent but not inside child, you can ignore them: the issue still exists. Notice though that there is literally nothing that prevent the situation specified in the test. 'anchor "parent/*"' in pf.conf just means: evaluate all the rules in parent and all the rules in any anchor that is a child of parent, recursively. (In reply to Matteo Riondato from comment #9) 'parent' and 'parent/*' mean different things. From the pf.conf man page: Anchors may end with the asterisk (‘*’) character, which signifies that all anchors attached at that point should be evaluated in the alphabetical ordering of their anchor name. For example, anchor "spam/*" will evaluate each rule in each anchor attached to the spam anchor. Note that it will only evaluate anchors that are directly attached to the spam anchor, and will not descend to evaluate anchors recursively. Only if the anchor is listed as 'spam/*' (or parent/*) do we descend into the parent/child anchor, so we need to be careful to maintain the distinction. (In reply to Kristof Provost from comment #10) Hi Kristof, Can you please tell me how you think the output of the test case I give in #5 should be printed? In my opinion, when using "pfctl -a \* -s rules", it should be printed as follows: pass all flags S/SA keep state anchor "parent" all { block drop in proto udp from any to any port = 23456 anchor "child" all { block drop in proto icmp } block drop in proto tcp from any to any port = 34567 } block drop in proto tcp from any to any port = 12345 You seem to think that it should not be printed like the above, and I don't understand why, so I'd like to understand how you think it should be printed. Right now the test case fails to print (in the sense that there are errors emitted when recursing into the parent anchor, see #5), and I don't think it should. Thanks, Matteo (In reply to Matteo Riondato from comment #11) I was convinced the patched version displayed 'anchor "parent"' on `pfctl -sr`, but that's not actually the case. Can you propose a commit message for this? I've not yet fully understood what went wrong and how this fixes it, but it certainly appears to. (In reply to Kristof Provost from comment #12) Thank you, Kristof. A commit message could be: -- START OF COMMIT MESSAGE pf: fix recursive printing of rules. When asked to print rules recursively, correctly recurse for anchors included in pf.conf with "anchorname/*". -- END OF COMMIT MESSAGE But I have to be clear: I am not confident that the patch does not broke other cases. Parts of the surrounding code are quite obscure to me. Also, perhaps a test case would be a good addition as well. I can try to come up with code for it, but it may take me some time. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=d86cf4435021d0abf3f3d65039583ee8cfde1be1 commit d86cf4435021d0abf3f3d65039583ee8cfde1be1 Author: Matteo Riondato <matteo@FreeBSD.org> AuthorDate: 2022-04-13 07:38:44 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2022-04-14 15:25:41 +0000 pfctl: fix recursive printing of rules When asked to print rules recursively, correctly recurse for anchors included in pf.conf with "anchorname/*". PR: 262590 Reviewed by: kp MFC after: 3 weeks sbin/pfctl/pfctl.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=eed3a36c547530be0eaf3ff93ebec9b22cc7bfaa commit eed3a36c547530be0eaf3ff93ebec9b22cc7bfaa Author: Matteo Riondato <matteo@FreeBSD.org> AuthorDate: 2022-04-13 07:38:44 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2022-05-04 12:49:50 +0000 pfctl: fix recursive printing of rules When asked to print rules recursively, correctly recurse for anchors included in pf.conf with "anchorname/*". PR: 262590 Reviewed by: kp MFC after: 3 weeks (cherry picked from commit d86cf4435021d0abf3f3d65039583ee8cfde1be1) sbin/pfctl/pfctl.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=407f7397d69e9c2fd103ee73538589d50216c7ac commit 407f7397d69e9c2fd103ee73538589d50216c7ac Author: Matteo Riondato <matteo@FreeBSD.org> AuthorDate: 2022-04-13 07:38:44 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2022-05-04 06:20:58 +0000 pfctl: fix recursive printing of rules When asked to print rules recursively, correctly recurse for anchors included in pf.conf with "anchorname/*". PR: 262590 Reviewed by: kp MFC after: 3 weeks (cherry picked from commit d86cf4435021d0abf3f3d65039583ee8cfde1be1) sbin/pfctl/pfctl.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) |
(This may be related to #252617) Wildcards in anchor names do not seem to be correctly interpreted by pfctl. Steps to reproduce: 1) Start blacklistd, even with the default /etc/blacklistd.conf 2) Enable blacklistd in sshd_config (UseBlacklist yes), and reload sshd 2) Add 'anchor "blacklistd/*" as the first rule in your pf.conf 3) Reload the rules 4) Fake some wrong logins on ssh (e.g., ssh notauser@yourhost), to trigger the blacklist Now, if I run "pfctl -a blacklistd -sA", I get blacklistd/22 and if I run "pfctl -a blacklistd/22 -s rules, I get: block drop in quick proto tcp from <port22> to any port = ssh which is fine. But if I run "pfctl -a 'blacklistd/*' -s rules", I get no output, which seems weird. Finally, if I run "pfctl -a '*' -s rules", I get: anchor "*" all { pfctl: DIOCGETRULES: Invalid argument } ... other rules, none of which is about the blacklistd anchors. so either I'm confused by how to see the rules for all anchors (under an anchor, possibly), or the wildcard seems to be misinterpreted. At this point I'm not even sure that the rules are loaded correctly, because I cannot verify it with pfctl.