@miniupnp Please look at this change and tell me what do you think about it. If you like it or not... I'm just trying to add some "selftest" which checks that miniupnpd could work with actual configuration on running machine.

In any case, some more testing needs to be done...

you know I don't like system() calls ;)

wouldn't it be preferable to provide a testing program ?

That system() is in #ifdef TEST_LINUX_DEBUG_APP. Meaning this is just test/debug part of code which I used for testing this test code :)

If you do not like that part behind #ifdef TEST_LINUX_DEBUG_APP (where is system() call) then it can be fully removed. I just let it there as somebody in future could be interesting in extending this code and testing could be useful.

And anyway, this #ifdef TEST_LINUX_DEBUG_APP is testing program.

@pali by the way what exactly does the test ? just check that add_filter_rule2() does its job ?

As the packets are not coming from a remote host, I'm not sure that is really useful

Yes, it checks that add_filter_rule2() works. As written in above description it eliminates issues when iptables is incorrectly configured to never call rules added by miniupnpd.

And second test which is does: when wan interface ip address does not match ext_ip= option (situation for NAT 1:1 or DMZ or other similar fancy setup with address translation) then test that external ip address can really forward packets to (internal) interface ip address -- and so it is not CGNAT or any other non-full-cone NAT which basically do not forward packets to miniupnpd machine.

So in setup when ext_ip is specified this test checks that this ext_ip option is valid. It prevents situation when in ext_ip option is a typo and by mistake incorrect external ip address was specified (which means that miniupnpd cannot work).

You have made things too complicated. Even in a double nat environment, miniupnpd should only do a simple thing, which is to do port redirect directly, regardless of whether it is public ip or not, whether it is 1:1 nat or what.

You should look at how huawei router handles these situations, or other router manufacturers, tplink or Cisco, they do not use miniupnpd implementations, they are just simple tasks.

See explanation in the first post. These checks are here to prevent problems when users start miniupnpd with incorrect parameters or configuration file and then complain that miniupnpd does not work. These checks and output from them will help to debug future issues (to both users and also people who resolving reported issues).

If other manufactures have software with less checks, it does not mean that also miniupnpd should do same thing and do not provide more checks and detailed output for debugging.

And in any case, miniupnpd has to know public ip address, so some more work is needed and simple stuff is not enough.

what about this top:
client--RouterA(miniupnpd)---RouterB(full cone NAT)---internet

how does the miniupnpd should work?

1. if client request port mapping?

what about this top: client--RouterA(miniupnpd)---RouterB(full cone NAT)---internet

how does the miniupnpd should work?

1. if client request port mapping?

EDIT: sorry, my original answer was wrong. In full cone NAT setup port forwarding is possible. I wrongly read NAT type. Full cone NAT is 1:1 when upstream router forward all traffic to downstream router, just changes destination IP address in every IP packet.

So if IGD UPnP software is aware of full cone NAT then it should normally process port mapping request.

miniupnpd is now aware of this kind of setup if you enable perform stun option or if you provide directly external ip address. This pull request add another check that miniupnpd correctly works in this full cone NAT setup.

what about this top: client--RouterA(miniupnpd)---RouterB(full cone NAT)---internet
how does the miniupnpd should work?

1. if client request port mapping?

EDIT: sorry, my original answer was wrong. In full cone NAT setup port forwarding is possible. I wrongly read NAT type. Full cone NAT is 1:1 when upstream router forward all traffic to downstream router, just changes destination IP address in every IP packet.

So if IGD UPnP software is aware of full cone NAT then it should normally process port mapping request.

miniupnpd is now aware of this kind of setup if you enable perform stun option or if you provide directly external ip address. This pull request add another check that miniupnpd correctly works in this full cone NAT setup.

ok.
I notice some issue in stun check.

some full-cone-nat router do 1:1 map, but use random external port

say:
externel_random_wan_port--->lan_port

it is not symmetric NAT but the code in miniupnpd

515         for (i = 0; i < 4; ++i) {
516                 if (!(have_mapped_addr & (1 << i)))
517                         continue;
518                 if (ntohs(mapped_addrs[i].sin_port) != local_ports[i] || memcmp(&mapped_addrs[i].sin_addr, ext_addr, sizeof(*ext_addr)) != 0) {
519                         /* External IP address or port was changed,
520                          * therefore symmetric NAT is active */
521                         syslog(LOG_NOTICE, "%s: #%d external address or port changed",
522                                "perform_stun", i);
523                         *restrictive_nat = 1;
524                 }
525         }

just check port changed and simple assume it is symmetric NAT but it is not.

some full-cone-nat router do 1:1 map, but use random external port

Then this is not full-cone nat 1:1 setup if it changes any port in packet. And in this case obviously UPnP IGD software cannot work.

If this kind of nat is not symmetric, but has some other name (sorry, I do not know correct terminology for other fancy setups), you could update comment. But in any case, it would not work.

Whole UPnP IGD software on router is designed only for normal peer to peer setup when no traffic is changed. Support for NAT 1:1 was added into miniupnpd just because it is possible to reconstruct what this NAT is doing and so making router behind NAT 1:1 to behave like if there is no NAT at all.

Anyway, this is off-topic for this pull request.

@miniupnp: any opinion?

I'm still not convinced the proposed test are neither 100% correct nor really useful.

Firewall should not block the traffic coming from the WAN to the LAN and to the LAN to the WAN, but if you open a port mapping, there is no obligation that it should be open if coming from the LAN.
In my opinion, the only way to perform a correct test would be from the WAN.

I understand your concerns. I will try to explain one of the used setups by internet games with server setup (when one of the player acted as server) where port forwarding is important role.

When e.g. 2 players are behind one public IPv4 address with IGD UPnP and other 2 players are on different part of Internet behind another public IPv4 address. One of these 4 players is chosen as server, ask IGD UPnP to forward required port and then all remaining 3 players access his server via IGD UPnP's public IPv4 address and port. So one client (which is on the same segment, behind same IPv4 address) target its packets from lan interface to miniupnpd's router IPv4 public address, and other two clients (which are on different part of Internet) access miniupnpd's router IPv4 public address from Internet wan. This setup was (or maybe still is) commonly used.

When translating this usage to those fancy setups with double NATs where NATs are 1:1 full-cone, then packets from local network targeting forwarded public IPv4 address and port must be sent from miniupnpd's router to upstream router which is doing that 1:1 full-cone NAT (because miniupnpd does not see public IPv4 address on wan interface). Upstream router then does that full-cone NATpacket mangling and send mangled packet back to the miniupnpd's router, which based on its UPnP forwarding table, route packet to the correct lan client.

One of checks in this pull request verify that above setup with those fancy double NATs are working correctly.

Hello @pali I am quite interested in this pull request, as I am behind a Double NAT (full cone upstream), I debugged with the pfsense folks over here: https://forum.netgate.com/topic/154153/test-request-upnp-fix-for-multiple-consoles-playing-the-same-game-static-port-outbound-nat/116?_=1644324180096. Turns out the "out of box" config for filter.inc needed two extra lines. That got it using the miniupnp rules (they were not matched previously), but the double nat was waiting for me behind that problem.

It would all work flawlessly if miniupnp would write a NAT rule like this:
nat log quick on igb0 inet proto udp from 192.168.7.2 port = 3074 to any keep state label "DemonwarePortMapping" rtable 0 -> 10.4.2.34 port 3074
Instead of this:
nat log quick on igb0 inet proto udp from 192.168.7.2 port = 3074 to any keep state label "DemonwarePortMapping" rtable 0 -> 75.43.56.3 port 3074

Where 10.4.2.34 is the WAN interface IP and the 75.43.56.3 is the ext_ip.
If I use the ext_ip option, it uses the public IP in the NAT rule, which will never work. The upstream NAT device can't send traffic back, and probably tosses the packet because the source IP is invalid. If I use the STUN option, it errors out and tells me NATing is impossible, which I know isn't the case. I can make manual rules which do work, but that is not a long term fix.

I am happy to help test!

I am running pfsense 2.5.2 and miniupnpd 2.2.1.

@encryptyd This pull request should help people behind double-nat or other similar setups to identify possible issues if something is incorrectly configured (as I explained in above comments).

Fixing configuration of fw rules for these network setups can be a next step after this... I see that double-nat or other network fancy setups are more and more common and people ask for it.

Thanks @pali
I think one of the most helpful things would be a configuration option to allow private IP addresses on the WAN interface.
I saw that it was discussed in other posts, but seemed like it was never added. Where did that discussion end up?

Full-cone NAT 1:1 setup on upstream router with private IP address on WAN interface on local router is already possible, just enable ext_perform_stun=yes option. But it requires that both local and upstream router is configured correctly. Code in this PR can help to avoid common misconfigurations.

Interesting. There may be an issue with the way that check is performed?
If I enable the stun option, it says forwarding is not possible in the debug messages. I know that it is possible, because if I add the same exact rules manually (no upnp), it works flawlessly.

So, apply this change and look at detailed debug error message what is wrong.

It shows this (I masked out some of the address):

Feb 8 14:32:44 firewall miniupnpd[16443]: STUN: Performing with host=stun.stunprotocol.org and port=3478 ...
Feb 8 14:32:44 firewall miniupnpd[16443]: resolve_stun_host: stun.stunprotocol.org:3478 => 18.191.223.12:3478
Feb 8 14:32:44 firewall miniupnpd[16443]: perform_stun: local ports 35975 47849 46602 30491
Feb 8 14:32:44 firewall miniupnpd[16443]: wait_for_stun_responses: waiting 3 secs and 0 usecs
Feb 8 14:32:44 firewall miniupnpd[16443]: wait_for_stun_responses: received responses: 1
Feb 8 14:32:44 firewall miniupnpd[16443]: wait_for_stun_responses: waiting 3 secs and 0 usecs
Feb 8 14:32:47 firewall miniupnpd[16443]: wait_for_stun_responses: select(): no more responses
Feb 8 14:32:47 firewall miniupnpd[16443]: wait_for_stun_responses: waiting 3 secs and 0 usecs
Feb 8 14:32:51 firewall miniupnpd[16443]: wait_for_stun_responses: select(): no more responses
Feb 8 14:32:51 firewall miniupnpd[16443]: wait_for_stun_responses: waiting 3 secs and 0 usecs
Feb 8 14:32:54 firewall miniupnpd[16443]: wait_for_stun_responses: select(): no more responses
Feb 8 14:32:54 firewall miniupnpd[16443]: parse_stun_response: Type 0x0101, Length 48, Magic Cookie 2112a442
Feb 8 14:32:54 firewall miniupnpd[16443]: parse_stun_response: MAPPED-ADDRESS 65.93.X.X:35975
Feb 8 14:32:54 firewall miniupnpd[16443]: parse_stun_response: RESPONSE-ORIGIN 18.191.223.12:3478
Feb 8 14:32:54 firewall miniupnpd[16443]: parse_stun_response: OTHER-ADDRESS 18.219.110.18:3479
Feb 8 14:32:54 firewall miniupnpd[16443]: parse_stun_response: XOR-MAPPED-ADDRESS 65.93.X.X:35975
Feb 8 14:32:54 firewall miniupnpd[16443]: perform_stun: 1 response out of 4 received
Feb 8 14:32:54 firewall miniupnpd[16443]: STUN: ext interface igb0 with IP address 10.0.3.60 is now behind restrictive NAT with public IP address 65.93.X.X: Port forwarding is now impossible

If there truly is something I can ask my ISP to change, that may be possible. I have a direct channel to their admins.

This is not output from the code from this pull request as it prints more information what is wrong... Anyway, this discussion starting to be too off-topic for this pull request, so probably move to separate issue.

No problem I can move it. In regards to this specific pull request, is there a way I can test it directly? I would need instructions on how to put that new version on my firewall (pfsense 2.5.2).

Compile it from sources, there is Makefile for every system. If you do not know how, read instructions for your pfsense system (Sorry I do not know this system).

I've checkout out your PR, and unfortunately cannot get it to compile in FreeBSD 12.2. I would appreciate any suggestions.
The main branch compiles ok, but when I check out your code, I get the following errors:

./upnpforwardcheck.c:50:21: error: variable has incomplete type 'struct sockaddr_in'
struct sockaddr_in sockaddr;
^
./upnpforwardcheck.c:50:9: note: forward declaration of 'struct sockaddr_in'
struct sockaddr_in sockaddr;
^
./upnpforwardcheck.c:52:44: error: use of undeclared identifier 'IPPROTO_UDP'
*receive_fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
^
./upnpforwardcheck.c:59:42: error: use of undeclared identifier 'SO_BINDTODEVICE'
if (setsockopt(*receive_fd, SOL_SOCKET, SO_BINDTODEVICE, if_name, strlen(if_name)) != 0) {
^
./upnpforwardcheck.c:68:22: warning: implicit declaration of function 'htons' is invalid in C99 [-Wimplicit-function-declaration]
sockaddr.sin_port = htons(0);
^
./upnpforwardcheck.c:85:18: warning: implicit declaration of function 'ntohs' is invalid in C99 [-Wimplicit-function-declaration]
*receive_port = ntohs(sockaddr.sin_port);
^
./upnpforwardcheck.c:87:41: error: use of undeclared identifier 'IPPROTO_UDP'
*send_fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
^
./upnpforwardcheck.c:95:39: error: use of undeclared identifier 'SO_BINDTODEVICE'
if (setsockopt(*send_fd, SOL_SOCKET, SO_BINDTODEVICE, if_name, strlen(if_name)) != 0) {
^
./upnpforwardcheck.c:181:79: error: use of undeclared identifier 'IPPROTO_UDP'
if (add_filter_rule2(if_name, NULL, if_addr_str, receive_port, receive_port, IPPROTO_UDP, "forwarding check") < 0) {
^
./upnpforwardcheck.c:203:44: error: use of undeclared identifier 'IPPROTO_UDP'
delete_filter_rule(if_name, receive_port, IPPROTO_UDP);
^
2 warnings and 7 errors generated.
*** Error code 1

Ok, this needs to be fixed... Could you try to apply following change?

diff --git a/miniupnpd/upnpforwardcheck.c b/miniupnpd/upnpforwardcheck.c
index 48652cc89dd7..a7796d6247ad 100644
--- a/miniupnpd/upnpforwardcheck.c
+++ b/miniupnpd/upnpforwardcheck.c
@@ -8,6 +8,8 @@
 #include <sys/select.h>
 #include <sys/types.h>
 #include <sys/socket.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
 #include <unistd.h>
 #include <netdb.h>
 #include <stdlib.h>

That certainly helped. Now down to just 2 compile errors:
cc -I. -DMINIUPNPD_GIT_REF="pali/master-d454ea1" -O2 -pipe -Wall -W -Wstrict-prototypes -fno-common -c -o upnpforwardcheck.o ./upnpforwardcheck.c
./upnpforwardcheck.c:61:42: error: use of undeclared identifier 'SO_BINDTODEVICE'
if (setsockopt(*receive_fd, SOL_SOCKET, SO_BINDTODEVICE, if_name, strlen(if_name)) != 0) {
^
./upnpforwardcheck.c:97:39: error: use of undeclared identifier 'SO_BINDTODEVICE'
if (setsockopt(*send_fd, SOL_SOCKET, SO_BINDTODEVICE, if_name, strlen(if_name)) != 0) {
^
2 errors generated.
*** Error code 1

Ok, here is fix for second part:

diff --git a/miniupnpd/upnpforwardcheck.c b/miniupnpd/upnpforwardcheck.c
index 48652cc89dd7..5e98966e79ff 100644
--- a/miniupnpd/upnpforwardcheck.c
+++ b/miniupnpd/upnpforwardcheck.c
@@ -56,11 +58,13 @@ static int create_sockets(const char *if_name, struct in_addr *send_addr, struct
 		return -1;
 	}
 
+#ifdef SO_BINDTODEVICE
 	if (setsockopt(*receive_fd, SOL_SOCKET, SO_BINDTODEVICE, if_name, strlen(if_name)) != 0) {
 		syslog(LOG_ERR, "%s: setsockopt(SOL_SOCKET, SO_BINDTODEVICE, %s): %m",
 		       "create_sockets", if_name);
 		return -1;
 	}
+#endif
 
 	memset(&sockaddr, 0, sizeof(sockaddr));
 	sockaddr.sin_family = AF_INET;
@@ -92,6 +96,7 @@ static int create_sockets(const char *if_name, struct in_addr *send_addr, struct
 		close(*receive_fd);
 	}
 
+#ifdef SO_BINDTODEVICE
 	if (setsockopt(*send_fd, SOL_SOCKET, SO_BINDTODEVICE, if_name, strlen(if_name)) != 0) {
 		syslog(LOG_ERR, "%s: setsockopt(SOL_SOCKET, SO_BINDTODEVICE, %s): %m",
 		       "create_sockets", if_name);
@@ -99,6 +104,7 @@ static int create_sockets(const char *if_name, struct in_addr *send_addr, struct
 		close(*send_fd);
 		return -1;
 	}
+#endif
 
 	memset(&sockaddr, 0, sizeof(sockaddr));
 	sockaddr.sin_family = AF_INET;

Almost there, thanks for helping. Looks like a linking issue now.

cc -O2 -pipe -Wall -W -Wstrict-prototypes -fno-common -o miniupnpd miniupnpd.o upnphttp.o upnpdescgen.o upnpsoap.o upnpredirect.o getifaddr.o daemonize.o upnpglobalvars.o options.o upnppermissions.o minissdp.o natpmp.o pcpserver.o pcplearndscp.o upnpevents.o upnputils.o getconnstatus.o upnpstun.o upnpforwardcheck.o upnppinhole.o asyncsendto.o portinuse.o upnpreplyparse.o minixml.o getifstats.o ifacewatcher.o getroute.o ipfrdr.o -lkvm -lssl -lcrypto
ld: error: undefined symbol: delete_redirect_and_filter_rules

referenced by upnpredirect.c
upnpredirect.o:(_upnp_delete_redir)
referenced by upnpredirect.c
upnpredirect.o:(upnp_delete_redirection)
referenced by upnpredirect.c
upnpredirect.o:(get_upnp_rules_state_list)
referenced by upnpredirect.c
upnpredirect.o:(remove_unused_rules)

ld: error: undefined symbol: get_redirect_rule_count

referenced by upnpredirect.c
upnpredirect.o:(upnp_get_portmapping_number_of_entries)
cc: error: linker command failed with exit code 1 (use -v to see invocation)
*** Error code 1

This is linker error. Are you building it correctly? Probably you need to use Makefile.bsd.
EDIT: Also check that you selected --firewall=pf (I guess this is what you are using).

Yes, I was using the BSD makefile, but didn't notice it was using the ipf firewall, not pf. Changed that, and it has completed successfully.

This is what it now shows in the log:

Feb 8 19:05:14 firewall miniupnpd[98465]: STUN: ext interface igb0 with private IP address 10.0.3.60 is now behind restrictive or symmetric NAT with public IP address 65.93.X.X which does not support port forwarding
Feb 8 19:05:14 firewall miniupnpd[98465]: NAT on upstream router blocks incoming connections set by miniupnpd
Feb 8 19:05:14 firewall miniupnpd[98465]: Turn off NAT on upstream router or change it to full-cone NAT 1:1 type
Feb 8 19:05:14 firewall miniupnpd[98465]: Port forwarding is now disabled

So now you see it... If igb0 is connected directly to ISP, you would have to ask your ISP to give you one exclusive IPv4 address (either directly or via full-cone NAT 1:1) and disable filtering on that address... Some of traffic is filtered on upstream igb0 network.

I appreciate your help on this.
I will see if My ISP can change things on their end.

I included above BSD compile fixes into git branch for this pull request.

@miniupnp Did you have a time to look my other comments / explanation about this PR? Due to big increase of usage of Double-NAT or DMZ setups, I think it really makes sense to add some tests and diagnostic help output to users...

I have one additional question @pali
I was hoping to test more of the version I compiled from your pull request using the ext_ip option.
This option allows miniupnpd to at least accept nat requests from clients in my netowrk setup, and it adds the rules into PF using the miniupnpd version that shipped with PFSense 2.5.2/FreeBSD 12.2/Miniupnpd 2.2.1).
When using the version I compiled from either your PR, or the master, neither will accept client port adds. I see this in the debug logs:
ioctl(dev, DIOCGETRULES, ...): Operation not supported by device
I attempted to create an account on the miniupnpd forum, but it seems their email activation is broken and I can't post there.
Any idea what might cause that error? I did some searching and found some very very old posts about it, but nothing current.
I'd appreciate any assistance/theories you can provide.

I have not touched anything related to ioctl/DIOCGETRULES and this looks like something bsd/pfsense specific about which I have absolutely no idea. For sure it is not related to those changes.

@pali
I've checked with my ISP and they have verified on their end the following 3 things:

1. All new incoming traffic for my public IP is routed to my private WAN IP – unfiltered
2. Source & destination UDP and TCP ports are preserved during the IP NAT both inbound and outbound (only the IP address changes)
3. All outbound traffic is unfiltered

With that in mind, why would the STUN check fail? Is miniupnpd trying to add rules to the upstream router?

Is miniupnpd trying to add rules to the upstream router?

No.

With that in mind, why would the STUN check fail?

In this case it looks like if firewall on box where is running miniupnpd is incorrectly configured and rules added by miniupnpd by runtime are ignored (or have lower priority). Try to completely disable any firewall or filtering on your local box where you are running miniupnpd.

@pali, thank you. That was the issue in getting STUN to finally work. I have to allow inbound traffic to the firewall IP.
I now see this in the log:

Feb 10 15:37:20 firewall miniupnpd[71397]: STUN: Performing with host=stun.stunprotocol.org and port=3478 ...
Feb 10 15:37:20 firewall miniupnpd[71397]: STUN: ext interface igb0 with IP address 10.0.3.60 is now behind unrestricted NAT 1:1 with public IP address 65.93.X.X: Port forwarding is now enabled

It is now finally adding nat rules as well, as follows:
miniupnpd rules/nat contents:
nat log quick on igb0 inet proto udp from 192.168.8.2 port = 3074 to any keep state label "DemonwarePortMapping" rtable 0 -> 65.93.X.X port 3074
rdr pass log quick on igb0 inet proto udp from any to any port = 3074 keep state label "DemonwarePortMapping" rtable 0 -> 192.168.8.2 port 3074

However these NAT rules are incorrect, but that isn't a problem with your code.
It should be:
nat log quick on igb0 inet proto udp from 192.168.8.2 port = 3074 to any keep state label "DemonwarePortMapping" rtable 0 -> 10.0.3.60 port 3074
rdr pass log quick on igb0 inet proto udp from any to any port = 3074 keep state label "DemonwarePortMapping" rtable 0 -> 192.168.8.2 port 3074

Without the WAN IP in the NAT, the traffic is invalid to the upstream router.

This is what goes on the wire:

15:30:43.699582 rule 734/0(match): pass in on lagg0.8: 192.168.8.2.3074 > 185.34.107.128.3074: UDP, length 3
15:30:43.699614 rule 114/0(match): pass out on igb0: 65.93.x.x.3074 > 185.34.107.128.3074: UDP, length 3

Packets leaving my firewall cannot have a source address which is anything other than my private IP. I'll have to start a new issue thread.

I opened #598.

Back to the my original pull request here. Do you think it is useful and is there something which should I improve in it? I think that the last issue with compile errors is already fixed.

@pali I'm sorry to answer with such a delay.

I'm pretty sure the tests performed would fail on some perfectly correct setups.
So we could modify your code to only perform the tests when a compile time or runtime option is enabled.
So if that is useful to you, you can perform the test but people who don't asked for it won't be blocked.

Ok! I pushed a new commit which implements compile time option for disable disabling of that and rebased everything on top of the master branch.

@miniupnp Ok now?

@miniupnp Any comments?

sorry I had no time to study it again

Ping? In the last version I have addressed all your concerns.

@miniupnp: ping?

ping?