From 98ad28e29092359b3e9a8f4e4b0f6f0d8871616a Mon Sep 17 00:00:00 2001 From: David Buscher Date: Sat, 3 Apr 2021 16:39:20 +0100 Subject: [PATCH 1/7] Fixed several long-standing issues with UPnP port forwarding 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. --- data/actionsmap/yunohost.yml | 1 - src/yunohost/firewall.py | 99 ++++++++++++++---------------------- 2 files changed, 37 insertions(+), 63 deletions(-) diff --git a/data/actionsmap/yunohost.yml b/data/actionsmap/yunohost.yml index 290952aa3..36116dcf3 100644 --- a/data/actionsmap/yunohost.yml +++ b/data/actionsmap/yunohost.yml @@ -1292,7 +1292,6 @@ firewall: - enable - disable - status - - reload nargs: "?" default: status --no-refresh: diff --git a/src/yunohost/firewall.py b/src/yunohost/firewall.py index bc21f1948..e9d05b7e0 100644 --- a/src/yunohost/firewall.py +++ b/src/yunohost/firewall.py @@ -35,7 +35,11 @@ from moulinette.utils.text import prependlines FIREWALL_FILE = "/etc/yunohost/firewall.yml" UPNP_CRON_JOB = "/etc/cron.d/yunohost-firewall-upnp" - +# A UDP port to use for the SSDP discovery phase of UPNP. +# Assigned by IANA to "Fujitsu ICL Terminal Emulator Program A", so no-one else is +# likely to use it (unlike port 1900 which is used by SSDP servers such +# as miniupnpd) +SSDP_CLIENT_PORT = 1901 logger = getActionLogger("yunohost.firewall") @@ -327,66 +331,57 @@ def firewall_upnp(action="status", no_refresh=False): """ firewall = firewall_list(raw=True) - enabled = firewall["uPnP"]["enabled"] - # Compatibility with previous version - if action == "reload": - logger.debug("'reload' action is deprecated and will be removed") - try: - # Remove old cron job - os.remove("/etc/cron.d/yunohost-firewall") - except Exception: - pass - action = "status" - no_refresh = False - - if action == "status" and no_refresh: - # Only return current state - return {"enabled": enabled} - elif action == "enable" or (enabled and action == "status"): + if action == "status": + enabled = firewall["uPnP"]["enabled"] + elif action == "enable": # Add cron job with open(UPNP_CRON_JOB, "w+") as f: f.write( - "*/50 * * * * root " + "*/10 * * * * root " "/usr/bin/yunohost firewall upnp status >>/dev/null\n" ) - # Open port 1900 to receive discovery message - if 1900 not in firewall["ipv4"]["UDP"]: - firewall_allow("UDP", 1900, no_upnp=True, no_reload=True) - if not enabled: - firewall_reload(skip_upnp=True) enabled = True - elif action == "disable" or (not enabled and action == "status"): + elif action == "disable": try: # Remove cron job os.remove(UPNP_CRON_JOB) except Exception: pass enabled = False - if action == "status": - no_refresh = True else: raise YunohostValidationError("action_invalid", action=action) - # Refresh port mapping using UPnP + # Refresh port mapping + refresh_success = True if not no_refresh: - 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], + callback=_on_rule_command_error, + ) + upnpc = miniupnpc.UPnP(localport=SSDP_CLIENT_PORT) upnpc.discoverdelay = 3000 - # 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], + callback=_on_rule_command_error, + ) + if nb_dev < 1: logger.error(m18n.n("upnp_dev_not_found")) - enabled = False + refresh_success = False else: try: # Select UPnP device upnpc.selectigd() except Exception: logger.debug("unable to select UPnP device", exc_info=1) - enabled = False + refresh_success = False else: # Iterate over ports for protocol in ["TCP", "UDP"]: @@ -398,7 +393,7 @@ def firewall_upnp(action="status", no_refresh=False): upnpc.deleteportmapping(port, protocol) except Exception: pass - firewall["uPnP"][protocol + "_TO_CLOSE"] = [] + del firewall["uPnP"][protocol + "_TO_CLOSE"] for port in firewall["uPnP"][protocol]: # Clean the mapping of this port @@ -423,34 +418,17 @@ def firewall_upnp(action="status", no_refresh=False): logger.debug( "unable to add port %d using UPnP", port, exc_info=1 ) - enabled = False + refresh_success = False + if refresh_success: + logger.debug("UPnP port refresh successful") + if action == "enable": + logger.success(m18n.n("upnp_enabled")) + elif action == "disable": + logger.success(m18n.n("upnp_disabled")) - _update_firewall_file(firewall) - - if enabled != firewall["uPnP"]["enabled"]: - firewall = firewall_list(raw=True) - firewall["uPnP"]["enabled"] = enabled - - _update_firewall_file(firewall) - - if not no_refresh: - # Display success message if needed - if action == "enable" and enabled: - logger.success(m18n.n("upnp_enabled")) - elif action == "disable" and not enabled: - logger.success(m18n.n("upnp_disabled")) - # Make sure to disable UPnP - elif action != "disable" and not enabled: - firewall_upnp("disable", no_refresh=True) - - if not enabled and (action == "enable" or 1900 in firewall["ipv4"]["UDP"]): - # Close unused port 1900 - firewall_disallow("UDP", 1900, no_reload=True) - if not no_refresh: - firewall_reload(skip_upnp=True) - - if action == "enable" and not enabled: - raise YunohostError("upnp_port_open_failed") + # Save state always (note that refreshing can change the "TO_CLOSE" states) + firewall["uPnP"]["enabled"] = enabled + _update_firewall_file(firewall) return {"enabled": enabled} @@ -472,9 +450,6 @@ def firewall_stop(): os.system("ip6tables -F") os.system("ip6tables -X") - if os.path.exists(UPNP_CRON_JOB): - firewall_upnp("disable") - def _get_ssh_port(default=22): """Return the SSH port to use From 667dba421a20831c4c3275e2dc13e757a1a8e81a Mon Sep 17 00:00:00 2001 From: David Buscher Date: Sat, 10 Jul 2021 16:58:24 +0100 Subject: [PATCH 2/7] Removed transient firewall rule for SSDP client port No longer requires opening a fixed port and then closing it again. Uses nftables sets to recognise the client port. --- src/yunohost/firewall.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/yunohost/firewall.py b/src/yunohost/firewall.py index e9d05b7e0..8449eedac 100644 --- a/src/yunohost/firewall.py +++ b/src/yunohost/firewall.py @@ -258,7 +258,13 @@ def firewall_reload(skip_upnp=False): "iptables -w -A INPUT -p icmp -j ACCEPT", "iptables -w -P INPUT DROP", ] - + # Set of nft rules for allowing SSDP discovery + # See https://github.com/mqus/nft-rules/blob/master/files/SSDP_client.md + rules += [ + "nft add set filter ssdp_out {type inet_service \\; timeout 5s \\;}", + "nft add rule filter OUTPUT ip daddr 239.255.255.250 udp dport 1900 set add udp sport @ssdp_out", + "nft add rule filter INPUT udp dport @ssdp_out accept", + ] # Execute each rule if process.run_commands(rules, callback=_on_rule_command_error): errors = True @@ -291,7 +297,11 @@ def firewall_reload(skip_upnp=False): "ip6tables -w -A INPUT -p icmpv6 -j ACCEPT", "ip6tables -w -P INPUT DROP", ] - + rules += [ + "nft add set ip6 filter ssdp_out {type inet_service \\; timeout 5s \\;}", + "nft add rule ip6 filter OUTPUT ip6 daddr {FF02::C, FF05::C, FF08::C, FF0E::C} udp dport 1900 set add udp sport @ssdp_out", + "nft add rule ip6 filter INPUT udp dport @ssdp_out accept", + ] # Execute each rule if process.run_commands(rules, callback=_on_rule_command_error): errors = True @@ -338,7 +348,7 @@ def firewall_upnp(action="status", no_refresh=False): # Add cron job with open(UPNP_CRON_JOB, "w+") as f: f.write( - "*/10 * * * * root " + "*/12 * * * * root " "/usr/bin/yunohost firewall upnp status >>/dev/null\n" ) enabled = True @@ -355,22 +365,12 @@ def firewall_upnp(action="status", no_refresh=False): # Refresh port mapping refresh_success = True if not no_refresh: - # Open port to receive discovery message - process.run_commands( - ["iptables -w -A INPUT -p udp --dport %d -j ACCEPT" % SSDP_CLIENT_PORT], - callback=_on_rule_command_error, - ) - upnpc = miniupnpc.UPnP(localport=SSDP_CLIENT_PORT) + upnpc = miniupnpc.UPnP() upnpc.discoverdelay = 3000 # 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], - callback=_on_rule_command_error, - ) if nb_dev < 1: logger.error(m18n.n("upnp_dev_not_found")) From 0fce089c54ae9afc51816359d53dd03503f8c540 Mon Sep 17 00:00:00 2001 From: David Buscher Date: Sat, 10 Jul 2021 17:28:36 +0100 Subject: [PATCH 3/7] Fix incompatibility with nftables Also removed reference to port 1901 --- src/yunohost/firewall.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/yunohost/firewall.py b/src/yunohost/firewall.py index 8449eedac..3792cf35c 100644 --- a/src/yunohost/firewall.py +++ b/src/yunohost/firewall.py @@ -35,11 +35,6 @@ from moulinette.utils.text import prependlines FIREWALL_FILE = "/etc/yunohost/firewall.yml" UPNP_CRON_JOB = "/etc/cron.d/yunohost-firewall-upnp" -# A UDP port to use for the SSDP discovery phase of UPNP. -# Assigned by IANA to "Fujitsu ICL Terminal Emulator Program A", so no-one else is -# likely to use it (unlike port 1900 which is used by SSDP servers such -# as miniupnpd) -SSDP_CLIENT_PORT = 1901 logger = getActionLogger("yunohost.firewall") @@ -233,10 +228,10 @@ def firewall_reload(skip_upnp=False): # IPv4 try: - process.check_output("iptables -w -L") + process.check_output("nft list ruleset -n -a") except process.CalledProcessError as e: logger.debug( - "iptables seems to be not available, it outputs:\n%s", + "nftables/nft seems to be not available, it outputs:\n%s", prependlines(e.output.rstrip(), "> "), ) logger.warning(m18n.n("iptables_unavailable")) @@ -272,10 +267,10 @@ def firewall_reload(skip_upnp=False): # IPv6 try: - process.check_output("ip6tables -L") + process.check_output("nft list ruleset -n -a") except process.CalledProcessError as e: logger.debug( - "ip6tables seems to be not available, it outputs:\n%s", + "ip6tables/nft seems to be not available, it outputs:\n%s", prependlines(e.output.rstrip(), "> "), ) logger.warning(m18n.n("ip6tables_unavailable")) From c486bb032f3b50df9a6dd7d4dfa22c8dec9c162b Mon Sep 17 00:00:00 2001 From: David Buscher Date: Sun, 26 Sep 2021 14:26:51 +0100 Subject: [PATCH 4/7] Accomodate change in nft syntax --- src/yunohost/firewall.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/yunohost/firewall.py b/src/yunohost/firewall.py index 3792cf35c..bf33cae50 100644 --- a/src/yunohost/firewall.py +++ b/src/yunohost/firewall.py @@ -228,7 +228,7 @@ def firewall_reload(skip_upnp=False): # IPv4 try: - process.check_output("nft list ruleset -n -a") + process.check_output("nft -n -a list ruleset") except process.CalledProcessError as e: logger.debug( "nftables/nft seems to be not available, it outputs:\n%s", @@ -267,7 +267,7 @@ def firewall_reload(skip_upnp=False): # IPv6 try: - process.check_output("nft list ruleset -n -a") + process.check_output("nft -n -a list ruleset") except process.CalledProcessError as e: logger.debug( "ip6tables/nft seems to be not available, it outputs:\n%s", From 665898ffa8b53cc7dd6f0286c0f86b21beb8efa1 Mon Sep 17 00:00:00 2001 From: David Buscher Date: Sat, 11 Dec 2021 09:21:06 +0000 Subject: [PATCH 5/7] Revert "Removed transient firewall rule for SSDP client port" This reverts commit 667dba421a20831c4c3275e2dc13e757a1a8e81a. --- src/yunohost/firewall.py | 41 ++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/src/yunohost/firewall.py b/src/yunohost/firewall.py index bf33cae50..e9d05b7e0 100644 --- a/src/yunohost/firewall.py +++ b/src/yunohost/firewall.py @@ -35,6 +35,11 @@ from moulinette.utils.text import prependlines FIREWALL_FILE = "/etc/yunohost/firewall.yml" UPNP_CRON_JOB = "/etc/cron.d/yunohost-firewall-upnp" +# A UDP port to use for the SSDP discovery phase of UPNP. +# Assigned by IANA to "Fujitsu ICL Terminal Emulator Program A", so no-one else is +# likely to use it (unlike port 1900 which is used by SSDP servers such +# as miniupnpd) +SSDP_CLIENT_PORT = 1901 logger = getActionLogger("yunohost.firewall") @@ -228,10 +233,10 @@ def firewall_reload(skip_upnp=False): # IPv4 try: - process.check_output("nft -n -a list ruleset") + process.check_output("iptables -w -L") except process.CalledProcessError as e: logger.debug( - "nftables/nft seems to be not available, it outputs:\n%s", + "iptables seems to be not available, it outputs:\n%s", prependlines(e.output.rstrip(), "> "), ) logger.warning(m18n.n("iptables_unavailable")) @@ -253,13 +258,7 @@ def firewall_reload(skip_upnp=False): "iptables -w -A INPUT -p icmp -j ACCEPT", "iptables -w -P INPUT DROP", ] - # Set of nft rules for allowing SSDP discovery - # See https://github.com/mqus/nft-rules/blob/master/files/SSDP_client.md - rules += [ - "nft add set filter ssdp_out {type inet_service \\; timeout 5s \\;}", - "nft add rule filter OUTPUT ip daddr 239.255.255.250 udp dport 1900 set add udp sport @ssdp_out", - "nft add rule filter INPUT udp dport @ssdp_out accept", - ] + # Execute each rule if process.run_commands(rules, callback=_on_rule_command_error): errors = True @@ -267,10 +266,10 @@ def firewall_reload(skip_upnp=False): # IPv6 try: - process.check_output("nft -n -a list ruleset") + process.check_output("ip6tables -L") except process.CalledProcessError as e: logger.debug( - "ip6tables/nft seems to be not available, it outputs:\n%s", + "ip6tables seems to be not available, it outputs:\n%s", prependlines(e.output.rstrip(), "> "), ) logger.warning(m18n.n("ip6tables_unavailable")) @@ -292,11 +291,7 @@ def firewall_reload(skip_upnp=False): "ip6tables -w -A INPUT -p icmpv6 -j ACCEPT", "ip6tables -w -P INPUT DROP", ] - rules += [ - "nft add set ip6 filter ssdp_out {type inet_service \\; timeout 5s \\;}", - "nft add rule ip6 filter OUTPUT ip6 daddr {FF02::C, FF05::C, FF08::C, FF0E::C} udp dport 1900 set add udp sport @ssdp_out", - "nft add rule ip6 filter INPUT udp dport @ssdp_out accept", - ] + # Execute each rule if process.run_commands(rules, callback=_on_rule_command_error): errors = True @@ -343,7 +338,7 @@ def firewall_upnp(action="status", no_refresh=False): # Add cron job with open(UPNP_CRON_JOB, "w+") as f: f.write( - "*/12 * * * * root " + "*/10 * * * * root " "/usr/bin/yunohost firewall upnp status >>/dev/null\n" ) enabled = True @@ -360,12 +355,22 @@ def firewall_upnp(action="status", no_refresh=False): # Refresh port mapping refresh_success = True if not no_refresh: - upnpc = miniupnpc.UPnP() + # Open port to receive discovery message + process.run_commands( + ["iptables -w -A INPUT -p udp --dport %d -j ACCEPT" % SSDP_CLIENT_PORT], + callback=_on_rule_command_error, + ) + upnpc = miniupnpc.UPnP(localport=SSDP_CLIENT_PORT) upnpc.discoverdelay = 3000 # 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], + callback=_on_rule_command_error, + ) if nb_dev < 1: logger.error(m18n.n("upnp_dev_not_found")) From bff33afabbcbfe32413b723204a4602daf4d71a8 Mon Sep 17 00:00:00 2001 From: David Buscher Date: Sun, 12 Dec 2021 10:48:22 +0000 Subject: [PATCH 6/7] Changed cron timing to avoid clashes with other regular cron jobs --- src/yunohost/firewall.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/yunohost/firewall.py b/src/yunohost/firewall.py index e9d05b7e0..88b4dd7ba 100644 --- a/src/yunohost/firewall.py +++ b/src/yunohost/firewall.py @@ -338,7 +338,7 @@ def firewall_upnp(action="status", no_refresh=False): # Add cron job with open(UPNP_CRON_JOB, "w+") as f: f.write( - "*/10 * * * * root " + "*/12 * * * * root " "/usr/bin/yunohost firewall upnp status >>/dev/null\n" ) enabled = True From 8f2bdfbddc108528b1886f88cb59ed4231917622 Mon Sep 17 00:00:00 2001 From: David Buscher Date: Sun, 6 Feb 2022 12:15:56 +0000 Subject: [PATCH 7/7] Removed redundant refresh of UPNP when loading firewall --- src/firewall.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/firewall.py b/src/firewall.py index 4764d972a..e31b567a6 100644 --- a/src/firewall.py +++ b/src/firewall.py @@ -235,7 +235,7 @@ def firewall_reload(skip_upnp=False): # Retrieve firewall rules and UPnP status firewall = firewall_list(raw=True) - upnp = firewall_upnp()["enabled"] if not skip_upnp else False + upnp = firewall_upnp(no_refresh=True)["enabled"] if not skip_upnp else False # IPv4 try: