[PATCH net 0/3] Netfilter fixes for net

[PATCH net 0/3] Netfilter fixes for net

[Pablo Neira Ayuso]

Hi,

The following patchset contains Netfilter fixes for net coming late
in the 5.17-rc process:

1) Revert port remap to mitigate shadowing service ports, this is causing
   problems in existing setups and this mitigation can be achieved with
   explicit ruleset, eg.

	... tcp sport < 16386 tcp dport >= 32768 masquerade random

  This patches provided a built-in policy similar to the one described above.

2) Disable register tracking infrastructure in nf_tables. Florian reported
   two issues:

   - Existing expressions with no implemented .reduce interface
     that causes data-store on register should cancel the tracking.
   - Register clobbering might be possible storing data on registers that
     are larger than 32-bits.

   This might lead to generating incorrect ruleset bytecode. These two
   issues are scheduled to be addressed in the next release cycle.

Please, pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git

Thanks.

----------------------------------------------------------------

The following changes since commit f8e9bd34cedd89b93b1167aa32ab8ecd6c2ccf4a:

  Merge branch 'smc-fix' (2022-03-03 10:34:18 +0000)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git HEAD

for you to fetch changes up to ed5f85d4229010235eab1e3d9acf6970d9304963:

  netfilter: nf_tables: disable register tracking (2022-03-12 16:07:38 +0100)

----------------------------------------------------------------
Florian Westphal (2):
      Revert "netfilter: nat: force port remap to prevent shadowing well-known ports"
      Revert "netfilter: conntrack: tag conntracks picked up in local out hook"

Pablo Neira Ayuso (1):
      netfilter: nf_tables: disable register tracking

 include/net/netfilter/nf_conntrack.h         |  1 -
 net/netfilter/nf_conntrack_core.c            |  3 --
 net/netfilter/nf_nat_core.c                  | 43 ++--------------------------
 net/netfilter/nf_tables_api.c                |  9 ++++--
 tools/testing/selftests/netfilter/nft_nat.sh |  5 ++--
 5 files changed, 12 insertions(+), 49 deletions(-)

[PATCH net 1/3] Revert "netfilter: nat: force port remap to prevent shadowing well-known ports"

[Pablo Neira Ayuso]

From: Florian Westphal <fw@strlen.de>

This reverts commit 878aed8db324bec64f3c3f956e64d5ae7375a5de.

This change breaks existing setups where conntrack is used with
asymmetric paths.

In these cases, the NAT transformation occurs on the syn-ack instead of
the syn:

1. SYN    x:12345 -> y -> 443 // sent by initiator, receiverd by responder
2. SYNACK y:443 -> x:12345 // First packet seen by conntrack, as sent by responder
3. tuple_force_port_remap() gets called, sees:
  'tcp from 443 to port 12345 NAT' -> pick a new source port, inititor receives
4. SYNACK y:$RANDOM -> x:12345   // connection is never established

While its possible to avoid the breakage with NOTRACK rules, a kernel
update should not break working setups.

An alternative to the revert is to augment conntrack to tag
mid-stream connections plus more code in the nat core to skip NAT
for such connections, however, this leads to more interaction/integration
between conntrack and NAT.

Therefore, revert, users will need to add explicit nat rules to avoid
port shadowing.

Link: https://lore.kernel.org/netfilter-devel/20220302105908.GA5852@breakpoint.cc/#R
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2051413
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_nat_core.c                  | 43 ++------------------
 tools/testing/selftests/netfilter/nft_nat.sh |  5 +--
 2 files changed, 5 insertions(+), 43 deletions(-)

diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 2d06a66899b2..ffcf6529afc3 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -494,38 +494,6 @@ static void nf_nat_l4proto_unique_tuple(struct nf_conntrack_tuple *tuple,
 	goto another_round;
 }
 
