[client] Do not misroute WireGuard packets to the STUN handler (#7059)

client/iface/bind/ice_bind.go

@@ -22,6 +22,16 @@ import (
 	nbnet "github.com/netbirdio/netbird/client/net"
 )
 
+const (
+	// wgMsgTypeHandshakeInitiation is the lowest WireGuard message type.
+	wgMsgTypeHandshakeInitiation uint32 = 1
+	// wgMsgTypeTransport is the highest WireGuard message type.
+	wgMsgTypeTransport uint32 = 4
+	// wgMinMsgSize is the smallest WireGuard message: transport data with an empty
+	// payload, which is what a keepalive is.
+	wgMinMsgSize = 32
+)
+
 type receiverCreator struct {
 	iceBind *ICEBind
 }
@@ -216,8 +226,15 @@ func (s *ICEBind) createReceiverFn(pc wgConn.BatchReader, conn *net.UDPConn, rxO
 		for i := 0; i < numMsgs; i++ {
 			msg := &(*msgs)[i]
 
-			// todo: handle err
-			if ok, _ := s.filterOutStunMessages(msg.Buffers, msg.N, msg.Addr); ok {
+			if ok, err := s.filterOutStunMessages(msg.Buffers, msg.N, msg.Addr); ok {
+				if err != nil {
+					log.Debugf("failed to handle STUN packet from %s: %v", msg.Addr, err)
+				}
+				// WireGuard reuses sizes and eps across reads and only skips a slot
+				// whose size is below the minimum message size. Leaving a consumed
+				// slot untouched makes it process this buffer again under the
+				// previous packet's length and endpoint.
+				sizes[i] = 0
 				continue
 			}
 			sizes[i] = msg.N
@@ -271,11 +288,16 @@ func (s *ICEBind) createOrUpdateMux() {
 
 func (s *ICEBind) filterOutStunMessages(buffers [][]byte, n int, addr net.Addr) (bool, error) {
 	for i := range buffers {
-		if !stun.IsMessage(buffers[i]) {
+		if n > len(buffers[i]) {
+			continue
+		}
+		pkt := buffers[i][:n]
+
+		if isWireGuardMsg(pkt) || !stun.IsMessage(pkt) {
 			continue
 		}
 
-		msg, err := s.parseSTUNMessage(buffers[i][:n])
+		msg, err := s.parseSTUNMessage(pkt)
 		if err != nil {
 			buffers[i] = []byte{}
 			return true, err
@@ -347,18 +369,34 @@ func putMessages(msgs *[]ipv6.Message, msgsPool *sync.Pool) {
 	msgsPool.Put(msgs)
 }
 
-func isTransportPkg(buffers [][]byte, n int) bool {
-	// The first buffer should contain at least 4 bytes for type
-	if len(buffers[0]) < 4 {
-		return true
+// isWireGuardMsg reports whether the packet carries a WireGuard message header: a
+// little-endian uint32 message type in the range 1..4, which leaves the three bytes
+// after the type byte zero, in a packet long enough to hold any WireGuard message.
+//
+// A well formed STUN message cannot take that shape. Its length field sits in the two
+// bytes the type must leave zero, and for a message of at least wgMinMsgSize bytes that
+// field holds at least 12, so the two framings do not overlap. The test has to be this
+// tight because stun.IsMessage only looks at the magic cookie, which in a WireGuard
+// message overlaps the receiver index: a session whose index happens to equal the cookie
+// would otherwise have all of its inbound data misrouted to the STUN handler until the
+// next rekey.
+func isWireGuardMsg(pkt []byte) bool {
+	if len(pkt) < wgMinMsgSize {
+		return false
 	}
 
-	// WireGuard packet type is a little-endian uint32 at start
-	packetType := binary.LittleEndian.Uint32(buffers[0][:4])
+	msgType := binary.LittleEndian.Uint32(pkt[:4])
+	return msgType >= wgMsgTypeHandshakeInitiation && msgType <= wgMsgTypeTransport
+}
 
-	// Check if packetType matches known WireGuard message types
-	if packetType == 4 && n > 32 {
-		return true
+// isTransportPkg reports whether the packet is WireGuard transport data carrying a
+// payload, which is what counts as peer activity. A keepalive holds no payload and is
+// exactly wgMinMsgSize bytes.
+func isTransportPkg(buffers [][]byte, n int) bool {
+	if n < 4 || n > len(buffers[0]) {
+		return false
 	}
-	return false
+
+	msgType := binary.LittleEndian.Uint32(buffers[0][:4])
+	return msgType == wgMsgTypeTransport && n > wgMinMsgSize
 }