From a14586b1423a933b28657867845e9398ff78c8c9 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Wed, 10 Jun 2026 17:44:23 +0200 Subject: [PATCH] Scope userspace firewall wildcard source rules per address family --- client/firewall/uspfilter/filter.go | 104 +++++++---------- .../firewall/uspfilter/filter_filter_test.go | 14 ++- client/firewall/uspfilter/filter_test.go | 2 +- .../firewall/uspfilter/peer_acl_dedup_test.go | 12 +- .../uspfilter/peer_family_scope_test.go | 106 ++++++++++++++++++ client/firewall/uspfilter/peer_index.go | 11 +- client/firewall/uspfilter/rule.go | 18 +-- 7 files changed, 175 insertions(+), 92 deletions(-) create mode 100644 client/firewall/uspfilter/peer_family_scope_test.go diff --git a/client/firewall/uspfilter/filter.go b/client/firewall/uspfilter/filter.go index ca297e1be..112a03eda 100644 --- a/client/firewall/uspfilter/filter.go +++ b/client/firewall/uspfilter/filter.go @@ -82,8 +82,9 @@ const ( var errNotSupported = errors.New("not supported with userspace firewall") // peerRules is the canonical list-based storage for peer ACL rules. -// Match order is significant: drop rules come before accept rules so -// callers should consult the slice in order. +// Drop and accept rules live in separate slices; drop-before-accept +// ordering comes from consulting the deny slice (and its index) before +// the accept one. type peerRules []*PeerRule type routeRules []*RouteRule @@ -105,14 +106,13 @@ func (r routeRules) Sort() { // threaded together through the build path so the builders take a single // argument instead of a long parameter list. type peerRuleSpec struct { - mgmtID []byte - sources []netip.Prefix - ipLayer gopacket.LayerType - matchAny bool - proto firewall.Protocol - sPort *firewall.Port - dPort *firewall.Port - action firewall.Action + mgmtID []byte + sources []netip.Prefix + ipLayer gopacket.LayerType + proto firewall.Protocol + sPort *firewall.Port + dPort *firewall.Port + action firewall.Action } // Iface is the network interface the userspace firewall attaches to: the @@ -577,9 +577,9 @@ func (m *Manager) RemoveNatRule(firewall.RouterPair) error { // addPeerRule installs an input-chain rule that matches packets // by source only. Called from AddFilterRule when the caller doesn't -// specify a destination. Mixed-family inputs are split: each family -// gets its own rule with a family-correct ipLayer so packet decoding -// matches what the matcher expects. +// specify a destination. Sources are expected to share one address +// family; the family selects the ipLayer so the ICMP variant matches +// what the decoder produces. func (m *Manager) addPeerRule( id []byte, sources []netip.Prefix, @@ -591,22 +591,9 @@ func (m *Manager) addPeerRule( m.mutex.Lock() defer m.mutex.Unlock() - if sourcesMatchAny(sources) { - spec := peerRuleSpec{ - mgmtID: id, - sources: sources, - ipLayer: layerTypeAll, - matchAny: true, - proto: proto, - sPort: sPort, - dPort: dPort, - action: action, - } - return m.addOnePeerRule(spec), nil - } - // Sources are a single family; normalize v4-mapped prefixes to plain - // v4 and pick the matching IP layer. + // v4 and pick the matching IP layer. A /0 source matches any address + // of its own family only, mirroring the kernel backends. normalized := make([]netip.Prefix, len(sources)) ipLayer := layers.LayerTypeIPv4 for i, p := range sources { @@ -616,14 +603,13 @@ func (m *Manager) addPeerRule( } } spec := peerRuleSpec{ - mgmtID: id, - sources: normalized, - ipLayer: ipLayer, - matchAny: false, - proto: proto, - sPort: sPort, - dPort: dPort, - action: action, + mgmtID: id, + sources: normalized, + ipLayer: ipLayer, + proto: proto, + sPort: sPort, + dPort: dPort, + action: action, } return m.addOnePeerRule(spec), nil } @@ -631,15 +617,16 @@ func (m *Manager) addPeerRule( // addOnePeerRule builds and registers a single-family peer rule, or // returns the existing rule when one with the same content key is // already installed. The caller must hold m.mutex. The content key is -// the shared GenerateRuleID with an empty destination, so peer -// rules dedup the same way route rules and the kernel backends do. +// the shared GenerateRuleID with an empty destination, so peer rules +// dedup the same way route rules and the kernel backends do; it is +// order-independent, so callers passing the same sources in any order +// dedup to one rule. // // There is no refcount: a content key is installed once and deleted on // the first DeleteFilterRule for that key. The caller must therefore // key its own tracking by the returned rule id so add and delete stay // balanced per content key; the acl manager does this via -// peerRulesPairs. The content key is order-independent, so callers -// passing the same sources in any order dedup to one rule. +// peerRulesPairs. func (m *Manager) addOnePeerRule(spec peerRuleSpec) *PeerRule { ruleID := nbid.GenerateRuleID(spec.sources, firewall.Network{}, spec.proto, spec.sPort, spec.dPort, spec.action) if existing, ok := m.peerRulesMap[ruleID]; ok { @@ -653,20 +640,17 @@ func (m *Manager) addOnePeerRule(spec peerRuleSpec) *PeerRule { func (m *Manager) buildPeerRule(ruleID nbid.RuleID, spec peerRuleSpec) *PeerRule { r := &PeerRule{ - id: ruleID, - mgmtId: spec.mgmtID, - sources: spec.sources, - matchAny: spec.matchAny, - action: spec.action, - srcPort: spec.sPort, - dstPort: spec.dPort, + id: ruleID, + mgmtId: spec.mgmtID, + sources: spec.sources, + action: spec.action, + srcPort: spec.sPort, + dstPort: spec.dPort, } - if !spec.matchAny { - r.sourceAddrs = make(map[netip.Addr]struct{}, len(spec.sources)) - for _, p := range spec.sources { - if p.Bits() == p.Addr().BitLen() { - r.sourceAddrs[p.Addr()] = struct{}{} - } + r.sourceAddrs = make(map[netip.Addr]struct{}, len(spec.sources)) + for _, p := range spec.sources { + if p.Bits() == p.Addr().BitLen() { + r.sourceAddrs[p.Addr()] = struct{}{} } } r.protoLayer = protoToLayer(spec.proto, spec.ipLayer) @@ -686,19 +670,6 @@ func (m *Manager) registerPeerRule(r *PeerRule) { m.peerRulesMap[r.id] = r } -// sourcesMatchAny reports whether the source list matches every source, -// i.e. contains an explicit /0 prefix. An empty list does not qualify: -// AddFilterRule rejects it with ErrNoSources, so "match any" is always -// the deliberate /0 case. -func sourcesMatchAny(sources []netip.Prefix) bool { - for _, p := range sources { - if p.Bits() == 0 { - return true - } - } - return false -} - // AddFilterRule is the unified entry point for both peer (input chain) // and route (forward chain) filtering rules. The destination // distinguishes the two semantics: a zero Network installs an @@ -836,6 +807,7 @@ func (m *Manager) resetState() { clear(m.peerRulesMap) clear(m.routeRulesMap) m.routeRules = m.routeRules[:0] + m.blockRules = nil m.udpHookOut.Store(nil) m.tcpHookOut.Store(nil) diff --git a/client/firewall/uspfilter/filter_filter_test.go b/client/firewall/uspfilter/filter_filter_test.go index 55ae04d96..c29c9ec4c 100644 --- a/client/firewall/uspfilter/filter_filter_test.go +++ b/client/firewall/uspfilter/filter_filter_test.go @@ -644,14 +644,24 @@ func TestPeerACLFilteringIPv6(t *testing.T) { shouldBeBlocked: false, }, { - name: "IPv6: v4 wildcard ICMP rule matches ICMPv6 via protoLayerMatches", + name: "IPv6: v4 wildcard ICMP rule does not match ICMPv6", srcIP: "fd00::1", dstIP: "fd00::100", proto: fw.ProtocolICMP, ruleIP: "0.0.0.0", ruleProto: fw.ProtocolICMP, ruleAction: fw.ActionAccept, - shouldBeBlocked: false, + shouldBeBlocked: true, + }, + { + name: "IPv4: v6 wildcard ICMP rule does not match ICMPv4", + srcIP: "100.10.0.1", + dstIP: "100.10.0.100", + proto: fw.ProtocolICMP, + ruleIP: "::", + ruleProto: fw.ProtocolICMP, + ruleAction: fw.ActionAccept, + shouldBeBlocked: true, }, } diff --git a/client/firewall/uspfilter/filter_test.go b/client/firewall/uspfilter/filter_test.go index 8d6610272..280ce9311 100644 --- a/client/firewall/uspfilter/filter_test.go +++ b/client/firewall/uspfilter/filter_test.go @@ -152,7 +152,7 @@ func TestManagerDeleteRule(t *testing.T) { } peerRule, ok := rule2.(*PeerRule) - require.True(t, ok, "rule should be a PeerRule") + require.True(t, ok, "rule should be a peer rule") inMap := func() bool { if peerRule.action == fw.ActionDrop { diff --git a/client/firewall/uspfilter/peer_acl_dedup_test.go b/client/firewall/uspfilter/peer_acl_dedup_test.go index 5c5038208..696766d80 100644 --- a/client/firewall/uspfilter/peer_acl_dedup_test.go +++ b/client/firewall/uspfilter/peer_acl_dedup_test.go @@ -47,12 +47,12 @@ func TestAddPeerFiltering_DeduplicatesIdenticalRules(t *testing.T) { } // TestDeletePeerFiltering_NoRefcountSingleDeleteRemoves locks the -// backend's no-refcount contract: a content key installed twice is -// still one rule, and the first DeleteFilterRule removes it. The -// backend does not refcount, so balance is the caller's job (it keys -// its tracking by the returned id and deletes once per key). If this -// ever silently grew a refcount, the acl manager's delete accounting -// would diverge from the kernel. +// backend's owner accounting for the same-owner case: a content key +// installed twice by the same owner registers one owner claim, so the +// first DeleteFilterRule removes the rule. Owner counting only kicks +// in for distinct management rule IDs (see the peer owner tests); the +// acl manager keys its tracking per (policy, content) and deletes once +// per key, so adds and deletes stay balanced. func TestDeletePeerFiltering_NoRefcountSingleDeleteRemoves(t *testing.T) { m := newTestManager(t) diff --git a/client/firewall/uspfilter/peer_family_scope_test.go b/client/firewall/uspfilter/peer_family_scope_test.go new file mode 100644 index 000000000..13c3a616e --- /dev/null +++ b/client/firewall/uspfilter/peer_family_scope_test.go @@ -0,0 +1,106 @@ +package uspfilter + +import ( + "net" + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + fw "github.com/netbirdio/netbird/client/firewall/manager" +) + +// peerACLCheck decodes the packet and runs it through the peer ACLs, +// returning the attributed management rule id and the drop verdict. +func peerACLCheck(t *testing.T, m *Manager, packet []byte) ([]byte, bool) { + t.Helper() + d := m.decoders.Get().(*decoder) + defer m.decoders.Put(d) + require.NoError(t, d.decodePacket(packet)) + src, _ := m.extractIPs(d) + return m.peerACLsBlock(src, d, packet) +} + +// TestPeerACL_MultiValuePortMatchesEachListedPort guards the multi-value +// port path: a rule listing several discrete destination ports must +// match a packet to each listed port and drop one that is not listed. +// Management currently splits a multi-port policy into one rule per port +// (and the wire format carries a single port), so this list shape is not +// emitted today; the test locks correct matching in case that changes. +func TestPeerACL_MultiValuePortMatchesEachListedPort(t *testing.T) { + m := newTestManager(t) + + src := net.ParseIP("192.168.1.1") + ports := &fw.Port{Values: []uint16{80, 443}} + _, err := m.AddFilterRule(nil, pfx(src), fw.Network{}, fw.ProtocolTCP, nil, ports, fw.ActionAccept) + require.NoError(t, err, "add multi-value port rule") + + for _, p := range []uint16{80, 443} { + _, blocked := peerACLCheck(t, m, createTestPacket(t, "192.168.1.1", "10.0.0.2", fw.ProtocolTCP, 12345, p)) + assert.False(t, blocked, "packet to listed port %d must match the rule", p) + } + + _, blocked := peerACLCheck(t, m, createTestPacket(t, "192.168.1.1", "10.0.0.2", fw.ProtocolTCP, 12345, 8080)) + assert.True(t, blocked, "packet to a port not in the list must not match the rule") +} + +// TestPeerACL_MatchAnyIsFamilyScoped verifies that a /0 source matches +// only packets of its own family: 0.0.0.0/0 must not match IPv6 packets +// and ::/0 must not match IPv4 packets, matching kernel backend +// semantics. +func TestPeerACL_MatchAnyIsFamilyScoped(t *testing.T) { + m := newTestManager(t) + + v4Packet := createTestPacket(t, "10.0.0.1", "10.0.0.2", fw.ProtocolUDP, 12345, 53) + v6Packet := v6UDPPacket(t, "fd00::1", "fd00::100", 53) + + v4Any := []netip.Prefix{netip.PrefixFrom(netip.IPv4Unspecified(), 0)} + rule, err := m.AddFilterRule(nil, v4Any, fw.Network{}, fw.ProtocolALL, nil, nil, fw.ActionAccept) + require.NoError(t, err, "add v4 /0 rule") + + _, blocked := peerACLCheck(t, m, v4Packet) + assert.False(t, blocked, "0.0.0.0/0 must match IPv4 packets") + _, blocked = peerACLCheck(t, m, v6Packet) + assert.True(t, blocked, "0.0.0.0/0 must not match IPv6 packets") + + require.NoError(t, m.DeleteFilterRule(rule)) + + v6Any := []netip.Prefix{netip.PrefixFrom(netip.IPv6Unspecified(), 0)} + _, err = m.AddFilterRule(nil, v6Any, fw.Network{}, fw.ProtocolALL, nil, nil, fw.ActionAccept) + require.NoError(t, err, "add v6 /0 rule") + + _, blocked = peerACLCheck(t, m, v6Packet) + assert.False(t, blocked, "::/0 must match IPv6 packets") + _, blocked = peerACLCheck(t, m, v4Packet) + assert.True(t, blocked, "::/0 must not match IPv4 packets") +} + +// TestRouteACL_MixedFamilyZeroSourcesStayFamilySafe verifies the route +// path keeps per-prefix family matching when a single rule carries both +// 0.0.0.0/0 and ::/0 sources, as blockInvalidRouted does. +func TestRouteACL_MixedFamilyZeroSourcesStayFamilySafe(t *testing.T) { + m := newTestManager(t) + + sources := []netip.Prefix{ + netip.PrefixFrom(netip.IPv4Unspecified(), 0), + netip.PrefixFrom(netip.IPv6Unspecified(), 0), + } + + _, err := m.AddFilterRule(nil, sources, fw.Network{Prefix: netip.MustParsePrefix("10.0.0.0/24")}, + fw.ProtocolALL, nil, nil, fw.ActionAccept) + require.NoError(t, err) + _, err = m.AddFilterRule(nil, sources, fw.Network{Prefix: netip.MustParsePrefix("fd00:1::/64")}, + fw.ProtocolALL, nil, nil, fw.ActionAccept) + require.NoError(t, err) + + v4Src := netip.MustParseAddr("192.168.1.1") + v6Src := netip.MustParseAddr("fd00::1") + + _, pass := m.routeACLsPass(v4Src, netip.MustParseAddr("10.0.0.5"), 255, 0, 0) + assert.True(t, pass, "v4 source must match the v4 destination rule via 0.0.0.0/0") + _, pass = m.routeACLsPass(v6Src, netip.MustParseAddr("fd00:1::5"), 255, 0, 0) + assert.True(t, pass, "v6 source must match the v6 destination rule via ::/0") + _, pass = m.routeACLsPass(v6Src, netip.MustParseAddr("10.0.0.5"), 255, 0, 0) + assert.True(t, pass, "v6 source still passes the v4 destination rule via ::/0 in the same source list") +} diff --git a/client/firewall/uspfilter/peer_index.go b/client/firewall/uspfilter/peer_index.go index 12e54d6ec..552ffd2d7 100644 --- a/client/firewall/uspfilter/peer_index.go +++ b/client/firewall/uspfilter/peer_index.go @@ -65,10 +65,11 @@ func (i *peerRuleIndex) reset() { } // match returns the first rule matching src and the decoded packet. -// Host rules are found by direct map lookup; nonHost rules need a -// per-rule source Contains() check, except match-any (/0) rules which -// apply to every source regardless of family (a v4 /0 also matches v6). -// Within either bucket the matcher runs the proto/port filter. +// Host rules are found by direct map lookup; nonHost rules run a +// per-rule source Contains() check. Containment is family-scoped, so +// a /0 source matches every address of its own family only (0.0.0.0/0 +// never matches v6 sources and ::/0 never matches v4). Within either +// bucket the matcher runs the proto/port filter. func (i *peerRuleIndex) match(src netip.Addr, d *decoder) ([]byte, bool, bool) { payloadLayer := d.decoded[1] @@ -78,7 +79,7 @@ func (i *peerRuleIndex) match(src netip.Addr, d *decoder) ([]byte, bool, bool) { } } for _, rule := range i.nonHost { - if !rule.matchAny && !prefixesContain(rule.sources, src) { + if !prefixesContain(rule.sources, src) { continue } if id, drop, ok := matchProto(rule, d, payloadLayer); ok { diff --git a/client/firewall/uspfilter/rule.go b/client/firewall/uspfilter/rule.go index fa40476c1..6d73b19ac 100644 --- a/client/firewall/uspfilter/rule.go +++ b/client/firewall/uspfilter/rule.go @@ -19,22 +19,16 @@ type PeerRule struct { // sources (/32 v4, /128 v6). Populated alongside sources; // consulted before falling back to prefix scan. sourceAddrs map[netip.Addr]struct{} - // matchAny is true when sources covers everything (0.0.0.0/0, - // ::/0). In that case neither sourceAddrs nor sources need to be - // consulted. - matchAny bool - protoLayer gopacket.LayerType - srcPort *firewall.Port - dstPort *firewall.Port - action firewall.Action + protoLayer gopacket.LayerType + srcPort *firewall.Port + dstPort *firewall.Port + action firewall.Action } // matchesSource reports whether the given source address is covered -// by this rule's source list. +// by this rule's source list. Prefix containment is family-scoped, so +// a /0 source matches every address of its own family only. func (r *PeerRule) matchesSource(src netip.Addr) bool { - if r.matchAny { - return true - } if _, ok := r.sourceAddrs[src]; ok { return true }