On Mon, Oct 25, 2021 at 05:18:48PM +0200, Kristof Provost wrote:
> On 25 Oct 2021, at 17:06, Alexandr Nedvedicky wrote:
> > Hello,
> >
> > On Fri, Oct 22, 2021 at 02:47:07PM +0200, Kristof Provost wrote:
> >> On 21 Oct 2021, at 20:33, Alexandr Nedvedicky wrote:
> >>> Hello,
> >>>
> >>>> I’ve had a bug report against FreeBSD’s pfctl which I think also applies
> >>>> to OpenBSD.
> >>>>
> >>>> The gist of it is that the macro expansion in labels/tags is done prior
> >>>> to
> >>>> the rule optimisation, which means that at least the $nr expansion can be
> >>>> wrong.
> >>>
> >>> I agree OpenBSD suffers from the same issue. Below is a diff for
> >>> OpenBSD.
> >>> The FreeBSD diff, which we got from Kristof, merged with rejects.
> >>> While
> >>> dealing with them, I came with slightly different version of the fix,
> >>> which
> >>> minimizes diff.
> >>>
> >> I’d initially gone that route as well, but decided I wanted all of the
> >> macro
> >> expansions to be done at the same time. In part to keep things simple, but
> >> also because I wasn’t 100% sure the rule number one would be the only one
> >> with issues. For example, if the optimiser decides to merge rules because
> >> it
> >> can merge address ranges $srcaddr or $dstaddr might end up being wrong.
> >
> > Klemens (kn@...) and I poked into it for a bit and it looks like
> > optimizer
> > won't attempt to merge rules, which have a label.
> >
> That is correct, but macros can also occur in tagname and match_tagname,
> which will not stop the optimiser from merging rules.
Yes, pfctl_optimize.c is pretty obvious in this regard.
To clarify: we did defer expansion of the *`$nr' macro alone* to after
superblocks have been created as that is the only step needed to fix
the bug you reported.
To illustrate:
$ cat tag.ruleset
pass to ::1
pass to ::2
pass to ::3
pass to ::4
pass to ::5
pass to ::6
pass tag "$nr"
$ pfctl -vvnf./tag.ruleset
Loaded 714 passive OS fingerprints
table <__automatic_0> const { ::1 ::2 ::3 ::4 ::5 ::6 }
@0 pass inet6 from any to <__automatic_0:0> flags S/SA
@1 pass all flags S/SA tag 1
$ cat label.ruleset
pass to ::1
pass to ::2
pass to ::3
pass to ::4
pass to ::5
pass to ::6
pass label "$nr"
$ pfctl -vvnf./label.ruleset
Loaded 714 passive OS fingerprints
table <__automatic_0> const { ::1 ::2 ::3 ::4 ::5 ::6 }
@0 pass inet6 from any to <__automatic_0:0> flags S/SA
@1 pass all flags S/SA label "1"
As far as *I* understand, `$nr' is the only macros that needs fixing.
I tested the other macros but could not find any combination of rules
and macros that would yield bogus labels or tags.
