Skip to content

miniupnpd: Update, revision, new access control and UCI options… - #28765

Draft
Self-Hosting-Group wants to merge 10 commits into
openwrt:masterfrom
Self-Hosting-Group:revision
Draft

miniupnpd: Update, revision, new access control and UCI options…#28765
Self-Hosting-Group wants to merge 10 commits into
openwrt:masterfrom
Self-Hosting-Group:revision

Conversation

@Self-Hosting-Group

@Self-Hosting-Group Self-Hosting-Group commented Mar 12, 2026

Copy link
Copy Markdown
Contributor

As this PR is extensive, the descriptions of the individual commits are collapsed here:

0. Update to 2.3.9 to fix issues, refresh building (merged 70ce349 2025-12-24, in 24.10/25.12 packages 2025-12-30)
  • Update daemon to 2.3.9 to fix removal of nftables rules in upnp_forward and return the correct internal port; also resulted in the excessive opening of new ports. Accept interface names starting with digits
  • Build from GitHub releases to get a reliable HTTPS server, as the HTTP-only/HTTPS mirror were only available ~85%/77% over 3 months Project main download mirror miniupnp.free.fr was down for 20 days miniupnp/miniupnp#770 https://stats.uptimerobot.com/DwGDxUB914
  • Build daemon with --disable-pppconn to remove the old/IGDv1-only extra WANPPPConnection SSDP announcements workaround not included in other implementations since >15y
  • Build daemon with --vendorcfg to allow customisation of the router/friendly name (+5 potential options) displayed in Windows Explorer, 384 bytes extra required on ARMv7 (binary)
  • Remove old (iptables variant only) patches, as no longer needed
  • Remove clean_ruleset_interval/threshold UCI config options as not standard/working since OpenWrt 22.03, as nftables not supported

Fixes: openwrt/openwrt#18011
Fixes: openwrt/luci#7759
Fixes: #26352

1. Update to 2.3.11 to fix issues

Update daemon to 2.3.11 to fix multiple issues, including security-related ones

Link: https://www.cve.org/CVERecord?id=CVE-2026-5720
Link: miniupnp/miniupnp#898

2. Patch for UPnP IGDv2 Microsoft/Apple compatibility
  • Add workaround to list port maps with the Windows IGDv2-incompatible client by returning an infinite (0) lease duration. To fix listing and editing via GUI (Explorer/Network), if daemon was compiled with IGDv2
  • Extend detection to older versions of Windows and add Xbox
  • Detect Apple IGDv2-incompatible clients and apply existing workaround, that only caused problems if PCP/NAT-PMP (prioritised) was disabled

(to merge with prior)

Link: https://github.com/Self-Hosting-Group/miniupnp/tree/upnp-igdv2-compat
Link: https://github.com/Self-Hosting-Group/wiki/wiki/Port-Mapping-Protocols-Overview#compatibility-issues

3. Patch to improve logging
  • Clearer logging of enabled protocols/ports, IPv6 mapping, and UPnP IGD compatibility mode… in start banner
  • Log warnings with an enabled allow third-party mapping option
  • Change the log level (less verbose) of many internal or normally occurring messages, and some error messages, without functional restrictions, to DEBUG, client requests/actions and their errors to INFO, and only daemon-wide messages to NOTICE/WARNING/ERR…
  • Comment out some log-filling messages when internal interfaces lose link, and for e.g. rule with label '%s' is not a IGD pinhole, as normally occurring

(to merge with prior)

Fixes: #17601
Fixes: #26483
Link: https://github.com/Self-Hosting-Group/miniupnp/tree/improve-logging

4. Package revision and new UCI options

The following settings UCI options been added or changed, and the previous options are migrated on updating:

