[PATCH] netfilter: nft_compat: check match/targetinfo attr size

[PATCH] netfilter: nft_compat: check match/targetinfo attr size

[Florian Westphal]

We copy accoring to ->target|matchsize, so check that the netlink attribute
(which can include padding and might be larger) contains enough data.

Reported-by: Julia Lawall <Julia.Lawall@lip6.fr>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nft_compat.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
index 454841b..6228c42 100644
--- a/net/netfilter/nft_compat.c
+++ b/net/netfilter/nft_compat.c
@@ -660,6 +660,9 @@ nft_match_select_ops(const struct nft_ctx *ctx,
 	if (IS_ERR(match))
 		return ERR_PTR(-ENOENT);
 
+	if (match->matchsize > nla_len(tb[NFTA_MATCH_INFO]))
+		return ERR_PTR(-EINVAL);
+
 	/* This is the first time we use this match, allocate operations */
 	nft_match = kzalloc(sizeof(struct nft_xt), GFP_KERNEL);
 	if (nft_match == NULL)
@@ -740,6 +743,9 @@ nft_target_select_ops(const struct nft_ctx *ctx,
 	if (IS_ERR(target))
 		return ERR_PTR(-ENOENT);
 
+	if (target->targetsize > nla_len(tb[NFTA_TARGET_INFO]))
+		return ERR_PTR(-EINVAL);
+
 	/* This is the first time we use this target, allocate operations */
 	nft_target = kzalloc(sizeof(struct nft_xt), GFP_KERNEL);
 	if (nft_target == NULL)
-- 
2.4.10

Re: [PATCH] netfilter: nft_compat: check match/targetinfo attr size

[Pablo Neira Ayuso]

On Wed, Mar 09, 2016 at 12:04:21AM +0100, Florian Westphal wrote:
> We copy accoring to ->target|matchsize, so check that the netlink attribute
> (which can include padding and might be larger) contains enough data.
> 
> Reported-by: Julia Lawall <Julia.Lawall@lip6.fr>
> Signed-off-by: Florian Westphal <fw@strlen.de>

I think xt_check_match() and xt_check_target() already validate this
for us.

Re: [PATCH] netfilter: nft_compat: check match/targetinfo attr size

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Mar 09, 2016 at 12:04:21AM +0100, Florian Westphal wrote:
> > We copy accoring to ->target|matchsize, so check that the netlink attribute
> > (which can include padding and might be larger) contains enough data.
> > 
> > Reported-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> I think xt_check_match() and xt_check_target() already validate this
> for us.

But AFAICS we copy before this:

nft_target_init:

size = XT_ALIGN(nla_len(tb[NFTA_TARGET_INFO]));
...
target_compat_from_user(target, nla_data(tb[NFTA_TARGET_INFO]), info);
   -> memcpy(out, in, t->targetsize);

xt_check_target(&par, size, proto, inv);  // checks size vs. targetsize

'target' is sized based on target->targetsize in
nft_target_select_ops().

So if tb[NFTA_TARGET_INFO] is != t->targetsize we might copy more
data than whats in 'in', no?

Re: [PATCH] netfilter: nft_compat: check match/targetinfo attr size

[Pablo Neira Ayuso]

On Thu, Mar 10, 2016 at 06:00:01PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Mar 09, 2016 at 12:04:21AM +0100, Florian Westphal wrote:
> > > We copy accoring to ->target|matchsize, so check that the netlink attribute
> > > (which can include padding and might be larger) contains enough data.
> > > 
> > > Reported-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > 
> > I think xt_check_match() and xt_check_target() already validate this
> > for us.
> 
> But AFAICS we copy before this:
> 
> nft_target_init:
> 
> size = XT_ALIGN(nla_len(tb[NFTA_TARGET_INFO]));
> ...
> target_compat_from_user(target, nla_data(tb[NFTA_TARGET_INFO]), info);
>    -> memcpy(out, in, t->targetsize);
> 
> xt_check_target(&par, size, proto, inv);  // checks size vs. targetsize
> 
> 'target' is sized based on target->targetsize in
> nft_target_select_ops().
> 
> So if tb[NFTA_TARGET_INFO] is != t->targetsize we might copy more
> data than whats in 'in', no?

Right, if the user passes less information, then we would be copying
more than what we have.

I'm going to place this in the nf-next, thanks Florian.