Re: [PATCH net] netfilter: xt_hashlimit: do not allow empty names

[Eric Dumazet]

On Fri, 2018-02-02 at 12:49 +0100, Pablo Neira Ayuso wrote:

> @Eric, I can give it a shot here to this, just let me know. Thanks!

Please go ahead Pablo !

Thanks.

Re: [PATCH net] netfilter: xt_hashlimit: do not allow empty names

[Pablo Neira Ayuso]

On Fri, Feb 02, 2018 at 01:12:08PM +0100, Jan Engelhardt wrote:
> On Friday 2018-02-02 12:55, Pablo Neira Ayuso wrote:
> 
> >On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote:
> >[...]
> >> bool net_valid_name(const char *name, size_t len)
> >> {
> >> ...
> >> }
> >
> >Am I missing anything in all these tricky string handling? Thanks!
> 
> One will have to watch out for calls like
> 
>   net_valid_name(NLA_DATA(some_ifla_name), IFNAMSIZ)
> 
> because quite some nlattrs (IFLA_NAME, IFLA_IFNAME to boot) are NLA_STRINGs
> (rather than NLA_NUL_STRINGs) and they are not \0-terminated.

As long as we use nla_str*() helpers, we're all good.

Re: [PATCH net] netfilter: xt_hashlimit: do not allow empty names

[Jan Engelhardt]

On Friday 2018-02-02 12:55, Pablo Neira Ayuso wrote:

>On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote:
>[...]
>> bool net_valid_name(const char *name, size_t len)
>> {
>> ...
>> }
>
>Am I missing anything in all these tricky string handling? Thanks!

One will have to watch out for calls like

net_valid_name(NLA_DATA(some_ifla_name), IFNAMSIZ)

because quite some nlattrs (IFLA_NAME, IFLA_IFNAME to boot) are NLA_STRINGs
(rather than NLA_NUL_STRINGs) and they are not \0-terminated.

Re: [PATCH net] netfilter: xt_hashlimit: do not allow empty names

[Pablo Neira Ayuso]

On Fri, Feb 02, 2018 at 12:55:22PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote:
> [...]
> > 
> > Or place this in the core, something like:
> > 
> > bool net_valid_name(const char *name, size_t len)
> > {
> > ...
> > }
> > 
> > then use it from dev_valid_name()
> > 
> > bool dev_valid_name(const char *name)
> > {
> > return net_valid_name(name, IFNAMSIZ);
> > }
> 
> Just to clarify, I think I prefer this approach, probably other
> subsystem can benefit from this generic approach too.
> 
> BTW, I wonder if we should use strnlen() instead of strlen() in
> dev_valid_name(), I guess this not useful for device name since they
> are guaranteed to be nul-terminated, but netfilter this would be good
> given userspace can send us a non nul-terminated string. But in
> general, I think it doesn't harm to use strnlen() in dev_valid_name().

Argh sorry, I'm talking about the hypothetical net_valid_name()
function, not dev_valid_name() itself.

Re: [PATCH net] netfilter: xt_hashlimit: do not allow empty names

[Pablo Neira Ayuso]

On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote:
[...]
> 
> Or place this in the core, something like:
> 
> bool net_valid_name(const char *name, size_t len)
> {
> ...
> }
> 
> then use it from dev_valid_name()
> 
> bool dev_valid_name(const char *name)
> {
> return net_valid_name(name, IFNAMSIZ);
> }

Just to clarify, I think I prefer this approach, probably other
subsystem can benefit from this generic approach too.

BTW, I wonder if we should use strnlen() instead of strlen() in
dev_valid_name(), I guess this not useful for device name since they
are guaranteed to be nul-terminated, but netfilter this would be good
given userspace can send us a non nul-terminated string. But in
general, I think it doesn't harm to use strnlen() in dev_valid_name().

Am I missing anything in all these tricky string handling? Thanks!

Re: [PATCH net] netfilter: xt_hashlimit: do not allow empty names

[Pablo Neira Ayuso]