upnpd.config UCI options Change Previous name
enabled Match default (1)
enable_protocols=upnp-igd Combined option enable_upnp=1
enable_protocols=pcp+nat-pmp Combined option enable_natpmp=1
allow_cgnat Accept extra values (2) use_stun
stun_host Accept port inclusion (3)
stun_port Removed, included in host
allow_third_party_mapping=0 Inverted/extended to PCP secure_mode=1
log_output Allow info log level
lease_file Set by default + IPv6 (4) upnp_lease_file
upnp_igd_compat=igdv1 Renamed/match default (1) igdv1=1
download_kbps In kbit/s and renamed (5) download
upload_kbps In kbit/s and renamed (5) upload
friendly_name New option, router name
http_port Renamed, rem. if default port
notify_interval Removed if <900s, minimum
internal_iface Migrated, new section
internal_network UCI options Change Previous name
interface New option
access_defaults New option (6)
accept_ports New option (6)
reject_ports New option (7)
check_acl New option (6)

Notes:

  1. Init UCI default now matches LuCI and initial config file defaults for: enabled=0 and upnp_igd_compat=igdv1
  2. Accept extra values for IPv4 CGNAT use, migrate option from X-Wrt and only use STUN when necessary with a private/CGNAT external IPv4
  3. Remove known unsupported STUN servers and set compatible by default
  4. Configure undocumented daemon option lease_file6=${lease_file}-ipv6 so that active IPv6 port maps are not lost when service restarts, e.g. by deleting an active port map. Remove option if UCI default set
  5. Gets converted, config file now defaults to interface link speed instead of 8/4 Mbit/s, which is removed on migration
  6. New options added for default ports that all devices on a network can map, client-specific permissions using the access control list (ACL) can extend/override the defaults. Access defaults: accept-high-ports accept-web+high-ports/accept-web-ports/accept-all-ports
  7. Reject ports; overrides other settings. By default reject unsafe: FTP (21), Telnet (23), DCE/NetBIOS/SMB (135/137-139/445), RDP (3389)

Code refactoring:

  • Add a function for logging and output to stderr, and extend logging
  • Revise daemon init/config-gen slightly by declare all UCI options (incl. booleans) according to the same principle and remove upnpd_write_bool
  • Document and reformat default /etc/config/upnpd UCI config file
  • Don't configure/flush unnecessary upnp_postrouting nftables chain

Fixes: #17413
Fixes: #29314
Fixes: #28644
Depends on: openwrt/luci#8415

5. Group/rearrange config-gen, refactoring
  • Group and rearrange UCI option declaration and config-gen by function/LuCI UI, and comment
  • Encode required XML entities of text UPnP IGD config options until the daemon does so using the created function xml_encode
  • Only generate UPnP IGD config if the protocol is enabled

(to merge with prior)

6. Rename UCI section to settings v2.0

Rename UCI section config (v1.0) -> settings (v2.0), helps on migration and to distinguish the updated config from the previous one

(to merge with prior)

7. Update ACL options, migrate section
  • The ACL is now rejected last if not accepted by access defaults. Add (disabled) ACL template entries on migration
  • Migrate ACL entries to the new section name acl_entry
  • The following ACL UCI options been added or changed, and the previous options are migrated on updating:
acl_entry UCI options Change Previous name
action New/updated values (1)
int_port Remove colon separator (2) int_ports
ext_port Remove colon separator (2) ext_ports
descr_filter New option (3)
  1. Allow disabled, and update action option to use the nftables terms (allow/deny -> accept/reject). To avoid adding inverted actions when changing via LuCI, ensure any missing are set, as LuCI and UCI had not matching action defaults. Missing actions are now ignored/logged
  2. Ensure that the hyphen (-) is only used as a port range separator by migration, as the colon (:) is not valid in LuCI
  3. Add missing UCI option to set a regular expression to check for a UPnP IGD IPv4 port map description, and fix the current collision with the comment field which was not noticed due to a daemon bug miniupnpd: Update to 2.3.7 and enable regex filter #24495 miniupnpd: Rewrite permission line parser miniupnp/miniupnp#853
  • Refactoring by adding a more universal usable is_port_or_range function instead of upnpd_get_port_range and check if it has a valid range, and removes a shellcheck warning
  • Rename conf_rule_add function to upnpd_add_acl_entry

(to merge with prior)

