[PATCH nf] ebtables: arpreply: Add the standard target sanity check

[PATCH nf] ebtables: arpreply: Add the standard target sanity check

[gfree.wind]

From: Gao Feng <gfree.wind@vip.163.com>

The info->target is from userspace and it would be used directly.
So we need to add the sanity check to make sure it is a valid standard
target, although the ebtables tool has already checked it. Kernel need
to check anything from userspace.

If the target was set as an evil value, it would break the ebtables
and cause a panic. Because the non-standard target is treated as one
offset.

Signed-off-by: Gao Feng <gfree.wind@vip.163.com>
---
 net/bridge/netfilter/ebt_arpreply.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bridge/netfilter/ebt_arpreply.c b/net/bridge/netfilter/ebt_arpreply.c
index 5929309..c4886d9 100644
--- a/net/bridge/netfilter/ebt_arpreply.c
+++ b/net/bridge/netfilter/ebt_arpreply.c
@@ -68,6 +68,9 @@ static int ebt_arpreply_tg_check(const struct xt_tgchk_param *par)
 	if (e->ethproto != htons(ETH_P_ARP) ||
 	    e->invflags & EBT_IPROTO)
 		return -EINVAL;
+	if (INVALID_TARGET)
+		return -EINVAL;
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH nf] ebtables: arpreply: Add the standard target sanity check

[Pablo Neira Ayuso]

On Fri, May 12, 2017 at 05:44:10PM +0800, gfree.wind@vip.163.com wrote:
> From: Gao Feng <gfree.wind@vip.163.com>
> 
> The info->target is from userspace and it would be used directly.
> So we need to add the sanity check to make sure it is a valid standard
> target, although the ebtables tool has already checked it. Kernel need
> to check anything from userspace.
> 
> If the target was set as an evil value, it would break the ebtables
> and cause a panic. Because the non-standard target is treated as one
> offset.
> 
> Signed-off-by: Gao Feng <gfree.wind@vip.163.com>
> ---
>  net/bridge/netfilter/ebt_arpreply.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/bridge/netfilter/ebt_arpreply.c b/net/bridge/netfilter/ebt_arpreply.c
> index 5929309..c4886d9 100644
> --- a/net/bridge/netfilter/ebt_arpreply.c
> +++ b/net/bridge/netfilter/ebt_arpreply.c
> @@ -68,6 +68,9 @@ static int ebt_arpreply_tg_check(const struct xt_tgchk_param *par)
>  	if (e->ethproto != htons(ETH_P_ARP) ||
>  	    e->invflags & EBT_IPROTO)
>  		return -EINVAL;
> +	if (INVALID_TARGET)

Please, add:

static inline bool ebt_invalid_target(int target)
{
        return (target < -NUM_STANDARD_TARGETS || target >= 0);
}

and use it in this fix.

So we can get rid of this obscure INVALID_TARGET macro.

Thanks.

Re: [PATCH nf] ebtables: arpreply: Add the standard target sanity check

[Pablo Neira Ayuso]

On Mon, May 15, 2017 at 06:56:02PM +0200, Pablo Neira Ayuso wrote:
> On Fri, May 12, 2017 at 05:44:10PM +0800, gfree.wind@vip.163.com wrote:
> > From: Gao Feng <gfree.wind@vip.163.com>
> > 
> > The info->target is from userspace and it would be used directly.
> > So we need to add the sanity check to make sure it is a valid standard
> > target, although the ebtables tool has already checked it. Kernel need
> > to check anything from userspace.
> > 
> > If the target was set as an evil value, it would break the ebtables
> > and cause a panic. Because the non-standard target is treated as one
> > offset.
> > 
> > Signed-off-by: Gao Feng <gfree.wind@vip.163.com>
> > ---
> >  net/bridge/netfilter/ebt_arpreply.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/net/bridge/netfilter/ebt_arpreply.c b/net/bridge/netfilter/ebt_arpreply.c
> > index 5929309..c4886d9 100644
> > --- a/net/bridge/netfilter/ebt_arpreply.c
> > +++ b/net/bridge/netfilter/ebt_arpreply.c
> > @@ -68,6 +68,9 @@ static int ebt_arpreply_tg_check(const struct xt_tgchk_param *par)
> >  	if (e->ethproto != htons(ETH_P_ARP) ||
> >  	    e->invflags & EBT_IPROTO)
> >  		return -EINVAL;
> > +	if (INVALID_TARGET)
> 
> Please, add:
> 
> static inline bool ebt_invalid_target(int target)
> {
>         return (target < -NUM_STANDARD_TARGETS || target >= 0);
> }
> 
> and use it in this fix.
> 
> So we can get rid of this obscure INVALID_TARGET macro.

Once this propagates to nf-next.git, you can send a follow up patch to
use this new function from more spots, so we can kill INVALID_TARGET
for good.

Re:Re: [PATCH nf] ebtables: arpreply: Add the standard target sanity check

[Gao Feng]

At 2017-05-16 00:56:59, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
>On Mon, May 15, 2017 at 06:56:02PM +0200, Pablo Neira Ayuso wrote:
>> On Fri, May 12, 2017 at 05:44:10PM +0800, gfree.wind@vip.163.com wrote:
>> > From: Gao Feng <gfree.wind@vip.163.com>
>> > 
>> > The info->target is from userspace and it would be used directly.
>> > So we need to add the sanity check to make sure it is a valid standard
>> > target, although the ebtables tool has already checked it. Kernel need
>> > to check anything from userspace.
>> > 
>> > If the target was set as an evil value, it would break the ebtables
>> > and cause a panic. Because the non-standard target is treated as one
>> > offset.
>> > 
>> > Signed-off-by: Gao Feng <gfree.wind@vip.163.com>
>> > ---
>> >  net/bridge/netfilter/ebt_arpreply.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> > 
>> > diff --git a/net/bridge/netfilter/ebt_arpreply.c b/net/bridge/netfilter/ebt_arpreply.c
>> > index 5929309..c4886d9 100644
>> > --- a/net/bridge/netfilter/ebt_arpreply.c
>> > +++ b/net/bridge/netfilter/ebt_arpreply.c
>> > @@ -68,6 +68,9 @@ static int ebt_arpreply_tg_check(const struct xt_tgchk_param *par)
>> >  	if (e->ethproto != htons(ETH_P_ARP) ||
>> >  	    e->invflags & EBT_IPROTO)
>> >  		return -EINVAL;
>> > +	if (INVALID_TARGET)
>> 
>> Please, add:
>> 
>> static inline bool ebt_invalid_target(int target)
>> {
>>         return (target < -NUM_STANDARD_TARGETS || target >= 0);
>> }
>> 
>> and use it in this fix.
>> 
>> So we can get rid of this obscure INVALID_TARGET macro.
>
>Once this propagates to nf-next.git, you can send a follow up patch to
>use this new function from more spots, so we can kill INVALID_TARGET
>for good.

OK, no problem.

Regards
Feng