[nf,v2,1/2] net: nf_tables: Make nft_meta expression more robust
diff mbox series

Message ID 20190719123921.1249-1-phil@nwl.cc
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series
  • [nf,v2,1/2] net: nf_tables: Make nft_meta expression more robust
Related show

Commit Message

Phil Sutter July 19, 2019, 12:39 p.m. UTC
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(-)

Comments

Pablo Neira Ayuso July 19, 2019, 4:35 p.m. UTC | #1
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.
Phil Sutter July 20, 2019, 3:15 p.m. UTC | #2
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
Pablo Neira Ayuso July 22, 2019, 7:53 p.m. UTC | #3
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.
Phil Sutter July 23, 2019, 3:06 p.m. UTC | #4
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
Pablo Neira Ayuso July 23, 2019, 6:38 p.m. UTC | #5
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 :-)

Patch
diff mbox series

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;