8. Separate service start and config-gen
  • Remove config_foreach upnpd "upnpd" and replace it with regular function call, as init was not designed for a multi-instance setup, as the same tmpconf will be used/overwritten, and non-anonymous section
  • Move code to make the custom vs. config file generation decision earlier, and only perform external interface detection with the second one, and rename function upnpd to upnpd_generate_config
  • Replace unnecessary if cases with elif in init/hotplug
  • Exit with 1 on errors to get an inactive service status
  • Use procd_add_reload_trigger "firewall" instead of listening /etc/config/firewall

(to merge with prior)

9. Rearrange init, format `firewall3.include`
  • Arrange start_service and main init functions first
  • Format firewall3.include using shfmt

(to merge with prior)

10. Service-wide access control settings

This implements variant B

upnpd.settings UCI options Change Previous name
access_defaults New option (1)
accept_ports New option (1)
reject_ports New option (2)
check_acl New option (1)

Notes:

  1. New options added for default ports that all devices can map, client-specific permissions using the access control list (ACL) can extend/override the defaults. Access defaults: accept-high-ports accept-web+high-ports/accept-web-ports/accept-all-ports
  2. Reject ports; overrides other settings. By default reject unsafe: FTP (21), Telnet (23), DCE/NetBIOS/SMB (135/137-139/445), RDP (3389)

(to merge with prior)

(The italic commits are intended to be merged with the prior ones after review)

Screenshots

The new service-wide access control functionality… can best be described using the LuCI screenshots:

New Access Control tab luci-access-control
Advanced Settings with new CGNAT functionality luci-advanced
UPnP IGD Adjustments tab (new) luci-igd
LuCI notification if the related package is not updated luci-notification
Full LuCI screenshot luci-screenshot

Depends on LuCI PR: openwrt/luci#8415
Tested on: OpenWrt 24.10.8/25.12.5 with iptables/nftables

miniupnpd: Core functionality issues
https://github.com/Self-Hosting-Group/miniupnpd-issues

The Port Control Protocol (PCP) is the successor to NAT-PMP, shares similar protocol concepts and packet formats, but supports IPv6 port mapping and options/extensions. For more information, see:
Port Mapping Protocols Overview and Comparison 2026+: About UPnP IGD & PCP/NAT-PMP
https://github.com/Self-Hosting-Group/wiki/wiki/Port-Mapping-Protocols-Overview

@Self-Hosting-Group

Copy link
Copy Markdown
Contributor Author

Hi @1715173329 @systemcrash @GeorgeSapkin. These late suggested changes only became clear when using the LuCI UI in practice. I have now prepared the remaining points for clarification, including screenshots, to hear your thoughts. Here are the two possible variants:

@Self-Hosting-Group

Self-Hosting-Group commented Mar 18, 2026

Copy link
Copy Markdown
Contributor Author

Variant A: Network access control

Provide network-wide access control settings that can be extended/overridden by access control list entries.

LuCI UI luci-network-access-control-edit luci-network-access-control
UCI config
config upnpd 'settings'
	option enabled                    '1'
	option enable_protocols           'all'
	…

config internal_network
	option interface                  'lan'
	option access_defaults            'accept-high-ports' # default none
	#option accept_ports
	#option reject_ports              '21 23 135 137-139 445 3389'
	option check_acl                  '0' # default 1

Variant B: Service access control

Enable networks in the Service Setup tab, and provide service-wide access control settings that can be extended/overridden by access control list entries, in a separate tab.

LuCI UI luci-access-control

And then the enable networks option separated in the Service Setup tab:

luci-access-control-separate-networks
UCI config
config upnpd 'settings'
	option enabled                    '1'
	option enable_protocols           'all'
	# Enable networks, multiple can be specified, separated by a space
	option internal_iface             'lan'
	option access_defaults            'accept-high-ports' # default none
	#option accept_ports
	#option reject_ports              '21 23 135 137-139 445 3389'
	option check_acl                  '0' # default 1

I'm leaning towards variant B (prepared), since it's clearer/simpler UI and UCI config, and network-wide permissions can also be handled via network-wide ACL entries (without accepting a list of ports), and keeping existing internal_iface UCI option with the space separated interfaces as is.

