Miniupnp full cone double NATincorrectly adding rules

jimp

If it can detect that it's behind NAT it seems like the smart thing to do would always be to use the interface address in that case, and ignore the ext IP/STUN IP address. Or at least check if the ext IP/STUN IP address is actually present on the interface and ignore it if it isn't.

Though I do love the idea of a config option to disable the ridiculous enforced private WAN behavior. Some of us don't need the hand holding because we know what we're doing when saddled with double NAT we can't avoid.

encrypt1d

@jimp
I found the actual logic error in obsdrdr.c in the add_nat_rule function.

I have implemented and tested successfully a config based option, details over at:

https://github.com/miniupnp/miniupnp/issues/598

Two follow on questions

If they still refuse to add the fix, are there patching options for pfSense that could generate official patches, that users would only download and apply if needed? It is admittedly a smallish number of customers that may have this issue. I would imagine most enterprises won't EVER turn on UPnP.
Will pfSense be exposing the rest of the config options in the miniupnpd.conf file in the future, perhaps as an advanced section in the setting GUI? The missing options get blasted each time you enable/disable the feature.

jimp

@encrypt1d said in Miniupnp full cone double NATincorrectly adding rules:

If they still refuse to add the fix, are there patching options for pfSense that could generate official patches, that users would only download and apply if needed? It is admittedly a smallish number of customers that may have this issue. I would imagine most enterprises won't EVER turn on UPnP.

There isn't a way to do that for compiled/binary packages. We'd have to always add them in. While we try to avoid doing that sort of thing because it adds technical debt, we can keep patches in the files/ dir of the port in the repo if need be.

We could have two variations of the port, one with the patches and one without, but that's also even more we would have to maintain.

Will pfSense be exposing the rest of the config options in the miniupnpd.conf file in the future, perhaps as an advanced section in the setting GUI? The missing options get blasted each time you enable/disable the feature.

We add things as the need arises, generally. It's not too difficult to add new GUI options, the ones that aren't there are typically not there only because nobody has ever asked for them formally, or if there is a feature request, just that no dev has got to them yet. If there are new options or existing options which are beneficial to users we can add them in any time.

I see you're having some trouble trying to convince them over on the miniupnp issue. I think some of them there are getting caught up in the semantics (and the "why?" of it) and not seeing how obviously wrong the current behavior is.

jimp

I replied on Github, hopefully what I said makes sense and doesn't confuse things further.

encrypt1d

@jimp

I think it was definitely helpful!
I've had "config file" on the brain since the start, but your idea is much better. It's fully automated now in the last diff I posted in the other thread. I still need to set the ext_ip or STUN option to get past those checks, but it works great! It may be the case that more fulsome implementations of UPnP clients might actually need a public IP in there, so they can do with that as they please. Game clients typically don't, as they just want to punch holes in the firewall, not talk UPnP to other clients. There's a lot to UPnP that I don't know, so I kind of get their resistance to changing anything up there. I think this is the nest solution we can have honestly. Thanks for the input.

jimp

Nice!

That change looks a lot cleaner than the config option as well.

Hopefully they respond positively since it appears to follow their suggestions for where the change belongs.

encrypt1d

@jimp

Looks like they committed a variation of my fix with slightly better error handling - but it is in!

https://github.com/miniupnp/miniupnp/commit/c0d3a176509b7f659fa713c0d11597bdbfae7ca5

So for all the double NAT folks out there, the fix is coming.

How does the process unfold here, does it get updated in the pfSense repo?

Bob.Dig

@encrypt1d That would be fantastic, can't believe it.

jimp

@encrypt1d said in Miniupnp full cone double NATincorrectly adding rules:

@jimp
How does the process unfold here, does it get updated in the pfSense repo?

Ideally, they'll put out a release, that release gets into the FreeBSD ports tree, and then we pull it in from there.

In the past we have also set the port in our tree to build from a specific commit on their master branch if I'm remembering right, we did that not long after they put in the nat on rule support so we could start testing it.

mluna

@jimp Hey, how are you?

I couldn't see anything related in the new BETA release 22.05. Do you think this fix will make it to the final release?

Marc05

It would be helpful to have this patch added in to help those with double NAT. It looks like last time it was updated on pfSense was ~4 years ago, and at this point it seems doubtful it's going to be updated any time soon.

mluna

@marc05 said in Miniupnp full cone double NATincorrectly adding rules:

It would be helpful to have this patch added in to help those with double NAT. It looks like last time it was updated on pfSense was ~4 years ago, and at this point it seems doubtful it's going to be updated any time soon.

Yeah, I wish someone uploaded a patch at least as I myself am unable to compile the fixed app.

jimp

The miniupnp project hasn't yet put out a release which includes that patch. We try not to incur technical debt or risk by adding in patches between releases when we can avoid it. Once they put out a new release we can update ours to use it.

mluna

@jimp totally makes sense. Thank you!

Marc05

It has been broken for many years now, so another couple of years doesn't sound too terrible in that perspective. Still, it sucks :(