From 8efed72c965a8668fab0ba111bb899c352786052 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 3 Oct 2026 20:50:36 +1000 Subject: [PATCH] Match exact all/any zone tokens, skip only interface-less ip zones, keep DNAT free of rule extras, reject '!' inside address lists --- internal/config/blrules.go | 6 ++++ internal/config/config_test.go | 13 +++++++ internal/config/rules.go | 11 ++++++ internal/nftables/compiler.go | 22 ++++++++---- internal/nftables/compiler_test.go | 58 +++++++++++++++++++++++++++--- 5 files changed, 100 insertions(+), 10 deletions(-) diff --git a/internal/config/blrules.go b/internal/config/blrules.go index f361ea1..4118d22 100644 --- a/internal/config/blrules.go +++ b/internal/config/blrules.go @@ -61,6 +61,9 @@ func (c *Config) validateBlrules() error { if _, ok := c.Zones[zs.Zone]; !ok { return fmt.Errorf("blrules[%d]: source zone %q not defined", i, zs.Zone) } + if !validAddrList(zs.Addr) { + return fmt.Errorf("blrules[%d]: source %q: '!' may only prefix the whole address list", i, zs.Addr) + } } } @@ -70,6 +73,9 @@ func (c *Config) validateBlrules() error { if _, ok := c.Zones[zs.Zone]; !ok { return fmt.Errorf("blrules[%d]: dest zone %q not defined", i, zs.Zone) } + if !validAddrList(zs.Addr) { + return fmt.Errorf("blrules[%d]: dest %q: '!' may only prefix the whole address list", i, zs.Addr) + } } } } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 012c997..7485bae 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -868,6 +868,19 @@ func TestValidateRules(t *testing.T) { }, wantErr: "dest zone \"missing\" not defined", }, + { + name: "negation prefixing the whole address list is valid", + rules: []Rule{ + {Action: RuleAccept, Source: "net:!192.0.2.1,198.51.100.1", Dest: "fw"}, + }, + }, + { + name: "negation inside an address list", + rules: []Rule{ + {Action: RuleAccept, Source: "net", Dest: "loc:192.0.2.1,!198.51.100.1"}, + }, + wantErr: "'!' may only prefix the whole address list", + }, { name: "all keyword is valid source", rules: []Rule{ diff --git a/internal/config/rules.go b/internal/config/rules.go index 1b36111..fb94e31 100644 --- a/internal/config/rules.go +++ b/internal/config/rules.go @@ -179,6 +179,9 @@ func (c *Config) validateRules() error { if _, ok := c.Zones[zs.Zone]; !ok { return fmt.Errorf("rule[%d]: source zone %q not defined", i, zs.Zone) } + if !validAddrList(zs.Addr) { + return fmt.Errorf("rule[%d]: source %q: '!' may only prefix the whole address list", i, zs.Addr) + } } } @@ -189,6 +192,9 @@ func (c *Config) validateRules() error { if _, ok := c.Zones[zs.Zone]; !ok { return fmt.Errorf("rule[%d]: dest zone %q not defined", i, zs.Zone) } + if !validAddrList(zs.Addr) { + return fmt.Errorf("rule[%d]: dest %q: '!' may only prefix the whole address list", i, zs.Addr) + } } } } @@ -243,6 +249,11 @@ func SplitZoneList(spec string) []ZoneSpec { return out } +// validAddrList reports whether '!' appears only at the start, negating the whole list. +func validAddrList(addr string) bool { + return !strings.Contains(strings.TrimPrefix(addr, "!"), "!") +} + // zoneFromSpec extracts the zone name from a zone spec like "net" or "net:192.168.1.0/24". func zoneFromSpec(spec string) string { for i, c := range spec { diff --git a/internal/nftables/compiler.go b/internal/nftables/compiler.go index 0a20e5a..1d5c409 100644 --- a/internal/nftables/compiler.go +++ b/internal/nftables/compiler.go @@ -15,7 +15,8 @@ import ( ) type Compiler struct { - cfg *config.Config + cfg *config.Config + warned map[string]bool } func NewCompiler(cfg *config.Config) *Compiler { @@ -26,6 +27,7 @@ func (c *Compiler) Compile() (*FirewallState, error) { state := &FirewallState{ Rules: make(map[string][]ManagedRule), } + c.warned = nil c.compileLoopback(state) if err := c.compileConntrackFastPath(state); err != nil { @@ -297,7 +299,9 @@ func (c *Compiler) compileRules(state *FirewallState) error { func (c *Compiler) applyRuleExtras(state *FirewallState, tag string, rule config.Rule) { for chain := range state.Rules { - c.applyChainExtras(state, chain, tag, rule) + if chain != "prerouting" { + c.applyChainExtras(state, chain, tag, rule) + } } } @@ -394,8 +398,8 @@ func specCount(srcSpec, dstSpec string, action config.RuleAction) int { // zoneSpecs expands a comma zone list; "all"/"any" forms keep their own comma (exclusion) syntax. func zoneSpecs(spec string) []config.ZoneSpec { - if strings.HasPrefix(spec, "all") || strings.HasPrefix(spec, "any") { - zone, addr := splitZoneSpec(spec) + zone, addr := splitZoneSpec(spec) + if base, _, _ := strings.Cut(strings.TrimSuffix(zone, "+"), "!"); base == "all" || base == "any" { return []config.ZoneSpec{{Zone: zone, Addr: addr}} } return config.SplitZoneList(spec) @@ -935,8 +939,14 @@ func (c *Compiler) resolveZoneInterfaces(zone string) []string { if len(ifaces) > 0 { return ifaces } - if z, ok := c.cfg.Zones[zone]; ok && z.Type != config.ZoneFirewall && !c.zoneHasHosts(zone) { - slog.Warn("compiler: zone has no interfaces, skipping its rules", "zone", zone) + if z, ok := c.cfg.Zones[zone]; ok && z.Type == config.ZoneIP && !c.zoneHasHosts(zone) { + if !c.warned[zone] { + if c.warned == nil { + c.warned = map[string]bool{} + } + c.warned[zone] = true + slog.Warn("compiler: zone has no interfaces, skipping its rules", "zone", zone) + } return nil } return []string{""} diff --git a/internal/nftables/compiler_test.go b/internal/nftables/compiler_test.go index 8287e49..75c2eac 100644 --- a/internal/nftables/compiler_test.go +++ b/internal/nftables/compiler_test.go @@ -1655,6 +1655,9 @@ func describeRule(r ManagedRule) string { case *expr.Payload: if m.Base == expr.PayloadBaseNetworkHeader && m.Len == 4 { name := map[uint32]string{12: "saddr", 16: "daddr"}[m.Offset] + if cmp.Op == expr.CmpOpNeq { + name = "!" + name + } parts = append(parts, fmt.Sprintf("%s=%d.%d.%d.%d", name, cmp.Data[0], cmp.Data[1], cmp.Data[2], cmp.Data[3])) } } @@ -1664,9 +1667,10 @@ func describeRule(r ManagedRule) string { func TestCompile_CommaZoneLists(t *testing.T) { tests := []struct { - name string - rule config.Rule - want map[string][]string + name string + rule config.Rule + blrule *config.BlruleRule + want map[string][]string }{ { name: "fw in source list goes to output", @@ -1698,6 +1702,31 @@ func TestCompile_CommaZoneLists(t *testing.T) { 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"}}, }, + { + 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"}}, + }, + { + 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"}}, + }, + { + name: "zone named like all/any keyword is a plain zone", + rule: config.Rule{Action: config.RuleAccept, Source: "lan", Dest: "anycast,net"}, + want: map[string][]string{"forward": {"iif=eth1 oif=eth3", "iif=eth1 oif=eth0"}}, + }, + { + name: "interface-less ipsec zone keeps zone-agnostic rule, ip zone skipped", + rule: config.Rule{Action: config.RuleAccept, Source: "lan", Dest: "vpn,dmz"}, + want: map[string][]string{"forward": {"iif=eth1"}}, + }, + { + name: "blrule zone list", + blrule: &config.BlruleRule{Action: config.BlruleDrop, Source: "net,anycast", Dest: "fw"}, + want: map[string][]string{"input": {"iif=eth0", "iif=eth3"}}, + }, } for _, tt := range tests { @@ -1707,13 +1736,19 @@ func TestCompile_CommaZoneLists(t *testing.T) { Zones: map[string]config.Zone{ "fw": {Type: config.ZoneFirewall}, "net": {Type: config.ZoneIP}, "lan": {Type: config.ZoneIP}, "svr": {Type: config.ZoneIP}, "dmz": {Type: config.ZoneIP}, + "anycast": {Type: config.ZoneIP}, "vpn": {Type: config.ZoneIPSec}, }, Interfaces: []config.Interface{ {Zone: "net", Interface: "eth0"}, {Zone: "lan", Interface: "eth1"}, {Zone: "svr", Interface: "eth2"}, + {Zone: "anycast", Interface: "eth3"}, }, Rules: []config.Rule{tt.rule}, PortGroups: make(map[string]config.PortGroup), } + tag := "rule:0" + if tt.blrule != nil { + cfg.Rules, cfg.Blrules, tag = nil, []config.BlruleRule{*tt.blrule}, "blrule:0" + } state, err := NewCompiler(cfg).Compile() if err != nil { t.Fatalf("Compile() error: %v", err) @@ -1721,7 +1756,7 @@ func TestCompile_CommaZoneLists(t *testing.T) { got := map[string][]string{} for chain, rules := range state.Rules { for _, r := range rules { - if r.Tag == "rule:0" { + if r.Tag == tag { got[chain] = append(got[chain], describeRule(r)) } } @@ -1789,6 +1824,21 @@ func TestCompile_ListExpansionCounts(t *testing.T) { } } +func TestCompile_DNATGetsNoRuleExtras(t *testing.T) { + compile := func(mark string) []expr.Any { + state, err := NewCompiler(listCfg(func(c *config.Config) { + c.Rules = []config.Rule{{Action: config.RuleDNAT, Source: "net", Dest: "fw:192.0.2.10", Proto: "tcp", DPort: config.PortSpec{"80"}, Mark: mark}} + })).Compile() + if err != nil { + t.Fatalf("Compile() error: %v", err) + } + return state.Rules["prerouting"][len(state.Rules["prerouting"])-1].Exprs + } + if plain, marked := compile(""), compile("0x1"); !reflect.DeepEqual(plain, marked) { + t.Errorf("DNAT prerouting rule changed by mark extra: %d exprs vs %d", len(plain), len(marked)) + } +} + func TestCompile_CommaZoneListLimitErrors(t *testing.T) { for _, r := range []config.Rule{ {Action: config.RuleAccept, Source: "net", Dest: "fw,lan", RateLimit: "10/sec:5"},