Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Several long-standing issues with UPnP port forwarding #1204

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

dbuscher
Copy link

@dbuscher dbuscher commented Apr 3, 2021

The problem

See YunoHost/issues#1463
...

Solution

Fixes local firewall rules to allow discovery of SSDP servers.
No longer disables UPnP forwarding when refreshing fails.
No longer disables UPnP forwarding on system reboot.
Cron job runs every 10 minutes to refresh the router tables more
promptly after the system or router reboots.
Removes the deprecated UPnP "reload" option.

...

PR Status

Works on my system (yunohost 4.2.0 testing) behind a home router. Others need to check it works with their networks.
...

How to test

...

Fixes local firewall rules to allow discovery of SSDP servers.
No longer disables UPnP forwarding when refreshing fails.
No longer disables UPnP forwarding on system reboot.
Cron job runs every 10 minutes to refresh the router tables more
promptly after the system or router reboots.
Removes the deprecated UPnP "reload" option.
@dbuscher dbuscher changed the title [fix] Several long-standing issues with UPnP port forwarding from issue #1463 [fix] Several long-standing issues with UPnP port forwarding Apr 3, 2021
@MX10-AC2N
Copy link

Hello, how can we test this PR ??

@dbuscher
Copy link
Author

I put my reply in YunoHost/issues#1463

@alexAubin alexAubin added this to In progress in 4.3.x via automation Jun 3, 2021
# Add cron job
with open(UPNP_CRON_JOB, "w+") as f:
f.write(
"*/50 * * * * root "
"*/10 * * * * root "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This frequencies could block other yunohost-api/cli yunohost call. I think we shouldn't run this every 10 minutes. This command could take more than 3 seconds due to the discover delay.

Opening a port is not action we should do to often. We might condition that with an independent code that check port diagnosis result before.

It's strange that a command called status make change on network. We probably should rename it...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to a frequency of once per 12 minutes would reduce the possibility of a clash - is this ok?

There is an alternative to repeatedly opening the port here:

https://github.com/mqus/nft-rules/blob/master/files/SSDP_client.md

This would involve using nft rules - is this acceptable?

I agree that the command name is misleading, but thought it risky to change it if it works...

upnpc = miniupnpc.UPnP(localport=1)
# Open port to receive discovery message
process.run_commands(
["iptables -w -A INPUT -p udp --dport %d -j ACCEPT" % SSDP_CLIENT_PORT],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could upnp could use ipv6 ?

Is there a security risk to let firewall open on this port ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe upnp specifically uses an IPv4 broadcast address.

See above re an alternative firewall methodology.

# Discover UPnP device(s)
logger.debug("discovering UPnP devices...")
nb_dev = upnpc.discover()
logger.debug("found %d UPnP device(s)", int(nb_dev))
# Close discovery port
process.run_commands(
["iptables -w -D INPUT -p udp --dport %d -j ACCEPT" % SSDP_CLIENT_PORT],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
["iptables -w -D INPUT -p udp --dport %d -j ACCEPT" % SSDP_CLIENT_PORT],
["iptables -w -D INPUT -p udp --dport %d -j DENY" % SSDP_CLIENT_PORT],

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code deletes the firewall rule which opens the port, while proposed change adds a new rule which overrides the existing rule - this would cause a conflict the next time the port is opened.

4.3.x automation moved this from In progress to Review in progress Jun 10, 2021
No longer requires opening a fixed port and then closing it again.
Uses nftables sets to recognise the client port.
Also removed reference to port 1901
@alexAubin alexAubin self-assigned this Jul 11, 2021
@alexAubin
Copy link
Member

Super quick review (sorry for the late feedback) : the changes sounds kinda legit, but I'm always strugglng to know how to properly test and validate this. @dbuscher : do you have some tips regarding how you tested this ? What brand of router are you using ?

@dbuscher
Copy link
Author

Thanks for having a look at this. In partial answer to your question, I will quote what I said in the issue comments:

I think the only way to test is to install this version on a system which is on a network with a UPnP router/gateway, and check that when you open ports on UPNP via the yunohost admin interface/command line that they are visible to the internet (e.g. yunohost diagnosis shows that your web server and mail server are visible from outside your local network). If that test is passed then you can check that the forwarding is restored if you reboot the router and the system (doing so simultaneously simulates a power cut, not so infrequent at my house!). Some routers will also show you which ports are opened under UPNP, and this is a quick way to verify operation.

By the way the way I installed my test version was to install yunohost v4.2 and then overwrite the main changed file (firewall.py). This is not entirely kosher, but I think it is valid and much easier than a full build and install.

I would add to this that one can quickly check that it is working by installing the miniupnpc executable using apt and then doing

upnpc -L

which lists the current upnp port forwarding configuration of the router.

My router is my ISP-provided router, the Virgin Media Hub 3.0. I changed ISP about a year ago and previously had a router from Sky TV. It had very similar behaviour with the yunohost upnp software, but I changed ISP before I made all the bugfixes, so I was not able to test the final version on this earlier router.

From what I can tell, all the problems that I fixed were to do with maintaining the state of the yunohost system (port access settings/recovering from temporary errors/ keeping track of whether upnp has been activated) and not to do with communication protocol with the router (although communication errors would trigger some of the underlying bugs, which is part of the reason why changing the version of miniupnp that was shipped with debian was so helpful). This should mean that any aspects of the software which worked with any given router before should continue to work with these fixes. This is a roundabout way of saying that lack of testing with a large variety of routers is not likely to be a problem - to me it would seem more useful to test with a number of different yunohost installations to check that the changes do not accidentally interfere with anything else in the system.

@alexAubin
Copy link
Member

Alrighty thanks for the quick and extensive answer 👍 That looks promising, I shall have a look at my home router to check if it supports UPnP and give this a try 👍

@alexAubin alexAubin moved this from Review in progress to In progress in 4.3.x Sep 6, 2021
@dbuscher
Copy link
Author

I have recently noticed on my machine that the changes introduced here cause problems with fail2ban, because fail2ban is currently using iptables commands and not nftables.

There is no legacy equivalent to the nftables commands used to fix UPNP, so my fix was to move fail2ban over to using nft using the approach documented here.

This works for me, but obviously if others want to use this UPNP fix the fail2ban package would need to be altered. So my question is, would submitting a PR for fail2ban to fix a problem due to a change in firewall.py be an acceptable route to go? It would seem to me that yunohost should upgrade fully to nftables, the only question is the timing.

@dbuscher
Copy link
Author

OK, so I decided that the best thing to do was to revert the changes which introduce nftables rules in order to reduce the propagation of these changes into everything which currently uses iptables, e.g. fail2ban and possibly wireguard. The only downside is the slight inefficiency of explicitly opening and closing the SSDP discovery port every 12 minutes compared to using an automatic nftables rule.

Hopefully this can be accepted into the main branch sometime soon...

@alexAubin alexAubin removed this from Ongoing / to be tested/reviewed in 4.3.x Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants