nftables: oob crash w. verdict maps & jumps

nftables: oob crash w. verdict maps & jumps

[Florian Westphal]

Hi.

Looks like we can memcpy more than we allocated in nft_set_elem_init().

reproducer:

nft -f /etc/nftables/ipv4-filter
nft add map filter ipmap '{ type ipv4_addr : verdict; }'
nft add chain filter foo
nft add element filter ipmap { 1.2.3.4 : jump foo }

-> we scribble over elem private size

You can see this when turning on SLUB debugging and deleting the jump
element again, we get 'Redzone overwritten'.

In nft_add_set_elem() we try to account for the length:

3372 
3373         if (nla[NFTA_SET_ELEM_DATA] != NULL) {
3374                 err = nft_data_init(ctx, &data, sizeof(data), &d2,
3375                                     nla[NFTA_SET_ELEM_DATA]);
3376                 if (err < 0)
3377                         goto err2;
[..]
 

3380                 if (set->dtype != NFT_DATA_VERDICT && d2.len != set->dlen)
3381                         goto err3;
3401                 nft_set_ext_add_length(&tmpl, NFT_SET_EXT_DATA, d2.len);
3402         }

but in above 'nft add element filter ipmap { 1.2.3.4 : jump foo }'

We have dtype NFT_DATA_VERDICT, d2.len of 8, and set->dlen of 16.

So account for d2.len (8), but then later copy a size of set->dlen (16) in
nft_set_elem_init:

3272         if (nft_set_ext_exists(ext, NFT_SET_EXT_DATA))
3273                 memcpy(nft_set_ext_data(ext), data, set->dlen);

Could someone please look at this?  I'm not sure what the intent is/was.

My hunch is that the check should be

 if (set->dtype != NFT_DATA_VERDICT || d2.len != set->dlen)

and libnftnl should also send a dlen of 16, like it already does for
accept/drop.

Thanks,
Florian

Re: nftables: oob crash w. verdict maps & jumps

[Patrick McHardy]

On 14.04, Florian Westphal wrote:
> Hi.
> 
> Looks like we can memcpy more than we allocated in nft_set_elem_init().
> 
> reproducer:
> 
> nft -f /etc/nftables/ipv4-filter
> nft add map filter ipmap '{ type ipv4_addr : verdict; }'
> nft add chain filter foo
> nft add element filter ipmap { 1.2.3.4 : jump foo }
> 
> -> we scribble over elem private size
> 
> You can see this when turning on SLUB debugging and deleting the jump
> element again, we get 'Redzone overwritten'.
> 
> In nft_add_set_elem() we try to account for the length:
> 
> 3372 
> 3373         if (nla[NFTA_SET_ELEM_DATA] != NULL) {
> 3374                 err = nft_data_init(ctx, &data, sizeof(data), &d2,
> 3375                                     nla[NFTA_SET_ELEM_DATA]);
> 3376                 if (err < 0)
> 3377                         goto err2;
> [..]
>  
> 
> 3380                 if (set->dtype != NFT_DATA_VERDICT && d2.len != set->dlen)
> 3381                         goto err3;
> 3401                 nft_set_ext_add_length(&tmpl, NFT_SET_EXT_DATA, d2.len);
> 3402         }
> 
> but in above 'nft add element filter ipmap { 1.2.3.4 : jump foo }'
> 
> We have dtype NFT_DATA_VERDICT, d2.len of 8, and set->dlen of 16.

Is that on x86 (32 bit)?

> So account for d2.len (8), but then later copy a size of set->dlen (16) in
> nft_set_elem_init:
> 
> 3272         if (nft_set_ext_exists(ext, NFT_SET_EXT_DATA))
> 3273                 memcpy(nft_set_ext_data(ext), data, set->dlen);
> 
> Could someone please look at this?  I'm not sure what the intent is/was.
> 
> My hunch is that the check should be
> 
>  if (set->dtype != NFT_DATA_VERDICT || d2.len != set->dlen)

We actually only care about userspace provided length for data.
The verdict representation is internal, so we want to determine
the size ourselves.

> and libnftnl should also send a dlen of 16, like it already does for
> accept/drop.

I'm surprsided, because in nf_tables_newset, for verdict maps we do:

                        desc.dlen = sizeof(struct nft_verdict);

in nft_verdict_init() we do:

                desc->len = sizeof(data->verdict);

So it should work out. I don't see how we could have arrived at the
value 8.