Final steps

  • Variant A/B ready
  • Wish to invert the introduced UCI option ignore_acl to check_acl
  • Handle get external IPv4 address with external_zone
  • Does an IPv6 ifup hotplug event exist? To avoid delaying the daemon start
  • LuCI todo (help wanted): Depending on the chosen variant, some sections (e.g. ACL) should appear as subsection and part of a tab, wish review of the wording
  • Retest with the final changes and merge (as single commit?)

@1715173329

Copy link
Copy Markdown
Member

Sorry, but I really almostly have no time to review this.
For the A/B implementation, I would say just choose the one you preferred ;)
Cc: @hnyman @neheb

@commodo

commodo commented May 13, 2026

Copy link
Copy Markdown
Contributor

Let's move ahead with this?

Please make the CI green on all builds, and let's merge this.

This looks good. It's likely better than the current state.
Everybody like perfection, but plenty of people like to get perfection on the first tries, while I prefer an incremental approach; and this looks good for an incremental approach.

@Self-Hosting-Group Self-Hosting-Group changed the title miniupnpd: Revision, new access control and UCI options… miniupnpd: Update, revision, new access control and UCI options… Jun 8, 2026
@Self-Hosting-Group
Self-Hosting-Group force-pushed the revision branch 5 times, most recently from 976bb72 to 03987f3 Compare June 10, 2026 06:03

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the full PR (fresh review, 10 commits). Commit messages match their diffs. Two inline notes below on the config-generation/migration logic; the large logging patch and the shfmt reformatting look consistent.


Generated by Claude Code

Comment thread net/miniupnpd/files/upnpd-migration.uci-defaults Outdated
Comment thread net/miniupnpd/files/miniupnpd.init Outdated
Update daemon to 2.3.11 to fix multiple issues, including
security-related ones

Link: https://www.cve.org/CVERecord?id=CVE-2026-5720
Link: miniupnp/miniupnp#898
Signed-off-by: Self Hosting Group <selfhostinggroup-git+openwrt@shost.ing>
- Add workaround to list port maps with the Windows IGDv2-incompatible
  client by returning an infinite (0) lease duration. To fix listing and
  editing via GUI (Explorer/Network), if daemon was compiled with IGDv2
- Extend detection to older versions of Windows and add Xbox
- Detect Apple IGDv2-incompatible clients and apply existing workaround,
  that only caused problems if PCP/NAT-PMP (prioritised) was disabled

(to merge with prior)

Link: https://github.com/Self-Hosting-Group/miniupnp/tree/upnp-igdv2-compat
Link: https://github.com/Self-Hosting-Group/wiki/wiki/Port-Mapping-Protocols-Overview#compatibility-issues
Signed-off-by: Self Hosting Group <selfhostinggroup-git+openwrt@shost.ing>
- Clearer logging of enabled protocols/ports, IPv6 mapping, and UPnP IGD
  compatibility mode… in start banner
- Log warnings with an enabled allow third-party mapping option
- Change the log level (less verbose) of many internal or normally
  occurring messages, and some error messages, without functional
  restrictions, to DEBUG, client requests/actions and their errors to
  INFO, and only daemon-wide messages to NOTICE/WARNING/ERR…
- Comment out some log-filling messages when internal interfaces lose
  link, and for e.g. `rule with label '%s' is not a IGD pinhole`, as
  normally occurring

(to merge with prior)

Fixes: https://redirect.github.com/openwrt/packages/issues/17601
Fixes: https://redirect.github.com/openwrt/packages/issues/26483
Link: https://github.com/Self-Hosting-Group/miniupnp/tree/improve-logging
Signed-off-by: Self Hosting Group <selfhostinggroup-git+openwrt@shost.ing>
The following settings UCI options been added or changed, and the
previous options are migrated on updating:

