From 16ef88754d4c63166ff8680541c7224f3965405e Mon Sep 17 00:00:00 2001
From: Andrew Dunham <andrew@du.nham.ca>
Date: Tue, 1 Oct 2024 12:17:59 -0400
Subject: [PATCH] net/portmapper: don't return unspecified/local external IPs

We were previously not checking that the external IP that we got back
from a UPnP portmap was a valid endpoint; add minimal validation that
this endpoint is something that is routeable by another host.

Updates tailscale/corp#23538

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: Id9649e7683394aced326d5348f4caa24d0efd532
---
 net/portmapper/upnp.go      |  13 +++++
 net/portmapper/upnp_test.go | 100 ++++++++++++++++++++++++++++--------
 2 files changed, 92 insertions(+), 21 deletions(-)

diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go
index 54b0f81b2..f1199f0a6 100644
--- a/net/portmapper/upnp.go
+++ b/net/portmapper/upnp.go
@@ -638,6 +638,19 @@ func (c *Client) tryUPnPPortmapWithDevice(
 		return netip.AddrPort{}, nil, err
 	}
 
+	// Do a bit of validation on the external IP; we've seen cases where
+	// UPnP devices return the public IP 0.0.0.0, which obviously doesn't
+	// work as an endpoint.
+	//
+	// See: https://github.com/tailscale/corp/issues/23538
+	if externalIP.IsUnspecified() {
+		c.logf("UPnP returned unspecified external IP %v", externalIP)
+		return netip.AddrPort{}, nil, fmt.Errorf("UPnP returned unspecified external IP")
+	} else if externalIP.IsLoopback() {
+		c.logf("UPnP returned loopback external IP %v", externalIP)
+		return netip.AddrPort{}, nil, fmt.Errorf("UPnP returned loopback external IP")
+	}
+
 	return netip.AddrPortFrom(externalIP, newPort), client, nil
 }
 
diff --git a/net/portmapper/upnp_test.go b/net/portmapper/upnp_test.go
index 79e05dd99..c41b535a5 100644
--- a/net/portmapper/upnp_test.go
+++ b/net/portmapper/upnp_test.go
@@ -599,13 +599,7 @@ func TestGetUPnPPortMapping(t *testing.T) {
 		)
 		for i := range 2 {
 			sawRequestWithLease.Store(false)
-			res, err := c.Probe(ctx)
-			if err != nil {
-				t.Fatalf("Probe: %v", err)
-			}
-			if !res.UPnP {
-				t.Errorf("didn't detect UPnP")
-			}
+			mustProbeUPnP(t, ctx, c)
 
 			gw, myIP, ok := c.gatewayAndSelfIP()
 			if !ok {
@@ -656,13 +650,7 @@ func TestGetUPnPPortMapping_NoValidServices(t *testing.T) {
 	c.debug.VerboseLogs = true
 
 	ctx := context.Background()
-	res, err := c.Probe(ctx)
-	if err != nil {
-		t.Fatalf("Probe: %v", err)
-	}
-	if !res.UPnP {
-		t.Errorf("didn't detect UPnP")
-	}
+	mustProbeUPnP(t, ctx, c)
 
 	gw, myIP, ok := c.gatewayAndSelfIP()
 	if !ok {
@@ -705,13 +693,7 @@ func TestGetUPnPPortMapping_Legacy(t *testing.T) {
 	c.debug.VerboseLogs = true
 
 	ctx := context.Background()
-	res, err := c.Probe(ctx)
-	if err != nil {
-		t.Fatalf("Probe: %v", err)
-	}
-	if !res.UPnP {
-		t.Errorf("didn't detect UPnP")
-	}
+	mustProbeUPnP(t, ctx, c)
 
 	gw, myIP, ok := c.gatewayAndSelfIP()
 	if !ok {
@@ -838,6 +820,58 @@ func TestProcessUPnPResponses(t *testing.T) {
 	}
 }
 
+// See: https://github.com/tailscale/corp/issues/23538
+func TestGetUPnPPortMapping_Invalid(t *testing.T) {
+	for _, responseAddr := range []string{
+		"0.0.0.0",
+		"127.0.0.1",
+	} {
+		t.Run(responseAddr, func(t *testing.T) {
+			igd, err := NewTestIGD(t.Logf, TestIGDOptions{UPnP: true})
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer igd.Close()
+
+			// This is a very basic fake UPnP server handler.
+			handlers := map[string]any{
+				"AddPortMapping":       testAddPortMappingResponse,
+				"GetExternalIPAddress": makeGetExternalIPAddressResponse(responseAddr),
+				"GetStatusInfo":        testGetStatusInfoResponse,
+				"DeletePortMapping":    "", // Do nothing for test
+			}
+
+			igd.SetUPnPHandler(&upnpServer{
+				t:    t,
+				Desc: huaweiRootDescXML,
+				Control: map[string]map[string]any{
+					"/ctrlt/WANPPPConnection_1": handlers,
+				},
+			})
+
+			c := newTestClient(t, igd)
+			defer c.Close()
+			c.debug.VerboseLogs = true
+
+			ctx := context.Background()
+			mustProbeUPnP(t, ctx, c)
+
+			gw, myIP, ok := c.gatewayAndSelfIP()
+			if !ok {
+				t.Fatalf("could not get gateway and self IP")
+			}
+
+			ext, ok := c.getUPnPPortMapping(ctx, gw, netip.AddrPortFrom(myIP, 12345), 0)
+			if ok {
+				t.Fatal("did not expect to get UPnP port mapping")
+			}
+			if ext.IsValid() {
+				t.Fatalf("expected no external address; got %v", ext)
+			}
+		})
+	}
+}
+
 type upnpServer struct {
 	t       *testing.T
 	Desc    string                    // root device XML
@@ -921,6 +955,18 @@ func (u *upnpServer) handleControl(w http.ResponseWriter, r *http.Request, handl
 	}
 }
 
+func mustProbeUPnP(tb testing.TB, ctx context.Context, c *Client) ProbeResult {
+	tb.Helper()
+	res, err := c.Probe(ctx)
+	if err != nil {
+		tb.Fatalf("Probe: %v", err)
+	}
+	if !res.UPnP {
+		tb.Fatalf("didn't detect UPnP")
+	}
+	return res
+}
+
 const testRootDesc = `<?xml version="1.0"?>
 <root xmlns="urn:schemas-upnp-org:device-1-0" configId="1337">
   <specVersion>
@@ -1058,3 +1104,15 @@ func (u *upnpServer) handleControl(w http.ResponseWriter, r *http.Request, handl
   </s:Body>
 </s:Envelope>
 `
+
+func makeGetExternalIPAddressResponse(ip string) string {
+	return fmt.Sprintf(`<?xml version="1.0"?>
+<s:Envelope xmlns:s="http://schemas.xmlsoap.org/soap/envelope/" s:encodingStyle="http://schemas.xmlsoap.org/soap/encoding/">
+  <s:Body>
+    <u:GetExternalIPAddressResponse xmlns:u="urn:schemas-upnp-org:service:WANIPConnection:1">
+      <NewExternalIPAddress>%s</NewExternalIPAddress>
+    </u:GetExternalIPAddressResponse>
+  </s:Body>
+</s:Envelope>
+`, ip)
+}