net/portmapper: never select port 0 in UPnP

Port 0 is interpreted, per the spec (but inconsistently among router
software) as requesting to map every single available port on the UPnP
gateway to the internal IP address. We'd previously avoided picking
ports below 1024 for one of the two UPnP methods (in #7457), and this
change moves that logic so that we avoid it in all cases.

Updates #8992

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I20d652c0cd47a24aef27f75c81f78ae53cc3c71e

net/portmapper/upnp.go

@@ -106,11 +106,13 @@ type upnpClient interface {
 // It is not used for anything other than labelling.
 const tsPortMappingDesc = "tailscale-portmap"
 
-// addAnyPortMapping abstracts over different UPnP client connections, calling the available
-// AddAnyPortMapping call if available for WAN IP connection v2, otherwise defaulting to the old
-// behavior of calling AddPortMapping with port = 0 to specify a wildcard port.
-// It returns the new external port (which may not be identical to the external port specified),
-// or an error.
+// addAnyPortMapping abstracts over different UPnP client connections, calling
+// the available AddAnyPortMapping call if available for WAN IP connection v2,
+// otherwise picking either the previous port (if one is present) or a random
+// port and trying to obtain a mapping using AddPortMapping.
+//
+// It returns the new external port (which may not be identical to the external
+// port specified), or an error.
 //
 // TODO(bradfitz): also returned the actual lease duration obtained. and check it regularly.
 func addAnyPortMapping(
@@ -121,6 +123,31 @@ func addAnyPortMapping(
 	internalClient string,
 	leaseDuration time.Duration,
 ) (newPort uint16, err error) {
+	// Some devices don't let clients add a port mapping for privileged
+	// ports (ports below 1024). Additionally, per section 2.3.18 of the
+	// UPnP spec, regarding the ExternalPort field:
+	//
+	//    If this value is specified as a wildcard (i.e. 0), connection
+	//    request on all external ports (that are not otherwise mapped)
+	//    will be forwarded to InternalClient. In the wildcard case, the
+	//    value(s) of InternalPort on InternalClient are ignored by the IGD
+	//    for those connections that are forwarded to InternalClient.
+	//    Obviously only one such entry can exist in the NAT at any time
+	//    and conflicts are handled with a “first write wins” behavior.
+	//
+	// We obviously do not want to open all ports on the user's device to
+	// the internet, so we want to do this prior to calling either
+	// AddAnyPortMapping or AddPortMapping.
+	//
+	// Pick an external port that's greater than 1024 by getting a random
+	// number in [0, 65535 - 1024] and then adding 1024 to it, shifting the
+	// range to [1024, 65535].
+	if externalPort < 1024 {
+		externalPort = uint16(rand.Intn(65535-1024) + 1024)
+	}
+
+	// First off, try using AddAnyPortMapping; if there's a conflict, the
+	// router will pick another port and return it.
 	if upnp, ok := upnp.(*internetgateway2.WANIPConnection2); ok {
 		return upnp.AddAnyPortMapping(
 			ctx,
@@ -135,15 +162,8 @@ func addAnyPortMapping(
 		)
 	}
 
-	// Some devices don't let clients add a port mapping for privileged
-	// ports (ports below 1024).
-	//
-	// Pick an external port that's greater than 1024 by getting a random
-	// number in [0, 65535 - 1024] and then adding 1024 to it, shifting the
-	// range to [1024, 65535].
-	if externalPort < 1024 {
-		externalPort = uint16(rand.Intn(65535-1024) + 1024)
-	}
+	// Fall back to using AddPortMapping, which requests a mapping to/from
+	// a specific external port.
 	err = upnp.AddPortMapping(
 		ctx,
 		"",