[nf PATCH v2 1/2] net: nf_tables: Make nft_meta expression more robust

[nf PATCH v2 1/2] net: nf_tables: Make nft_meta expression more robust

[Phil Sutter]

nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
situations where required data is missing breaks inverted checks
like e.g.:

| meta iifname != eth0 accept

This rule will never match if there is no input interface (or it is not
known) which is not intuitive and, what's worse, breaks consistency of
iptables-nft with iptables-legacy.

Fix this by falling back to placing a value in dreg which never matches
(avoiding accidental matches):

{I,O}IF:
	Use invalid ifindex value zero.

{BRI_,}{I,O}IFNAME, {I,O}IFKIND:
	Use an empty string which is neither a valid interface name nor
	kind.

{I,O}IFTYPE:
	Use ARPHRD_VOID (0xFFFF).

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Apply same fix to net/bridge/netfilter/nft_meta_bridge.c as well.
---
 net/bridge/netfilter/nft_meta_bridge.c |  6 +---
 net/netfilter/nft_meta.c               | 45 +++++++++++---------------
 2 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/net/bridge/netfilter/nft_meta_bridge.c b/net/bridge/netfilter/nft_meta_bridge.c
index bed66f536b345..a98dec2cf0cfd 100644
--- a/net/bridge/netfilter/nft_meta_bridge.c
+++ b/net/bridge/netfilter/nft_meta_bridge.c
@@ -30,13 +30,9 @@ static void nft_meta_bridge_get_eval(const struct nft_expr *expr,
 	switch (priv->key) {
 	case NFT_META_BRI_IIFNAME:
 		br_dev = nft_meta_get_bridge(in);
-		if (!br_dev)
-			goto err;
 		break;
 	case NFT_META_BRI_OIFNAME:
 		br_dev = nft_meta_get_bridge(out);
-		if (!br_dev)
-			goto err;
 		break;
 	case NFT_META_BRI_IIFPVID: {
 		u16 p_pvid;
@@ -64,7 +60,7 @@ static void nft_meta_bridge_get_eval(const struct nft_expr *expr,
 		goto out;
 	}
 
-	strncpy((char *)dest, br_dev->name, IFNAMSIZ);
+	strncpy((char *)dest, br_dev ? br_dev->name : "", IFNAMSIZ);
 	return;
 out:
 	return nft_meta_get_eval(expr, regs, pkt);
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 76866f77e3435..ee3b54692cc7e 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -11,6 +11,7 @@
 #include <linux/netlink.h>
 #include <linux/netfilter.h>
 #include <linux/netfilter/nf_tables.h>
+#include <linux/if_arp.h>
 #include <linux/in.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
@@ -60,34 +61,22 @@ void nft_meta_get_eval(const struct nft_expr *expr,
 		*dest = skb->mark;
 		break;
 	case NFT_META_IIF:
-		if (in == NULL)
-			goto err;
-		*dest = in->ifindex;
+		*dest = in ? in->ifindex : 0;
 		break;
 	case NFT_META_OIF:
-		if (out == NULL)
-			goto err;
-		*dest = out->ifindex;
+		*dest = out ? out->ifindex : 0;
 		break;
 	case NFT_META_IIFNAME:
-		if (in == NULL)
-			goto err;
-		strncpy((char *)dest, in->name, IFNAMSIZ);
+		strncpy((char *)dest, in ? in->name : "", IFNAMSIZ);
 		break;
 	case NFT_META_OIFNAME:
-		if (out == NULL)
-			goto err;
-		strncpy((char *)dest, out->name, IFNAMSIZ);
+		strncpy((char *)dest, out ? out->name : "", IFNAMSIZ);
 		break;
 	case NFT_META_IIFTYPE:
-		if (in == NULL)
-			goto err;
-		nft_reg_store16(dest, in->type);
+		nft_reg_store16(dest, in ? in->type : ARPHRD_VOID);
 		break;
 	case NFT_META_OIFTYPE:
-		if (out == NULL)
-			goto err;
-		nft_reg_store16(dest, out->type);
+		nft_reg_store16(dest, out ? out->type : ARPHRD_VOID);
 		break;
 	case NFT_META_SKUID:
 		sk = skb_to_full_sk(skb);
@@ -216,16 +205,20 @@ void nft_meta_get_eval(const struct nft_expr *expr,
 		nft_reg_store8(dest, secpath_exists(skb));
 		break;
 #endif
-	case NFT_META_IIFKIND:
-		if (in == NULL || in->rtnl_link_ops == NULL)
-			goto err;
-		strncpy((char *)dest, in->rtnl_link_ops->kind, IFNAMSIZ);
+	case NFT_META_IIFKIND: {
+		const struct rtnl_link_ops *rl_ops =
+			in ? in->rtnl_link_ops : NULL;
+
+		strncpy((char *)dest, rl_ops ? rl_ops->kind : "", IFNAMSIZ);
 		break;
-	case NFT_META_OIFKIND:
-		if (out == NULL || out->rtnl_link_ops == NULL)
-			goto err;
-		strncpy((char *)dest, out->rtnl_link_ops->kind, IFNAMSIZ);
+	}
+	case NFT_META_OIFKIND: {
+		const struct rtnl_link_ops *rl_ops =
+			out ? out->rtnl_link_ops : NULL;
+
+		strncpy((char *)dest, rl_ops ? rl_ops->kind : "", IFNAMSIZ);
 		break;
+	}
 	default:
 		WARN_ON(1);
 		goto err;
-- 
2.22.0

[PATCH 2/2] net: netfilter: nft_meta_bridge: Eliminate 'out' label

[Phil Sutter]

The label is used just once and the code it points at is not reused, no
point in keeping it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/bridge/netfilter/nft_meta_bridge.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/bridge/netfilter/nft_meta_bridge.c b/net/bridge/netfilter/nft_meta_bridge.c
index a98dec2cf0cfd..1804e867f7151 100644
--- a/net/bridge/netfilter/nft_meta_bridge.c
+++ b/net/bridge/netfilter/nft_meta_bridge.c
@@ -57,13 +57,11 @@ static void nft_meta_bridge_get_eval(const struct nft_expr *expr,
 		return;
 	}
 	default:
-		goto out;
+		return nft_meta_get_eval(expr, regs, pkt);
 	}
 
 	strncpy((char *)dest, br_dev ? br_dev->name : "", IFNAMSIZ);
 	return;
-out:
-	return nft_meta_get_eval(expr, regs, pkt);
 err:
 	regs->verdict.code = NFT_BREAK;
 }
-- 
2.22.0

Re: [nf PATCH v2 1/2] net: nf_tables: Make nft_meta expression more robust

[Pablo Neira Ayuso]

On Fri, Jul 19, 2019 at 02:39:20PM +0200, Phil Sutter wrote:
> nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
> situations where required data is missing breaks inverted checks
> like e.g.:
> 
> | meta iifname != eth0 accept
> 
> This rule will never match if there is no input interface (or it is not
> known) which is not intuitive and, what's worse, breaks consistency of
> iptables-nft with iptables-legacy.
> 
> Fix this by falling back to placing a value in dreg which never matches
> (avoiding accidental matches):
> 
> {I,O}IF:
> 	Use invalid ifindex value zero.
> 
> {BRI_,}{I,O}IFNAME, {I,O}IFKIND:
> 	Use an empty string which is neither a valid interface name nor
> 	kind.
> 
> {I,O}IFTYPE:
> 	Use ARPHRD_VOID (0xFFFF).

What could it be done with?

NFT_META_BRI_IIFPVID
NFT_META_BRI_IIFPVPROTO

Those will still not work for

        meta ibrpvid != 40

if interface is not available.

For VPROTO probably it's possible. I don't have a solution for
IIFPVID.

Re: [nf PATCH v2 1/2] net: nf_tables: Make nft_meta expression more robust

[Phil Sutter]

Hi,

On Fri, Jul 19, 2019 at 06:35:21PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Jul 19, 2019 at 02:39:20PM +0200, Phil Sutter wrote:
> > nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
> > situations where required data is missing breaks inverted checks
> > like e.g.:
> > 
> > | meta iifname != eth0 accept
> > 
> > This rule will never match if there is no input interface (or it is not
> > known) which is not intuitive and, what's worse, breaks consistency of
> > iptables-nft with iptables-legacy.
> > 
> > Fix this by falling back to placing a value in dreg which never matches
> > (avoiding accidental matches):
> > 
> > {I,O}IF:
> > 	Use invalid ifindex value zero.
> > 
> > {BRI_,}{I,O}IFNAME, {I,O}IFKIND:
> > 	Use an empty string which is neither a valid interface name nor
> > 	kind.
> > 
> > {I,O}IFTYPE:
> > 	Use ARPHRD_VOID (0xFFFF).
> 
> What could it be done with?
> 
> NFT_META_BRI_IIFPVID
> NFT_META_BRI_IIFPVPROTO
> 
> Those will still not work for
> 
>         meta ibrpvid != 40
> 
> if interface is not available.
> 
> For VPROTO probably it's possible. I don't have a solution for
> IIFPVID.

VLAN IDs 0 and 4095 are reserved, we could use those. I refrained from
changing bridge VLAN matches because of IIFPVPROTO, no idea if there's
an illegal value we could use for that. If you have an idea, I'm all for
it. :)

Thanks, Phil

Re: [nf PATCH v2 1/2] net: nf_tables: Make nft_meta expression more robust

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 2221 bytes --]

On Sat, Jul 20, 2019 at 05:15:02PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Fri, Jul 19, 2019 at 06:35:21PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Jul 19, 2019 at 02:39:20PM +0200, Phil Sutter wrote:
> > > nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
> > > situations where required data is missing breaks inverted checks
> > > like e.g.:
> > > 
> > > | meta iifname != eth0 accept
> > > 
> > > This rule will never match if there is no input interface (or it is not
> > > known) which is not intuitive and, what's worse, breaks consistency of
> > > iptables-nft with iptables-legacy.
> > > 
> > > Fix this by falling back to placing a value in dreg which never matches
> > > (avoiding accidental matches):
> > > 
> > > {I,O}IF:
> > > 	Use invalid ifindex value zero.
> > > 
> > > {BRI_,}{I,O}IFNAME, {I,O}IFKIND:
> > > 	Use an empty string which is neither a valid interface name nor
> > > 	kind.
> > > 
> > > {I,O}IFTYPE:
> > > 	Use ARPHRD_VOID (0xFFFF).
> > 
> > What could it be done with?
> > 
> > NFT_META_BRI_IIFPVID
> > NFT_META_BRI_IIFPVPROTO
> > 
> > Those will still not work for
> > 
> >         meta ibrpvid != 40
> > 
> > if interface is not available.
> > 
> > For VPROTO probably it's possible. I don't have a solution for
> > IIFPVID.
> 
> VLAN IDs 0 and 4095 are reserved, we could use those. I refrained from
> changing bridge VLAN matches because of IIFPVPROTO, no idea if there's
> an illegal value we could use for that. If you have an idea, I'm all for
> it. :)

