From 42a4dab6a3d24a72085108b42cc58c29f6872074 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Fri, 9 Oct 2026 23:39:23 +1100 Subject: [PATCH 1/4] Emit one family guard per rule, none in ip/ip6 tables --- internal/nftables/compiler.go | 96 +++++++++++++++++++++++++++--- internal/nftables/compiler_test.go | 78 ++++++++++++------------ internal/nftables/guards_test.go | 92 ++++++++++++++++++++++++++++ 3 files changed, 218 insertions(+), 48 deletions(-) create mode 100644 internal/nftables/guards_test.go diff --git a/internal/nftables/compiler.go b/internal/nftables/compiler.go index 53d5b55..0af760c 100644 --- a/internal/nftables/compiler.go +++ b/internal/nftables/compiler.go @@ -67,10 +67,70 @@ func (c *Compiler) Compile() (*FirewallState, error) { } c.compileMSSClamp(state) limitLogs(state, c.cfg.Settings.LogLimit) + familyGuards(state, c.cfg.Settings.AddressFamily) return state, nil } +// familyGuards leaves each rule at most one meta nfproto guard, ahead of its first network-header +// payload as nft emits it, and drops rules whose guards contradict; nft list cannot decode +// conflicting guards. An ip/ip6 table is its own guard: guards go, other-family rules are dropped. +func familyGuards(state *FirewallState, family config.AddressFamily) { + table := map[config.AddressFamily]byte{config.FamilyIP: unix.NFPROTO_IPV4, config.FamilyIP6: unix.NFPROTO_IPV6}[family] + for chain, rules := range state.Rules { + var out []ManagedRule + for _, r := range rules { + if exprs, ok := normalizeGuards(r.Exprs, table); ok { + r.Exprs = exprs + out = append(out, r) + } + } + state.Rules[chain] = out + } +} + +func normalizeGuards(in []expr.Any, table byte) ([]expr.Any, bool) { + var fam byte + at := -1 + out := make([]expr.Any, 0, len(in)) + for i := 0; i < len(in); i++ { + if p := nfprotoGuard(in, i); p != 0 { + if (fam != 0 && p != fam) || (table != 0 && p != table) { + return nil, false + } + if fam == 0 { + fam, at = p, len(out) + } + i++ + continue + } + out = append(out, in[i]) + } + if fam == 0 || table != 0 { + return out, true + } + if l3 := slices.IndexFunc(out, func(e expr.Any) bool { + p, ok := e.(*expr.Payload) + return ok && p.Base == expr.PayloadBaseNetworkHeader + }); l3 >= 0 && l3 < at { + at = l3 + } + return slices.Insert(out, at, matchNFProto(fam)...), true +} + +// nfprotoGuard is the family a meta nfproto == match at in[i] guards for, else 0. +func nfprotoGuard(in []expr.Any, i int) byte { + m, ok := in[i].(*expr.Meta) + if !ok || m.Key != expr.MetaKeyNFPROTO || i+1 >= len(in) { + return 0 + } + c, ok := in[i+1].(*expr.Cmp) + if !ok || c.Op != expr.CmpOpEq || c.Register != m.Register || len(c.Data) != 1 { + return 0 + } + return c.Data[0] +} + // limitLogs puts a limit in front of every log expression. A limit stops the // whole rule, so like shorewall's separate LOG rule, a log followed by an action // splits into a limited log-only rule and the same rule without the log. A @@ -1697,6 +1757,7 @@ func matchTCPFlags(flags, mask byte) []expr.Any { func matchSmurfDrop(iface string) []expr.Any { var exprs []expr.Any exprs = append(exprs, matchIfaceName(true, iface)...) + exprs = append(exprs, matchNFProto(unix.NFPROTO_IPV4)...) exprs = append(exprs, &expr.Payload{DestRegister: 1, Base: expr.PayloadBaseNetworkHeader, Offset: 12, Len: 4}, &expr.Bitwise{ @@ -1857,32 +1918,49 @@ func parseSPortOrRange(s string) ([]expr.Any, error) { } func matchSourceCIDR(cidr string) ([]expr.Any, error) { - return matchAddrCIDR(cidr, true) + return matchGuardedCIDR(cidr, true) } func matchDestCIDR(cidr string) ([]expr.Any, error) { - return matchAddrCIDR(cidr, false) + return matchGuardedCIDR(cidr, false) } -// matchOrigDest guards the daddr match with the address's nfproto so it is family-correct in the inet table. -func matchOrigDest(addr string) ([]expr.Any, error) { +// matchGuardedCIDR guards an address match with its family's nfproto, unless the list mixes families. +func matchGuardedCIDR(cidr string, isSrc bool) ([]expr.Any, error) { + e, err := matchAddrCIDR(cidr, isSrc) + if err != nil { + return nil, err + } + if p := addrFamily(cidr); p != 0 { + return append(matchNFProto(p), e...), nil + } + return e, nil +} + +// addrFamily is the NFPROTO shared by every address in a (negated) comma list, or 0 if they mix. +func addrFamily(list string) byte { var proto byte - for i, a := range strings.Split(strings.TrimPrefix(addr, "!"), ",") { + for i, a := range strings.Split(strings.TrimPrefix(list, "!"), ",") { a, _, _ = strings.Cut(a, "/") p := byte(unix.NFPROTO_IPV6) if ip := net.ParseIP(a); ip != nil && ip.To4() != nil { p = unix.NFPROTO_IPV4 } if i > 0 && p != proto { - return nil, fmt.Errorf("%q mixes IPv4 and IPv6 addresses", addr) + return 0 } proto = p } + return proto +} + +// matchOrigDest guards the daddr match with the address's nfproto so it is family-correct in the inet table. +func matchOrigDest(addr string) ([]expr.Any, error) { dst, err := matchDestCIDR(addr) - if err != nil { - return nil, err + if err == nil && addrFamily(addr) == 0 { + err = fmt.Errorf("%q mixes IPv4 and IPv6 addresses", addr) } - return append(matchNFProto(proto), dst...), nil + return dst, err } func matchNFProto(proto byte) []expr.Any { diff --git a/internal/nftables/compiler_test.go b/internal/nftables/compiler_test.go index dccbc3a..1bed22b 100644 --- a/internal/nftables/compiler_test.go +++ b/internal/nftables/compiler_test.go @@ -210,7 +210,7 @@ func TestMatchSourceCIDR_IPv6(t *testing.T) { } for _, tt := range tests { - exprs, err := matchSourceCIDR(tt.input) + exprs, err := matchAddrCIDR(tt.input, true) if tt.wantErr { if err == nil { t.Errorf("matchSourceCIDR(%q) should fail", tt.input) @@ -241,7 +241,7 @@ func TestMatchDestCIDR_IPv6(t *testing.T) { } for _, tt := range tests { - exprs, err := matchDestCIDR(tt.input) + exprs, err := matchAddrCIDR(tt.input, false) if tt.wantErr { if err == nil { t.Errorf("matchDestCIDR(%q) should fail", tt.input) @@ -961,7 +961,7 @@ func TestCompile_RateLimit(t *testing.T) { } func TestNegatedAddress(t *testing.T) { - exprs, err := matchSourceCIDR("!192.168.1.0/24") + exprs, err := matchAddrCIDR("!192.168.1.0/24", true) if err != nil { t.Fatalf("matchSourceCIDR(!192.168.1.0/24) error: %v", err) } @@ -973,7 +973,7 @@ func TestNegatedAddress(t *testing.T) { t.Errorf("negated address should use CmpOpNeq, got %v", cmp.Op) } - exprs, err = matchDestCIDR("!10.0.0.1") + exprs, err = matchAddrCIDR("!10.0.0.1", false) if err != nil { t.Fatalf("matchDestCIDR(!10.0.0.1) error: %v", err) } @@ -1968,57 +1968,57 @@ func TestCompile_CommaZoneLists(t *testing.T) { { name: "address list after colon belongs to one zone", rule: config.Rule{Action: config.RuleAccept, Source: "lan", Dest: "net:192.0.2.1,198.51.100.1"}, - want: map[string][]string{"forward": {"iif=eth1 oif=eth0 daddr=192.0.2.1", "iif=eth1 oif=eth0 daddr=198.51.100.1"}}, + want: map[string][]string{"forward": {"iif=eth1 oif=eth0 ip4 daddr=192.0.2.1", "iif=eth1 oif=eth0 ip4 daddr=198.51.100.1"}}, }, { name: "zone:address inside a list", rule: config.Rule{Action: config.RuleAccept, Source: "lan,svr:203.0.113.7", Dest: "fw"}, - want: map[string][]string{"input": {"iif=eth1", "iif=eth2 saddr=203.0.113.7"}}, + want: map[string][]string{"input": {"iif=eth1", "iif=eth2 ip4 saddr=203.0.113.7"}}, }, { name: "dnat source list", rule: config.Rule{Action: config.RuleDNAT, Source: "net,lan", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}}, want: map[string][]string{"prerouting": {"iif=eth0", "iif=eth1"}, - "forward": {"iif=eth0 oif=eth2 daddr=192.0.2.10", "iif=eth1 oif=eth2 daddr=192.0.2.10"}}, + "forward": {"iif=eth0 oif=eth2 ip4 daddr=192.0.2.10", "iif=eth1 oif=eth2 ip4 daddr=192.0.2.10"}}, }, { name: "dnat source list skips the target zone", rule: config.Rule{Action: config.RuleDNAT, Source: "net,svr", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}}, - want: map[string][]string{"prerouting": {"iif=eth0"}, "forward": {"iif=eth0 oif=eth2 daddr=192.0.2.10"}}, + want: map[string][]string{"prerouting": {"iif=eth0"}, "forward": {"iif=eth0 oif=eth2 ip4 daddr=192.0.2.10"}}, }, { name: "dnat lone source zone may equal the target zone", rule: config.Rule{Action: config.RuleDNAT, Source: "svr", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}}, - want: map[string][]string{"prerouting": {"iif=eth2"}, "forward": {"iif=eth2 oif=eth2 daddr=192.0.2.10"}}, + want: map[string][]string{"prerouting": {"iif=eth2"}, "forward": {"iif=eth2 oif=eth2 ip4 daddr=192.0.2.10"}}, }, { name: "dnat exclusion source skips the target zone", rule: config.Rule{Action: config.RuleDNAT, Source: "all!fw,anycast", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}}, want: map[string][]string{"prerouting": {"iif=eth1", "iif=eth0"}, - "forward": {"iif=eth1 oif=eth2 daddr=192.0.2.10", "iif=eth0 oif=eth2 daddr=192.0.2.10"}}, + "forward": {"iif=eth1 oif=eth2 ip4 daddr=192.0.2.10", "iif=eth0 oif=eth2 ip4 daddr=192.0.2.10"}}, }, { name: "dnat intrazone exclusion source keeps the target zone", rule: config.Rule{Action: config.RuleDNAT, Source: "all+!fw,anycast,lan", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}}, want: map[string][]string{"prerouting": {"iif=eth0", "iif=eth2"}, - "forward": {"iif=eth0 oif=eth2 daddr=192.0.2.10", "iif=eth2 oif=eth2 daddr=192.0.2.10"}}, + "forward": {"iif=eth0 oif=eth2 ip4 daddr=192.0.2.10", "iif=eth2 oif=eth2 ip4 daddr=192.0.2.10"}}, }, { name: "dnat all source expands per zone and skips fw and the target zone", rule: config.Rule{Action: config.RuleDNAT, Source: "all", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}}, want: map[string][]string{"prerouting": {"iif=eth3", "iif=eth1", "iif=eth0"}, - "forward": {"iif=eth3 oif=eth2 daddr=192.0.2.10", "iif=eth1 oif=eth2 daddr=192.0.2.10", "iif=eth0 oif=eth2 daddr=192.0.2.10"}}, + "forward": {"iif=eth3 oif=eth2 ip4 daddr=192.0.2.10", "iif=eth1 oif=eth2 ip4 daddr=192.0.2.10", "iif=eth0 oif=eth2 ip4 daddr=192.0.2.10"}}, }, { name: "dnat any+ source keeps the target zone", rule: config.Rule{Action: config.RuleDNAT, Source: "any+", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}}, want: map[string][]string{"prerouting": {"iif=eth3", "iif=eth1", "iif=eth0", "iif=eth2"}, - "forward": {"iif=eth3 oif=eth2 daddr=192.0.2.10", "iif=eth1 oif=eth2 daddr=192.0.2.10", "iif=eth0 oif=eth2 daddr=192.0.2.10", "iif=eth2 oif=eth2 daddr=192.0.2.10"}}, + "forward": {"iif=eth3 oif=eth2 ip4 daddr=192.0.2.10", "iif=eth1 oif=eth2 ip4 daddr=192.0.2.10", "iif=eth0 oif=eth2 ip4 daddr=192.0.2.10", "iif=eth2 oif=eth2 ip4 daddr=192.0.2.10"}}, }, { name: "dnat to fw accepts in input", rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "fw:192.0.2.1", Proto: "tcp", DPort: config.PortSpec{"80"}}, - want: map[string][]string{"prerouting": {"iif=eth0"}, "input": {"iif=eth0 daddr=192.0.2.1"}}, + want: map[string][]string{"prerouting": {"iif=eth0"}, "input": {"iif=eth0 ip4 daddr=192.0.2.1"}}, }, { name: "redirect accepts in input without daddr", @@ -2028,13 +2028,13 @@ func TestCompile_CommaZoneLists(t *testing.T) { { name: "dnat source address list", rule: config.Rule{Action: config.RuleDNAT, Source: "net:192.0.2.5,198.51.100.5", Dest: "svr:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}}, - want: map[string][]string{"prerouting": {"iif=eth0 saddr=192.0.2.5", "iif=eth0 saddr=198.51.100.5"}, - "forward": {"iif=eth0 oif=eth2 saddr=192.0.2.5 daddr=192.0.2.10", "iif=eth0 oif=eth2 saddr=198.51.100.5 daddr=192.0.2.10"}}, + want: map[string][]string{"prerouting": {"iif=eth0 ip4 saddr=192.0.2.5", "iif=eth0 ip4 saddr=198.51.100.5"}, + "forward": {"iif=eth0 oif=eth2 ip4 saddr=192.0.2.5 daddr=192.0.2.10", "iif=eth0 oif=eth2 ip4 saddr=198.51.100.5 daddr=192.0.2.10"}}, }, { name: "negated address list stays one AND-ed rule", rule: config.Rule{Action: config.RuleAccept, Source: "net:!192.0.2.5,198.51.100.5", Dest: "fw"}, - want: map[string][]string{"input": {"iif=eth0 !saddr=192.0.2.5 !saddr=198.51.100.5"}}, + want: map[string][]string{"input": {"iif=eth0 ip4 !saddr=192.0.2.5 !saddr=198.51.100.5"}}, }, { name: "zone named like all/any keyword is a plain zone", @@ -2049,7 +2049,7 @@ func TestCompile_CommaZoneLists(t *testing.T) { { name: "interface-less zone kept when address narrows it", rule: config.Rule{Action: config.RuleAccept, Source: "lan", Dest: "vpn:192.0.2.1"}, - want: map[string][]string{"forward": {"iif=eth1 daddr=192.0.2.1"}}, + want: map[string][]string{"forward": {"iif=eth1 ip4 daddr=192.0.2.1"}}, }, { name: "fw source matches dest zone oif", @@ -2059,25 +2059,25 @@ func TestCompile_CommaZoneLists(t *testing.T) { { name: "fw to all has no oif", rule: config.Rule{Action: config.RuleAccept, Source: "fw", Dest: "all:192.0.2.1"}, - want: map[string][]string{"output": {"daddr=192.0.2.1"}}, + want: map[string][]string{"output": {"ip4 daddr=192.0.2.1"}}, }, { name: "dnat origdest", rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}, OrigDest: "203.0.113.5"}, want: map[string][]string{"prerouting": {"iif=eth0 ip4 daddr=203.0.113.5"}, - "forward": {"iif=eth0 oif=eth2 daddr=192.0.2.17"}}, + "forward": {"iif=eth0 oif=eth2 ip4 daddr=192.0.2.17"}}, }, { name: "dnat origdest list", rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}, OrigDest: "203.0.113.5,203.0.113.6"}, want: map[string][]string{"prerouting": {"iif=eth0 ip4 daddr=203.0.113.5", "iif=eth0 ip4 daddr=203.0.113.6"}, - "forward": {"iif=eth0 oif=eth2 daddr=192.0.2.17"}}, + "forward": {"iif=eth0 oif=eth2 ip4 daddr=192.0.2.17"}}, }, { name: "dnat negated origdest list", rule: config.Rule{Action: config.RuleDNAT, Source: "net", Dest: "svr:192.0.2.17", Proto: "tcp", DPort: config.PortSpec{"80"}, OrigDest: "!203.0.113.5,203.0.113.6"}, want: map[string][]string{"prerouting": {"iif=eth0 ip4 !daddr=203.0.113.5 !daddr=203.0.113.6"}, - "forward": {"iif=eth0 oif=eth2 daddr=192.0.2.17"}}, + "forward": {"iif=eth0 oif=eth2 ip4 daddr=192.0.2.17"}}, }, { name: "accept origdest", @@ -2487,7 +2487,7 @@ func TestCompile_RejectPerProto(t *testing.T) { } func TestNegatedAddressList(t *testing.T) { - exprs, err := matchDestCIDR("!192.0.2.1,198.51.100.1") + exprs, err := matchAddrCIDR("!192.0.2.1,198.51.100.1", false) if err != nil { t.Fatalf("matchDestCIDR error: %v", err) } @@ -2713,7 +2713,7 @@ func TestCompile_ConntrackZones(t *testing.T) { { name: "source and dest addresses", ct: config.ConntrackRule{Action: config.ConntrackDrop, Source: "net:192.0.2.1,198.51.100.1", Dest: "fw:203.0.113.1"}, - want: map[string][]string{"raw_prerouting": {"iif=eth0 saddr=192.0.2.1 daddr=203.0.113.1", "iif=eth0 saddr=198.51.100.1 daddr=203.0.113.1"}}, + want: map[string][]string{"raw_prerouting": {"iif=eth0 ip4 saddr=192.0.2.1 daddr=203.0.113.1", "iif=eth0 ip4 saddr=198.51.100.1 daddr=203.0.113.1"}}, }, { name: "fw source goes to raw_output with dest oif", @@ -2723,7 +2723,7 @@ func TestCompile_ConntrackZones(t *testing.T) { { name: "all matches no interface", ct: config.ConntrackRule{Action: config.ConntrackNoTrack, Source: "all", Dest: "fw:192.0.2.53"}, - want: map[string][]string{"raw_prerouting": {"daddr=192.0.2.53"}}, + want: map[string][]string{"raw_prerouting": {"ip4 daddr=192.0.2.53"}}, }, { name: "interface-less zone fails closed", @@ -2783,7 +2783,7 @@ func TestCompile_ConntrackZones(t *testing.T) { { name: "negated addresses stay one AND-ed match", ct: config.ConntrackRule{Action: config.ConntrackDrop, Source: "net:!192.0.2.1,198.51.100.1"}, - want: map[string][]string{"raw_prerouting": {"iif=eth0 !saddr=192.0.2.1 !saddr=198.51.100.1"}}, + want: map[string][]string{"raw_prerouting": {"iif=eth0 ip4 !saddr=192.0.2.1 !saddr=198.51.100.1"}}, }, { name: "sport", @@ -2803,7 +2803,7 @@ func TestCompile_ConntrackZones(t *testing.T) { { name: "fw dest zone with address matches daddr", ct: config.ConntrackRule{Action: config.ConntrackNoTrack, Source: "net", Dest: "fw:192.0.2.1"}, - want: map[string][]string{"raw_prerouting": {"iif=eth0 daddr=192.0.2.1"}}, + want: map[string][]string{"raw_prerouting": {"iif=eth0 ip4 daddr=192.0.2.1"}}, }, { name: "unknown zone fails closed", @@ -2858,7 +2858,7 @@ func TestCompile_ConntrackZones(t *testing.T) { { name: "dest zone with address matches daddr", ct: config.ConntrackRule{Action: config.ConntrackNoTrack, Source: "net", Dest: "lan:203.0.113.10"}, - want: map[string][]string{"raw_prerouting": {"iif=eth0 daddr=203.0.113.10"}}, + want: map[string][]string{"raw_prerouting": {"iif=eth0 ip4 daddr=203.0.113.10"}}, }, } for _, tt := range tests { @@ -2957,7 +2957,7 @@ func TestCompile_ConntrackHelperZones(t *testing.T) { { name: "dest zone with address in prerouting", ct: config.ConntrackRule{Source: "net", Dest: "lan:203.0.113.10", Proto: "tcp", DPort: config.PortSpec{"21"}}, - want: map[string][]string{"helper_prerouting": {"iif=eth0 daddr=203.0.113.10"}}, + want: map[string][]string{"helper_prerouting": {"iif=eth0 ip4 daddr=203.0.113.10"}}, }, { name: "dest zone without address is rejected in prerouting", @@ -3023,7 +3023,7 @@ func TestCompile_AllIncludesFirewallMatches(t *testing.T) { { name: "all address kept on added fw rules", rule: config.Rule{Action: config.RuleAccept, Source: "all:192.0.2.5", Dest: "all"}, - want: map[string][]string{"input": {"saddr=192.0.2.5"}, "output": {"saddr=192.0.2.5"}, "forward": {"saddr=192.0.2.5"}}, + want: map[string][]string{"input": {"ip4 saddr=192.0.2.5"}, "output": {"ip4 saddr=192.0.2.5"}, "forward": {"ip4 saddr=192.0.2.5"}}, }, { name: "dnat with all source skips fw", @@ -3607,17 +3607,17 @@ func TestCompile_DestPlusOverridesIntraZone(t *testing.T) { func TestCompile_HostsIntraZone(t *testing.T) { state := mustCompile(t, hostsCfg(nil)) want := []string{ - "iif=wlo1 ip4 saddr=192.0.2.0/24 oif=enp2s0 ip4 daddr=198.51.100.0/24", - "iif=enp2s0 ip4 saddr=198.51.100.0/24 oif=wlo1 ip4 daddr=192.0.2.0/24", + "iif=wlo1 ip4 saddr=192.0.2.0/24 oif=enp2s0 daddr=198.51.100.0/24", + "iif=enp2s0 ip4 saddr=198.51.100.0/24 oif=wlo1 daddr=192.0.2.0/24", } if got := describeTagged(state, "forward", "intra:lan"); !reflect.DeepEqual(got, want) { t.Errorf("lan intra = %q, want %q", got, want) } want = []string{ - "iif=wlo1 ip4 !saddr=192.0.2.0/24 oif=enp2s0 ip4 !daddr=198.51.100.0/24", - "iif=wlo1 ip6 oif=enp2s0 ip6", - "iif=enp2s0 ip4 !saddr=198.51.100.0/24 oif=wlo1 ip4 !daddr=192.0.2.0/24", - "iif=enp2s0 ip6 oif=wlo1 ip6", + "iif=wlo1 ip4 !saddr=192.0.2.0/24 oif=enp2s0 !daddr=198.51.100.0/24", + "iif=wlo1 ip6 oif=enp2s0", + "iif=enp2s0 ip4 !saddr=198.51.100.0/24 oif=wlo1 !daddr=192.0.2.0/24", + "iif=enp2s0 ip6 oif=wlo1", } if got := describeTagged(state, "forward", "intra:net"); !reflect.DeepEqual(got, want) { t.Errorf("net intra = %q, want %q", got, want) @@ -3630,8 +3630,8 @@ func TestCompile_HostsIntraZone(t *testing.T) { t.Errorf("explicit lan lan policy must replace implicit accept, got %q", got) } want = []string{ - "iif=wlo1 ip4 saddr=192.0.2.0/24 oif=enp2s0 ip4 daddr=198.51.100.0/24", - "iif=enp2s0 ip4 saddr=198.51.100.0/24 oif=wlo1 ip4 daddr=192.0.2.0/24", + "iif=wlo1 ip4 saddr=192.0.2.0/24 oif=enp2s0 daddr=198.51.100.0/24", + "iif=enp2s0 ip4 saddr=198.51.100.0/24 oif=wlo1 daddr=192.0.2.0/24", } if got := describeTagged(state, "forward", "policy:0"); !reflect.DeepEqual(got, want) { t.Errorf("lan lan policy = %q, want %q", got, want) @@ -3650,7 +3650,7 @@ func TestCompile_HostsRouteBack(t *testing.T) { if got := describeTagged(mustCompile(t, hostsCfg(sameIface(false))), "forward", "intra:lan"); len(got) != 0 { t.Errorf("same-interface hosts without routeback = %q, want none", got) } - want := []string{"iif=wlo1 ip4 saddr=192.0.2.0/24 oif=wlo1 ip4 daddr=192.0.2.0/24"} + want := []string{"iif=wlo1 ip4 saddr=192.0.2.0/24 oif=wlo1 daddr=192.0.2.0/24"} if got := describeTagged(mustCompile(t, hostsCfg(sameIface(true))), "forward", "intra:lan"); !reflect.DeepEqual(got, want) { t.Errorf("routeback hosts intra = %q, want %q", got, want) } diff --git a/internal/nftables/guards_test.go b/internal/nftables/guards_test.go new file mode 100644 index 0000000..670b364 --- /dev/null +++ b/internal/nftables/guards_test.go @@ -0,0 +1,92 @@ +package nftables + +import ( + "os" + "os/exec" + "testing" + + "github.com/google/nftables/expr" + + "git.unkin.net/unkin/tomswall/internal/config" +) + +func guardCfg(af config.AddressFamily) *config.Config { + return hostsCfg(func(c *config.Config) { + c.Settings.AddressFamily = af + c.Hosts[0].Addresses = append(c.Hosts[0].Addresses, "2001:db8::/64") + c.Hosts[0].Exclusions = []string{"192.0.2.9", "2001:db8::9"} + c.Interfaces[0].Options.NoSmurfs = true + c.Rules = append(c.Rules, + config.Rule{Action: config.RuleDrop, Source: "net:203.0.113.7", Dest: "lan:192.0.2.5"}, + config.Rule{Action: config.RuleDrop, Source: "net:2001:db8:1::7", Dest: "lan:2001:db8::5"}) + }) +} + +func TestCompile_FamilyGuardsDecodable(t *testing.T) { + addrLen := map[config.AddressFamily]uint32{config.FamilyIP: 4, config.FamilyIP6: 16} + for _, af := range []config.AddressFamily{config.FamilyINET, config.FamilyIP, config.FamilyIP6} { + t.Run(string(af), func(t *testing.T) { + for chain, rules := range mustCompile(t, guardCfg(af)).Rules { + for _, r := range rules { + guards, l3 := 0, false + for i, e := range r.Exprs { + if nfprotoGuard(r.Exprs, i) != 0 { + guards++ + if l3 { + t.Errorf("%s %s: guard after a network payload: %s", chain, r.Tag, describeRule(r)) + } + } + if p, ok := e.(*expr.Payload); ok && p.Base == expr.PayloadBaseNetworkHeader { + l3 = true + if n := addrLen[af]; n != 0 && (p.Len == 4 || p.Len == 16) && p.Len != n { + t.Errorf("%s %s: other-family address in %s table: %s", chain, r.Tag, af, describeRule(r)) + } + } + } + if max := map[bool]int{true: 1, false: 0}[af == config.FamilyINET]; guards > max { + t.Errorf("%s %s: %d family guards in %s table: %s", chain, r.Tag, guards, af, describeRule(r)) + } + } + } + }) + } +} + +// TestNetnsNftListDecodes applies each family in a fresh user+net namespace and requires +// nft(8) to list the ruleset and a second plan to be empty. Needs unshare and nft. +func TestNetnsNftListDecodes(t *testing.T) { + if af := os.Getenv("TOMSWALL_NETNS_CHILD"); af != "" { + netnsChild(t, config.AddressFamily(af)) + return + } + if os.Getenv("TOMSWALL_NETNS_TEST") == "" { + t.Skip("set TOMSWALL_NETNS_TEST=1 to run (needs unshare and nft)") + } + for _, af := range []config.AddressFamily{config.FamilyINET, config.FamilyIP, config.FamilyIP6} { + cmd := exec.Command("unshare", "-rn", os.Args[0], "-test.run=^TestNetnsNftListDecodes$", "-test.v") + cmd.Env = append(os.Environ(), "TOMSWALL_NETNS_CHILD="+string(af)) + if out, err := cmd.CombinedOutput(); err != nil { + t.Errorf("%s: %v\n%s", af, err, out) + } + } +} + +func netnsChild(t *testing.T, af config.AddressFamily) { + e, err := NewEngine(guardCfg(af)) + if err != nil { + t.Fatal(err) + } + cs, err := e.Plan() + if err != nil { + t.Fatal(err) + } + if err := e.Apply(cs); err != nil { + t.Fatal(err) + } + if out, err := exec.Command("nft", "list", "ruleset").CombinedOutput(); err != nil { + t.Fatalf("nft list ruleset: %v\n%s", err, out) + } + if cs, err = e.Plan(); err != nil || len(cs.Add)+len(cs.Remove) != 0 { + t.Fatalf("second plan not empty: %d add, %d remove, err %v", len(cs.Add), len(cs.Remove), err) + } +} From 9a65973d7f1be6e3611e55bc986aa2867b0e2bdc Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Fri, 9 Oct 2026 23:48:53 +1100 Subject: [PATCH 2/4] Expand address matches per family, fail compile on conflicting guards --- internal/nftables/compiler.go | 196 ++++++++++++++++++----------- internal/nftables/compiler_test.go | 27 +--- 2 files changed, 130 insertions(+), 93 deletions(-) diff --git a/internal/nftables/compiler.go b/internal/nftables/compiler.go index 0af760c..472794f 100644 --- a/internal/nftables/compiler.go +++ b/internal/nftables/compiler.go @@ -67,47 +67,79 @@ func (c *Compiler) Compile() (*FirewallState, error) { } c.compileMSSClamp(state) limitLogs(state, c.cfg.Settings.LogLimit) - familyGuards(state, c.cfg.Settings.AddressFamily) + if err := familyGuards(state, c.cfg.Settings.AddressFamily); err != nil { + return nil, err + } return state, nil } // familyGuards leaves each rule at most one meta nfproto guard, ahead of its first network-header -// payload as nft emits it, and drops rules whose guards contradict; nft list cannot decode -// conflicting guards. An ip/ip6 table is its own guard: guards go, other-family rules are dropped. -func familyGuards(state *FirewallState, family config.AddressFamily) { +// payload as nft emits it; nft list cannot decode repeated or conflicting guards. Rules are built per +// family, so a conflict is a compiler bug and fails the compile. An ip/ip6 table is its own guard: +// guards go and other-family rules are dropped. +func familyGuards(state *FirewallState, family config.AddressFamily) error { table := map[config.AddressFamily]byte{config.FamilyIP: unix.NFPROTO_IPV4, config.FamilyIP6: unix.NFPROTO_IPV6}[family] for chain, rules := range state.Rules { var out []ManagedRule for _, r := range rules { - if exprs, ok := normalizeGuards(r.Exprs, table); ok { - r.Exprs = exprs - out = append(out, r) + fam, ok := guardFamily(r.Exprs) + if !ok { + return fmt.Errorf("%s %s: conflicting address families", chain, r.Tag) } + if table != 0 && fam != 0 && fam != table { + continue + } + r.Exprs = normalizeGuards(r.Exprs, fam, table != 0) + out = append(out, r) } state.Rules[chain] = out } + return nil } -func normalizeGuards(in []expr.Any, table byte) ([]expr.Any, bool) { - var fam byte +// guardFamily is the family exprs' nfproto guards require (0: none); ok is false when they conflict. +func guardFamily(exprs []expr.Any) (fam byte, ok bool) { + for i := range exprs { + if p := nfprotoGuard(exprs, i); p != 0 { + if fam != 0 && p != fam { + return 0, false + } + fam = p + } + } + return fam, true +} + +// famsAgree reports whether families (0: any) can all hold for one packet. +func famsAgree(fams ...byte) bool { + var f byte + for _, p := range fams { + if p != 0 && f != 0 && p != f { + return false + } + if p != 0 { + f = p + } + } + return true +} + +func normalizeGuards(in []expr.Any, fam byte, strip bool) []expr.Any { at := -1 out := make([]expr.Any, 0, len(in)) for i := 0; i < len(in); i++ { - if p := nfprotoGuard(in, i); p != 0 { - if (fam != 0 && p != fam) || (table != 0 && p != table) { - return nil, false - } - if fam == 0 { - fam, at = p, len(out) + if nfprotoGuard(in, i) != 0 { + if at < 0 { + at = len(out) } i++ continue } out = append(out, in[i]) } - if fam == 0 || table != 0 { - return out, true + if fam == 0 || strip { + return out } if l3 := slices.IndexFunc(out, func(e expr.Any) bool { p, ok := e.(*expr.Payload) @@ -115,7 +147,7 @@ func normalizeGuards(in []expr.Any, table byte) ([]expr.Any, bool) { }); l3 >= 0 && l3 < at { at = l3 } - return slices.Insert(out, at, matchNFProto(fam)...), true + return slices.Insert(out, at, matchNFProto(fam)...) } // nfprotoGuard is the family a meta nfproto == match at in[i] guards for, else 0. @@ -403,7 +435,7 @@ func (c *Compiler) compileConntrack(state *FirewallState) error { func (c *Compiler) compileConntrackPair(state *FirewallState, tag, chain string, ct config.ConntrackRule, srcZone, srcAddr, dstZone, dstAddr string) error { if _, ok := c.cfg.Zones[dstZone]; ok && chain == "raw_prerouting" && - (dstAddr == "" || strings.HasPrefix(dstAddr, "!")) { + !narrows(dstAddr) { return fmt.Errorf("conntrack DEST zone %q needs an address in prerouting", dstZone) } srcIfaces, dstIfaces := c.resolveZone(srcZone, srcAddr), []zoneMatch{{}} @@ -425,6 +457,9 @@ func (c *Compiler) compileConntrackPair(state *FirewallState, tag, chain string, } for _, m := range matches { exprs := m.exprs + if _, ok := guardFamily(exprs); !ok { + continue + } switch ct.Action { case config.ConntrackNoTrack: exprs = append(exprs, &expr.Notrack{}) @@ -724,12 +759,33 @@ func isZoneExclusion(spec string) bool { return ok && (base == "all" || base == "any") } -// splitAddrs yields one alternative per listed address; a negated list stays one AND-ed match. +// splitAddrs yields one alternative per listed address. A negated list ("everything except") becomes +// one AND-ed match per family: the family's negations, or the bare family (a /0) when it has none. func splitAddrs(addr string) []string { - if addr == "" || strings.HasPrefix(addr, "!") { - return []string{addr} + if addr == "" { + return []string{""} } - return strings.Split(addr, ",") + if !strings.HasPrefix(addr, "!") { + return strings.Split(addr, ",") + } + v4, v6 := splitFamily(strings.Split(addr[1:], ",")) + out := []string{"0.0.0.0/0", "::/0"} + if len(v4) > 0 { + out[0] = "!" + strings.Join(v4, ",") + } + if len(v6) > 0 { + out[1] = "!" + strings.Join(v6, ",") + } + return out +} + +// narrows reports whether a splitAddrs alternative restricts addresses within its family. +func narrows(addr string) bool { + if addr == "" || strings.HasPrefix(addr, "!") { + return false + } + p, err := parsePrefix(addr) + return err != nil || p.Bits() > 0 } func (c *Compiler) compileZonePair(state *FirewallState, tag, srcZone, srcAddr, dstZone, dstAddr, origDest, proto string, @@ -750,7 +806,7 @@ func (c *Compiler) compileZonePair(state *FirewallState, tag, srcZone, srcAddr, return err } if origDest != "" { - od, err := matchOrigDest(origDest) + od, err := matchDestCIDR(origDest) if err != nil { return fmt.Errorf("origdest: %w", err) } @@ -761,6 +817,9 @@ func (c *Compiler) compileZonePair(state *FirewallState, tag, srcZone, srcAddr, for _, m := range matches { exprs := m.exprs + if _, ok := guardFamily(exprs); !ok { + continue + } if section != "" && section != config.SectionAll { exprs = append(exprs, matchSection(section)...) @@ -810,7 +869,7 @@ func (c *Compiler) compileDNATRule(state *FirewallState, tag, srcZone, srcAddr, var odExprs []expr.Any if origDest != "" { var err error - if odExprs, err = matchOrigDest(origDest); err != nil { + if odExprs, err = matchDestCIDR(origDest); err != nil { return fmt.Errorf("origdest: %w", err) } } @@ -824,6 +883,10 @@ func (c *Compiler) compileDNATRule(state *FirewallState, tag, srcZone, srcAddr, if ip == nil { return fmt.Errorf("invalid DNAT address %q", dnatAddr) } + natFam := byte(0) + if action != config.RuleRedirect { + natFam = addrFamily(dnatAddr) + } for _, srcIface := range srcIfaces { zm, err := zoneMatchExprs(srcIface, true) @@ -841,6 +904,9 @@ func (c *Compiler) compileDNATRule(state *FirewallState, tag, srcZone, srcAddr, exprs = append(exprs, src...) } exprs = append(exprs, odExprs...) + if f, ok := guardFamily(exprs); !ok || !famsAgree(f, natFam) { + continue + } exprs = append(exprs, m.exprs...) @@ -941,7 +1007,7 @@ func (c *Compiler) compilePolicies(state *FirewallState) error { for _, si := range srcIfaces { for _, di := range dstIfaces { - if sz == dz && intraZoneSkip(si, di) { + if sz == dz && intraZoneSkip(si, di) || chain != "input" && !famsAgree(matchFamily(si), matchFamily(di)) { continue } exprs, err := zonePairExprs(si, di, chain) @@ -1038,25 +1104,27 @@ func (c *Compiler) compileSNAT(state *FirewallState) error { for i, snat := range c.cfg.SNAT { tag := fmt.Sprintf("snat:%d", i) - var exprs []expr.Any - destIface, _ := splitZoneSpec(snat.Dest) - exprs = append(exprs, matchIfaceName(false, destIface)...) - - if snat.Source != "" { - srcExprs, err := matchSourceCIDR(snat.Source) - if err != nil { - return fmt.Errorf("snat[%d]: %w", i, err) + var heads [][]expr.Any + for _, src := range splitAddrs(snat.Source) { + head := matchIfaceName(false, destIface) + if src != "" { + srcExprs, err := matchSourceCIDR(src) + if err != nil { + return fmt.Errorf("snat[%d]: %w", i, err) + } + head = append(head, srcExprs...) + } + if snat.Action != config.SNATAddress || famsAgree(addrFamily(src), addrFamily(snat.Address)) { + heads = append(heads, head) } - exprs = append(exprs, srcExprs...) } matches, err := l4Matches(snat.Proto, snat.DPort, snat.SPort) if err != nil { return fmt.Errorf("snat[%d]: %w", i, err) } - head := exprs - exprs = nil + var exprs []expr.Any if snat.Mark != "" { exprs = append(exprs, matchMark(snat.Mark)...) @@ -1102,12 +1170,14 @@ func (c *Compiler) compileSNAT(state *FirewallState) error { } } - for _, m := range matches { - state.Rules["postrouting"] = append(state.Rules["postrouting"], ManagedRule{ - Chain: "postrouting", - Exprs: append(append(append([]expr.Any{}, head...), m.exprs...), exprs...), - Tag: tag, - }) + for _, head := range heads { + for _, m := range matches { + state.Rules["postrouting"] = append(state.Rules["postrouting"], ManagedRule{ + Chain: "postrouting", + Exprs: slices.Concat(head, m.exprs, exprs), + Tag: tag, + }) + } } } @@ -1398,7 +1468,7 @@ func (c *Compiler) resolveZone(zone, addr string) []zoneMatch { if len(out) > 0 || hasHosts { return out } - if addr != "" && !strings.HasPrefix(addr, "!") { + if narrows(addr) { return []zoneMatch{{}} } if !c.warned[zone] { @@ -1925,42 +1995,28 @@ func matchDestCIDR(cidr string) ([]expr.Any, error) { return matchGuardedCIDR(cidr, false) } -// matchGuardedCIDR guards an address match with its family's nfproto, unless the list mixes families. +// matchGuardedCIDR guards a single-family splitAddrs alternative with its nfproto; a /0 is the guard alone. func matchGuardedCIDR(cidr string, isSrc bool) ([]expr.Any, error) { + if !narrows(cidr) && !strings.HasPrefix(cidr, "!") { + return matchNFProto(addrFamily(cidr)), nil + } e, err := matchAddrCIDR(cidr, isSrc) if err != nil { return nil, err } - if p := addrFamily(cidr); p != 0 { - return append(matchNFProto(p), e...), nil - } - return e, nil + return append(matchNFProto(addrFamily(cidr)), e...), nil } -// addrFamily is the NFPROTO shared by every address in a (negated) comma list, or 0 if they mix. +// addrFamily is the NFPROTO of a single-family splitAddrs alternative, 0 for none; unparsable is IPv6. func addrFamily(list string) byte { - var proto byte - for i, a := range strings.Split(strings.TrimPrefix(list, "!"), ",") { - a, _, _ = strings.Cut(a, "/") - p := byte(unix.NFPROTO_IPV6) - if ip := net.ParseIP(a); ip != nil && ip.To4() != nil { - p = unix.NFPROTO_IPV4 - } - if i > 0 && p != proto { - return 0 - } - proto = p + if list == "" { + return 0 } - return proto -} - -// matchOrigDest guards the daddr match with the address's nfproto so it is family-correct in the inet table. -func matchOrigDest(addr string) ([]expr.Any, error) { - dst, err := matchDestCIDR(addr) - if err == nil && addrFamily(addr) == 0 { - err = fmt.Errorf("%q mixes IPv4 and IPv6 addresses", addr) + a, _, _ := strings.Cut(strings.TrimPrefix(list, "!"), ",") + if p, err := parsePrefix(a); err == nil && p.Addr().Is4() { + return unix.NFPROTO_IPV4 } - return dst, err + return unix.NFPROTO_IPV6 } func matchNFProto(proto byte) []expr.Any { diff --git a/internal/nftables/compiler_test.go b/internal/nftables/compiler_test.go index 1bed22b..baf804f 100644 --- a/internal/nftables/compiler_test.go +++ b/internal/nftables/compiler_test.go @@ -2032,9 +2032,9 @@ func TestCompile_CommaZoneLists(t *testing.T) { "forward": {"iif=eth0 oif=eth2 ip4 saddr=192.0.2.5 daddr=192.0.2.10", "iif=eth0 oif=eth2 ip4 saddr=198.51.100.5 daddr=192.0.2.10"}}, }, { - name: "negated address list stays one AND-ed rule", + name: "negated address list is one AND-ed rule per family", rule: config.Rule{Action: config.RuleAccept, Source: "net:!192.0.2.5,198.51.100.5", Dest: "fw"}, - want: map[string][]string{"input": {"iif=eth0 ip4 !saddr=192.0.2.5 !saddr=198.51.100.5"}}, + want: map[string][]string{"input": {"iif=eth0 ip4 !saddr=192.0.2.5 !saddr=198.51.100.5", "iif=eth0 ip6"}}, }, { name: "zone named like all/any keyword is a plain zone", @@ -2605,25 +2605,6 @@ func TestCompile_ColonRanges(t *testing.T) { } } -func TestMatchOrigDest_FamilyGuard(t *testing.T) { - for addr, want := range map[string]byte{ - "203.0.113.5": unix.NFPROTO_IPV4, - "!203.0.113.0/24,192.0.2.1": unix.NFPROTO_IPV4, - "2001:db8::5": unix.NFPROTO_IPV6, - } { - e, err := matchOrigDest(addr) - if err != nil { - t.Fatalf("%s: %v", addr, err) - } - if m, ok := e[0].(*expr.Meta); !ok || m.Key != expr.MetaKeyNFPROTO || e[1].(*expr.Cmp).Data[0] != want { - t.Errorf("%s: missing nfproto %d guard: %v", addr, want, e[:2]) - } - } - if _, err := matchOrigDest("!203.0.113.5,2001:db8::5"); err == nil { - t.Error("mixed IPv4/IPv6 origdest: want error") - } -} - func TestCompile_OrigDestForwardRejected(t *testing.T) { cfg := &config.Config{ Settings: config.Settings{TableName: "test", AddressFamily: config.FamilyINET}, @@ -2781,9 +2762,9 @@ func TestCompile_ConntrackZones(t *testing.T) { want: map[string][]string{"raw_output": {"oif=eth1"}}, }, { - name: "negated addresses stay one AND-ed match", + name: "negated addresses are one AND-ed match per family", ct: config.ConntrackRule{Action: config.ConntrackDrop, Source: "net:!192.0.2.1,198.51.100.1"}, - want: map[string][]string{"raw_prerouting": {"iif=eth0 ip4 !saddr=192.0.2.1 !saddr=198.51.100.1"}}, + want: map[string][]string{"raw_prerouting": {"iif=eth0 ip4 !saddr=192.0.2.1 !saddr=198.51.100.1", "iif=eth0 ip6"}}, }, { name: "sport", From bc653126478aba4ca47e4e4dbab9c85d5b5ecc13 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Fri, 9 Oct 2026 23:48:53 +1100 Subject: [PATCH 3/4] Cover per-family expansion across rules, NAT, tunnels and blrules --- internal/nftables/guards_test.go | 149 ++++++++++++++++++++++++++++++- 1 file changed, 148 insertions(+), 1 deletion(-) diff --git a/internal/nftables/guards_test.go b/internal/nftables/guards_test.go index 670b364..75643c7 100644 --- a/internal/nftables/guards_test.go +++ b/internal/nftables/guards_test.go @@ -3,9 +3,12 @@ package nftables import ( "os" "os/exec" + "reflect" + "strings" "testing" "github.com/google/nftables/expr" + "golang.org/x/sys/unix" "git.unkin.net/unkin/tomswall/internal/config" ) @@ -18,7 +21,24 @@ func guardCfg(af config.AddressFamily) *config.Config { c.Interfaces[0].Options.NoSmurfs = true c.Rules = append(c.Rules, config.Rule{Action: config.RuleDrop, Source: "net:203.0.113.7", Dest: "lan:192.0.2.5"}, - config.Rule{Action: config.RuleDrop, Source: "net:2001:db8:1::7", Dest: "lan:2001:db8::5"}) + config.Rule{Action: config.RuleDrop, Source: "net:2001:db8:1::7", Dest: "lan:2001:db8::5"}, + config.Rule{Action: config.RuleDrop, Source: "vpn", Dest: "net:!192.0.2.1"}, + config.Rule{Action: config.RuleDrop, Source: "vpn", Dest: "net:!192.0.2.1,2001:db8::1"}, + config.Rule{Action: config.RuleDrop, Source: "vpn:203.0.113.7", Dest: "net:!2001:db8::5"}, + config.Rule{Action: config.RuleDNAT, Source: "vpn", Dest: "lan:192.0.2.10", Proto: "tcp", + DPort: config.PortSpec{"80"}, OrigDest: "!203.0.113.5,2001:db8::5"}, + config.Rule{Action: config.RuleDrop, Source: "vpn:192.0.2.77,2001:db8:7::7", Dest: "fw"}) + c.Blrules = []config.BlruleRule{{Action: config.BlruleDrop, Source: "vpn:!192.0.2.1,2001:db8::1", Dest: "fw"}} + c.SNAT = []config.SNATRule{ + {Action: config.SNATMasquerade, Dest: "wlo1", Source: "!192.0.2.0/24,2001:db8::/48"}, + {Action: config.SNATAddress, Address: "203.0.113.1", Dest: "wlo1", Source: "!192.0.2.9,2001:db8::9"}, + {Action: config.SNATAddress, Address: "203.0.113.1", Dest: "wlo1", Source: "2001:db8::/48"}, + } + c.Tunnels = []config.Tunnel{{Type: "gre", Zone: "vpn", Gateways: []string{"203.0.113.50", "2001:db8:5::1"}}} + c.StaticNAT = []config.StaticNAT{ + {External: "203.0.113.60", Interface: "wlo1", Internal: "192.0.2.60"}, + {External: "2001:db8:6::1", Interface: "wlo1", Internal: "2001:db8::60"}, + } }) } @@ -52,6 +72,133 @@ func TestCompile_FamilyGuardsDecodable(t *testing.T) { } } +func TestCompile_FamilyGuardsPerFamily(t *testing.T) { + state := mustCompile(t, guardCfg(config.FamilyINET)) + for _, tt := range []struct { + chain, tag string + want []string + }{ + {"forward", "rule:3", []string{ + "iif=tun0 oif=wlo1 ip4 !daddr=192.0.2.0/24 !daddr=192.0.2.1", + "iif=tun0 oif=wlo1 ip4 daddr=192.0.2.9 !daddr=192.0.2.1", + "iif=tun0 oif=enp2s0 ip4 !daddr=198.51.100.0/24 !daddr=192.0.2.1", + "iif=tun0 oif=wlo1 ip6 !daddr=2001:db8::/64", + "iif=tun0 oif=wlo1 ip6 daddr=2001:db8::9", + "iif=tun0 oif=enp2s0 ip6", + }}, + {"forward", "rule:4", []string{ + "iif=tun0 oif=wlo1 ip4 !daddr=192.0.2.0/24 !daddr=192.0.2.1", + "iif=tun0 oif=wlo1 ip4 daddr=192.0.2.9 !daddr=192.0.2.1", + "iif=tun0 oif=enp2s0 ip4 !daddr=198.51.100.0/24 !daddr=192.0.2.1", + "iif=tun0 oif=wlo1 ip6 !daddr=2001:db8::/64 !daddr=2001:db8::1", + "iif=tun0 oif=wlo1 ip6 daddr=2001:db8::9 !daddr=2001:db8::1", + "iif=tun0 oif=enp2s0 ip6 !daddr=2001:db8::1", + }}, + {"forward", "rule:5", []string{ + "iif=tun0 oif=wlo1 ip4 !daddr=192.0.2.0/24 saddr=203.0.113.7", + "iif=tun0 oif=wlo1 ip4 daddr=192.0.2.9 saddr=203.0.113.7", + "iif=tun0 oif=enp2s0 ip4 !daddr=198.51.100.0/24 saddr=203.0.113.7", + }}, + {"prerouting", "rule:6", []string{"iif=tun0 ip4 !daddr=203.0.113.5"}}, + {"forward", "rule:6:accept", []string{"iif=tun0 oif=wlo1 ip4 daddr=192.0.2.0/24 !daddr=192.0.2.9 daddr=192.0.2.10"}}, + {"input", "rule:7", []string{"iif=tun0 ip4 saddr=192.0.2.77", "iif=tun0 ip6 saddr=2001:db8:7::7"}}, + {"input", "blrule:0", []string{"iif=tun0 ip4 !saddr=192.0.2.1", "iif=tun0 ip6 !saddr=2001:db8::1"}}, + {"postrouting", "snat:0", []string{"oif=wlo1 ip4 !saddr=192.0.2.0/24", "oif=wlo1 ip6 !saddr=2001:db8::/48"}}, + {"postrouting", "snat:1", []string{"oif=wlo1 ip4 !saddr=192.0.2.9"}}, + {"postrouting", "snat:2", nil}, + {"input", "tunnel:0", []string{"ip4 saddr=203.0.113.50", "ip6 saddr=2001:db8:5::1"}}, + {"prerouting", "staticnat:dnat:0", []string{"iif=wlo1 ip4 daddr=203.0.113.60"}}, + {"postrouting", "staticnat:snat:1", []string{"oif=wlo1 ip6 saddr=2001:db8::60"}}, + } { + if got := describeTagged(state, tt.chain, tt.tag); !reflect.DeepEqual(got, tt.want) { + t.Errorf("%s %s = %q, want %q", tt.chain, tt.tag, got, tt.want) + } + } +} + +// TestCompile_NegatedV4DropKeepsV6 checks a single-family table keeps its family's half of a +// negated DROP: "everything except 192.0.2.1" still drops all IPv6. +func TestCompile_NegatedV4DropKeepsV6(t *testing.T) { + for af, want := range map[config.AddressFamily][]string{ + config.FamilyIP: {"iif=tun0 oif=wlo1 !daddr=192.0.2.0/24 !daddr=192.0.2.1", "iif=tun0 oif=wlo1 daddr=192.0.2.9 !daddr=192.0.2.1", "iif=tun0 oif=enp2s0 !daddr=198.51.100.0/24 !daddr=192.0.2.1"}, + config.FamilyIP6: {"iif=tun0 oif=wlo1 !daddr=2001:db8::/64", "iif=tun0 oif=wlo1 daddr=2001:db8::9", "iif=tun0 oif=enp2s0"}, + } { + if got := describeTagged(mustCompile(t, guardCfg(af)), "forward", "rule:3"); !reflect.DeepEqual(got, want) { + t.Errorf("%s rule:3 = %q, want %q", af, got, want) + } + } +} + +func TestSplitAddrs(t *testing.T) { + for in, want := range map[string][]string{ + "": {""}, + "192.0.2.1,2001:db8::1": {"192.0.2.1", "2001:db8::1"}, + "!192.0.2.1": {"!192.0.2.1", "::/0"}, + "!2001:db8::1": {"0.0.0.0/0", "!2001:db8::1"}, + "!192.0.2.1,2001:db8::1,198.51.100.0/24": {"!192.0.2.1,198.51.100.0/24", "!2001:db8::1"}, + } { + if got := splitAddrs(in); !reflect.DeepEqual(got, want) { + t.Errorf("splitAddrs(%q) = %q, want %q", in, got, want) + } + } +} + +func TestMatchGuardedCIDR(t *testing.T) { + for in, want := range map[string]string{ + "192.0.2.1": "ip4 saddr=192.0.2.1", + "2001:db8::/48": "ip6 saddr=2001:db8::/48", + "!192.0.2.1,198.51.100.0/24": "ip4 !saddr=192.0.2.1 !saddr=198.51.100.0/24", + "!2001:db8::1": "ip6 !saddr=2001:db8::1", + "0.0.0.0/0": "ip4", + "::/0": "ip6", + } { + e, err := matchSourceCIDR(in) + if err != nil { + t.Fatalf("%s: %v", in, err) + } + if got := describeRule(ManagedRule{Exprs: e}); got != want { + t.Errorf("matchSourceCIDR(%q) = %q, want %q", in, got, want) + } + } + if e, _ := matchDestCIDR("!2001:db8::5"); describeRule(ManagedRule{Exprs: e}) != "ip6 !daddr=2001:db8::5" { + t.Errorf("matchDestCIDR(!2001:db8::5) = %q", describeRule(ManagedRule{Exprs: e})) + } + if _, err := matchSourceCIDR("!nonsense"); err == nil { + t.Error("invalid negated address: want error") + } +} + +func TestFamilyGuards(t *testing.T) { + v4, _ := matchSourceCIDR("192.0.2.1") + v6, _ := matchDestCIDR("2001:db8::1") + both, _ := matchDestCIDR("198.51.100.1") + state := func(e ...[]expr.Any) *FirewallState { + var r []expr.Any + for _, x := range e { + r = append(r, x...) + } + return &FirewallState{Rules: map[string][]ManagedRule{"input": {{Exprs: r, Tag: "t"}}}} + } + s := state(v4, both) + if err := familyGuards(s, config.FamilyINET); err != nil || describeRule(s.Rules["input"][0]) != "ip4 saddr=192.0.2.1 daddr=198.51.100.1" { + t.Errorf("same-family guards not merged: %v %q", err, describeRule(s.Rules["input"][0])) + } + if err := familyGuards(state(v4, v6), config.FamilyINET); err == nil || !strings.Contains(err.Error(), "conflicting") { + t.Errorf("conflicting guards: want error, got %v", err) + } + s = state(v6) + if err := familyGuards(s, config.FamilyIP); err != nil || len(s.Rules["input"]) != 0 { + t.Errorf("ip table must drop IPv6 rules: %v %v", err, s.Rules["input"]) + } + s = state(v6) + if err := familyGuards(s, config.FamilyIP6); err != nil || describeRule(s.Rules["input"][0]) != "daddr=2001:db8::1" { + t.Errorf("ip6 table must strip the guard: %v %q", err, describeRule(s.Rules["input"][0])) + } + if !famsAgree(0, unix.NFPROTO_IPV4, 0, unix.NFPROTO_IPV4) || famsAgree(unix.NFPROTO_IPV4, 0, unix.NFPROTO_IPV6) { + t.Error("famsAgree") + } +} + // TestNetnsNftListDecodes applies each family in a fresh user+net namespace and requires // nft(8) to list the ruleset and a second plan to be empty. Needs unshare and nft. func TestNetnsNftListDecodes(t *testing.T) { From 9bfdf292add7786e5e1cd19173e7be40a02072f4 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Fri, 9 Oct 2026 23:52:02 +1100 Subject: [PATCH 4/4] Count limiter expansion after table-family filtering --- internal/nftables/compiler.go | 39 +++++++++++++++++++++----- internal/nftables/compiler_test.go | 45 ++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 7 deletions(-) diff --git a/internal/nftables/compiler.go b/internal/nftables/compiler.go index 472794f..894cf59 100644 --- a/internal/nftables/compiler.go +++ b/internal/nftables/compiler.go @@ -79,7 +79,7 @@ func (c *Compiler) Compile() (*FirewallState, error) { // family, so a conflict is a compiler bug and fails the compile. An ip/ip6 table is its own guard: // guards go and other-family rules are dropped. func familyGuards(state *FirewallState, family config.AddressFamily) error { - table := map[config.AddressFamily]byte{config.FamilyIP: unix.NFPROTO_IPV4, config.FamilyIP6: unix.NFPROTO_IPV6}[family] + table := tableFamily(family) for chain, rules := range state.Rules { var out []ManagedRule for _, r := range rules { @@ -98,6 +98,11 @@ func familyGuards(state *FirewallState, family config.AddressFamily) error { return nil } +// tableFamily is the NFPROTO an ip/ip6 table is restricted to, 0 for inet. +func tableFamily(family config.AddressFamily) byte { + return map[config.AddressFamily]byte{config.FamilyIP: unix.NFPROTO_IPV4, config.FamilyIP6: unix.NFPROTO_IPV6}[family] +} + // guardFamily is the family exprs' nfproto guards require (0: none); ok is false when they conflict. func guardFamily(exprs []expr.Any) (fam byte, ok bool) { for i := range exprs { @@ -503,7 +508,7 @@ func (c *Compiler) compileRules(state *FirewallState) error { return fmt.Errorf("rule[%d]: %w", i, err) } if len(matches)*c.specCount(rule.Source, rule.Dest, rule.OrigDest, fwZone, rule.Action) > 1 { - return fmt.Errorf("rule[%d]: ratelimit/connlimit cannot be combined with proto, port, zone or address lists (each expanded rule would get its own limiter)", i) + return fmt.Errorf("rule[%d]: ratelimit/connlimit cannot be combined with proto, port, zone or address lists, or with a negated address in an inet table (it expands to one rule per family); each expanded rule would get its own limiter", i) } } @@ -681,19 +686,39 @@ func (c *Compiler) compileDNATAccept(state *FirewallState, tag, srcZone, srcAddr return nil } -// specCount is how many zone/address combinations compileOneRule expands src and dst into. +// specCount is how many rules compileOneRule emits for src and dst once familyGuards has dropped +// cross-family address combinations and those outside an ip/ip6 table's family. func (c *Compiler) specCount(srcSpec, dstSpec, origDest, fwZone string, action config.RuleAction) int { + table := tableFamily(c.cfg.Settings.AddressFamily) + count := func(addrs ...string) int { + n := 0 + var walk func(i int, fams []byte) + walk = func(i int, fams []byte) { + if !famsAgree(fams...) { + return + } + if i == len(addrs) { + n++ + return + } + for _, a := range splitAddrs(addrs[i]) { + walk(i+1, append(fams, addrFamily(a))) + } + } + walk(0, []byte{table}) + return n + } n := 0 if action == config.RuleDNAT || action == config.RuleRedirect { for _, src := range c.dnatSourceSpecs(srcSpec, fwZone) { - n += len(splitAddrs(src.Addr)) + n += count(src.Addr, origDest) } - return n * len(splitAddrs(origDest)) + return n } for _, p := range c.zonePairs(srcSpec, dstSpec, fwZone) { - n += len(splitAddrs(p[0].Addr)) * len(splitAddrs(p[1].Addr)) + n += count(p[0].Addr, p[1].Addr, origDest) } - return n * len(splitAddrs(origDest)) + return n } // zonePairs is the src/dst zone expansion of a non-DNAT rule, with fw added beside all/any. diff --git a/internal/nftables/compiler_test.go b/internal/nftables/compiler_test.go index baf804f..3dde993 100644 --- a/internal/nftables/compiler_test.go +++ b/internal/nftables/compiler_test.go @@ -3644,3 +3644,48 @@ func TestCompile_HostsRouteBack(t *testing.T) { t.Errorf("routeback hosts lan lan policy = %q, want %q", got, want) } } + +func TestCompile_LimitNegatedAddrPerTableFamily(t *testing.T) { + for _, tt := range []struct { + name string + family config.AddressFamily + source string + wantErr bool + }{ + {"ip v4 negation", config.FamilyIP, "net:!192.0.2.1", false}, + {"ip v6 negation", config.FamilyIP, "net:!2001:db8::1", false}, + {"ip mixed negation", config.FamilyIP, "net:!192.0.2.1,2001:db8::1", false}, + {"ip6 v6 negation", config.FamilyIP6, "net:!2001:db8::1", false}, + {"ip6 v4 negation", config.FamilyIP6, "net:!192.0.2.1", false}, + {"inet v4 negation", config.FamilyINET, "net:!192.0.2.1", true}, + } { + t.Run(tt.name, func(t *testing.T) { + cfg := &config.Config{ + Settings: config.Settings{TableName: "test", AddressFamily: tt.family}, + Zones: map[string]config.Zone{"fw": {Type: config.ZoneFirewall}, "net": {Type: config.ZoneIP}}, + Interfaces: []config.Interface{{Zone: "net", Interface: "eth0"}}, + Rules: []config.Rule{{Action: config.RuleAccept, Source: tt.source, Dest: "fw", Proto: "tcp", DPort: config.PortSpec{"22"}, RateLimit: "10/sec"}}, + PortGroups: make(map[string]config.PortGroup), + } + state, err := NewCompiler(cfg).Compile() + if tt.wantErr { + if err == nil || !strings.Contains(err.Error(), "negated address in an inet table") { + t.Fatalf("Compile() error = %v, want negated-address inet error", err) + } + return + } + if err != nil { + t.Fatalf("Compile() error: %v", err) + } + n := 0 + for _, r := range state.Rules["input"] { + if r.Tag == "rule:0" { + n++ + } + } + if n != 1 { + t.Errorf("got %d rule:0 rules, want 1", n) + } + }) + } +}