Re: [PATCH nft] Check for NULL netlink attributes

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

Hi Davide,

I suggest this patch subject:

        nf_tables: nft_inner: validate mandatory meta and payload netlink attributes

On Tue, Jun 04, 2024 at 11:36:21PM +0200, Davide Ornaghi wrote:
> Payload and meta nftables exprs can be embedded into inner exprs via
> nft_expr_inner_parse, which doesn't check for NULL netlink attributes,
> unlike other exprs passing through select_ops.
> Add the missing checks to nft_meta_inner_init and nft_payload_inner_init
> to prevent dereferencing NULL pointers and eventually causing UAF reads on commit_mutex.

I'd suggest this patch description:

"Check for mandatory netlink attributes in payload and meta expression
when used embedded from the inner expression, otherwise NULL pointer
dereference is possible if userspace."

And please add:

Fixes: a150d122b6bd ("netfilter: nft_meta: add inner match support")
Fixes: 3a07327d10a0 ("netfilter: nft_inner: support for inner tunnel header matching")

Thanks a lot for submitting your fix.

> Signed-off-by: Davide Ornaghi <d.ornaghi97@gmail.com>
> ---
>  net/netfilter/nft_meta.c    | 2 ++
>  net/netfilter/nft_payload.c | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index ba0d3683a..e2893077b 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -839,6 +839,8 @@ static int nft_meta_inner_init(const struct nft_ctx
> *ctx,
>         struct nft_meta *priv = nft_expr_priv(expr);
>         unsigned int len;
> 
> +       if (!tb[NFTA_META_KEY] || !tb[NFTA_META_DREG])
> +               return -EINVAL;
>         priv->key = ntohl(nla_get_be32(tb[NFTA_META_KEY]));
>         switch (priv->key) {
>         case NFT_META_PROTOCOL:
> diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
> index 0c43d748e..4c6f15ad0 100644
> --- a/net/netfilter/nft_payload.c
> +++ b/net/netfilter/nft_payload.c
> @@ -650,6 +650,9 @@ static int nft_payload_inner_init(const struct nft_ctx
> *ctx,
>         struct nft_payload *priv = nft_expr_priv(expr);
>         u32 base;
> 
> +       if (!tb[NFTA_PAYLOAD_BASE] || !tb[NFTA_PAYLOAD_OFFSET] ||
> +           !tb[NFTA_PAYLOAD_LEN] || !tb[NFTA_PAYLOAD_DREG])
> +               return -EINVAL;
>         base   = ntohl(nla_get_be32(tb[NFTA_PAYLOAD_BASE]));
>         switch (base) {
>         case NFT_PAYLOAD_TUN_HEADER:
> --
> 2.34.1