Miniupnp full cone double NATincorrectly adding rules

jimp

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

The answer from the miniupnp maintainers is that this is a problem with "miniupnp's pf firewall backend".
https://github.com/miniupnp/miniupnp/issues/598
How do we move this towards a solution?

That's actually code in miniupnpd which injects the rules into pf on FreeBSD, not something specific to pfSense. Looks like in the stun/ext_ip case it's using the external address when it should be the internal (or just the interface name).

I am still trying to get a build environment going without much luck over in this post, so that I can try debugging/patching miniupnp myself. https://forum.netgate.com/topic/169749/pfsense-compile-requirements-for-3rd-party-software/11

The code in their repo which sets up the external part of the NAT rule is around here:

https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/pf/obsdrdr.c#L379 (maybe some code above there that sets parts of it up, too)

and it gets called from here:
https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/pf/obsdrdr.c#L670

encrypt1d

@jimp
The miniupnp devs mentioned this:

I see that external IP adress is used by pf backend only when ENABLE_PORT_TRIGGERING is enabled. Try to disable it before recompilation (edit config.h to do so)

I am currently trying to figure out how to do that. My builds aren't successfully merging patches yet unfortunately. The config.h file where the compiler def is defined is not in the port dist file, so I can't tell where it might be getting defined.

It sounds promising though.

jimp

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

I am currently trying to figure out how to do that. My builds aren't successfully merging patches yet unfortunately. The config.h file where the compiler def is defined is not in the port dist file, so I can't tell where it might be getting defined.

That is generated automatically by the make configure step which happens as a part of any port build. It's controlled by the arguments passed to configure. I don't see a knob in the FreeBSD port to change that variable, or even a conditional in their configure file that would leave it out, so you may need to patch it out of there manually.

--- configure.orig	2022-02-11 13:40:00 UTC
+++ configure
@@ -592,7 +592,6 @@ fi

 echo "/* Enable IGD2 \"Port Triggering\" as defined in Section 2.5.16" >> ${CONFIGFILE}
 echo " * figure 2.2 in UPnP-gw-WANIPConnection-v2-Service.pdf */" >> ${CONFIGFILE}
-echo "#define ENABLE_PORT_TRIGGERING" >> ${CONFIGFILE}

 echo "" >> ${CONFIGFILE}
 echo "#define OS_NAME		\"$OS_NAME\"" >> ${CONFIGFILE}

Might be best to keep that kind of detail in your other thread, though.

jimp

Looking at their code, though, without ENABLE_PORT_TRIGGERING defined it will disable the outbound NAT rules entirely.

jimp

Revisiting this a little there are two separate cases here:

Manually setting the ext IP address should use that IP address in the NAT rules because that feature is intended for using IP alias or CARP VIPs on a WAN interface so that UPnP doesn't need to use the main interface address. This appears to be working OK.
The STUN case is forming invalid outbound rules (nat on ...) using the IP address it discovered from STUN. This isn't valid because that address is not present on the firewall, it's an address on an upstream device.

Only the second case is problematic.

For inbound connections (rdr), STUN is working and a client can open and successfully test a port with a private WAN with 1:1 setup upstream passing all traffic to it.

See also: https://redmine.pfsense.org/issues/12797

encrypt1d

Manually setting the ext IP address should use that IP address in the NAT rules because that feature is intended for using IP alias or CARP VIPs on a WAN interface so that UPnP doesn't need to use the main interface address. This appears to be working OK.

Yes, for non-double NAT scenarios where the WAN interface is a public IP, this works. But in a double NAT, it sets the nat rule to use the public IP, which won't work for the same reason as you point out in item 2. I just tested again to be sure. I agree the rdr rules are good, its only ever been the NAT rules that are an issue.
Essentially, both the ext_ip and STUN options behave the same for me.

I'm getting closer to having a build environment going where I can try and make miniupnp allow a private IP on the WAN interface. It's been a process.

jimp

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

Yes, for non-double NAT scenarios where the WAN interface is a public IP, this works. But in a double NAT, it sets the nat rule to use the public IP, which won't work for the same reason as you point out in item 2.

Those two scenarios are mutually exclusive, though. The external IP address is doing exactly what the configuration is intended to do when it's set since it would have to be in the NAT rules.

Unless they have some code in the backend that changes the behavior when it detects double NAT, which they might. The stock miniupnpd.conf description does mention using that field with double NAT after it mentions using multiple IP addresses.

The double NAT scenario seems much more viable with STUN. Not really a good reason not to rely on STUN for that since there are good public STUN servers like the one from Google, but if both ways can work that should be OK, too.

encrypt1d

@jimp

Looking at their code, though, without ENABLE_PORT_TRIGGERING defined it will disable the outbound NAT rules entirely.

I have my builds finally working. And tested this.

You are quite correct. Removing that compiler definition results in miniupnp only adding the rdr rule, no nat.

So the next thing I tried was to disable the checks for RFC1918 addresses. My config file is not using the ext_ip or STUN options for this test. There were two places, and once I overrode their checks, it works flawlessly for me in my double NAT 1:1 network setup. I have to say that after all this time it is quite satisfying to see call of duty reporting OPEN NAT on all my clients, without having to mess around with any NAT/Firewall rules.

If anything, I have a solution for myself, but I'd sure like this work for everyone, so I will report my results to the miniupnp maintainers. It's a very small code change to add an option to the config file to do this.

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.