miniupnp / miniupnp

UPnP IGD implementation
http://miniupnp.free.fr/
BSD 3-Clause "New" or "Revised" License
1.46k stars 456 forks source link

Mappings should be bidirectional #240

Open zrm opened 7 years ago

zrm commented 7 years ago

Mappings created by miniupnpd add a DNAT rule but no SNAT rule.

The SNAT rule is needed so that an application relying on the mapping to avoid need for keepalives doesn't have an outgoing packet translated to some wrong external port, especially with UDP.

Bidirectional mappings are required by the NAT-PMP and PCP RFCs, see e.g. RFC6886 Section 3.9.

Adding the SNAT rule is unfortunately not a comprehensive solution. The SNAT rule prevents connection tracking conflicts from being resolved by translating the external port used by the mapping. Outgoing packets would be dropped and incoming packets would be sent to the wrong internal address/port. So it is also necessary to prevent traffic from any other internal address/port from being translated to the external port of a mapping.

miniupnp commented 7 years ago

what would you suggest ? ie what iptables commands would you write for making a bidirectional mapping ?

zrm commented 7 years ago

You could set aside a range of ports for the default SNAT or MASQUERADE rules, e.g.

iptables -t nat -A POSTROUTING -p udp -o external0 -j MASQUERADE --to-ports 49152-65535 iptables -t nat -A POSTROUTING -p tcp -o external0 -j MASQUERADE --to-ports 49152-65535 iptables -t nat -A POSTROUTING -o external0 -j MASQUERADE

Then don't allow mappings in that port range and when a mapping is created it gets a higher priority rule specifying the external port of the mapping:

iptables -t nat -I POSTROUTING -o external0 -p udp -s 192.168.1.2 --sport 1234 -j MASQUERADE --to-ports 5678

The drawback is that you end up doing port translation for non-mapped outgoing connections even when there is no mapping on that port. A possibility is to split the default MASQUERADE rule over mappings, e.g. for UDP mappings on external ports 2000 and 3000:

iptables -t nat -A POSTROUTING -o external0 -p udp --sport 0:1999 -j MASQUERADE --to-ports 1-1999 iptables -t nat -A POSTROUTING -o external0 -p udp --sport 2000:2999 -j MASQUERADE --to-ports 2001-2999 iptables -t nat -A POSTROUTING -o external0 -p udp --sport 3000:65535 -j MASQUERADE --to-ports 3001-65535

But that is more complicated and requires more rules. You would also have to change the default MASQUERADE rules as above, then dump conntrack, check for conflicts and roll back and choose another external port if there are any. And if the span between two mappings is too small (or zero) you would have to choose some other ports regardless, otherwise it's too likely that the tiny span range could run out of ports for resolving conflicts. Whereas if everything without an explicit mapping is using a range of ports not used for mappings then you don't have to worry about any of that.

Naturally if you get a PCP PEER request, don't assign it the external port of an existing mapping unless the internal address/port are the ones from that mapping.

It feels like there should be a simpler solution than this I know.

miniupnp commented 7 years ago

there is already some support for PCP PEER using SNAT make sure you enable PCP_PEER ( ./genconfig.sh --pcp-peer ) It was added by Peter Tatrai

zrm commented 7 years ago

I have some hope that the new cttimeout code in Linux will eventually allow longer-than-default timeouts for PCP PEER without iptables, but I haven't discovered any way to assign a timeout policy to an already-existing or libnetfilter_conntrack-created conntrack entry. And if you just set the timeout to longer-than-default without a timeout policy it gets reset to the default by the next packet.

The main trouble here is with MAP though. MAP should be great for UDP distributed hashtables and the like, where you have many thousands of peers but it could be hours or days between sending traffic to any given one, because you could use one MAP for all of them and save the gateway from having to keep any long-term state on the thousands of peers. Provided the MAP is bidirectional.

kghost commented 6 years ago

For example, if the router has multiple wan networks, inbound connection and outbound connection may go through different wans.

I suggest miniupnpd should mark the outbound connection, so we can use policy route to direct the outbound connection.