Backport OpenBSD syntax of "scrub" option for "match" and "pass" rules

[vegeta_tuxpowered.net]

Summary

This patch introduces OpenBSD syntax of "scrub" option for "match" and "pass" rules and the "set reassemble" flag. The patch is backward-compatible, pf.conf can be still written in FreeBSD-style.

A patch for tests will follow.

Sponsored by: InnoGames GmbH
Obtained from: OpenBSD

[kp]

Is this against a recent main? Because that has a (much more basic) "match" implementation already.

(I've not had a deeper look yet, and don't expect to be able to in the next few days either.)

[vegeta_tuxpowered.net]

! In D38025#863920, @kp wrote:
Is this against a recent main? Because that has a (much more basic) "match" implementation already.

Just in case I have rebased on main just a moment ago and the resulting diff is identical. This patch extends your implementation of "match" rules. I have changed the review description to better describe what this patch is about. My goal is to introduce syntax from OpenBSD: pass and match rules performing scrub and nat and eventually add nat64 support. This is the 1st patch which only adds the scrub functionality.

(I've not had a deeper look yet, and don't expect to be able to in the next few days either.)

No worries, take your time.

BTW, why I'm even looking into this topic in the first place. First is of course NAT64. Supporting legacy IPv4 is for me a waste of time and a security risk. I want my servers to run only on IPv6 but they still need to be accessible from IPv4 clients and they must be able to access some external IPv4 services too.

I thought about porting only the "af-to" target from OpenBSD but then I've looked at their syntax and noticed how much cleaner it is. The current situation of "scrub" rules in FreeBSD is problematic. They are stateless and evaluated twice (pf_normalize_ip/6 pf_normalize_tcp). I've ran into performance issues with this in a setup where I had to provide different MSS clamping to different networks. Furthermore they provide limited filtering, for example you can't "scrub" only particular icmp type. The issue with limited filtering applies to NAT rules too. I had a real case where I wanted to "rdr" icmp/icmpv6 echo requests. I forward some traffic to some systems behind a pf-based loadbalancer but for pings the loadbalancer itself must answer. But the nat ruleset does not allow for granular filtering of icmp types and codes. So I ended up with "rdr" of all icmp and icmpv6 traffic to the loadbalancer which broke IPv6 because the NDP messages got affected by the "rdr" rule too. I ended up with "rdr" on traffic from 2003::/3 (which holds all GUAs) but that still applies to all ICMPv6 messages, not just echo requests.

Fully-capable and stateful filtering, like OpenBSD does, would solve all those issues. I hope you now understand why I'm pushing for OpenBSD syntax in FreeBSD's pf.

And the last but not the least, I'd like to ask a question of compatibility. The patch above is backward compatible. This means that pf_normalize_ip/6() checks the rules in the "scrub" ruleset and runs some code for actions of those rules. When one migrates their ruleset to the OpenBSD syntax, some of code paths become unnecessary. I was thinking to surround some sections with #ifdef COMPAT_FREEBSD13 (or 14, or whenever we can get this merged) and provide alternative, simpler code. Would you like to see such changes?

[freebsd_igalic.co]

I've really just looked properly at the man page, since i don't know the code, so that's what my comments are about.

other than that, i find the sprinkling of "int" in this code base questionable. int is a pretty meaningless type when going from one OS, ISA & compiler combination to the next

share/man/man5/pf.conf.5

1494

I reckon queue and rtable should be marked up here

1498

this should be: "the logging is done"

1500

this should probably be understood as a warning

[vegeta_tuxpowered.net]

Update the diff with context.

[kp]

We're failing a couple of test cases in /usr/tests/sbin/pfctl with this patch. At least one of the failures is due to the duplicate allow-opts, so an easy fix.

Overall very impressive work, and seemingly close to ready to go in.

sbin/pfctl/parse.y

5304

Can we keep state on match rules?

6246

