diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index 6a525c54f..34cae5840 100644 --- a/net/portmapper/upnp.go +++ b/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, "",