Skip to content

net/upnp: add missing firewall anchors#3165

Merged
fichtner merged 3 commits intoopnsense:masterfrom
Tawmu:upnp-fwrules
Nov 9, 2022
Merged

net/upnp: add missing firewall anchors#3165
fichtner merged 3 commits intoopnsense:masterfrom
Tawmu:upnp-fwrules

Conversation

@Tawmu
Copy link
Contributor

@Tawmu Tawmu commented Oct 19, 2022

Later releases of miniupnpd require additional firewall anchors to function properly. This commit adds those.

@fichtner fichtner self-assigned this Oct 19, 2022
@fichtner
Copy link
Member

@Tawmu thanks, do you have a quick reference to when they started to use these anchors (changelog possibly)?

@Tawmu
Copy link
Contributor Author

Tawmu commented Oct 19, 2022

@fichtner my primary motivation for this is to get multiple game consoles behind an opnsense router working properly when trying to join lobbies together - there was quite a bit of work done between miniupnpd and another firewall distro to get this working.

Sometime in 2020/21 miniupnpd fixed some NAT rule generation (I think around version 2.2.0, the changelog is a little lacking (https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/Changelog.txt#L90)) - part of the reason for adding these extra anchors in is to ensure the rules are inserted at the proper place (i.e. before other NAT rules generated by opnsense).

I'll be submitting an update to ports for a miniupnpd package update soon to improve this behavior in opnsense.

Forgive me if it's not very clear or I've done something wrong - I don't usually dip my feet into BSD based stuff so pf and opnsense dev is relatively new to me.


$fw->registerAnchor('miniupnpd', 'rdr');
$fw->registerAnchor('miniupnpd', 'fw');
$fw->registerAnchor('miniupnpd', 'nat', '', 'head');
Copy link
Member

@fichtner fichtner Oct 20, 2022

Choose a reason for hiding this comment

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

'head' might be much to the surprise of the user, but then I don't expect upnp active for anyone but the game console use case in a home setting as you describe. but maybe we should warn users more about the potential security issues. not sure where or how

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree - I guess the other situation is that (automatic/manual) NAT rules could potentially break UPNP without any user notification if it wasn't at the top.

Perhaps some kind of warning on the opnsense UPNP plugin page? I imagine that would be sufficient since a user needs to go and enable the UPNP service manually after install anyway.

Copy link
Member

Choose a reason for hiding this comment

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

let's merge this one and work on the other one to include some sort of sensible warning

@fichtner
Copy link
Member

thanks for the details. from the ports PR I see that before we do this the update must be carried out, right? that could be a slow grind in the FreeBSD bugzilla, but we shall see :)

@Tawmu
Copy link
Contributor Author

Tawmu commented Oct 21, 2022

Yeah AFAIK miniupnpd needs to be updated first but I guess it wouldn't hurt to merge this in anticipation (pending above comment about warning the user)

@fichtner fichtner merged commit 1781291 into opnsense:master Nov 9, 2022
@fichtner
Copy link
Member

fichtner commented Nov 9, 2022

Merged, thanks!

@fx-byte
Copy link

fx-byte commented Dec 30, 2022

Is there any way for me to add this code change manually in a current build?

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants