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.
On 3 September 2023 20:09:10 CEST, Alexandr Nedvedicky <[email protected]> wrote: >Hello, > >On Sun, Sep 03, 2023 at 06:29:51PM +0200, Alexander Bluhm wrote: >> On Sun, Sep 03, 2023 at 06:17:12PM +0200, Florian Obser wrote: >> > On 2023-09-03 18:13 +02, Alexander Bluhm <[email protected]> wrote: >> > > On Sun, Sep 03, 2023 at 05:59:18PM +0200, Alexandr Nedvedicky wrote: >> > >> Hello, >> > >> >> > >> On Sun, Sep 03, 2023 at 05:10:02PM +0200, Alexander Bluhm wrote: >> > >> > On Sun, Sep 03, 2023 at 04:12:35AM +0200, Alexandr Nedvedicky wrote: >> > >> > > in my opinion is to fix pf_match_rule() function, so ICMP error >> > >> > > message >> > >> > > will no longer match 'keep state' rule. Diff below is for IPv4. I >> > >> > > still >> > >> > > need to think of more about IPv6. My gut feeling is it will be very >> > >> > > similar. >> > >> > >> > >> > Thanks for the detailed analysis. >> > >> > >> > >> > You proposed fix means that our default pf would block icmp error >> > >> > packets now. >> > >> > >> > >> > block return # block stateless traffic >> > >> > pass # establish keep-state >> > >> > >> > >> > To have the old behaviour one would write >> > >> >> > >> I think icmp error message, if legit, is allowed because it matches >> > >> state created by 'pass' rule. At least this is my understanding. >> > >> >> > >> Or is there something else going on which I'm missing? >> > > >> > > If icmp packets are legit, they work with the existing pass keep-state >> > > rule in default pf.conf. >> > > >> > > For passing unrelated icmp packets, e.g. with assymetric routing, >> > > one can add a pass no-state rule. >> > >> > ... which you would have in place already if you have asymmetric >> > routing. Or keep state (sloppy), does it work with sloppy, too? >> >> Passing the icmp packet with keep state sloppy, even in case there >> is no matching state, could make sense. Especially as Florian uses >> sloppy for asymmetric routing. >> >> Should we add "&& (r->rule_flag & PFRULE_STATESLOPPY) == 0" to >> Sasha's diff? Then sloppy rules would pass the packet as before. >> > > Yes, I think it is a good idea. diff below does that. > I also added the same check for ICMPv6 to make behavior > consistent. > >thanks and >regards >sashan > >--------8<---------------8<---------------8<------------------8<-------- >diff --git a/sys/net/pf.c b/sys/net/pf.c >index 4f0fc3f91a9..6c3499af160 100644 >--- a/sys/net/pf.c >+++ b/sys/net/pf.c >@@ -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: -- Sent from a mobile device. Please excuse poor formatting.
