Better words (#1497)

* Fix recv_error receipt limit allowance

* backport #1463 recv_error behavior changes

---------

Co-authored-by: JackDoan <me@jackdoan.com>

CHANGELOG.md

@@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [1.9.7] - 2025-10-10
+
+### Security
+
+- Fix an issue where Nebula could incorrectly accept and process a packet from an erroneous source IP when the sender's
+  certificate is configured with unsafe_routes (cert v1/v2) or multiple IPs (cert v2). (#1494)
+
+### Changed
+
+- Disable sending `recv_error` messages when a packet is received outside the allowable counter window. (#1459)
+- Improve error messages and remove some unnecessary fatal conditions in the Windows and generic udp listener. (#1543)
+
 ## [1.9.6] - 2025-7-15
 
 ### Added
@@ -687,7 +699,8 @@ created.)
 
 - Initial public release.
 
-[Unreleased]: https://github.com/slackhq/nebula/compare/v1.9.6...HEAD
+[Unreleased]: https://github.com/slackhq/nebula/compare/v1.9.7...HEAD
+[1.9.7]: https://github.com/slackhq/nebula/releases/tag/v1.9.7
 [1.9.6]: https://github.com/slackhq/nebula/releases/tag/v1.9.6
 [1.9.5]: https://github.com/slackhq/nebula/releases/tag/v1.9.5
 [1.9.4]: https://github.com/slackhq/nebula/releases/tag/v1.9.4

hostmap.go

@@ -22,7 +22,6 @@ const defaultPromoteEvery = 1000       // Count of packets sent before we try mo
 const defaultReQueryEvery = 5000       // Count of packets sent before re-querying a hostinfo to the lighthouse
 const defaultReQueryWait = time.Minute // Minimum amount of seconds to wait before re-querying a hostinfo the lighthouse. Evaluated every ReQueryEvery
 const MaxRemotes = 10
-const maxRecvError = 4
 
 // MaxHostInfosPerVpnIp is the max number of hostinfos we will track for a given vpn ip
 // 5 allows for an initial handshake and each host pair re-handshaking twice
@@ -220,7 +219,6 @@ type HostInfo struct {
 	remoteIndexId   uint32
 	localIndexId    uint32
 	vpnIp           netip.Addr
-	recvError       atomic.Uint32
 	remoteCidr      *bart.Table[struct{}]
 	relayState      RelayState
 
@@ -705,13 +703,6 @@ func (i *HostInfo) SetRemoteIfPreferred(hm *HostMap, newRemote netip.AddrPort) b
 	return false
 }
 
-func (i *HostInfo) RecvErrorExceeded() bool {
-	if i.recvError.Add(1) >= maxRecvError {
-		return true
-	}
-	return true
-}
-
 func (i *HostInfo) CreateRemoteCIDR(c *cert.NebulaCertificate) {
 	if len(c.Details.Ips) == 1 && len(c.Details.Subnets) == 0 {
 		// Simple case, no CIDRTree needed
@@ -723,8 +714,7 @@ func (i *HostInfo) CreateRemoteCIDR(c *cert.NebulaCertificate) {
 		//TODO: IPV6-WORK what to do when ip is invalid?
 		nip, _ := netip.AddrFromSlice(ip.IP)
 		nip = nip.Unmap()
-		bits, _ := ip.Mask.Size()
-		remoteCidr.Insert(netip.PrefixFrom(nip, bits), struct{}{})
+		remoteCidr.Insert(netip.PrefixFrom(nip, nip.BitLen()), struct{}{})
 	}
 
 	for _, n := range c.Details.Subnets {

hostmap_test.go

@@ -1,9 +1,11 @@
 package nebula
 
 import (
+	"net"
 	"net/netip"
 	"testing"
 
+	"github.com/slackhq/nebula/cert"
 	"github.com/slackhq/nebula/config"
 	"github.com/slackhq/nebula/test"
 	"github.com/stretchr/testify/assert"
@@ -87,6 +89,40 @@ func TestHostMap_MakePrimary(t *testing.T) {
 	assert.Nil(t, h2.next)
 }
 
+func TestHostInfo_CreateRemoteCIDR(t *testing.T) {
+	h := HostInfo{}
+	c := &cert.NebulaCertificate{
+		Details: cert.NebulaCertificateDetails{
+			Ips: []*net.IPNet{
+				{
+					IP:   net.IPv4(1, 2, 3, 4),
+					Mask: net.IPv4Mask(255, 255, 255, 0),
+				},
+			},
+		},
+	}
+
+	// remoteCidr should be empty with only 1 ip address present in the certificate
+	h.CreateRemoteCIDR(c)
+	assert.Empty(t, h.remoteCidr)
+
+	// remoteCidr should be populated if there is also a subnet in the certificate
+	c.Details.Subnets = []*net.IPNet{
+		{
+			IP:   net.IPv4(9, 2, 3, 4),
+			Mask: net.IPv4Mask(255, 255, 255, 0),
+		},
+	}
+	h.CreateRemoteCIDR(c)
+	assert.NotEmpty(t, h.remoteCidr)
+	_, ok := h.remoteCidr.Lookup(netip.MustParseAddr("1.2.3.0"))
+	assert.False(t, ok, "An ip address within the certificates network should not be found")
+	_, ok = h.remoteCidr.Lookup(netip.MustParseAddr("1.2.3.4"))
+	assert.True(t, ok, "An exact ip address match should be found")
+	_, ok = h.remoteCidr.Lookup(netip.MustParseAddr("9.2.3.4"))
+	assert.True(t, ok, "An ip address within the subnets should be found")
+}
+
 func TestHostMap_DeleteHostInfo(t *testing.T) {
 	l := test.NewLogger()
 	hm := newHostMap(

outside.go

@@ -286,16 +286,18 @@ func (f *Interface) handleHostRoaming(hostinfo *HostInfo, ip netip.AddrPort) {
 
 }
 
+// handleEncrypted returns true if a packet should be processed, false otherwise
 func (f *Interface) handleEncrypted(ci *ConnectionState, addr netip.AddrPort, h *header.H) bool {
-	// If connectionstate exists and the replay protector allows, process packet
-	// Else, send recv errors for 300 seconds after a restart to allow fast reconnection.
-	if ci == nil || !ci.window.Check(f.l, h.MessageCounter) {
+	// If connectionstate does not exist, send a recv error, if possible, to encourage a fast reconnect
+	if ci == nil {
 		if addr.IsValid() {
 			f.maybeSendRecvError(addr, h.RemoteIndex)
-			return false
-		} else {
+		}
 		return false
 	}
+	// If the window check fails, refuse to process the packet, but don't send a recv error
+	if !ci.window.Check(f.l, h.MessageCounter) {
+		return false
 	}
 
 	return true
@@ -458,10 +460,6 @@ func (f *Interface) handleRecvError(addr netip.AddrPort, h *header.H) {
 		return
 	}
 
-	if !hostinfo.RecvErrorExceeded() {
-		return
-	}
-
 	if hostinfo.remote.IsValid() && hostinfo.remote != addr {
 		f.l.Infoln("Someone spoofing recv_errors? ", addr, hostinfo.remote)
 		return

udp/udp_generic.go

@@ -10,9 +10,11 @@ package udp
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net"
 	"net/netip"
+	"time"
 
 	"github.com/sirupsen/logrus"
 	"github.com/slackhq/nebula/config"
@@ -80,13 +82,23 @@ func (u *GenericConn) ListenOut(r EncReader, lhf LightHouseHandlerFunc, cache *f
 	fwPacket := &firewall.Packet{}
 	nb := make([]byte, 12, 12)
 
+	var lastRecvErr time.Time
+
 	for {
 		// Just read one packet at a time
 		n, rua, err := u.ReadFromUDPAddrPort(buffer)
 		if err != nil {
+			if errors.Is(err, net.ErrClosed) {
 				u.l.WithError(err).Debug("udp socket is closed, exiting read loop")
 				return
 			}
+			// Dampen unexpected message warns to once per minute
+			if lastRecvErr.IsZero() || time.Since(lastRecvErr) > time.Minute {
+				lastRecvErr = time.Now()
+				u.l.WithError(err).Warn("unexpected udp socket receive error")
+			}
+			continue
+		}
 
 		r(
 			netip.AddrPortFrom(rua.Addr().Unmap(), rua.Port()),

udp/udp_rio_windows.go

@@ -14,6 +14,7 @@ import (
 	"sync"
 	"sync/atomic"
 	"syscall"
+	"time"
 	"unsafe"
 
 	"github.com/sirupsen/logrus"
@@ -69,7 +70,7 @@ func NewRIOListener(l *logrus.Logger, addr netip.Addr, port int) (*RIOConn, erro
 
 	u := &RIOConn{l: l}
 
-	err := u.bind(&windows.SockaddrInet6{Addr: addr.As16(), Port: port})
+	err := u.bind(l, &windows.SockaddrInet6{Addr: addr.As16(), Port: port})
 	if err != nil {
 		return nil, fmt.Errorf("bind: %w", err)
 	}
@@ -85,11 +86,11 @@ func NewRIOListener(l *logrus.Logger, addr netip.Addr, port int) (*RIOConn, erro
 	return u, nil
 }
 
-func (u *RIOConn) bind(sa windows.Sockaddr) error {
+func (u *RIOConn) bind(l *logrus.Logger, sa windows.Sockaddr) error {
 	var err error
 	u.sock, err = winrio.Socket(windows.AF_INET6, windows.SOCK_DGRAM, windows.IPPROTO_UDP)
 	if err != nil {
-		return err
+		return fmt.Errorf("winrio.Socket error: %w", err)
 	}
 
 	// Enable v4 for this socket
@@ -103,35 +104,40 @@ func (u *RIOConn) bind(sa windows.Sockaddr) error {
 	size := uint32(unsafe.Sizeof(flag))
 	err = syscall.WSAIoctl(syscall.Handle(u.sock), syscall.SIO_UDP_CONNRESET, (*byte)(unsafe.Pointer(&flag)), size, nil, 0, &ret, nil, 0)
 	if err != nil {
-		return err
+		// This is a best-effort to prevent errors from being returned by the udp recv operation.
+		// Quietly log a failure and continue.
+		l.WithError(err).Debug("failed to set UDP_CONNRESET ioctl")
 	}
+
 	ret = 0
 	flag = 0
 	size = uint32(unsafe.Sizeof(flag))
 	SIO_UDP_NETRESET := uint32(syscall.IOC_IN | syscall.IOC_VENDOR | 15)
 	err = syscall.WSAIoctl(syscall.Handle(u.sock), SIO_UDP_NETRESET, (*byte)(unsafe.Pointer(&flag)), size, nil, 0, &ret, nil, 0)
 	if err != nil {
-		return err
+		// This is a best-effort to prevent errors from being returned by the udp recv operation.
+		// Quietly log a failure and continue.
+		l.WithError(err).Debug("failed to set UDP_NETRESET ioctl")
 	}
 
 	err = u.rx.Open()
 	if err != nil {
-		return err
+		return fmt.Errorf("error rx.Open(): %w", err)
 	}
 
 	err = u.tx.Open()
 	if err != nil {
-		return err
+		return fmt.Errorf("error tx.Open(): %w", err)
 	}
 
 	u.rq, err = winrio.CreateRequestQueue(u.sock, packetsPerRing, 1, packetsPerRing, 1, u.rx.cq, u.tx.cq, 0)
 	if err != nil {
-		return err
+		return fmt.Errorf("error CreateRequestQueue: %w", err)
 	}
 
 	err = windows.Bind(u.sock, sa)
 	if err != nil {
-		return err
+		return fmt.Errorf("error windows.Bind(): %w", err)
 	}
 
 	return nil
@@ -144,15 +150,22 @@ func (u *RIOConn) ListenOut(r EncReader, lhf LightHouseHandlerFunc, cache *firew
 	fwPacket := &firewall.Packet{}
 	nb := make([]byte, 12, 12)
 
+	var lastRecvErr time.Time
+
 	for {
 		// Just read one packet at a time
 		n, rua, err := u.receive(buffer)
+
 		if err != nil {
 			if errors.Is(err, net.ErrClosed) {
 				u.l.WithError(err).Debug("udp socket is closed, exiting read loop")
 				return
 			}
-			u.l.WithError(err).Error("unexpected udp socket receive error")
+			// Dampen unexpected message warns to once per minute
+			if lastRecvErr.IsZero() || time.Since(lastRecvErr) > time.Minute {
+				lastRecvErr = time.Now()
+				u.l.WithError(err).Warn("unexpected udp socket receive error")
+			}
 			continue
 		}