Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion client/internal/portforward/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
log "github.com/sirupsen/logrus"

"github.com/netbirdio/netbird/client/internal/portforward/pcp"
"github.com/netbirdio/netbird/client/internal/portforward/upnp"
)

// discoverGateway is the function used for NAT gateway discovery.
Expand All @@ -24,7 +25,31 @@ func defaultDiscoverGateway(ctx context.Context) (nat.NAT, error) {
}
log.Debugf("PCP discovery failed: %v, trying NAT-PMP/UPnP", err)

return nat.DiscoverGateway(ctx)
// Multicast SSDP is not delivered on all networks (hypervisor bridges,
// IGMP-snooping switches), while MiniUPnPd-based gateways also answer
// unicast M-SEARCH sent directly to them. Run a unicast UPnP search
// against the default gateway alongside the multicast discovery.
unicastResult := make(chan nat.NAT, 1)
go func() {
gateway, err := upnp.Discover(ctx)
if err != nil {
log.Debugf("unicast UPnP discovery failed: %v", err)
unicastResult <- nil
return
}
unicastResult <- gateway
}()

gateway, err := nat.DiscoverGateway(ctx)
if err == nil {
return gateway, nil
}

if gateway := <-unicastResult; gateway != nil {
log.Debugf("gateway found via unicast UPnP discovery")
return gateway, nil
}
return nil, err
}

// State is persisted only for crash recovery cleanup
Expand Down
155 changes: 155 additions & 0 deletions client/internal/portforward/upnp/nat.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
package upnp

import (
"context"
"fmt"
"math"
"math/rand"
"net"
"sync"
"time"

"github.com/huin/goupnp"
"github.com/libp2p/go-nat"
)

var _ nat.NAT = (*NAT)(nil)

// igdClient is the subset of the goupnp IGD service client API used for port mappings.
type igdClient interface {
GetExternalIPAddressCtx(ctx context.Context) (string, error)
AddPortMappingCtx(ctx context.Context, remoteHost string, externalPort uint16, protocol string, internalPort uint16, internalClient string, enabled bool, description string, leaseDuration uint32) error
DeletePortMappingCtx(ctx context.Context, remoteHost string, externalPort uint16, protocol string) error
GetNATRSIPStatusCtx(ctx context.Context) (rsipAvailable bool, natEnabled bool, err error)
}

// NAT implements the go-nat NAT interface using a UPnP IGD discovered via
// unicast SSDP. Port mapping semantics match go-nat's UPnP implementation:
// a random external port is chosen and remembered per internal port so
// renewals keep the same mapping.
type NAT struct {
client igdClient
natType string
root *goupnp.RootDevice

mu sync.Mutex
ports map[int]int // internal port -> mapped external port
}

// Type returns the kind of NAT port mapping service that is used.
func (n *NAT) Type() string {
return n.natType
}

// GetDeviceAddress returns the internal address of the gateway device.
func (n *NAT) GetDeviceAddress() (net.IP, error) {
addr, err := net.ResolveUDPAddr("udp4", n.root.URLBase.Host)
if err != nil {
return nil, err
}
return addr.IP, nil
}
Comment on lines +44 to +51

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Unbounded host resolution on untrusted IGD data.

GetDeviceAddress/GetInternalAddress resolve n.root.URLBase.Host via net.ResolveUDPAddr/net.Dial with no timeout, unlike GetExternalAddress which wraps its call in a 10s context. URLBase.Host comes from the untrusted device-description XML returned by whatever answered the SSDP search, so if it's a hostname (not a literal IP) this can trigger an unbounded DNS lookup.

🛡️ Suggested fix avoiding DNS resolution entirely
 func (n *NAT) GetDeviceAddress() (net.IP, error) {
-	addr, err := net.ResolveUDPAddr("udp4", n.root.URLBase.Host)
+	host, _, err := net.SplitHostPort(n.root.URLBase.Host)
 	if err != nil {
 		return nil, err
 	}
-	return addr.IP, nil
+	ip := net.ParseIP(host)
+	if ip == nil {
+		return nil, fmt.Errorf("device URLBase host is not an IP: %s", host)
+	}
+	return ip, nil
 }

