On 25 Oct 2021, at 17:38, Klemens Nanni wrote:
> 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.
Super. Glad to hear my worries were unfounded.
Best regards,
Kristof
