From 5f015acdaa3fb0d44858bd2ab7e7597db1e01517 Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Sun, 9 Aug 2020 16:01:39 -0700 Subject: [PATCH] Improve robustness for broken UPnP IGDs --- miss/miss.cpp | 57 ++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 7 deletions(-) diff --git a/miss/miss.cpp b/miss/miss.cpp index 58f1951..b79bdcd 100644 --- a/miss/miss.cpp +++ b/miss/miss.cpp @@ -55,7 +55,7 @@ static struct port_entry { static const int k_WolPorts[] = { 9, 47009 }; -bool UPnPMapPort(struct UPNPUrls* urls, struct IGDdatas* data, int proto, const char* myAddr, int port, bool enable, bool indefinite) +bool UPnPMapPort(struct UPNPUrls* urls, struct IGDdatas* data, int proto, const char* myAddr, int port, bool enable, bool indefinite, bool validationPass) { char intClient[16]; char intPort[6]; @@ -95,15 +95,27 @@ bool UPnPMapPort(struct UPNPUrls* urls, struct IGDdatas* data, int proto, const if (err == 714) { // NoSuchEntryInArray printf("NOT FOUND" NL); + + if (validationPass) { + // On validation, we found a missing entry. Convert this entry to indefinite + // to see if it will stick. + indefinite = true; + } } else if (err == 606) { printf("UNAUTHORIZED" NL); + + // If we're just validating, we're done. We can't know if the entry was + // actually applied but we'll return true to avoid false errors if it was. + if (validationPass) { + return true; + } } else if (err == UPNPCOMMAND_SUCCESS) { // Some routers change the description, so we can't check that here if (!strcmp(intClient, myAddr)) { if (atoi(leaseDuration) == 0) { - printf("OK (Permanent)" NL); + printf("OK (Static)" NL); // If we have an existing permanent mapping, we can just leave it alone. if (enable) { @@ -114,6 +126,11 @@ bool UPnPMapPort(struct UPNPUrls* urls, struct IGDdatas* data, int proto, const printf("OK (%s seconds remaining)" NL, leaseDuration); } + // If we're just validating, we found an entry, so we're done. + if (validationPass) { + return true; + } + if (!enable) { // This is our entry. Go ahead and nuke it printf("Deleting UPnP mapping for %s %s -> %s...", protoStr, portStr, myAddr); @@ -131,6 +148,11 @@ bool UPnPMapPort(struct UPNPUrls* urls, struct IGDdatas* data, int proto, const else { printf("CONFLICT: %s %s" NL, intClient, desc); + // If we're just validating, we found an entry, so we're done. + if (validationPass) { + return true; + } + // Some UPnP IGDs won't let unauthenticated clients delete other conflicting port mappings // for security reasons, but we will give it a try anyway. If GameStream is not enabled, // we will leave the conflicting entry alone to avoid disturbing another PC's port forwarding @@ -172,11 +194,16 @@ bool UPnPMapPort(struct UPNPUrls* urls, struct IGDdatas* data, int proto, const err = UPNP_AddPortMapping( urls->controlURL, data->first.servicetype, portStr, portStr, myAddr, myDesc, protoStr, nullptr, leaseDuration); - if (err == 725 && !indefinite) { // OnlyPermanentLeasesSupported + if (err != UPNPCOMMAND_SUCCESS && !indefinite) { + // This may be a broken IGD that doesn't like non-static mappings. Try a static + // mapping before finally giving up. err = UPNP_AddPortMapping( urls->controlURL, data->first.servicetype, portStr, portStr, myAddr, myDesc, protoStr, nullptr, "0"); - printf("PERMANENT "); + printf("STATIC RETRY "); + } + else if (indefinite) { + printf("STATIC "); } if (err == UPNPCOMMAND_SUCCESS) { printf("OK" NL); @@ -291,8 +318,9 @@ bool UPnPHandleDeviceList(struct UPNPDev* list, bool enable, char* lanAddrOverri portMappingInternalAddress = localAddress; } + // Create the port mappings for (int i = 0; i < ARRAYSIZE(k_Ports); i++) { - if (!UPnPMapPort(&urls, &data, k_Ports[i].proto, portMappingInternalAddress, k_Ports[i].port, enable, false)) { + if (!UPnPMapPort(&urls, &data, k_Ports[i].proto, portMappingInternalAddress, k_Ports[i].port, enable, false, false)) { success = false; } } @@ -316,13 +344,28 @@ bool UPnPHandleDeviceList(struct UPNPDev* list, bool enable, char* lanAddrOverri char broadcastAddrStr[128]; inet_ntop(AF_INET, &broadcastAddr, broadcastAddrStr, sizeof(broadcastAddrStr)); - UPnPMapPort(&urls, &data, IPPROTO_UDP, broadcastAddrStr, k_WolPorts[i], enable, true); + UPnPMapPort(&urls, &data, IPPROTO_UDP, broadcastAddrStr, k_WolPorts[i], enable, true, false); } } else { // When we're mapping the WOL ports upstream of our router, we map directly to // the port on the upstream address (likely our router's WAN interface). - UPnPMapPort(&urls, &data, IPPROTO_UDP, lanAddrOverride, k_WolPorts[i], enable, true); + UPnPMapPort(&urls, &data, IPPROTO_UDP, lanAddrOverride, k_WolPorts[i], enable, true, false); + } + } + + // Validate the rules are present and correct if they claimed to be added successfully + if (success && enable) { + // Wait 10 seconds for the router state to quiesce + printf("Waiting before UPnP port validation..."); + Sleep(10000); + printf("done" NL); + + // Perform the validation pass (converting any now missing entries to permanent ones) + for (int i = 0; i < ARRAYSIZE(k_Ports); i++) { + if (!UPnPMapPort(&urls, &data, k_Ports[i].proto, portMappingInternalAddress, k_Ports[i].port, enable, false, true)) { + success = false; + } } }