upnpd.config UCI options     | Change                    | Previous name
-----------------------------|---------------------------|--------------
enabled                      | Match default         (1) |
enable_protocols=upnp-igd    | Combined option           | enable_upnp=1
enable_protocols=pcp+nat-pmp | Combined option           | enable_natpmp=1
allow_cgnat                  | Accept extra values   (2) | use_stun
stun_host                    | Accept port inclusion (3) |
stun_port                    | Removed, included in host |
allow_third_party_mapping=0  | Inverted/extended to PCP  | secure_mode=1
log_output                   | Allow info log level      |
lease_file                   | Set by default + IPv6 (4) | upnp_lease_file
upnp_igd_compat=igdv1        | Renamed/match default (1) | igdv1=1
download_kbps                | In kbit/s and renamed (5) | download
upload_kbps                  | In kbit/s and renamed (5) | upload
friendly_name                | New option, router name   |
http_port                    | Renamed, rem. if default  | port
notify_interval              | Removed if <900s, minimum |
internal_iface               | Migrated, new section     |

internal_network UCI options | Change                    | Previous name
-----------------------------|---------------------------|--------------
interface                    | New option                |
access_defaults              | New option            (6) |
accept_ports                 | New option            (6) |
reject_ports                 | New option            (7) |
check_acl                    | New option            (6) |

Notes:
1. Init UCI default now matches LuCI and initial config file defaults
   for: enabled=0 and upnp_igd_compat=igdv1
2. Accept extra values for IPv4 CGNAT use, migrate option from X-Wrt and
   only use STUN when necessary with a private/CGNAT external IPv4
3. Remove known unsupported STUN servers and set compatible by default
4. Configure undocumented daemon option `lease_file6=${lease_file}-ipv6`
   so that active IPv6 port maps are not lost when service restarts,
   e.g. by deleting an active port map. Remove option if UCI default set
5. Gets converted, config file now defaults to interface link speed
   instead of 8/4 Mbit/s, which is removed on migration
6. New options added for default ports that all devices on a network can
   map, client-specific permissions using the access control list (ACL)
   can extend/override the defaults. Access defaults: accept-high-ports
   accept-web+high-ports/accept-web-ports/accept-all-ports
7. Reject ports; overrides other settings. By default reject unsafe:
   FTP (21), Telnet (23), DCE/NetBIOS/SMB (135/137-139/445), RDP (3389)

Code refactoring:
- Add a function for logging and output to stderr, and extend logging
- Revise daemon init/config-gen slightly by declare all UCI options
  (incl. booleans) according to the same principle and remove
  `upnpd_write_bool`
- Document and reformat default `/etc/config/upnpd` UCI config file
- Don't configure/flush unnecessary `upnp_postrouting` nftables chain

Fixes: https://redirect.github.com/openwrt/packages/issues/17413
Fixes: https://redirect.github.com/openwrt/packages/issues/29314
Fixes: https://redirect.github.com/openwrt/packages/pull/28644
Depends on: https://redirect.github.com/openwrt/luci/pull/8415

Signed-off-by: Self Hosting Group <selfhostinggroup-git+openwrt@shost.ing>
- Group and rearrange UCI option declaration and config-gen by
  function/LuCI UI, and comment
- Encode required XML entities of text UPnP IGD config options until the
  daemon does so using the created function `xml_encode`
- Only generate UPnP IGD config if the protocol is enabled

(to merge with prior)

Signed-off-by: Self Hosting Group <selfhostinggroup-git+openwrt@shost.ing>
Rename UCI section `config` (v1.0) -> `settings` (v2.0), helps on
migration and to distinguish the updated config from the previous one

(to merge with prior)

Signed-off-by: Self Hosting Group <selfhostinggroup-git+openwrt@shost.ing>
- The ACL is now rejected last if not accepted by access defaults. Add
  (disabled) ACL template entries on migration
- Migrate ACL entries to the new section name `acl_entry`
- The following ACL UCI options been added or changed, and the previous
  options are migrated on updating:

acl_entry UCI options       | Change                     | Previous name
----------------------------|----------------------------|--------------
action                      | New/updated values     (1) |
int_port                    | Remove colon separator (2) | int_ports
ext_port                    | Remove colon separator (2) | ext_ports
descr_filter                | New option             (3) |

1. Allow disabled, and update action option to use the nftables terms
   (allow/deny -> accept/reject). To avoid adding inverted actions when
   changing via LuCI, ensure any missing are set, as LuCI and UCI had
   not matching action defaults. Missing actions are now ignored/logged
2. Ensure that the hyphen (-) is only used as a port range separator by
   migration, as the colon (:) is not valid in LuCI
