miniupnpd: Add check that port forwarding is not blocked by local or upstream router firewall #565
Conversation
|
@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 wouldn't it be preferable to provide a testing program ? |
|
That system() is in If you do not like that part behind And anyway, this |
|
@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 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: how does the miniupnpd should work?
|
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. some full-cone-nat router do 1:1 map, but use random external port it is not symmetric NAT but the code in miniupnpd just check port changed and simple assume it is |
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. |
|
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: Where 10.4.2.34 is the WAN interface IP and the 75.43.56.3 is the ext_ip. 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 |
|
Full-cone NAT 1:1 setup on upstream router with private IP address on WAN interface on local router is already possible, just enable |
|
Interesting. There may be an issue with the way that check is performed? |
|
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 ... 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. ./upnpforwardcheck.c:50:21: error: variable has incomplete type 'struct sockaddr_in' |
|
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: |
|
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: get_redirect_rule_count
|
|
This is linker error. Are you building it correctly? Probably you need to use Makefile.bsd. |
|
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 |
|
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 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 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
With that in mind, why would the STUN check fail? Is miniupnpd trying to add rules to the upstream router? |
No.
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. Feb 10 15:37:20 firewall miniupnpd[71397]: STUN: Performing with host=stun.stunprotocol.org and port=3478 ... It is now finally adding nat rules as well, as follows: However these NAT rules are incorrect, but that isn't a problem with your code. 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 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. |
…upstream router firewall People reported more issues to miniupnpd which resulted to not be issue in miniupnpd but rather issues in miniupnpd configuration files or issues in misconfigured local firewalls or issues on upstream routers. E.g. users put incorrect interface in ext_ifname= option, or swapped lan and wan interfaces, or configured iptables to never run miniupnpd chains, or did not enable NAT 1:1 correctly on upstream router to forward traffic to miniupnpd local router in double NAT network setups. All these issues caused that miniupnpd though that everything is working fine (it responded to user requests that port forwarding is configured and is running) but in reality network traffic was not forwarded at all. To prevent these issues to happen add checks that miniupnpd can correctly insert UDP forwarding rule into firewall and test that miniupnpd can receive UDP packet on unblocked UDP port. For double NAT setups and similar network topologies (where DMZ or NAT 1:1 is used) this checks that upstream router is correctly configured, opened for port forwarding and does not block incoming traffic. But such check does not verify that port forwarding would be possible in any case (e.g. it does not check anything with TCP) and there are lot of other ways how to misconfigure network setup either on local miniupnpd router or on upstream router. But at least eliminates some of mentioned common errors. Better diagnostic test can be performed by ext_perform_stun=yes option.
When miniupnpd is compiled with --disable-firewall-checks then ignore results of firewall checks. Beware that miniupnpd in this case may report incorrect status of port, so use with care. In some rare cases firewall checks may fail on correct setups, so allow users to compile miniupnpd version without these checks at the cost of the reporting incorrect information about forwarded ports.
|
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? |
People reported more issues to miniupnpd which resulted to not be issue in
miniupnpd but rather issues in miniupnpd configuration files or issues in
misconfigured local firewalls or issues on upstream routers.
E.g. users put incorrect interface in ext_ifname= option, or swapped lan
and wan interfaces, or configured iptables to never run miniupnpd chains,
or did not enable NAT 1:1 correctly on upstream router to forward traffic
to miniupnpd local router in double NAT network setups.
All these issues caused that miniupnpd though that everything is working
fine (it responded to user requests that port forwarding is configured and
is running) but in reality network traffic was not forwarded at all.
To prevent these issues to happen add checks that miniupnpd can correctly
insert UDP forwarding rule into firewall and test that miniupnpd can
receive UDP packet on unblocked UDP port. For double NAT setups and similar
network topologies (where DMZ or NAT 1:1 is used) this checks that upstream
router is correctly configured, opened for port forwarding and does not
block incoming traffic.
But such check does not verify that port forwarding would be possible in
any case (e.g. it does not check anything with TCP) and there are lot of
other ways how to misconfigure network setup either on local miniupnpd
router or on upstream router. But at least eliminates some of mentioned
common errors.
Better diagnostic test can be performed by ext_perform_stun=yes option.