[PATCH nf 0/1.5 RFC] netfilter: nat: source port shadowing

[PATCH nf 0/1.5 RFC] netfilter: nat: source port shadowing

[Florian Westphal]

In brief:

Given
internal_client -- <router:service> -- external_client

The internal client can create NAT entry on router in a way so that
an external client trying to contact router:service is reverse-natted
to internal_client:service instead.

Long version: https://breakpointingbad.com/2021/09/08/Port-Shadows-via-Network-Alchemy.html

First patch extends nft_nat.sh selftest with above scenario plus three
ruleset changes that prevent this (sport-filtering, notrack for
router:service and sport-based port remapping).

I tried to come up with a kernel based solution as well, but those
are not really nice either.

The first option is attached: kernel queries udp with reversed addresses
to see if this maps to a socket.  If so, remap.

Major pain point: adds entanglement to socket layer and will
need extra glue for CONFIG_IPV6=m case.

Second option is similar to the attached patch, but instead of
sk lookup check the porposed new source port versus the
'ip_local_reserved_ports' sysctl.

Would require userspace to set the reserved ports accordingly.

I also had a look at the 'socket' match, but it cannot be used.
It would have to be extended so that the socket lookup is done
on arbirary saddr/daddr/sport/dport combination, or at the very
least we'd need a 'invert' flag.

Existing 'socket' uses the addresses/ports in the packet, but
we'd need the addresses/ports of the hypothetical reply.

Things that do not work:
Change the sport range to only cover the IANA reserved port
range.  It sounds tempting but it sabotages dns resolvers using random
source ports.

Comments welcome.

Florian Westphal (2):
  selftests: nft_nat: add udp hole punch test case
  netfilter: nf_nat: don't allow source ports that shadow local port

 net/netfilter/nf_nat_core.c                  |  41 +++++-
 tools/testing/selftests/netfilter/nft_nat.sh | 145 +++++++++++++++++++
 2 files changed, 183 insertions(+), 3 deletions(-)

-- 
2.32.0

[PATCH nf 1/2] selftests: nft_nat: add udp hole punch test case

[Florian Westphal]

Add a test case that demonstrates port shadowing via UDP.

ns2 sends packet to ns1, from source port used by a udp service on the
router, ns0.  Then, ns1 sends packet to ns0:service, but that ends up getting
forwarded to ns2.

Also add three test cases that demonstrate mitigations:
1. disable use of $port as source from 'unstrusted' origin
2. make the service untracked.  This prevents masquerade entries
   from having any effects.
3. add forced PAT via 'random' mode to translate the "wrong" sport
   into an acceptable range.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tools/testing/selftests/netfilter/nft_nat.sh | 145 +++++++++++++++++++
 1 file changed, 145 insertions(+)

diff --git a/tools/testing/selftests/netfilter/nft_nat.sh b/tools/testing/selftests/netfilter/nft_nat.sh
index d7e07f4c3d7f..da1c1e4b6c86 100755
--- a/tools/testing/selftests/netfilter/nft_nat.sh
+++ b/tools/testing/selftests/netfilter/nft_nat.sh
@@ -741,6 +741,149 @@ EOF
 	return $lret
 }
 
+# test port shadowing.
+# create two listening services, one on router (ns0), one
+# on client (ns2), which is masqueraded from ns1 point of view.
+# ns2 sends udp packet coming from service port to ns1, on a highport.
+# Later, if n1 uses same highport to connect to ns0:service, packet
+# might be port-forwarded to ns2 instead.
+
+# second argument tells if we expect the 'fake-entry' to take effect
+# (CLIENT) or not (ROUTER).
+test_port_shadow()
+{
+	local test=$1
+	local expect=$2
+	local daddrc="10.0.1.99"
+	local daddrs="10.0.1.1"
+	local result=""
+	local logmsg=""
+
+	echo ROUTER | ip netns exec "$ns0" nc -w 5 -u -l -p 1405 >/dev/null 2>&1 &
+	nc_r=$!
+
+	echo CLIENT | ip netns exec "$ns2" nc -w 5 -u -l -p 1405 >/dev/null 2>&1 &
+	nc_c=$!
+
+	# make shadow entry, from client (ns2), going to (ns1), port 41404, sport 1405.
+	echo "fake-entry" | ip netns exec "$ns2" nc -w 1 -p 1405 -u "$daddrc" 41404 > /dev/null
+
+	# ns1 tries to connect to ns0:1405.  With default settings this should connect
+	# to client, it matches the conntrack entry created above.
+
+	result=$(echo "" | ip netns exec "$ns1" nc -w 1 -p 41404 -u "$daddrs" 1405)
+
+	if [ "$result" = "$expect" ] ;then
+		echo "PASS: portshadow test $test: got reply from ${expect}${logmsg}"
+	else
+		echo "ERROR: portshadow test $test: got reply from \"$result\", not $expect as intended"
+		ret=1
+	fi
+
+	kill $nc_r $nc_c 2>/dev/null
+
+	# flush udp entries for next test round, if any
+	ip netns exec "$ns0" conntrack -F >/dev/null 2>&1
+}
+
+# This prevents port shadow of router service via packet filter,
+# packets claiming to originate from service port from internal
+# network are dropped.
+test_port_shadow_filter()
+{
+	local family=$1
+
+ip netns exec "$ns0" nft -f /dev/stdin <<EOF
+table $family filter {
+	chain forward {
+		type filter hook forward priority 0; policy accept;
+		meta iif veth1 udp sport 1405 drop
+	}
+}
+EOF
+	test_port_shadow "port-filter" "ROUTER"
+
+	ip netns exec "$ns0" nft delete table $family filter
+}
+
+# This prevents port shadow of router service via notrack.
+test_port_shadow_notrack()
+{
+	local family=$1
+
+ip netns exec "$ns0" nft -f /dev/stdin <<EOF
+table $family raw {
+	chain prerouting {
+		type filter hook prerouting priority -300; policy accept;
+		meta iif veth0 udp dport 1405 notrack
+		udp dport 1405 notrack
+	}
+	chain output {
+		type filter hook output priority -300; policy accept;
+		udp sport 1405 notrack
+	}
+}
+EOF
+	test_port_shadow "port-notrack" "ROUTER"
+
+	ip netns exec "$ns0" nft delete table $family raw
+}
+
+# This prevents port shadow of router service via sport remap.
+test_port_shadow_pat()
+{
+	local family=$1
+
+ip netns exec "$ns0" nft -f /dev/stdin <<EOF
+table $family pat {
+	chain postrouting {
+		type nat hook postrouting priority -1; policy accept;
+		meta iif veth1 udp sport <= 1405 masquerade to : 1406-65535 random
+	}
+}
+EOF
+	test_port_shadow "pat" "ROUTER"
+
+	ip netns exec "$ns0" nft delete table $family pat
+}
+
+test_port_shadowing()
+{
+	local family="ip"
+
+	ip netns exec "$ns0" sysctl net.ipv4.conf.veth0.forwarding=1 > /dev/null
+	ip netns exec "$ns0" sysctl net.ipv4.conf.veth1.forwarding=1 > /dev/null
+
+	ip netns exec "$ns0" nft -f /dev/stdin <<EOF
+table $family nat {
+	chain postrouting {
+		type nat hook postrouting priority 0; policy accept;
+		meta oif veth0 masquerade
+	}
+}
+EOF
+	if [ $? -ne 0 ]; then
+		echo "SKIP: Could not add add $family masquerade hook"
+		return $ksft_skip
+	fi
+
+	# test default behaviour. Packet from ns1 to ns0 is redirected to ns2.
+	test_port_shadow "default" "CLIENT"
+
+	# test packet filter based mitigation: prevent forwarding of
+	# packets claiming to come from the service port.
+	test_port_shadow_filter "$family"
+
+	# test conntrack based mitigation: connections going or coming
+	# from router:service bypass connection tracking.
+	test_port_shadow_notrack "$family"
+
+	# test nat based mitigation: fowarded packets coming from service port
+	# are masqueraded with random highport.
+	test_port_shadow_pat "$family"
+
+	ip netns exec "$ns0" nft delete table $family nat
+}
 
 # ip netns exec "$ns0" ping -c 1 -q 10.0.$i.99
 for i in 0 1 2; do
@@ -861,6 +1004,8 @@ reset_counters
 $test_inet_nat && test_redirect inet
 $test_inet_nat && test_redirect6 inet
 
+test_port_shadowing
+
 if [ $ret -ne 0 ];then
 	echo -n "FAIL: "
 	nft --version
-- 
2.32.0

Re: [PATCH nf 1/2] selftests: nft_nat: add udp hole punch test case

[Pablo Neira Ayuso]

On Thu, Sep 23, 2021 at 03:12:42PM +0200, Florian Westphal wrote:
> Add a test case that demonstrates port shadowing via UDP.
> 
> ns2 sends packet to ns1, from source port used by a udp service on the
> router, ns0.  Then, ns1 sends packet to ns0:service, but that ends up getting
> forwarded to ns2.
> 
> Also add three test cases that demonstrate mitigations:
> 1. disable use of $port as source from 'unstrusted' origin
> 2. make the service untracked.  This prevents masquerade entries
>    from having any effects.
> 3. add forced PAT via 'random' mode to translate the "wrong" sport
>    into an acceptable range.

Applied, thanks

[PATCH RFC 2/2] netfilter: nf_nat: don't allow source ports that shadow local port

[Florian Westphal]

PoC, incomplete -- ipv4 udp only.

Ipv6 support needs help from ipv6 indirection infra.

Also lacks direction support: the check should only be done
for nf_conn objects created by externally generated packets.

Don't apply.
---
 net/netfilter/nf_nat_core.c | 41 ++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 273117683922..843b639200f8 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -24,6 +24,7 @@
 #include <net/netfilter/nf_nat.h>
 #include <net/netfilter/nf_nat_helper.h>
 #include <uapi/linux/netfilter/nf_nat.h>
+#include <net/udp.h>
 
 #include "nf_internals.h"
 
@@ -372,6 +373,30 @@ find_best_ips_proto(const struct nf_conntrack_zone *zone,
 	}
 }
 
+static bool is_port_shadow(struct net *net, const struct nf_conntrack_tuple *tuple)
+{
+	const struct sock *sk;
+	__be32 saddr, daddr;
+	__be16 sport, dport;
+
+	if (tuple->src.l3num != NFPROTO_IPV4 ||
+	    tuple->dst.protonum != IPPROTO_UDP)
+		return false;
+
+	saddr = tuple->dst.u3.ip;
+	daddr = tuple->src.u3.ip;
+	sport = tuple->dst.u.udp.port;
+	dport = tuple->src.u.udp.port;
+
+	sk = __udp4_lib_lookup(net, saddr, sport, daddr, dport, 0, 0, &udp_table, NULL);
+
+	/* if this returns a socket, then replies might be reverse-natted and
+	 * forwarded instead of being delivered to the local socket.
+	 */
+
+	return sk != NULL;
+}
+
 /* Alter the per-proto part of the tuple (depending on maniptype), to
  * give a unique tuple in the given range if possible.
  *
@@ -483,6 +508,10 @@ static void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
 another_round:
 	for (i = 0; i < attempts; i++, off++) {
 		*keyptr = htons(min + off % range_size);
+
+		if (maniptype == NF_NAT_MANIP_SRC && is_port_shadow(nf_ct_net(ct), tuple))
+			continue;
+
 		if (!nf_nat_used_tuple(tuple, ct))
 			return;
 	}
@@ -507,6 +536,7 @@ get_unique_tuple(struct nf_conntrack_tuple *tuple,
 		 struct nf_conn *ct,
 		 enum nf_nat_manip_type maniptype)
 {
+	bool force_random = range->flags & NF_NAT_RANGE_PROTO_RANDOM_ALL;
 	const struct nf_conntrack_zone *zone;
 	struct net *net = nf_ct_net(ct);
 
@@ -520,8 +550,12 @@ get_unique_tuple(struct nf_conntrack_tuple *tuple,
 	 * So far, we don't do local source mappings, so multiple
 	 * manips not an issue.
 	 */
