nftables: 'miniupnpd' chain gets overruled by default drop policy on base chain

[NicholasFahey]

Trying to get miniupnpd working with nftables and I am most of the way there, but I believe there is a still a fundamental issue with how the nftables support is implemented in miniupnpd.

Kernel: Linux 5.3.6
miniupnpd version: 2.1.20191006

My base ruleset is this:

table ip filter {
        chain input {
		type filter hook input priority 0; policy drop;
                # Accept some LAN traffic and established WAN traffic here; drop everything else
               ...
	}
	chain forward {
		type filter hook forward priority 0; policy drop;
                # Accept LAN traffic and established WAN traffic here; drop everything else
                ...
        }
}

table ip nat {
	chain prerouting {
		type nat hook prerouting priority -100; policy accept;
	}

	chain postrouting {
		type nat hook postrouting priority 100; policy accept;
		oifname "eth2" masquerade;
	}
}

After starting miniupnpd and having it add a port forwarding rule I end up with this in the miniupnpd tables:

...
table inet miniupnpd {
	chain forward {
		type filter hook forward priority -25; policy accept;
                iif "eth2" @th,16,16 40005 @nh,128,32 3232260974 @nh,72,8 6 accept
	}
}
table ip miniupnpd {
	chain prerouting {
		type nat hook prerouting priority -100; policy accept;
                iif "eth2" tcp dport 40005 dnat to 192.168.99.110:40005
	}

	chain postrouting {
		type nat hook postrouting priority 100; policy accept;
	}
}
table ip6 miniupnpd {
	chain prerouting {
		type nat hook prerouting priority -100; policy accept;
	}

	chain postrouting {
		type nat hook postrouting priority 100; policy accept;
	}
}

Those rules look correct but I'm still not able to pass traffic on that port. I believe the issue is that the traffic has to be accepted by both the miniupnpd forward chain and my filter table forward chain. If I add in packet counters to the 2 miniupnpd rules I can see that the packets hit those rules but still end up being dropped despite the higher priority on the miniupnpd chain.

To be honest, nftables docs is a bit unclear in this regard but I think this line from these docs lays it out pretty clearly:

NOTE: if a packet gets accepted and there is another base chain in the same hook which is ordered with a later priority, the packet will be evaluated again. That is, packets will traverse chains in a given hook, until it is dropped or no more base chains exist. Drops take instant effect, no further rules or chains are evaluated.

So every chain in the forward hook, for example, needs to accept the packet or else it will be dropped, regardless of chain priority. This seems really silly to me and seems to make nftables priorities virtually useless IMO but that behavior is in line with the behavior I am seeing.

