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:
