pf: Document broadcast/multicast forwarding through route-to

[rcm]

Summary

pf_route() and pf_route6() forward broadcast and multicast traffic
when a route-to rule matches, without any check against the output
interface's broadcast domain. This is a deliberate property of the
route option code path, but it is not documented and the workaround
is non-obvious.

Document the behavior in pf.conf(5) with example block-out rules on
the target interface, scoped with the received-on qualifier so that
only forwarded traffic is dropped while the router's own broadcast
and multicast traffic continues to pass.

Add regression tests covering the full broadcast/multicast and
forwarded/local matrix on both IPv4 and IPv6.

Sponsored by: Rubicon Communications, LLC ("Netgate")

[glebius]

I always assumed "policy routing" by packet filters a tool that allows to shoot into ones leg. I can imagine some weird scenarios where people would use pf to actually inject packets where it won't be routed by the normal stack.

[rcm]

In D56559#1294663, @glebius wrote:

I always assumed "policy routing" by packet filters a tool that allows to shoot into ones leg. I can imagine some weird scenarios where people would use pf to actually inject packets where it won't be routed by the normal stack.

That's fair. Though, currently, an operator who uses pf route-to without realizing it can emit L2 broadcasts across broadcast domains is foot-shooting themselves unknowingly. :)

[eugen_grosbein.net]

In D56559#1294664, @rcm wrote:

In D56559#1294663, @glebius wrote:

I always assumed "policy routing" by packet filters a tool that allows to shoot into ones leg. I can imagine some weird scenarios where people would use pf to actually inject packets where it won't be routed by the normal stack.

That's fair. Though, currently, an operator who uses pf route-to without realizing it can emit L2 broadcasts across broadcast domains is foot-shooting themselves unknowingly. :)

We do not prevent a superuser from foot-shooting and we do not assume that superuser is stupid. A warning should be enough.

[rcm]

In D56559#1294669, @eugen_grosbein.net wrote:

In D56559#1294664, @rcm wrote:

In D56559#1294663, @glebius wrote:

I always assumed "policy routing" by packet filters a tool that allows to shoot into ones leg. I can imagine some weird scenarios where people would use pf to actually inject packets where it won't be routed by the normal stack.

That's fair. Though, currently, an operator who uses pf route-to without realizing it can emit L2 broadcasts across broadcast domains is foot-shooting themselves unknowingly. :)

We do not prevent a superuser from foot-shooting and we do not assume that superuser is stupid. A warning should be enough.

Another potential solution to this that came up in discussion with @kp was using block out quick rule(s) to plug up any problematic leaks.

So I think the question is really: are we okay with pf_route
forwarding broadcasts when ip_forward (nominally) doesn't? If yes, then I do agree at least a few lines in the man page be introduced that calls out this difference, and maybe prescribes example rules that can be used to plug any undesirable leaks.

[lytboris_gmail.com]

sys/netpfil/pf/pf.c

9810 ↗

(On Diff #176027)

To cope with concerns mentioned by @glebius I'd put here a deprecated-upon-creation sysctl to revert packet processing to the previous behavior. This sysctl should be set to "drop, not forward" value by default.

10170 ↗

(On Diff #176027)

Same as above

[kp]

There are good arguments for both blocking and allowing this I believe.
I'm not entirely sure where I fall. On the one hand, yes, users should be allowed to shoot themselves in the foot if they really want to, but on the other hand, it's non-obvious that this will happen. There are going to be a lot more users in the "I didn't want this to happen but it did" camp than there'd be in the "I want to do this dumb thing and pf won't let me." camp.

I do also see test failures in sys/netpfil/pf/pfsync:rt_af, sys/netpfil/pf/src_track:sn_types_compat and sys/netpfil/pf/src_track:sn_types_pass with this patch. I've not debugged these, but this change does appear to be responsible.

sys/netpfil/pf/pf.c

9810 ↗

(On Diff #176027)

I really wouldn't do that. We'd be making this already very complex and error-prone code even more complex.

[rcm]

In D56559#1295079, @kp wrote:

I do also see test failures in sys/netpfil/pf/pfsync:rt_af, sys/netpfil/pf/src_track:sn_types_compat and sys/netpfil/pf/src_track:sn_types_pass with this patch. I've not debugged these, but this change does appear to be responsible.

I initially hit a failure in pfsync:rt_af but after rebuilding and trying again, I am no longer hitting that failure (or failures in src_track:*):

pfsync:rt_af  ->  passed  [7.679s]
route_to:bcast_baseline  ->  passed  [4.693s]
route_to:bcast_directed  ->  passed  [1.755s]
route_to:bcast_limited  ->  passed  [2.479s]
src_track:sn_types_compat  ->  passed  [4.351s]
src_track:sn_types_pass  ->  passed  [3.050s]

Weird.

[glebius]

In D56559#1294810, @rcm wrote:

So I think the question is really: are we okay with pf_route
forwarding broadcasts when ip_forward (nominally) doesn't? If yes, then I do agree at least a few lines in the man page be introduced that calls out this difference, and maybe prescribes example rules that can be used to plug any undesirable leaks.

I think we are okay. Packet filters are really designed to make network stacks to malfunction.

A good documentation change would be better than a policy.

[rcm]

In D56559#1295233, @glebius wrote:

In D56559#1294810, @rcm wrote:

So I think the question is really: are we okay with pf_route
forwarding broadcasts when ip_forward (nominally) doesn't? If yes, then I do agree at least a few lines in the man page be introduced that calls out this difference, and maybe prescribes example rules that can be used to plug any undesirable leaks.

I think we are okay. Packet filters are really designed to make network stacks to malfunction.

A good documentation change would be better than a policy.

That's fine by me. I will pivot to an addition to pf.conf(5) and tweak the test cases here in the original patch to instead use rules to plug the leaks.

[rcm]

Okay, I think I've got this sorted now. The patch now leaves pf.c untouched and instead:

• Adds a note to pf.conf(5) ROUTING documenting the broadcast/multicast forwarding gap and showing four canonical block-out rules (limited bcast, directed bcast, v4 mcast, v6 mcast) scoped with received-on any to avoid blocking locally originated traffic.
• Adds test cases covering the documented use case and the block-out rules.

[kp]

LGTM.

I suppose we could spell the example rules like this too:
block out quick on $wan from any to { 255.255.255.255, ($wan:broadcast), 224.0.0.0/4, ff00::/8 } received-on any
but they're fine as they are. They result in the same rules in the kernel anyway.

(Please do shorten the lines in the commit message. See e.g. https://freebsdfoundation.org/wp-content/uploads/2020/11/Writing-Commit-Messages.pdf)

[glebius]

Thanks!