3. Add missing UCI option to set a regular expression to check for a
   UPnP IGD IPv4 port map description, and fix the current collision
   with the comment field which was not noticed due to a daemon bug
   https://redirect.github.com/openwrt/packages/pull/24495
   https://redirect.github.com/miniupnp/miniupnp/pull/853

- Refactoring by adding a more universal usable `is_port_or_range`
  function instead of `upnpd_get_port_range` and check if it has a valid
  range, and removes a shellcheck warning
- Rename `conf_rule_add` function to `upnpd_add_acl_entry`

(to merge with prior)

Signed-off-by: Self Hosting Group <selfhostinggroup-git+openwrt@shost.ing>
- Remove `config_foreach upnpd "upnpd"` and replace it with regular
  function call, as init was not designed for a multi-instance setup, as
  the same `tmpconf` will be used/overwritten, and non-anonymous section
- Move code to make the custom vs. config file generation decision
  earlier, and only perform external interface detection with the second
  one, and rename function `upnpd` to `upnpd_generate_config`
- Replace unnecessary `if` cases with `elif` in init/hotplug
- Exit with 1 on errors to get an inactive service status
- Use `procd_add_reload_trigger "firewall"` instead of listening
  `/etc/config/firewall`

(to merge with prior)

Signed-off-by: Self Hosting Group <selfhostinggroup-git+openwrt@shost.ing>
- Arrange `start_service` and main init functions first
- Format `firewall3.include` using shfmt

(to merge with prior)

Signed-off-by: Self Hosting Group <selfhostinggroup-git+openwrt@shost.ing>
This implements variant B

upnpd.settings UCI options   | Change                    | Previous name
-----------------------------|---------------------------|--------------
access_defaults              | New option            (1) |
accept_ports                 | New option            (1) |
reject_ports                 | New option            (2) |
check_acl                    | New option            (1) |

Notes:
1. New options added for default ports that all devices can map,
   client-specific permissions using the access control list (ACL) can
   extend/override the defaults. Access defaults: accept-high-ports
   accept-web+high-ports/accept-web-ports/accept-all-ports
2. Reject ports; overrides other settings. By default reject unsafe:
   FTP (21), Telnet (23), DCE/NetBIOS/SMB (135/137-139/445), RDP (3389)

(to merge with prior)

Signed-off-by: Self Hosting Group <selfhostinggroup-git+openwrt@shost.ing>
@Self-Hosting-Group
Self-Hosting-Group force-pushed the revision branch 5 times, most recently from 397f547 to d357cb5 Compare August 17, 2026 10:30

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed against the force-pushed series (previous review was at 397f547). The whole series was recreated, but the material delta is the 2.3.10 → 2.3.11 bump, the two patches rebased onto the new context, and two fixes in the init/migration scripts.

Both findings from the previous review look resolved: the kbit/s conversion is now download * 8 * 1024 / 1000, which matches the old bitrate_down=$((download * 1024 * 8)) against the new bitrate_down=$((download_kbps * 1000)); and reject_ports now emits a deny pair covering the internal and the external port independently, so the ext≠int bypass is closed.

The patch refresh looks like a clean rebase — only hunk offsets change, plus the genuinely new upstream context in PinholeVerification (int_ip_resolved) and the new pf/obsdrdr.c / pf/pfpinhole.c DIOCCHANGERULE sites, which are consistent with the rest of the patch. Two inline notes below, one of them a nit.

Commit checks

  • 02c95a5, 811a601, c31e6a8 — the Fixes: / Depends on: trailers point at redirect.github.com/openwrt/... rather than github.com/openwrt/.... That host is a link-redirect proxy artifact (the PR description uses the canonical github.com URLs for the same references), and these trailers land permanently in the openwrt/packages history.

Generated by Claude Code

Comment thread net/miniupnpd/files/upnpd-migration.uci-defaults
Comment thread net/miniupnpd/files/miniupnpd.init
@@ -1,3 +1,2 @@
chain upnp_forward {}
chain upnp_prerouting {}
chain upnp_postrouting {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is correct, chain is not used by miniupnpd at all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment