tech-net archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: Dynamic & static address grouping in NPF
> On 31 Jul 2025, at 10:41 PM, Christoph Badura <bad%bsd.de@localhost> wrote:
>
> On Thu, Jul 24, 2025 at 09:25:30AM +0000, Emmanuel Nyarko wrote:
>> In an attempt to fix this PR reported by Josh Mayor: https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=59507
>>
>> the crash ( from the PR ) was here "$clientgroup={ $self, $otherip }”
>> $self =. Dynamic, $otherip = raw IP
>>
>> and this is because he was listing a dynamic address first and a raw IP second on the same rule.
>>
>> the basic issue I found was
>> if you have a rule
>> pass from { ifaddrs(wm0), 192.164.64.7 } on the same rule,
>>
>> an assertion failure.
>>
>> /*
>> * L3 block cannot be inserted in the middle of a group.
>> * In fact, it never is. Check and start the group after.
>> */
>> if (ingroup) {
>> assert(ctx->nblocks == ctx->gblock);
>> npfctl_bpf_group_exit(ctx);
>> }
>>
>>
>> This is a comment rmind@ left in a function that fetches the address family.
>>
>> Group here means : a group of addresses ( not the group keyword )
>> { 192.168.65.7, 192.168.64.8, 192.168.64.9 } => a group
>
> No, group here means a grouping on filter conditions as explained here:
> https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_bpf_comp.c?r=1.19#70
>
>> What he means: always load the address family first.
>
> Sort of. What this actually means is that he wants to load the address
> family *once* and before the first static address is filtered.
Then unless i change how groupings are handled in NPF. because
Because it cramps all filtering elements into one group and then this intent of his is violated.
>
>> and when you list a static address, NPF extracts the address family and uses first when evaluating a group.
>>
>> (000) ld M[0] // 4 bytes offset the packet header to get the version
>> (001) jeq #0x04 // the address family fetched on the rule ( version 4 here )
>>
>> /// before you load and compare the addresses.
>
> Yes.
>
>> But NPF doesn’t extract and check the version to use in filtering when you enter a dynamic address,
>>
>> when you put { Ifaddrs(wm0), 192.168.64.7 } in the same group,
>
> Pretty much. That is because "ifaddrs(wm0)" is internally implemented as
> an IFADDR table and it does a BPF_COP call to do the table lookup. And the
> table lookup doesn't care about the BPF code having loaded the address
> family into the BPF memory.
>
>> now when it is now about to evaluate the static IP on rule
>>
>> doesn’t allow fetching the address family because NPF thinks it should be the first thing checked.
>
> Correct.
>
>> So it tries to exit the group,
>
> Where does it try to exit the group? I don't see that.
>
>> With this behaviour , NPF tries to tie each rule to one address family.
>
>> based on this comment by rmind,
>> /*
>> * L3 block cannot be inserted in the middle of a group.
>> * In fact, it never is. Check and start the group after.
>> */
>> if this is a statement we agree with, then I make it a compile time error
>> when you try to insert a raw IP in the middle of a group that begins with
>> a dynamic address.
>
> You misunderstand the statement. It's also a bit misleading. What it says
> is that the L3 block of statements should be the first thing done in such a
> group, if it is necessary. And certainly it shouldn't be done multiple
> times in the group.
> But that comment was added in r1.1 of npf_bpf_comp.c and the filt_addr_list
> stuff was added in r1.55 of npf_parse.y, i.e. much, much later. So this
> comment doesn't reflect current reality.
>
>> you are free to list only dynamic addresses in one group.(that doesn’t
>> seg fault because no address family check )
>
> That's also not accurate.
What do you mean here by “not accurate” ?
Pass from { ifaddrs(wm0), ifaddrs(lo0), ifaddrs(en0) }
this never seg faults. and if you check the BPF code, it doesn’t load
the address family from the memory store. it just loads the table id and
does a BPF_COP call to do a table lookup.
Maybe I don't understand what a dynamic address on a firewall rule is
>
> The following example asserts:
> ----------------------------------------------------------------------------
> group default {
> pass from { ifaddrs(wm0), 192.164.64.7 }
> }
> ----------------------------------------------------------------------------
But according to the NPF manual,
ifaddrs ==> a dynamic list, which i believe to be an addresses whose value isn’t resolved
At the time the rules are compiled.
the statement: “only dynamic addresses in one group doesn’t hit a seg fault"
i meant
Pass from { ifaddrs(wm0), ifaddrs(lo0), ifaddrs(en0) }
and this doesn’t segfault.
my statement said “only dynamic addresses “ and i’m confused why you are including 192.164.64.7
in your example.
>
> This is because, leaving out unimported details, it gets translated into the
> following group
>
> ifnet_table(wm0) or bpf_cidr(192.164.64.8)
>
> and bpf_cidr() tries to add fetch_l3(AF_INET). Which, btw doesn't do any harm.
>
> But the following doesn't.
>
> ----------------------------------------------------------------------------
> group default {
> pass from { 192.164.64.7, ifaddrs(wm0) }
> }
> ----------------------------------------------------------------------------
>
> Because it gets translated into:
>
> bpf_cidr(192.164.64.8/32) or ifnet_table(wm0)
>
> Here bpf_cidr() can add the fetch_l3(AF_INET) but if we're dealing with a
> IPv6 address, the code generated by fetch_l3() will jump passt the
> ifnet_table(wm0) too, I think, which is wrong.
Alright noted. so you think we should separate cidr groupings and table groupings.
>
> Please add test cases. ;-)
>
> But that seems wrong to me. It should get translated into
>
> ( bpf_cidr(192.164.64.8/32) ) or ifnet_table(wm0)
>
> That is, the bpf_cidr() should be put into it's own group.
>
> Consider this:
>
> ----------------------------------------------------------------------------
> group default {
> pass from { 192.164.64.7, 2001:db8::1, ifaddrs(wm0),
> 3fff:1334::/64, 192.0.2.0/24, ifaddrs(wm1) }
> }
> ----------------------------------------------------------------------------
>
> This could be translated into
>
> ( bpf_cidr(192.164.64.7/32) or bpf_cidr(192.0.2.0/24) ) or
> ( bpf_cidr(2001:db8::1/128) or bpf_cidr(3fff:1334::/64) or
> ( ifnet_table(wm0) or ifnet_table(wm1) )
>
> And then bpf_cidr() could add the fetch_l3() at the beginning of each group
> and it would work.
>
> Note that the order of the terms doesn't matter.
so in conclusion you think NPF should allow different family addresses
on one rule and the order shouldn’t matter ?
> Except for performance, of
> course.
>
> --chris
Emmanuel
Home |
Main Index |
Thread Index |
Old Index