* nftables: oob crash w. verdict maps & jumps
@ 2015-04-14 13:50 Florian Westphal
2015-04-14 14:03 ` Patrick McHardy
0 siblings, 1 reply; 4+ messages in thread
From: Florian Westphal @ 2015-04-14 13:50 UTC (permalink / raw)
To: netfilter-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: nftables: oob crash w. verdict maps & jumps
2015-04-14 13:50 nftables: oob crash w. verdict maps & jumps Florian Westphal
@ 2015-04-14 14:03 ` Patrick McHardy
2015-04-14 14:17 ` Florian Westphal
0 siblings, 1 reply; 4+ messages in thread
From: Patrick McHardy @ 2015-04-14 14:03 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: nftables: oob crash w. verdict maps & jumps
2015-04-14 14:03 ` Patrick McHardy
@ 2015-04-14 14:17 ` Florian Westphal
2015-04-14 14:28 ` Patrick McHardy
0 siblings, 1 reply; 4+ messages in thread
From: Florian Westphal @ 2015-04-14 14:17 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Florian Westphal, netfilter-devel
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!
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: nftables: oob crash w. verdict maps & jumps
2015-04-14 14:17 ` Florian Westphal
@ 2015-04-14 14:28 ` Patrick McHardy
0 siblings, 0 replies; 4+ messages in thread
From: Patrick McHardy @ 2015-04-14 14:28 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-04-14 14:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-14 13:50 nftables: oob crash w. verdict maps & jumps Florian Westphal
2015-04-14 14:03 ` Patrick McHardy
2015-04-14 14:17 ` Florian Westphal
2015-04-14 14:28 ` Patrick McHardy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.