-	if (maniptype == NF_NAT_MANIP_SRC &&
-	    !(range->flags & NF_NAT_RANGE_PROTO_RANDOM_ALL)) {
+	if (maniptype == NF_NAT_MANIP_SRC && !force_random) {
+		if (is_port_shadow(nf_ct_net(ct), orig_tuple)) {
+			force_random = true;
+			goto find_best_ips;
+		}
+
 		/* try the original tuple first */
 		if (in_range(orig_tuple, range)) {
 			if (!nf_nat_used_tuple(orig_tuple, ct)) {
@@ -536,6 +570,7 @@ get_unique_tuple(struct nf_conntrack_tuple *tuple,
 		}
 	}
 
+find_best_ips:
 	/* 2) Select the least-used IP/proto combination in the given range */
 	*tuple = *orig_tuple;
 	find_best_ips_proto(zone, tuple, range, ct, maniptype);
@@ -545,7 +580,7 @@ get_unique_tuple(struct nf_conntrack_tuple *tuple,
 	 */
 
 	/* Only bother mapping if it's not already in range and unique */
-	if (!(range->flags & NF_NAT_RANGE_PROTO_RANDOM_ALL)) {
+	if (!force_random) {
 		if (range->flags & NF_NAT_RANGE_PROTO_SPECIFIED) {
 			if (!(range->flags & NF_NAT_RANGE_PROTO_OFFSET) &&
 			    l4proto_in_range(tuple, maniptype,
-- 
2.32.0

Re: [PATCH RFC 2/2] netfilter: nf_nat: don't allow source ports that shadow local port

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> PoC, incomplete -- ipv4 udp only.
> 
> Ipv6 support needs help from ipv6 indirection infra.
> 
> Also lacks direction support: the check should only be done
> for nf_conn objects created by externally generated packets.
 
Alternate fix idea:

1. store skb->skb_iif in nf_conn.

This means locally vs. remote-generated nf_conn can be identified
via ct->skb_iff != 0.

2. For "remote" case, force following behaviour:
   check that sport > dport and sport > 1024.

OTOH, this isn't transparent to users and might cause issues
with very very old "credential passing" applications that insist
on using privileged port range (< 1024) :-/

Re: [PATCH RFC 2/2] netfilter: nf_nat: don't allow source ports that shadow local port

[Pablo Neira Ayuso]

On Fri, Oct 01, 2021 at 03:21:28PM +0200, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > PoC, incomplete -- ipv4 udp only.
> > 
> > Ipv6 support needs help from ipv6 indirection infra.
> > 
> > Also lacks direction support: the check should only be done
> > for nf_conn objects created by externally generated packets.
>  
> Alternate fix idea:
> 
> 1. store skb->skb_iif in nf_conn.
> 
> This means locally vs. remote-generated nf_conn can be identified
> via ct->skb_iff != 0.
> 
> 2. For "remote" case, force following behaviour:
>    check that sport > dport and sport > 1024.
> 
> OTOH, this isn't transparent to users and might cause issues
> with very very old "credential passing" applications that insist
> on using privileged port range (< 1024) :-/

Can't this be just expressed through ruleset? I mean, conditionally
masquerade depending on whether the packet is locally generated or
not, for remove for sport > 1024 range.

Re: [PATCH RFC 2/2] netfilter: nf_nat: don't allow source ports that shadow local port

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Oct 01, 2021 at 03:21:28PM +0200, Florian Westphal wrote:
> > Alternate fix idea:
> > 
> > 1. store skb->skb_iif in nf_conn.
> > 
> > This means locally vs. remote-generated nf_conn can be identified
> > via ct->skb_iff != 0.
> > 
> > 2. For "remote" case, force following behaviour:
> >    check that sport > dport and sport > 1024.
> > 
> > OTOH, this isn't transparent to users and might cause issues
> > with very very old "credential passing" applications that insist
> > on using privileged port range (< 1024) :-/
> 
> Can't this be just expressed through ruleset? I mean, conditionally
> masquerade depending on whether the packet is locally generated or
> not, for remove for sport > 1024 range.

Yes, see patch #1, it demos a couple of ruleset based fixes/mitigations
for this problem.