Re: nftables: oob crash w. verdict maps & jumps

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> > nft -f /etc/nftables/ipv4-filter
> > nft add map filter ipmap '{ type ipv4_addr : verdict; }'
> > nft add chain filter foo
> > nft add element filter ipmap { 1.2.3.4 : jump foo }
> > 
> > -> we scribble over elem private size
> > 
> > You can see this when turning on SLUB debugging and deleting the jump
> > element again, we get 'Redzone overwritten'.
> > 
> > In nft_add_set_elem() we try to account for the length:
> > 
> > 3372 
> > 3373         if (nla[NFTA_SET_ELEM_DATA] != NULL) {
> > 3374                 err = nft_data_init(ctx, &data, sizeof(data), &d2,
> > 3375                                     nla[NFTA_SET_ELEM_DATA]);
> > 3376                 if (err < 0)
> > 3377                         goto err2;
> > [..]
> >  
> > 
> > 3380                 if (set->dtype != NFT_DATA_VERDICT && d2.len != set->dlen)
> > 3381                         goto err3;
> > 3401                 nft_set_ext_add_length(&tmpl, NFT_SET_EXT_DATA, d2.len);
> > 3402         }
> > 
> > but in above 'nft add element filter ipmap { 1.2.3.4 : jump foo }'
> > 
> > We have dtype NFT_DATA_VERDICT, d2.len of 8, and set->dlen of 16.
> 
> Is that on x86 (32 bit)?

Its on x86_64.

> > So account for d2.len (8), but then later copy a size of set->dlen (16) in
> > nft_set_elem_init:
> > 
> > 3272         if (nft_set_ext_exists(ext, NFT_SET_EXT_DATA))
> > 3273                 memcpy(nft_set_ext_data(ext), data, set->dlen);
> > 
> > Could someone please look at this?  I'm not sure what the intent is/was.
> > 
> > My hunch is that the check should be
> > 
> >  if (set->dtype != NFT_DATA_VERDICT || d2.len != set->dlen)
> 
> We actually only care about userspace provided length for data.
> The verdict representation is internal, so we want to determine
> the size ourselves.

I see, makes sense.

> > and libnftnl should also send a dlen of 16, like it already does for
> > accept/drop.
> 
> I'm surprsided, because in nf_tables_newset, for verdict maps we do:
> 
>                         desc.dlen = sizeof(struct nft_verdict);
> 
> in nft_verdict_init() we do:
> 
>                 desc->len = sizeof(data->verdict);
> 
> So it should work out. I don't see how we could have arrived at the
> value 8.

Thanks for the hint, we set dlen size to sizeof(void*) in this function,
this makes the error go away:

@@ -4355,7 +4355,7 @@ static int nft_verdict_init(const struct nft_ctx *ctx, struct nft_data *data,
 
                chain->use++;
                data->verdict.chain = chain;
-               desc->len = sizeof(data);
+               desc->len = sizeof(*data);
                break;
        }

Perhaps it would be better to set desc->len unconditionally, seems we want to set it to 16 unconditionally
regardless of data->verdict.code?

Thanks!

Re: nftables: oob crash w. verdict maps & jumps

[Patrick McHardy]

On 14.04, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > > So account for d2.len (8), but then later copy a size of set->dlen (16) in
> > > nft_set_elem_init:
> > > 
> > > 3272         if (nft_set_ext_exists(ext, NFT_SET_EXT_DATA))
> > > 3273                 memcpy(nft_set_ext_data(ext), data, set->dlen);
> > > 
> > > Could someone please look at this?  I'm not sure what the intent is/was.
> > > 
> > > My hunch is that the check should be
> > > 
> > >  if (set->dtype != NFT_DATA_VERDICT || d2.len != set->dlen)
> > 
> > We actually only care about userspace provided length for data.
> > The verdict representation is internal, so we want to determine
> > the size ourselves.
> 
> I see, makes sense.
> 
> > > and libnftnl should also send a dlen of 16, like it already does for
> > > accept/drop.
> > 
> > I'm surprsided, because in nf_tables_newset, for verdict maps we do:
> > 
> >                         desc.dlen = sizeof(struct nft_verdict);
> > 
> > in nft_verdict_init() we do:
> > 
> >                 desc->len = sizeof(data->verdict);
> > 
> > So it should work out. I don't see how we could have arrived at the
> > value 8.
> 
> Thanks for the hint, we set dlen size to sizeof(void*) in this function,
> this makes the error go away:

Crap :)

> @@ -4355,7 +4355,7 @@ static int nft_verdict_init(const struct nft_ctx *ctx, struct nft_data *data,
>  
>                 chain->use++;
>                 data->verdict.chain = chain;
> -               desc->len = sizeof(data);
> +               desc->len = sizeof(*data);
>                 break;
>         }
> 
> Perhaps it would be better to set desc->len unconditionally, seems we want to set it to 16 unconditionally
> regardless of data->verdict.code?

Yeah, that seems unnecessary error prone. Lets set it to sizeof(data->verdict)
unconditionally I would say.