On Sun, Jan 28, 2018 at 09:54:05AM -0800, Eric Dumazet wrote:
> On Sun, 2018-01-28 at 07:41 -0800, Eric Dumazet wrote:
> > From: Eric Dumazet 
> > 
> > Syzbot reported a WARN() in proc_create_data() [1]
> > 
> > Issue here is that xt_hashlimit does not check that user space provided
> > an empty table name.
> 
> > Signed-off-by: Eric Dumazet 
> > Reported-by: syzbot 
> > ---
> >  net/netfilter/xt_hashlimit.c |2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> > index 
> > 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75
> >  100644
> > --- a/net/netfilter/xt_hashlimit.c
> > +++ b/net/netfilter/xt_hashlimit.c
> > @@ -894,6 +894,8 @@ static int hashlimit_mt_check_common(const struct 
> > xt_mtchk_param *par,
> > return -ERANGE;
> > }
> >  
> > +   if (!name[0])
> > +   return -EINVAL;
> > mutex_lock(_mutex);
> > *hinfo = htable_find_get(net, name, par->family);
> > if (*hinfo == NULL) {
> 
> I wonder if we should also check if name includes a '/' ?
> 
> if (!name[0] || strchr(name, '/'))
> return -EINVAL;

We can probably add our own variant of dev_valid_name?

bool nf_valid_name(const char *name, size_t len)
{
if (*name == '\0')
return false;
if (strlen(name) >= len)
return false;
if (!strcmp(name, ".") || !strcmp(name, ".."))
return false;

while (*name) {
if (*name == '/' || *name == ':' || isspace(*name))
return false;
name++;
}
return true;
}

Or place this in the core, something like:

bool net_valid_name(const char *name, size_t len)
{
...
}

then use it from dev_valid_name()

bool dev_valid_name(const char *name)
{
return net_valid_name(name, IFNAMSIZ);
}

@Eric, I can give it a shot here to this, just let me know. Thanks!

Re: [PATCH net] netfilter: xt_hashlimit: do not allow empty names

[Florian Westphal]

Eric Dumazet  wrote:
> On Sun, 2018-01-28 at 07:41 -0800, Eric Dumazet wrote:
> > From: Eric Dumazet 
> > 
> > Syzbot reported a WARN() in proc_create_data() [1]
> > 
> > Issue here is that xt_hashlimit does not check that user space provided
> > an empty table name.
> 
> > Signed-off-by: Eric Dumazet 
> > Reported-by: syzbot 
> > ---
> >  net/netfilter/xt_hashlimit.c |2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> > index 
> > 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75
> >  100644
> > --- a/net/netfilter/xt_hashlimit.c
> > +++ b/net/netfilter/xt_hashlimit.c
> > @@ -894,6 +894,8 @@ static int hashlimit_mt_check_common(const struct 
> > xt_mtchk_param *par,
> > return -ERANGE;
> > }
> >  
> > +   if (!name[0])
> > +   return -EINVAL;
> > mutex_lock(_mutex);
> > *hinfo = htable_find_get(net, name, par->family);
> > if (*hinfo == NULL) {
> 
> I wonder if we should also check if name includes a '/' ?
> 
> if (!name[0] || strchr(name, '/'))
> return -EINVAL;

I think there should be a helper for this to reject

- 0 length
- /
- .
- ..

in hashlimit case name is IFNMASIZ so we could even use
dev_valid_name() here.

Re: [PATCH net] netfilter: xt_hashlimit: do not allow empty names

[Eric Dumazet]

On Sun, 2018-01-28 at 07:41 -0800, Eric Dumazet wrote:
> From: Eric Dumazet 
> 
> Syzbot reported a WARN() in proc_create_data() [1]
> 
> Issue here is that xt_hashlimit does not check that user space provided
> an empty table name.

> Signed-off-by: Eric Dumazet 
> Reported-by: syzbot 
> ---
>  net/netfilter/xt_hashlimit.c |2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index 
> 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75
>  100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -894,6 +894,8 @@ static int hashlimit_mt_check_common(const struct 
> xt_mtchk_param *par,
>   return -ERANGE;
>   }
>  
> + if (!name[0])
> + return -EINVAL;
>   mutex_lock(_mutex);
>   *hinfo = htable_find_get(net, name, par->family);
>   if (*hinfo == NULL) {

I wonder if we should also check if name includes a '/' ?

if (!name[0] || strchr(name, '/'))
return -EINVAL;