I think we can add something like:

        NFT_META_BRI_IIFVLAN

just to check for br_vlan_enabled(), from userspace we can check for
exists/missing as a boolean, so we don't have to worry on assuming an
unused value for things like this. This can be added in the next
release cycle.

Regarding IIFTYPE / OIFTYPE, if there is no in-trees interfaces using
"", I think we are fine, probably send a patch to propose to disallow
this to net.

These ones are missing:

        NFT_META_IIFGROUP
        NFT_META_OIFGROUP

For these two, the default group (0) should be fine since every
interface is falling under this category by default.

I can squash this small patch to this one and push it one.

Thanks.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 592 bytes --]

diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index d8d04ec1face..e2c6857c0f68 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -177,14 +177,10 @@ void nft_meta_get_eval(const struct nft_expr *expr,
 		*dest = raw_smp_processor_id();
 		break;
 	case NFT_META_IIFGROUP:
-		if (in == NULL)
-			goto err;
-		*dest = in->group;
+		*dest = in ? in->group : 0;
 		break;
 	case NFT_META_OIFGROUP:
-		if (out == NULL)
-			goto err;
-		*dest = out->group;
+		*dest = out ? out->group : 0;
 		break;
 #ifdef CONFIG_CGROUP_NET_CLASSID
 	case NFT_META_CGROUP:

Re: [nf PATCH v2 1/2] net: nf_tables: Make nft_meta expression more robust

[Phil Sutter]

Hi Pablo,

On Mon, Jul 22, 2019 at 09:53:21PM +0200, Pablo Neira Ayuso wrote:
> On Sat, Jul 20, 2019 at 05:15:02PM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Fri, Jul 19, 2019 at 06:35:21PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Jul 19, 2019 at 02:39:20PM +0200, Phil Sutter wrote:
> > > > nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
> > > > situations where required data is missing breaks inverted checks
> > > > like e.g.:
> > > > 
> > > > | meta iifname != eth0 accept
> > > > 
> > > > This rule will never match if there is no input interface (or it is not
> > > > known) which is not intuitive and, what's worse, breaks consistency of
> > > > iptables-nft with iptables-legacy.
> > > > 
> > > > Fix this by falling back to placing a value in dreg which never matches
> > > > (avoiding accidental matches):
> > > > 
> > > > {I,O}IF:
> > > > 	Use invalid ifindex value zero.
> > > > 
> > > > {BRI_,}{I,O}IFNAME, {I,O}IFKIND:
> > > > 	Use an empty string which is neither a valid interface name nor
> > > > 	kind.
> > > > 
> > > > {I,O}IFTYPE:
> > > > 	Use ARPHRD_VOID (0xFFFF).
> > > 
> > > What could it be done with?
> > > 
> > > NFT_META_BRI_IIFPVID
> > > NFT_META_BRI_IIFPVPROTO
> > > 
> > > Those will still not work for
> > > 
> > >         meta ibrpvid != 40
> > > 
> > > if interface is not available.
> > > 
> > > For VPROTO probably it's possible. I don't have a solution for
> > > IIFPVID.
> > 
> > VLAN IDs 0 and 4095 are reserved, we could use those. I refrained from
> > changing bridge VLAN matches because of IIFPVPROTO, no idea if there's
> > an illegal value we could use for that. If you have an idea, I'm all for
> > it. :)
> 
> I think we can add something like:
> 
>         NFT_META_BRI_IIFVLAN
> 
> just to check for br_vlan_enabled(), from userspace we can check for
> exists/missing as a boolean, so we don't have to worry on assuming an
> unused value for things like this. This can be added in the next
> release cycle.

Adding existence checks where missing is indeed a good idea, but doesn't
quite solve the problem we're facing here. :)

[...]
> These ones are missing:
> 
>         NFT_META_IIFGROUP
>         NFT_META_OIFGROUP
> 
> For these two, the default group (0) should be fine since every
> interface is falling under this category by default.
> 
> I can squash this small patch to this one and push it one.

My problem with these "sane defaults" is that we may cause inconsistent
behaviour in rulesets: In prerouting, 'meta oifgroup 0' will match no
matter which interface the packet will be routed to. Yes, prerouting
implies there is no output interface (yet), but I consider this an
implementation detail and there will likely be cases where it is not as
easy to spot why something can't work.

Cheers, Phil

Re: [nf PATCH v2 1/2] net: nf_tables: Make nft_meta expression more robust

[Pablo Neira Ayuso]

On Tue, Jul 23, 2019 at 05:06:44PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Mon, Jul 22, 2019 at 09:53:21PM +0200, Pablo Neira Ayuso wrote:
> > On Sat, Jul 20, 2019 at 05:15:02PM +0200, Phil Sutter wrote:
> > > Hi,
> > > 
> > > On Fri, Jul 19, 2019 at 06:35:21PM +0200, Pablo Neira Ayuso wrote:
> > > > On Fri, Jul 19, 2019 at 02:39:20PM +0200, Phil Sutter wrote:
> > > > > nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
> > > > > situations where required data is missing breaks inverted checks
> > > > > like e.g.:
> > > > > 
> > > > > | meta iifname != eth0 accept
> > > > > 
> > > > > This rule will never match if there is no input interface (or it is not
> > > > > known) which is not intuitive and, what's worse, breaks consistency of
> > > > > iptables-nft with iptables-legacy.
> > > > > 
> > > > > Fix this by falling back to placing a value in dreg which never matches
> > > > > (avoiding accidental matches):
> > > > > 
> > > > > {I,O}IF:
> > > > > 	Use invalid ifindex value zero.
> > > > > 
> > > > > {BRI_,}{I,O}IFNAME, {I,O}IFKIND:
> > > > > 	Use an empty string which is neither a valid interface name nor
> > > > > 	kind.
> > > > > 
> > > > > {I,O}IFTYPE:
> > > > > 	Use ARPHRD_VOID (0xFFFF).
> > > > 
> > > > What could it be done with?
> > > > 
> > > > NFT_META_BRI_IIFPVID
> > > > NFT_META_BRI_IIFPVPROTO
> > > > 
> > > > Those will still not work for
> > > > 
> > > >         meta ibrpvid != 40
> > > > 
> > > > if interface is not available.
> > > > 
> > > > For VPROTO probably it's possible. I don't have a solution for
> > > > IIFPVID.
> > > 
> > > VLAN IDs 0 and 4095 are reserved, we could use those. I refrained from
> > > changing bridge VLAN matches because of IIFPVPROTO, no idea if there's
> > > an illegal value we could use for that. If you have an idea, I'm all for
> > > it. :)
> > 
> > I think we can add something like:
> > 
> >         NFT_META_BRI_IIFVLAN
> > 
> > just to check for br_vlan_enabled(), from userspace we can check for
> > exists/missing as a boolean, so we don't have to worry on assuming an
> > unused value for things like this. This can be added in the next
> > release cycle.
> 
> Adding existence checks where missing is indeed a good idea, but doesn't
> quite solve the problem we're facing here. :)

I'm not proposing to use this approach for _every key_. For this one
specifically I think it's meaningful to have a NFT_META_BRI_IIFVLAN
since vlan is an optional configuration.

> [...]
> > These ones are missing:
> > 
> >         NFT_META_IIFGROUP
> >         NFT_META_OIFGROUP
> > 
> > For these two, the default group (0) should be fine since every
> > interface is falling under this category by default.
> > 
> > I can squash this small patch to this one and push it one.
> 
> My problem with these "sane defaults" is that we may cause inconsistent
> behaviour in rulesets: In prerouting, 'meta oifgroup 0' will match no
> matter which interface the packet will be routed to. Yes, prerouting
> implies there is no output interface (yet), but I consider this an
> implementation detail and there will likely be cases where it is not as
> easy to spot why something can't work.

I think disallowing 'meta oifgroup 0' from prerouting (and the input
path in general) is fine from the control plane configuration, but you
know Florian disagrees with this. Probably this can be left as is for
IIFNAME and OIFNAME, which as used by iptables-nft for compatibility
with legacy. For new keys, check for invalid configuration I would
argue that is fine.

But let's wait for Florian to say what he thinks :-)