I think @4np ran into this same issue and I read through @paul-chambers discussion with him here but I believe @4np ended up resolving this by changing his default forward policy to accept which is not a viable solution in my (and probably many others') case.

Therefore, I don't think creating separate miniupnpd tables is the right approach, as most (secure) routers have a default drop policy on their base forward chain and will drop the port-forwarded packets regardless of the miniupnpd rules. If I add the miniupnpd rules directly to my forward chain then everything works as expected.

I think a potentially straight-forward fix for this would be to let the user configure which nat and filter table they want miniupnpd to add the rules to, in addition to which chains. This would allow users to add the rules directly to their base chains if they choose. This would obviously have an impact on how miniupnpd deletes rules when it shuts down, as we would only want it to delete the rules it created and not delete the whole table. Probably not the most elegant solution but not sure of another way to get around nftables' bizarre handling of chain priorities.

Really appreciate the work on bringing nftables support to miniupnpd! Almost there!

[4np]

Hah, I would prefer to drop by default as well 😉

[paul-chambers]

I don't claim to be an expert on nftables, but the approach used is in line with the philosophy as I understand it (and also used by the sshguard implementation).

Yes, the nftables documentation leaves plenty of room for improvement. They feel like brief notes left by someone with a great deal of context not shared by the typical reader - much is left unsaid. In particular, I had to unlearn some iptables assumptions before nftables starts to make sense. I suspect those two factors are a big part of why nftables isn't gaining traction more quickly.

Yes, the packet will be evaluated by every chain registered on a hook, in order of priority (numerically increasing) A big reason for this is to avoid inter-dependency and close-coupling of unrelated functions. For example, miniupnpd and sshguard are effectively isolated within their own, partitioned namespaces.

The default behavior is to 'accept' packets that didn't match any of the rules in the chain. You can override this to be 'drop', but if a packet is dropped, the evaluation stops at that point, no further rules or chains will be evaluated. 'accept' does not terminate the entire evaluation in the same fashion, only short-circuits the evaluation of that chain and moves onto the next. Though I suspect there are subtle differences in how 'accept' used in the 'type' statement and in the rules themselves are interpreted which I haven't fully understood.

Personally, I don't consider the nftables implementation of miniupnpd 'broken' (though I can hardly be considered to be impartial). Understanding and applying nftables does take some mental adjustment after mastering iptables, however.

[NicholasFahey]

Hey Paul, thanks for the response! I definitely agree with you that the nftables implementation of miniupnpd is far from broken. I was just hoping for some flexibility that would allow for compatibility with a greater number of base nftables setups.

I do like nftables' ability to partition rules into logically separate namespaces but still doesn't seem like the right approach in this particular situation.

As implemented now, the forward rules that miniupnpd adds have basically no effect...if my base forward chain already accepted those packets, then they'll still be accepted; if it dropped those packets, they will still be dropped. So what's the point of adding those rules?

For sshguard, on the other hand, I believe this implementation makes sense as the rules that sshguard is adding are to drop certain packets; this is in direct contrast to miniupnpd whose rules are attempting to accept certain packets. So I don't think the two services can or should take the same approach on how they configure firewalls.

Unfortunately, due to some limitations in nftables (for example, inability to jump between chains in separate tables), I have been unable to come up with a work-around to the way this is currently implemented.

Hope that all makes sense to you and look forward to hearing your thoughts.

[imess]

Maybe a simple 'jump' from base chain to 'non-base' chain(a chain without type or hook statement) should do the job, just like the old way 'iptables -j custom_chain'. See the 'Jumping to chain' from the official wiki

[paul-chambers]

Essentially I fixed what was there but not working; I didn't design it. I think what makes more sense in the nftables world is to use a map, like the example given on https://wiki.nftables.org/wiki-nftables/index.php/Maps

[JAMESMTL]

now i'm no dev, but the current strategy of hard-coding nft_table as miniupnpd creates an issue for those of us who drop by default in our forward chains. I see two possible solutions to this and neither involves maps.

a) allow the user to configure both table and chain names and leave it to the user to create and destroy the tables and chains within our existing rulesets. Basically configure rule paths in conf file.

b) allow the user to add a custom mark. if a custom mark is set in the conf file (ex 0x00000001) then ct mark/set and accept the packet instead of just accept. We can then just filter for the mark within our own rulesets.

cheers

ps: also use counters flag in conf file would be awesome. if true then add counter to the generated rules

[JAMESMTL]

I would like to bump this issue as it is pretty much a show stopper. While I don't drop by default as a chain policy I do drop all wan ingress traffic that does not match a permitted service.

The way miniupnpd works now breaks under any config such as this simple example. I believe this example (equivalent to a default drop policy) is representative of the philosophy on how the majority of linux based router firewalls operate today

table ip nat {
	chain POSTROUTING {
		type nat hook postrouting priority srcnat; policy accept;
		oifname { "ppp0" } masquerade
	}
}
table inet filter {
	chain INPUT {
		type filter hook input priority filter; policy accept;
		ct state established,related accept
		ct state invalid drop
		iifname { "ppp0" } drop
		iifname "lan0" accept
		iifname "lo" accept
		reject
	}

	chain FORWARD {
		type filter hook forward priority filter; policy accept;
		ct state established,related accept
		ct state invalid drop
		iifname { "ppp0" } ct state new ip id 0 accept
		iifname { "ppp0" } drop
		oifname { "ppp0" } accept
		drop
	}

}

