From 473d5a79d716085f18b5e4125a57593ac3b1f1f0 Mon Sep 17 00:00:00 2001
From: Viktor Liu <viktor@netbird.io>
Date: Fri, 31 May 2024 22:44:26 +0900
Subject: [PATCH] Ignore candidates whose IP falls into a routed network.

This will prevent peer connections via other peers.
---
 client/internal/engine.go    | 19 ++++++++++++++---
 client/internal/peer/conn.go | 40 +++++++++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/client/internal/engine.go b/client/internal/engine.go
index 9c96dc50def..83e6928f4bc 100644
--- a/client/internal/engine.go
+++ b/client/internal/engine.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"maps"
 	"math/rand"
 	"net"
 	"net/netip"
@@ -117,7 +118,8 @@ type Engine struct {
 	TURNs []*stun.URI
 
 	// clientRoutes is the most recent list of clientRoutes received from the Management Service
-	clientRoutes route.HAMap
+	clientRoutes   route.HAMap
+	clientRoutesMu sync.RWMutex
 
 	clientCtx    context.Context
 	clientCancel context.CancelFunc
@@ -240,7 +242,9 @@ func (e *Engine) Stop() error {
 		return err
 	}
 
+	e.clientRoutesMu.Lock()
 	e.clientRoutes = nil
+	e.clientRoutesMu.Unlock()
 
 	// very ugly but we want to remove peers from the WireGuard interface first before removing interface.
 	// Removing peers happens in the conn.Close() asynchronously
@@ -738,7 +742,9 @@ func (e *Engine) updateNetworkMap(networkMap *mgmProto.NetworkMap) error {
 		log.Errorf("failed to update clientRoutes, err: %v", err)
 	}
 
+	e.clientRoutesMu.Lock()
 	e.clientRoutes = clientRoutes
+	e.clientRoutesMu.Unlock()
 
 	protoDNSConfig := networkMap.GetDNSConfig()
 	if protoDNSConfig == nil {
@@ -1088,7 +1094,8 @@ func (e *Engine) receiveSignalEvents() {
 					log.Errorf("failed on parsing remote candidate %s -> %s", candidate, err)
 					return err
 				}
-				conn.OnRemoteCandidate(candidate)
+
+				conn.OnRemoteCandidate(candidate, e.GetClientRoutes())
 			case sProto.Body_MODE:
 			}
 
@@ -1282,11 +1289,17 @@ func (e *Engine) newDnsServer() ([]*route.Route, dns.Server, error) {
 
 // GetClientRoutes returns the current routes from the route map
 func (e *Engine) GetClientRoutes() route.HAMap {
-	return e.clientRoutes
+	e.clientRoutesMu.RLock()
+	defer e.clientRoutesMu.RUnlock()
+
+	return maps.Clone(e.clientRoutes)
 }
 
 // GetClientRoutesWithNetID returns the current routes from the route map, but the keys consist of the network ID only
 func (e *Engine) GetClientRoutesWithNetID() map[route.NetID][]*route.Route {
+	e.clientRoutesMu.RLock()
+	defer e.clientRoutesMu.RUnlock()
+
 	routes := make(map[route.NetID][]*route.Route, len(e.clientRoutes))
 	for id, v := range e.clientRoutes {
 		routes[id.NetID()] = v
diff --git a/client/internal/peer/conn.go b/client/internal/peer/conn.go
index 3c4cfb13a27..5f329bf7f12 100644
--- a/client/internal/peer/conn.go
+++ b/client/internal/peer/conn.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"net"
+	"net/netip"
 	"runtime"
 	"strings"
 	"sync"
@@ -18,6 +19,7 @@ import (
 	"github.com/netbirdio/netbird/client/internal/wgproxy"
 	"github.com/netbirdio/netbird/iface"
 	"github.com/netbirdio/netbird/iface/bind"
+	"github.com/netbirdio/netbird/route"
 	signal "github.com/netbirdio/netbird/signal/client"
 	sProto "github.com/netbirdio/netbird/signal/proto"
 	nbnet "github.com/netbirdio/netbird/util/net"
@@ -353,7 +355,7 @@ func (conn *Conn) Open(ctx context.Context) error {
 
 	err = conn.agent.GatherCandidates()
 	if err != nil {
-		return err
+		return fmt.Errorf("gather candidates: %v", err)
 	}
 
 	// will block until connection succeeded
@@ -370,7 +372,7 @@ func (conn *Conn) Open(ctx context.Context) error {
 		return err
 	}
 
-	// dynamically set remote WireGuard port is other side specified a different one from the default one
+	// dynamically set remote WireGuard port if other side specified a different one from the default one
 	remoteWgPort := iface.DefaultWgPort
 	if remoteOfferAnswer.WgListenPort != 0 {
 		remoteWgPort = remoteOfferAnswer.WgListenPort
@@ -779,7 +781,7 @@ func (conn *Conn) OnRemoteAnswer(answer OfferAnswer) bool {
 }
 
 // OnRemoteCandidate Handles ICE connection Candidate provided by the remote peer.
-func (conn *Conn) OnRemoteCandidate(candidate ice.Candidate) {
+func (conn *Conn) OnRemoteCandidate(candidate ice.Candidate, haRoutes route.HAMap) {
 	log.Debugf("OnRemoteCandidate from peer %s -> %s", conn.config.Key, candidate.String())
 	go func() {
 		conn.mu.Lock()
@@ -789,6 +791,10 @@ func (conn *Conn) OnRemoteCandidate(candidate ice.Candidate) {
 			return
 		}
 
+		if candidateViaRoutes(candidate, haRoutes) {
+			return
+		}
+
 		err := conn.agent.AddRemoteCandidate(candidate)
 		if err != nil {
 			log.Errorf("error while handling remote candidate from peer %s", conn.config.Key)
@@ -806,3 +812,31 @@ func (conn *Conn) RegisterProtoSupportMeta(support []uint32) {
 	protoSupport := signal.ParseFeaturesSupported(support)
 	conn.meta.protoSupport = protoSupport
 }
+
+func candidateViaRoutes(candidate ice.Candidate, clientRoutes route.HAMap) bool {
+	var routePrefixes []netip.Prefix
+	for _, routes := range clientRoutes {
+		if len(routes) > 0 && routes[0] != nil {
+			routePrefixes = append(routePrefixes, routes[0].Network)
+		}
+	}
+
+	addr, err := netip.ParseAddr(candidate.Address())
+	if err != nil {
+		log.Errorf("Failed to parse IP address %s: %v", candidate.Address(), err)
+		return false
+	}
+
+	for _, prefix := range routePrefixes {
+		// default route is
+		if prefix.Bits() == 0 {
+			continue
+		}
+
+		if prefix.Contains(addr) {
+			log.Debugf("Ignoring candidate [%s], its address is part of routed network %s", candidate.String(), prefix)
+			return true
+		}
+	}
+	return false
+}