From b45089ad851ebc3f9affd2e06d20f40a0934ffd4 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Sun, 21 Jan 2024 22:52:32 -0500 Subject: [PATCH] net/portmapper: handle cases where we have no supported clients This no longer results in a nil pointer exception when we get a valid UPnP response with no supported clients. Updates #10911 Signed-off-by: Andrew Dunham Change-Id: I6e3715a49a193ff5261013871ad7fff197a4d77e --- net/portmapper/upnp.go | 17 ++- net/portmapper/upnp_test.go | 216 ++++++++++++++++++++++++++++++++++++ 2 files changed, 232 insertions(+), 1 deletion(-) diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index 67d9cbbab..0f44463e6 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -252,7 +252,8 @@ func getUPnPRootDevice(ctx context.Context, logf logger.Logf, debug DebugKnobs, } // selectBestService picks the "best" service from the given UPnP root device -// to use to create a port mapping. +// to use to create a port mapping. It may return (nil, nil) if no supported +// service was found in the provided *goupnp.RootDevice. // // loc is the parsed location that was used to fetch the given RootDevice. // @@ -559,6 +560,20 @@ func (c *Client) tryUPnPPortmapWithDevice( return netip.AddrPort{}, nil, err } + // If we have no client, we cannot continue; this can happen if we get + // a valid UPnP response that does not contain any of the service types + // that we know how to use. + if client == nil { + // For debugging, print all available services that we aren't + // using because they're not supported; use c.vlogf so we don't + // spam the logs unless verbose debugging is turned on. + rootDev.Device.VisitServices(func(s *goupnp.Service) { + c.vlogf("unsupported UPnP service: Type=%q ID=%q ControlURL=%q", s.ServiceType, s.ServiceId, s.ControlURL.Str) + }) + + return netip.AddrPort{}, nil, fmt.Errorf("no supported UPnP clients") + } + // Start by trying to make a temporary lease with a duration. var newPort uint16 newPort, err = addAnyPortMapping( diff --git a/net/portmapper/upnp_test.go b/net/portmapper/upnp_test.go index 8748cf427..4ca332ff1 100644 --- a/net/portmapper/upnp_test.go +++ b/net/portmapper/upnp_test.go @@ -165,6 +165,172 @@ http://10.0.0.1:2828 +` + + // Huawei, https://github.com/tailscale/tailscale/issues/10911 + huaweiRootDescXML = ` + + + 1 + 0 + + + urn:dslforum-org:device:InternetGatewayDevice:1 + HG531 V1 + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2600-587f668bdd9a + 000000000001 + + + urn:www-huawei-com:service:DeviceConfig:1 + urn:www-huawei-com:serviceId:DeviceConfig1 + /desc/DevCfg.xml + /ctrlt/DeviceConfig_1 + /evt/DeviceConfig_1 + + + urn:dslforum-org:service:LANConfigSecurity:1 + urn:dslforum-org:serviceId:LANConfigSecurity1 + /desc/LANSec.xml + /ctrlt/LANConfigSecurity_1 + /evt/LANConfigSecurity_1 + + + urn:dslforum-org:service:Layer3Forwarding:1 + urn:dslforum-org:serviceId:Layer3Forwarding1 + /desc/L3Fwd.xml + /ctrlt/Layer3Forwarding_1 + /evt/Layer3Forwarding_1 + + + + + urn:dslforum-org:device:WANDevice:1 + WANDevice + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2601-587f668bdd9a + 000000000001 + + + urn:dslforum-org:service:WANDSLInterfaceConfig:1 + urn:dslforum-org:serviceId:WANDSLInterfaceConfig1 + /desc/WanDslIfCfg.xml + /ctrlt/WANDSLInterfaceConfig_1 + /evt/WANDSLInterfaceConfig_1 + + + urn:dslforum-org:service:WANCommonInterfaceConfig:1 + urn:dslforum-org:serviceId:WANCommonInterfaceConfig1 + /desc/WanCommonIfc1.xml + /ctrlt/WANCommonInterfaceConfig_1 + /evt/WANCommonInterfaceConfig_1 + + + + + urn:dslforum-org:device:WANConnectionDevice:1 + WANConnectionDevice + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2603-587f668bdd9a + 000000000001 + + + urn:dslforum-org:service:WANPPPConnection:1 + urn:dslforum-org:serviceId:WANPPPConnection1 + /desc/WanPppConn.xml + /ctrlt/WANPPPConnection_1 + /evt/WANPPPConnection_1 + + + urn:dslforum-org:service:WANEthernetConnectionManagement:1 + urn:dslforum-org:serviceId:WANEthernetConnectionManagement1 + /desc/WanEthConnMgt.xml + /ctrlt/WANEthernetConnectionManagement_1 + /evt/WANEthernetConnectionManagement_1 + + + urn:dslforum-org:service:WANDSLLinkConfig:1 + urn:dslforum-org:serviceId:WANDSLLinkConfig1 + /desc/WanDslLink.xml + /ctrlt/WANDSLLinkConfig_1 + /evt/WANDSLLinkConfig_1 + + + + + + + urn:dslforum-org:device:LANDevice:1 + LANDevice + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2602-587f668bdd9a + 000000000001 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration4 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_4 + /evt/WLANConfiguration_4 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration3 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_3 + /evt/WLANConfiguration_3 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration2 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_2 + /evt/WLANConfiguration_2 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration1 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_1 + /evt/WLANConfiguration_1 + + + urn:dslforum-org:service:LANHostConfigManagement:1 + urn:dslforum-org:serviceId:LANHostConfigManagement1 + /desc/LanHostCfgMgmt.xml + /ctrlt/LANHostConfigManagement_1 + /evt/LANHostConfigManagement_1 + + + + + http://127.0.0.1 + + ` ) @@ -233,6 +399,14 @@ func TestGetUPnPClient(t *testing.T) { "*internetgateway2.WANIPConnection1", "saw UPnP type WANIPConnection1 at http://127.0.0.1:NNN/rootDesc.xml; MikroTik Router (MikroTik), method=none\n", }, + { + "huawei", + huaweiRootDescXML, + // services not supported and thus returns nil, but shouldn't crash + "", + "", + }, + // TODO(bradfitz): find a PPP one in the wild } for _, tt := range tests { @@ -375,6 +549,48 @@ func TestGetUPnPPortMapping(t *testing.T) { } } +// TestGetUPnPPortMapping_NoValidServices tests that getUPnPPortMapping doesn't +// crash when a valid UPnP response with no supported services is discovered +// and parsed. +// +// See https://github.com/tailscale/tailscale/issues/10911 +func TestGetUPnPPortMapping_NoValidServices(t *testing.T) { + igd, err := NewTestIGD(t.Logf, TestIGDOptions{UPnP: true}) + if err != nil { + t.Fatal(err) + } + defer igd.Close() + + igd.SetUPnPHandler(&upnpServer{ + t: t, + Desc: huaweiRootDescXML, + }) + + c := newTestClient(t, igd) + defer c.Close() + 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") + } + + gw, myIP, ok := c.gatewayAndSelfIP() + if !ok { + t.Fatalf("could not get gateway and self IP") + } + + // This shouldn't panic + _, ok = c.getUPnPPortMapping(ctx, gw, netip.AddrPortFrom(myIP, 12345), 0) + if ok { + t.Fatal("did not expect to get UPnP port mapping") + } +} + func TestGetUPnPPortMappingNoResponses(t *testing.T) { igd, err := NewTestIGD(t.Logf, TestIGDOptions{UPnP: true}) if err != nil {