On Mon, Sep 04, 2023 at 03:58:02PM +0200, Alexandr Nedvedicky wrote: > Hello, > > On Mon, Sep 04, 2023 at 03:28:00PM +0200, Alexander Bluhm wrote: > > On Sun, Sep 03, 2023 at 11:00:56PM +0200, Alexandr Nedvedicky wrote: > > > Hello, > > > > > > On Sun, Sep 03, 2023 at 09:26:29PM +0200, Florian Obser wrote: > > > > FYI, I'm not using sloppy, and I don't have a network with asymmetric > > > > routing > > > > at the moment. I only remembered that we used sloppy for a while at my > > > > previous job. I think we settled on no-state because it was faster than > > > > sloppy and less hastle. > > > > > > > > > > From my perspective 'no state' vs. 'keep state (sloppy)' are valid > > > approaches. > > > Both are equally good. Perhaps 'no state' option keeps code bit more > > > simple. > > > Because if we will go with sloppy state, then we need to include a > > > small > > > tweak to pf_test_rule() too, so 'keep state' (and nat-to) are not > > > ignored. > > > > I do not think that ICMP error messages should create a state. If > > the rule is sloppy, just pass them. nat-to does not work in this > > case, but I would ignore that. If you use sloppy states, don't be > > surprized about strange effects in corner cases. > > > > Understood. However would not be better to allow 'unsolicited' icmp > error messages by 'no state' rules only? Such approach makes me > slightly more comfortable, because the ruleset behavior is bit more > predictable: > > keep state > keep state (sloppy) > don't match ICMP error replies, because those should match > existing state. > > no state > option can match any packet (including 'unsolicited' icmp error > replies) so firewall admins can deploy workarounds to address some > awkward corner cases > > may be I'm overthinking it given the issue has not been noticed for ages.
I think we are bikeshedding the solution for a problem that nobody had in real life for 20 years. Peter wants to block the packets and Florian wants sloppy rules. Just commit the two chunks below. And never ask again who has a problem with nat-to and stateless icmp error packets :-) Maybe write /* icmp error packet must match existing state */ clarify that comments talk about errors and not ping. > @@ -4148,6 +4148,10 @@ enter_ruleset: > (r->rule_flag & PFRULE_STATESLOPPY) == 0 && > ctx->icmp_dir != PF_IN), > TAILQ_NEXT(r, entries)); > + /* icmp packet must match existing state */ > + PF_TEST_ATTRIB(r->keep_state && ctx->state_icmp && > + (r->rule_flag & PFRULE_STATESLOPPY) == 0, > + TAILQ_NEXT(r, entries)); > break; > > case IPPROTO_ICMPV6: > @@ -4165,6 +4169,10 @@ enter_ruleset: > ctx->icmp_dir != PF_IN && > ctx->icmptype != ND_NEIGHBOR_ADVERT), > TAILQ_NEXT(r, entries)); > + /* icmp packet must match existing state */ > + PF_TEST_ATTRIB(r->keep_state && ctx->state_icmp && > + (r->rule_flag & PFRULE_STATESLOPPY) == 0, > + TAILQ_NEXT(r, entries)); > break; > > default:
