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

[PATCH v3 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 leads to unexpected behaviour
with inverted checks like so:

| 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.e. zero for interface index and an
empty string for interface name.
---
Changes since v1:
- Apply same fix to net/bridge/netfilter/nft_meta_bridge.c as well.

Changes since v2:
- Limit this fix to address only expressions returning an interface
  index or name. As Florian correctly criticized, these changes may be
  problematic as they tend to change nftables' behaviour. Hence fix only
  the cases needed to establish iptables-nft compatibility.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/bridge/netfilter/nft_meta_bridge.c |  6 +-----
 net/netfilter/nft_meta.c               | 16 ++++------------
 2 files changed, 5 insertions(+), 17 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 f1b1d948c07b4..f69afb9ff3cbe 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -60,24 +60,16 @@ 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)
-- 
2.22.0

[PATCH v3 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: [PATCH v3 1/2] net: nf_tables: Make nft_meta expression more robust

[Pablo Neira Ayuso]

On Tue, Jul 23, 2019 at 03:27:52PM +0200, Phil Sutter wrote:
> nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
> situations where required data is missing leads to unexpected behaviour
> with inverted checks like so:
> 
> | 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.e. zero for interface index and an
> empty string for interface name.
> ---
> Changes since v1:
> - Apply same fix to net/bridge/netfilter/nft_meta_bridge.c as well.
> 
> Changes since v2:
> - Limit this fix to address only expressions returning an interface
>   index or name. As Florian correctly criticized, these changes may be
>   problematic as they tend to change nftables' behaviour. Hence fix only
>   the cases needed to establish iptables-nft compatibility.

This leaves things in an inconsistent situation.

What's the concern here? Matching iifgroup/oifgroup from the wrong
packet path?

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Changes since v1:
> > - Apply same fix to net/bridge/netfilter/nft_meta_bridge.c as well.
> > 
> > Changes since v2:
> > - Limit this fix to address only expressions returning an interface
> >   index or name. As Florian correctly criticized, these changes may be
> >   problematic as they tend to change nftables' behaviour. Hence fix only
> >   the cases needed to establish iptables-nft compatibility.
> 
> This leaves things in an inconsistent situation.
> 
> What's the concern here? Matching iifgroup/oifgroup from the wrong
> packet path?

There is no solution for this problem -- thats the core issue.

Purely from a technical p.o.v., we're asking to match oif, but its not
there, so to me, the current behaviour is the right one: break, because
we can't provide the requested information.

On the other hand, this makes things rather un-intuitive when asking for
not eq "foo" -- and not getting a match.

Plus, there is the "compat" angle.

For iifname and ifindex we're safe because "" is not a valid interface
name and 0 is not a valid ifindex.  But for everything else, I'd be
extra careful.

One alternative is to add the "compat" netlink attribute for
iptables-nft and only change behaviour for iptables-nft sake.

But I feel that noone has real evidence in this matter, just hunches.

If in doubt, I would even prefer to keep things as-is and add the
compat attr for meta so we get iptables-nft to behave like
iptables-legacy and keep nft as-is.

This will become relevant once we get a saner behaviour for non-matching
sets (such as a default action):

meta iifname vmap { "foo" : jump foochain, "bar" : accept, default : jump "rest" }

Would you expect the "packet has no incoming interface" to hit the
default action or not?

If we change things now (set ifindex 0 / "" name), I do not think
we can't revert it later.

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

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> If we change things now (set ifindex 0 / "" name), I do not think
> we can't revert it later.

Grrr, should not reply at this hour.  I meant
"I do not think we can revert it later" -- if we change it now
it will be set in stone.

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

[Pablo Neira Ayuso]

On Wed, Jul 24, 2019 at 12:33:06AM +0200, Florian Westphal wrote:
[...]
> If we change things now (set ifindex 0 / "" name), I do not think
> we can't revert it later.

OK, let's start simple as you propose, with iif/oif/iifname/oifname
and we revisit this later on.

Thanks for explaining.

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

[Pablo Neira Ayuso]

On Tue, Jul 23, 2019 at 03:27:52PM +0200, Phil Sutter wrote:
> nft_meta_get_eval()'s tendency to bail out setting NFT_BREAK verdict in
> situations where required data is missing leads to unexpected behaviour
> with inverted checks like so:
> 
> | 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.e. zero for interface index and an
> empty string for interface name.

Applied, thanks.

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

[Pablo Neira Ayuso]

On Tue, Jul 23, 2019 at 03:27:53PM +0200, Phil Sutter wrote:
> The label is used just once and the code it points at is not reused, no
> point in keeping it.

Also applied, thanks.