Also applies to: 71-85

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/portforward/upnp/nat.go` around lines 44 - 51,
GetDeviceAddress and GetInternalAddress should not resolve n.root.URLBase.Host
through DNS, since that host comes from untrusted IGD XML and can block
indefinitely. Update these paths to avoid name resolution entirely by parsing
only literal IPs from URLBase.Host (for example, extracting the host component
and validating it as an IP) and reject or handle non-literal hosts explicitly.
Keep the same safety approach in both NAT methods so they behave consistently
with GetExternalAddress’s bounded network behavior.


// GetExternalAddress returns the external address of the gateway device.
func (n *NAT) GetExternalAddress() (net.IP, error) {
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()

ipString, err := n.client.GetExternalIPAddressCtx(ctx)
if err != nil {
return nil, err
}

ip := net.ParseIP(ipString)
if ip == nil {
return nil, nat.ErrNoExternalAddress
}
return ip, nil
}

// GetInternalAddress returns the local address used to reach the gateway device.
func (n *NAT) GetInternalAddress() (net.IP, error) {
conn, err := net.Dial("udp4", n.root.URLBase.Host)
if err != nil {
return nil, err
}
defer func() {
_ = conn.Close()
}()

localAddr, ok := conn.LocalAddr().(*net.UDPAddr)
if !ok {
return nil, nat.ErrNoInternalAddress
}
return localAddr.IP, nil
}

// AddPortMapping maps a port on the local host to an external port.
func (n *NAT) AddPortMapping(ctx context.Context, protocol string, internalPort int, description string, timeout time.Duration) (int, error) {
proto, err := mapProtocol(protocol)
if err != nil {
return 0, err
}

internalIP, err := n.GetInternalAddress()
if err != nil {
return 0, fmt.Errorf("get internal address: %w", err)
}
leaseDuration := uint32(timeout / time.Second)

n.mu.Lock()
externalPort := n.ports[internalPort]
n.mu.Unlock()

if externalPort > 0 {
err = n.client.AddPortMappingCtx(ctx, "", uint16(externalPort), proto, uint16(internalPort), internalIP.String(), true, description, leaseDuration)
if err == nil {
return externalPort, nil
}
}

for range 3 {
port := randomPort()
err = n.client.AddPortMappingCtx(ctx, "", uint16(port), proto, uint16(internalPort), internalIP.String(), true, description, leaseDuration)
if err == nil {
n.mu.Lock()
n.ports[internalPort] = port
n.mu.Unlock()
return port, nil
}
}
return 0, err
}

// DeletePortMapping removes a port mapping.
func (n *NAT) DeletePortMapping(ctx context.Context, protocol string, internalPort int) error {
proto, err := mapProtocol(protocol)
if err != nil {
return err
}

n.mu.Lock()
externalPort := n.ports[internalPort]
delete(n.ports, internalPort)
n.mu.Unlock()

if externalPort == 0 {
return nil
}
return n.client.DeletePortMappingCtx(ctx, "", uint16(externalPort), proto)
}
Comment on lines +124 to +140

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

DeletePortMapping silently no-ops when the external port isn't cached — breaks cleanup on freshly-discovered gateways.

DeletePortMapping only calls DeletePortMappingCtx when n.ports[internalPort] > 0; otherwise it returns nil (success) without touching the gateway. Since state.go's Cleanup() calls discoverGateway(ctx) fresh, and natFromService always constructs a brand-new &NAT{ports: make(map[int]int)} (line 215), the ports map at cleanup time is always empty for the unicast UPnP path — meaning the actual router-side port mapping is never deleted, and the caller has no way to know cleanup silently failed. The stale mapping only disappears once its own UPnP lease expires.

Consider persisting the external port outside the ephemeral NAT instance (e.g. on the State struct alongside InternalPort/Protocol) so it survives rediscovery, or have Cleanup() reuse the same gateway instance used for the original AddPortMapping.

🧰 Tools
🪛 ast-grep (0.44.0)

[warning] 138-138: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint16(externalPort)
Note: [CWE-190] Integer Overflow or Wraparound.

(integer-overflow-narrowing-conversion-go)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/portforward/upnp/nat.go` around lines 124 - 140,
DeletePortMapping currently relies on NAT.ports cache state, so a freshly
rediscovered gateway in Cleanup() will no-op and leave the router mapping
behind. Update the cleanup flow so the external port is preserved outside the
ephemeral NAT instance, likely on State alongside InternalPort and Protocol, or
make Cleanup() reuse the same gateway/client used by AddPortMapping. Then adjust
NAT.DeletePortMapping to use the persisted external port instead of returning
nil when n.ports[internalPort] is empty.


func mapProtocol(s string) (string, error) {
switch s {
case "udp":
return "UDP", nil
case "tcp":
return "TCP", nil
default:
return "", fmt.Errorf("invalid protocol: %s", s)
}
}

func randomPort() int {
return rand.Intn(math.MaxUint16-10000) + 10000
}
Loading
Loading