-static bool tuple_force_port_remap(const struct nf_conntrack_tuple *tuple)
-{
-	u16 sp, dp;
-
-	switch (tuple->dst.protonum) {
-	case IPPROTO_TCP:
-		sp = ntohs(tuple->src.u.tcp.port);
-		dp = ntohs(tuple->dst.u.tcp.port);
-		break;
-	case IPPROTO_UDP:
-	case IPPROTO_UDPLITE:
-		sp = ntohs(tuple->src.u.udp.port);
-		dp = ntohs(tuple->dst.u.udp.port);
-		break;
-	default:
-		return false;
-	}
-
-	/* IANA: System port range: 1-1023,
-	 *         user port range: 1024-49151,
-	 *      private port range: 49152-65535.
-	 *
-	 * Linux default ephemeral port range is 32768-60999.
-	 *
-	 * Enforce port remapping if sport is significantly lower
-	 * than dport to prevent NAT port shadowing, i.e.
-	 * accidental match of 'new' inbound connection vs.
-	 * existing outbound one.
-	 */
-	return sp < 16384 && dp >= 32768;
-}
-
 /* Manipulate the tuple into the range given. For NF_INET_POST_ROUTING,
  * we change the source to map into the range. For NF_INET_PRE_ROUTING
  * and NF_INET_LOCAL_OUT, we change the destination to map into the
@@ -539,17 +507,11 @@ get_unique_tuple(struct nf_conntrack_tuple *tuple,
 		 struct nf_conn *ct,
 		 enum nf_nat_manip_type maniptype)
 {
-	bool random_port = range->flags & NF_NAT_RANGE_PROTO_RANDOM_ALL;
 	const struct nf_conntrack_zone *zone;
 	struct net *net = nf_ct_net(ct);
 
 	zone = nf_ct_zone(ct);
 
-	if (maniptype == NF_NAT_MANIP_SRC &&
-	    !random_port &&
-	    !ct->local_origin)
-		random_port = tuple_force_port_remap(orig_tuple);
-
 	/* 1) If this srcip/proto/src-proto-part is currently mapped,
 	 * and that same mapping gives a unique tuple within the given
 	 * range, use that.
@@ -558,7 +520,8 @@ 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 && !random_port) {
+	if (maniptype == NF_NAT_MANIP_SRC &&
+	    !(range->flags & NF_NAT_RANGE_PROTO_RANDOM_ALL)) {
 		/* try the original tuple first */
 		if (in_range(orig_tuple, range)) {
 			if (!nf_nat_used_tuple(orig_tuple, ct)) {
@@ -582,7 +545,7 @@ get_unique_tuple(struct nf_conntrack_tuple *tuple,
 	 */
 
 	/* Only bother mapping if it's not already in range and unique */
-	if (!random_port) {
+	if (!(range->flags & NF_NAT_RANGE_PROTO_RANDOM_ALL)) {
 		if (range->flags & NF_NAT_RANGE_PROTO_SPECIFIED) {
 			if (!(range->flags & NF_NAT_RANGE_PROTO_OFFSET) &&
 			    l4proto_in_range(tuple, maniptype,
diff --git a/tools/testing/selftests/netfilter/nft_nat.sh b/tools/testing/selftests/netfilter/nft_nat.sh
index 79fe627b9e81..eb8543b9a5c4 100755
--- a/tools/testing/selftests/netfilter/nft_nat.sh
+++ b/tools/testing/selftests/netfilter/nft_nat.sh
@@ -880,9 +880,8 @@ EOF
 		return $ksft_skip
 	fi
 
-	# test default behaviour. Packet from ns1 to ns0 is not redirected
-	# due to automatic port translation.
-	test_port_shadow "default" "ROUTER"
+	# 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.
-- 
2.30.2

Re: [PATCH net 1/3] Revert "netfilter: nat: force port remap to prevent shadowing well-known ports"

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Pablo Neira Ayuso <pablo@netfilter.org>:

On Sat, 12 Mar 2022 23:03:13 +0100 you wrote:
> From: Florian Westphal <fw@strlen.de>
> 
> This reverts commit 878aed8db324bec64f3c3f956e64d5ae7375a5de.
> 
> This change breaks existing setups where conntrack is used with
> asymmetric paths.
> 
> [...]

Here is the summary with links:
  - [net,1/3] Revert "netfilter: nat: force port remap to prevent shadowing well-known ports"
    https://git.kernel.org/netdev/net/c/a82c25c366b0
  - [net,2/3] Revert "netfilter: conntrack: tag conntracks picked up in local out hook"
    https://git.kernel.org/netdev/net/c/ee0a4dc9f317
  - [net,3/3] netfilter: nf_tables: disable register tracking
    https://git.kernel.org/netdev/net/c/ed5f85d42290

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH net 2/3] Revert "netfilter: conntrack: tag conntracks picked up in local out hook"

[Pablo Neira Ayuso]

From: Florian Westphal <fw@strlen.de>

This was a prerequisite for the ill-fated
"netfilter: nat: force port remap to prevent shadowing well-known ports".

As this has been reverted, this change can be backed out too.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_conntrack.h | 1 -
 net/netfilter/nf_conntrack_core.c    | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index 8731d5bcb47d..b08b70989d2c 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -97,7 +97,6 @@ struct nf_conn {
 	unsigned long status;
 
 	u16		cpu;
-	u16		local_origin:1;
 	possible_net_t ct_net;
 
 #if IS_ENABLED(CONFIG_NF_NAT)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index d6aa5b47031e..bf1e17c678f1 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1748,9 +1748,6 @@ resolve_normal_ct(struct nf_conn *tmpl,
 			return 0;
 		if (IS_ERR(h))
 			return PTR_ERR(h);
-
-		ct = nf_ct_tuplehash_to_ctrack(h);
-		ct->local_origin = state->hook == NF_INET_LOCAL_OUT;
 	}
 	ct = nf_ct_tuplehash_to_ctrack(h);
 
-- 
2.30.2

[PATCH net 3/3] netfilter: nf_tables: disable register tracking

[Pablo Neira Ayuso]

The register tracking infrastructure is incomplete, it might lead to
generating incorrect ruleset bytecode, disable it by now given we are
late in the release process.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index c86748b3873b..d71a33ae39b3 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -8260,6 +8260,12 @@ void nf_tables_trans_destroy_flush_work(void)
 }
 EXPORT_SYMBOL_GPL(nf_tables_trans_destroy_flush_work);
 
+static bool nft_expr_reduce(struct nft_regs_track *track,
+			    const struct nft_expr *expr)
+{
+	return false;
+}
+
 static int nf_tables_commit_chain_prepare(struct net *net, struct nft_chain *chain)
 {
 	const struct nft_expr *expr, *last;
@@ -8307,8 +8313,7 @@ static int nf_tables_commit_chain_prepare(struct net *net, struct nft_chain *cha
 		nft_rule_for_each_expr(expr, last, rule) {
 			track.cur = expr;
 
-			if (expr->ops->reduce &&
-			    expr->ops->reduce(&track, expr)) {
+			if (nft_expr_reduce(&track, expr)) {
 				expr = track.cur;
 				continue;
 			}
-- 
2.30.2

Re: [PATCH net 0/3] Netfilter fixes for net

[Jakub Kicinski]

On Sat, 12 Mar 2022 23:03:12 +0100 Pablo Neira Ayuso wrote:
> 1) Revert port remap to mitigate shadowing service ports, this is causing
>    problems in existing setups and this mitigation can be achieved with
>    explicit ruleset, eg.
> 
> 	... tcp sport < 16386 tcp dport >= 32768 masquerade random
> 
>   This patches provided a built-in policy similar to the one described above.
> 
> 2) Disable register tracking infrastructure in nf_tables. Florian reported
>    two issues:
> 
>    - Existing expressions with no implemented .reduce interface
>      that causes data-store on register should cancel the tracking.
>    - Register clobbering might be possible storing data on registers that
>      are larger than 32-bits.
> 
>    This might lead to generating incorrect ruleset bytecode. These two
>    issues are scheduled to be addressed in the next release cycle.

Minor nit for the future - it'd still be useful to have Fixes tags even
for reverts or current release fixes so that lowly backporters (myself
included) do not have to dig into history to double confirm patches
are not needed in the production kernels we maintain. Thanks!

Re: [PATCH net 0/3] Netfilter fixes for net

[Florian Westphal]

Jakub Kicinski <kuba@kernel.org> wrote:
> Minor nit for the future - it'd still be useful to have Fixes tags even
> for reverts or current release fixes so that lowly backporters (myself
> included) do not have to dig into history to double confirm patches
> are not needed in the production kernels we maintain. Thanks!

Understood, will do so next time.

For the record, the tags would have been:

Fixes: 878aed8db324 ("netfilter: nat: force port remap to prevent shadowing well-known ports")
Fixes: 4a6fbdd801e8 ("netfilter: conntrack: tag conntracks picked up in local out hook")
Fixes: 12e4ecfa244b ("netfilter: nf_tables: add register tracking infrastructure")

... all were merged v5.17-rc1 onwards.

Re: [PATCH net 0/3] Netfilter fixes for net

[Jakub Kicinski]

On Tue, 15 Mar 2022 00:07:19 +0100 Florian Westphal wrote:
> Jakub Kicinski <kuba@kernel.org> wrote:
> > Minor nit for the future - it'd still be useful to have Fixes tags even
> > for reverts or current release fixes so that lowly backporters (myself
> > included) do not have to dig into history to double confirm patches
> > are not needed in the production kernels we maintain. Thanks!  
> 
> Understood, will do so next time.
> 
> For the record, the tags would have been:
> 
> Fixes: 878aed8db324 ("netfilter: nat: force port remap to prevent shadowing well-known ports")
> Fixes: 4a6fbdd801e8 ("netfilter: conntrack: tag conntracks picked up in local out hook")
> Fixes: 12e4ecfa244b ("netfilter: nf_tables: add register tracking infrastructure")
> 
> ... all were merged v5.17-rc1 onwards.

Thanks!