Re: [PATCH v2] netfilter: nft_ct: reject ambiguous conntrack expressions in inet tables

[Florian Westphal <fw@strlen.de>]

Nikolaos Gkarlis <nickgarlis@gmail.com> wrote:
> The kernel allows netlink messages that use the legacy NFT_CT_SRC and
> NFT_CT_DST keys in inet tables without an accompanying nft_meta
> expression specifying NFT_META_NFPROTO. This results in ambiguous
> conntrack expressions that cannot be reliably evaluated during packet
> processing.
> 
> When that happens, the register size calculation defaults to IPv6 (16
> bytes) regardless of the actual packet family.
> 
> This causes two issues:
> 1. For IPv4 packets, only 4 bytes contain valid address data while 12
>    bytes contain uninitialized memory during comparison.

I don't think so, they are zeroed out, see nf_ct_get_tuple();
init_conntrack copies the entire thing so I don't see how stack garbage
can be placed in struct nf_conn and thus I don't see how registers can
contains crap.  Do they?  If yes, please provide a bit more information
on how this happens (e.g. kmsan report or similar).

> 2. nft userspace cannot properly display these rules ([invalid type]).

Thats a userspace bug; userspace that makes use of NFT_CT_SRC/DST must
provide the dependency.

This is not the kernels job, the kernel only must make sure that we
can't crash or otherwise leak hidden state (e.g. kernel memory
contents).

> The bug is not reproducible through standard nft commands, which use
> NFT_CT_SRC_IP(6) and NFT_CT_DST_IP(6) keys when NFT_META_NFPROTO is
> not defined.

Thats because not all kernel releases have NFT_CT_DST_IP(6), they were
added later.  Switching userspace will break compatibility with older
kernels.  That said, this key was added in v4.17 so we could change
nftables now to always use NFT_CT_DST_IP(6) instead and avoid adding
the NFPROTO implicit dep for this case.

> Fix by adding a pointer to the parent nft_rule in nft_expr, allowing
> iteration over preceding expressions to ensure that an nft_meta with
> NFT_META_NFPROTO has been defined.

But why?  As far as I can see nothing is broken.

We don't force dependencies for other expressions either, why make
an exception here?

I'm sorry that I did not mention this earlier; in v1 i assumed intent
was to zap unused code (but its still used by nft), hence i did not
mention the above.

> Adding a pointer from nft_expr to nft_rule may be controversial, but
> it was the only approach I could come up with that provides context
> about preceding expressions when validating an expression.

We should not force this unless not doing it causes crash or
other misbehaviour, such as leaking private kernel memory content.

>  struct nft_expr {
>  	const struct nft_expr_ops	*ops;
> +	const struct nft_rule *rule;
>  	unsigned char			data[]
>  		__attribute__((aligned(__alignof__(u64))));

Ouch, sorry, I think this is a non-starter, nft_expr
should be kept as small as possible.

That said, I don't see why its necessary to add this pointer here,
it could be provided via nft_ctx for example.