This line is redundant. 6248 already has match. (Also, this list needs to be sorted alphabetically, so it's in the wrong place too).

sbin/pfctl/pfctl_parser.c

1165

We've got this twice. See also line 1127.

sys/netpfil/pf/pf.c

4891

M_NOWAIT can fail allocation. We need to deal with the ri == NULL case here.

4930

if (r->log && pd->act.log) ?
Also, given the pf_rule_to_actions() above, is it even possible for pd->act.log to be true and r->log be false? Or vice versa?

sys/netpfil/pf/pf_ioctl.c

5642

V_pf_status.reass = *reass & PF_REASS_ENABLED?
If nothing else that prevents us from accidentally setting random flags briefly, which might turn into a very difficult to debug race condition years down the line.

sys/netpfil/pf/pf_norm.c

1219

Does this mean we scrub even when there's no scrub rule?

[vegeta_tuxpowered.net]

Address issues raised in some of comments.

[vegeta_tuxpowered.net]

sbin/pfctl/parse.y

5304

I don't think we should. As far as I understand OpenBSD the "match" rules only allow for setting multiple parameters (like scrub or nat) without specifying if packet is passed or blocked. Match condition A? Then set mss. Match condition B? Then log. Finally match condition C, "pass" and create a state, thus applying actions from "match" rules. If "match" rules created states how would they be different from "pass" rules?

I've checked how this works on FreeBSD 13.1 GENERIC (since it has some support of pass rules) and it seems broken. You can specify "match … keep state". The syntax is accepted but it does not result in creation of a state.

Older FreeBSD code would ensure that "keep state" is possible only for "not-block" and the only "not-block" would be "pass". But since ef950daa35d43dd396958ca28ce9de0514daf873 there is also "pass". Which can't create state. But the syntax allows for it.

Maybe this particular line of the my patch could be even be ported to the existing FreeBSD 13?

6246

Fixed.

sys/net/pfvar.h

1023

@kp I'm unsure if I did the math correctly here. Now I've updated it to 336B, because its dividable by 64b/8B and 340B * 12 = 4032, so it fits within a page. Is this proper?

sys/netpfil/pf/pf.c

4891

Fixed.

4930

Indeed that was wrong. With logging of "match" rules directly when matching happens, and the same for the NAT rule, there is no need for those complicated checks. In fact this is what OpenBSD did too later. I've updated the code to match that of OpenBSD right before introduction of PF_LOG_MATCHES.

sys/netpfil/pf/pf_ioctl.c

5642

That would rather be

V_pf_status.reass = *reass & (PF_REASS_ENABLED|PF_REASS_NODF);

sys/netpfil/pf/pf_norm.c

1219

I think the issue is that there is no good definition of what scrubbing actually is. This single name carries out two very distinctive operations:

1. Sanitation of IPv4¹, IPv6¹ and TCP².
2. Applying scrub modifications to the IP and TCP layers³.

As far as I understand in FreBSD sanitation is optional. Once say "no scrub" (or even if you have no scrub rules? Is there a default scrub rule?) you forfeit all benefits of sanitation. Even if your goal was just to, for example, not have MSS clamping for particular traffic.

OpenBSD's approach is that general sanitation is unavoidable and fragmentation handling is controlled by the set reassemble yes | no [no-df] option in pf.conf.

The way I wrote this patch is that if "scrub" rules are present they are obeyed for sanitation of packets. If there are none, sanitation happens always, like in OpenBSD. But now I see that I had forgotten to make such override for TCP. I'll update the patch alter to address this issue.

[1] Length in header not matching the measured length, too small header length, fragment flag without fragment offset, badly-aligned fragment offset, too big total length of fragmented packets, lack of jumbo flag for big IPv6 packets, broken IPv6 options.

[2] Improper TCP flags, bad header length.

[3] Enforce MTU / Hop Limit, enforce TCP MSS.

[vegeta_tuxpowered.net]

Make normalization functions behave in more straightforward manner. If there are no scrub rules then the normalization of IP and TCP is enforced just like in OpenBSD. Otherwise if scrub rules are present, obey them.

[vegeta_tuxpowered.net]

sys/net/pfvar.h

981

At this moment pfync and pfctl are not yet fixed to handle the 16-bit flags. I'll update them later.

[vegeta_tuxpowered.net]

sys/netpfil/pf/pf.h

619

This conflicts with PFRULE_DN_IS_PIPE which is also assigned to pf_rule_actions->flags.

[kp]

sbin/pfctl/parse.y

5304

I don't think we should. As far as I understand OpenBSD the "match" rules only allow for setting multiple parameters (like scrub or nat) without specifying if packet is passed or blocked. Match condition A? Then set mss. Match condition B? Then log. Finally match condition C, "pass" and create a state, thus applying actions from "match" rules. If "match" rules created states how would they be different from "pass" rules?

Ah, I see.

Maybe this particular line of the my patch could be even be ported to the existing FreeBSD 13?

Possibly. I try to avoid behaviour changes on stable branches, but perhaps this is a bug fix more than a behaviour change.

sbin/pfctl/pfctl_parser.c

1129

This seems to be why the pfctl pf0039 test fails. We no longer print 'fragment' on the rule that had it set.

sys/net/pfvar.h

981

We're going to have to be very careful about pfsync compatibility then.

1023

That should be okay, yes. The intent behind this (I believe, I didn't write this) is to keep struct pf_kstate compact for performance reasons. It's almost inevitable that we're going to keep growing it with new features though.

[vegeta_tuxpowered.net]

Fix "fragment" in printing rules. Use proper integer types. Restore actions for pfsynced states. Expand pfsync_state->state_flags to 16b.

[vegeta_tuxpowered.net]

sbin/pfctl/pfctl_parser.c

1129

Fixed.

sys/net/pfvar.h

981

I've done it the same way as OpenBSD did during transition. There are 2 spare bytes, I've added a new struct member there. In sending and receiving both the the old 8-bit and the new 16-bit struct members are used.

sys/netpfil/pf/pf.h

619

@kp I think the simplest way to address this conflict (as I'd rather keep PFSTATE_.* flags with the same values as OpenBSD does) is to provide pf_rule_actions->dnflags and set it in pf_rule_to_actions(). This is is what I have uploaded now.

However it makes me raise a question why are there are now 2 separate variables for those flags (pfctl_rule/pf_krule->free_flags and pfctl_eth_rule/pf_keth_rule->dnflags). And there still is the 32-bit pfctl_rule/pf_krule->rule_flags which could store those flags just fine if they are moved to higher bits.

[kp]

sys/netpfil/pf/pf.h

619

It's 'dnflags' for ethernet rules, 'free_flags' for layer-3 rules.

We can't move the flags, because that'll break ABI compatibility with userspace.

[vegeta_tuxpowered.net]

Created PFSTATE_DN_IS_PIPE and PFSTATE_DN_IS_QUEUE mapped from corresponding PFRULE_DN_IS_.*. Grouped all of PFRULE_.* and PFSTATE_.* flags together, aligned them and documented to which variables they get assigned.

[vegeta_tuxpowered.net]

sys/netpfil/pf/pf.h

619

Please find the updated patch where DN flags follow the existing mechanism of translating PFRULE_.* flags on rules into PFSTATE_.* flags on states.

With flags translated onto pf_kstate->state_flags (s->state_flags |= pd->act.flags in pf_create_state()) in we can hope to be able to make those flags work properly with pfsync too one day. That is out of scope of this review and I'll submit a proposal for pfsync separately soon-ish but let's already have code preparing us for that opportunity.

[kp]

sys/netpfil/pf/pf.h

619

Sounds like a good plan.

I expect to land this (and the tests) late next week or early the week after, once I'm back from AsiaBSDCon.

[vegeta_tuxpowered.net]

sys/netpfil/pf/if_pfsync.c

585 ↗

(On Diff #119762)

I've borrowed this idea of combining the 8- and 16-bit state flags from OpenBSD before I came up with D39392. If D39392 gets approved, this part could be removed.

[vegeta_tuxpowered.net]

@kp , I see that you've merged it on 2023-04-13. But this review is still opened. What's the procedure here, will you close it or should I abandon it?

[kp]

You may need to abandon it. I must have done something wrong so the trigger didn’t fire.

[vegeta_tuxpowered.net]

This revision has been merged.