Scope userspace firewall wildcard source rules per address family

2026-09-17 12:19:07 +02:00 · 2026-06-10 17:44:23 +02:00
parent 09c0063d71
commit a14586b142
7 changed files with 175 additions and 92 deletions
@@ -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)

@@ -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,
 		},
 	}

@@ -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 {
@@ -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)

@@ -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")
+}
@@ -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 {
@@ -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
 	}