The only way I have been able to use miniupnpd in testing with nftables to accept where new connections which have an id of 0 but i'm sure everyone would agree that this would be less than ideal in production.

I would feel much more comfortable matching predefined connmark. This could be implemented as accept + connmark and then everyone would be happy. sshguard could match on accept and the rest of us could match on a specific connmark.

[miniupnp]

@JAMESMTL chain names are already configurable. Allow to configure table name is quite easy.

But I know nothing about custom mark.

[JAMESMTL]

What I meant by a custom mark is that miniupnpd writes to inet miniupnpd like the second rule and what I was suggesting is to write like the first rule

table inet miniupnpd {
        chain MINIUPNPD {
                type filter hook forward priority -25; policy accept;
                iif "ppp0" th dport 8096 @nh,128,32 3232236082 @nh,72,8 6 meta mark set 0x10000000 accept
                iif "ppp0" th dport 8096 @nh,128,32 3232236082 @nh,72,8 6 accept
        }
}

Otherwise if going with the configurable table names like this

upnp_forward_table=inet filter
upnp_nat4_table=ip nat
upnp_nat6_table=ip6 nat

then please leave the add / delete of nftables tables and chains to the user otherwise when restarting miniupnpd we will destroy our firewalls when miniupnpd deletes the chains.

[paul-chambers]

When I have time, I'll be switching to an ipset-based approach, which makes a whole lot more sense than the original author's design (I started out fixing a broken implementation, turns out the design was broken, too).

Unfortunately my day job is sucking up most of my spare time too, at the moment.

[JAMESMTL]

I'm really sorry about this Paul. I wanted to avoid the mention of refactoring as that would implicate a significant amount of someone else's time which is why I tried to find some common ground with the custom mark suggestion. If I were to take a step back and look at how I handle port forwards on nftables, you would see that I use a map for the ip nat table and a set for the inet forward table. That kind of makes it hard to argue with you.

If you do go ahead and refactor the code could you make it that the family and table names are configurable via .conf?

Cheers

[svenauhagen]

Hi,

I am sending in a series of PR to address this problem.
They will do the following:

1. Use the inet chain for nat as well so there is only one table
2. Allow the name of the Filter and NAT table to be changed
3. This is probably somewhat controversial: The c code should not create and delete the table and chains. The scripts should be used to do that. Otherwise there is no way to integrate miniupnpd into a custom firewall concept that uses nftables for other rules as well.

NFTables is very flexible but it comes with some restrictions because of that. If there is a second filter chain than all packets that were passed before with the miniupnpd chain will be reevaluated. This also means that if the chain is a drop chain you loose the packets. In that case you really want to use a custom chain and jump to it in your filter chain. miniupnpd should save all accept rules in that custom chain.
For NAT it is the same, a second chain will also evaluate the packets again and therefore it is possible that a second SNAT or DNAT is performed.
The 3 PRs will allow for custom setups to configure miniupnpd to be integrated.

Best
Sven

[prydom]

I recently had to solve this problem on my own nftables-based router/firewall and took a similar tactic to what @svenauhagen described in #422 (comment).

While my commits are not upstream ready, and so I won't be sending a pull request unless I receive a request from a contributor who wants to help me clean the contribution up, anyone running into this problem today may review my branch at https://github.com/prydom/miniupnp/commits/nftables-coexistance and adapt the commits to your own setup.

My commits:

