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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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 :-)