1. Add the missing option to change the table name used by miniupnpd (RDR_TABLE_NAME wasn't previously exposed via the .conf). This is probably ready for a small PR, if someone wants to review prydom@f7acbc4.
2. Disable setting Netfilter hooks (https://wiki.nftables.org/wiki-nftables/index.php/Netfilter_hooks) on any of the chains created by miniupnpd so that I can jump to them from my "policy drop" chain. I think to be upstream ready this needs to either be runtime configurable or needs to be properly documented as handled only via shell script.
3. Delete the chain/table teardown in shutdown_redirect and use nft_flush.sh from Systemd (service unit not committed, but it uses ExecStopPost=) to flush the miniupnpd chains when miniupnpd is gracefully stopped. This won't be ready until a decision is made about what to do with chain cleanup. Miniupnpd should never delete tables it shares with other router/firewall software.

[svenauhagen]

@prydom good to hear that you used a similiar tactic.
Maybe also have a look at my code.
There are three feature branches, I am waiting for them to be merged.

1. https://github.com/svenauhagen/miniupnp/tree/feature/nftablesnat
2. https://github.com/svenauhagen/miniupnp/tree/feature/nftablesnatname
3. https://github.com/svenauhagen/miniupnp/tree/feature/nftablesinit

If something is missing it would be good if you can add it on top?

[prydom]

@svenauhagen, I saw #562 however I didn't check your fork to see if you had completed the other two PRs you said you were working on. I just reviewed all three and they generally look good to me. I do think a script based setup/teardown makes the most sense moving forward.

If I may suggest one change, for svenauhagen@eb3bc9b I think you may want to avoid creating any new chains with "policy drop". If a distro picks this up and naively calls nft_init.sh in their init scripts this may result in inadvertently locking some people out of their routed subnets. All chains with Netfilter hooks are always run as we've all found out in this issue :).

Another important note is that I do currently have my NAT rules implemented as two seperate tables (ip and ip6) and so your initial PR #562 does represent a breaking change, at least to my router/firewall configuration. However it should be a simple fix to merge those tables prior to switching back to upstream. Thankfully, I don't think that a distro upgrade (or rolling release) that may distribute these patches would result in a loss of connectivity for the vast majority of NAT configurations so I don't think there are any changes to the PR that you need to make.

[svenauhagen]

@prydom thank you for your feedback. yes I think so too that the scripts are the best way. It is also done like that with iptables.

That is a good point with policy drop, but on the other hand the miniupnpd filter chain is pretty much not needed if the general policy is accept. Maybe a bigger update to the documentation to explain the situation is needed?

Yes, the NAT change is a breaking change but it makes the layout much easier since only one table is needed now for NAT and FILTER. I think that is also the design goal of nftables. There should definitely be a note on the Changelog about that.

[miniupnp]

@svenauhagen everything is OK now ?

[miniupnp]

#562 #581 #584

[svenauhagen]

@miniupnp yes, the three commits take care of the problem from this issue.
The tables and chains are now created with the scripts and not by the miniupnpd daemon anymore so they can be changed.
The user has to execute the script or make sure that a custom table/chain is in place.
As a summary the new default chain and table layout is:

table inet filter {
chain forward {
type filter hook forward priority 0;
policy drop;

    # miniupnpd
    jump miniupnpd

    # Add other rules here
}

# miniupnpd
chain miniupnpd {
}

chain prerouting {
    type nat hook prerouting priority -100;
    policy accept;

    # miniupnpd
    jump prerouting_miniupnpd

    # Add other rules here
}

chain postrouting {
    type nat hook postrouting priority 100;
    policy accept;

    # miniupnpd
    jump postrouting_miniupnpd

    # Add other rules here
}

chain prerouting_miniupnpd {
}

chain postrouting_miniupnpd {
}

}

and the following config settings can be used to change the tables and chains

upnp_table_name=filter
upnp_nattable_name=filter
upnp_forward_chain=miniupnpd
upnp_nat_chain=prerouting_miniupnpd
upnp_nat_postrouting_chain=postrouting_miniupnpd

[miniupnp]

I fixed https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/netfilter_nft/README.md

I think I can close this issue now.