Re: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is NULL-terminated

[Phil Sutter]

On Fri, Aug 18, 2017 at 04:32:47PM +, David Laight wrote:
> From: Phil Sutter
> > Sent: 18 August 2017 11:52
> > On Fri, Aug 18, 2017 at 09:19:16AM +, David Laight wrote:
> > > From: Phil Sutter
> > > > Sent: 17 August 2017 18:09
> > > > To: Stephen Hemminger
> > > > Cc: netdev@vger.kernel.org
> > > > Subject: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is 
> > > > NULL-terminated
> > > >
> > > > Signed-off-by: Phil Sutter 
> > > > ---
> > > >  ip/ipntable.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/ip/ipntable.c b/ip/ipntable.c
> > > > index 879626ee4f491..7be1f04d33d90 100644
> > > > --- a/ip/ipntable.c
> > > > +++ b/ip/ipntable.c
> > > > @@ -633,7 +633,8 @@ static int ipntable_show(int argc, char **argv)
> > > > } else if (strcmp(*argv, "name") == 0) {
> > > > NEXT_ARG();
> > > >
> > > > -   strncpy(filter.name, *argv, 
> > > > sizeof(filter.name));
> > > > +   strncpy(filter.name, *argv, sizeof(filter.name) 
> > > > - 1);
> > > > +   filter.name[sizeof(filter.name) - 1] = '\0';
> > >
> > > Why not check for overflow instead?
> > >   if (filter.name[sizeof(filter.name) - 1])
> > >   usage("filer name too long");
> > 
> > sizeof(filter.name) is 1024, which is maybe a bit over the top for
> > something a user would input. So I found a better way avoiding all this
> > at once: I made filter.name a const char *, then just assigned *argv to
> > it. This should be safe since rtnl_dump_filter() and therefore
> > print_ntable() callback is called from inside ipntable_show() so *argv
> > is not accessed outside of it's scope.
> > 
> > What do you think?
> 
> There isn't a scope problem, *argv is program data (written to unusable
> stack space by the kernel during exec.

Ah, thanks for the info!

> If the filter is done in userpace it is ok, but I'd have thought
> it would be passed to kernel later on?

No, it just calls sends RTM_GETNEIGHTBL to kernel and throws away
anything that doesn't match the filter. So filtering happens in
user space exclusively.

Thanks, Phil

RE: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is NULL-terminated

[David Laight]

From: Phil Sutter
> Sent: 18 August 2017 11:52
> On Fri, Aug 18, 2017 at 09:19:16AM +, David Laight wrote:
> > From: Phil Sutter
> > > Sent: 17 August 2017 18:09
> > > To: Stephen Hemminger
> > > Cc: netdev@vger.kernel.org
> > > Subject: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is 
> > > NULL-terminated
> > >
> > > Signed-off-by: Phil Sutter 
> > > ---
> > >  ip/ipntable.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/ip/ipntable.c b/ip/ipntable.c
> > > index 879626ee4f491..7be1f04d33d90 100644
> > > --- a/ip/ipntable.c
> > > +++ b/ip/ipntable.c
> > > @@ -633,7 +633,8 @@ static int ipntable_show(int argc, char **argv)
> > >   } else if (strcmp(*argv, "name") == 0) {
> > >   NEXT_ARG();
> > >
> > > - strncpy(filter.name, *argv, sizeof(filter.name));
> > > + strncpy(filter.name, *argv, sizeof(filter.name) - 1);
> > > + filter.name[sizeof(filter.name) - 1] = '\0';
> >
> > Why not check for overflow instead?
> > if (filter.name[sizeof(filter.name) - 1])
> > usage("filer name too long");
> 
> sizeof(filter.name) is 1024, which is maybe a bit over the top for
> something a user would input. So I found a better way avoiding all this
> at once: I made filter.name a const char *, then just assigned *argv to
> it. This should be safe since rtnl_dump_filter() and therefore
> print_ntable() callback is called from inside ipntable_show() so *argv
> is not accessed outside of it's scope.
> 
> What do you think?

There isn't a scope problem, *argv is program data (written to unusable
stack space by the kernel during exec.

If the filter is done in userpace it is ok, but I'd have thought
it would be passed to kernel later on?

David

Re: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is NULL-terminated

[Phil Sutter]

Hi David,

On Fri, Aug 18, 2017 at 09:19:16AM +, David Laight wrote:
> From: Phil Sutter
> > Sent: 17 August 2017 18:09
> > To: Stephen Hemminger
> > Cc: netdev@vger.kernel.org
> > Subject: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is 
> > NULL-terminated
> > 
> > Signed-off-by: Phil Sutter 
> > ---
> >  ip/ipntable.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ip/ipntable.c b/ip/ipntable.c
> > index 879626ee4f491..7be1f04d33d90 100644
> > --- a/ip/ipntable.c
> > +++ b/ip/ipntable.c
> > @@ -633,7 +633,8 @@ static int ipntable_show(int argc, char **argv)
> > } else if (strcmp(*argv, "name") == 0) {
> > NEXT_ARG();
> > 
> > -   strncpy(filter.name, *argv, sizeof(filter.name));
> > +   strncpy(filter.name, *argv, sizeof(filter.name) - 1);
> > +   filter.name[sizeof(filter.name) - 1] = '\0';
> 
> Why not check for overflow instead?
>   if (filter.name[sizeof(filter.name) - 1])
>   usage("filer name too long");

sizeof(filter.name) is 1024, which is maybe a bit over the top for
something a user would input. So I found a better way avoiding all this
at once: I made filter.name a const char *, then just assigned *argv to
it. This should be safe since rtnl_dump_filter() and therefore
print_ntable() callback is called from inside ipntable_show() so *argv
is not accessed outside of it's scope.

What do you think?

Thanks, Phil

RE: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is NULL-terminated

[David Laight]

From: Phil Sutter
> Sent: 17 August 2017 18:09
> To: Stephen Hemminger
> Cc: netdev@vger.kernel.org
> Subject: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is 
> NULL-terminated
> 
> Signed-off-by: Phil Sutter 
> ---
>  ip/ipntable.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/ip/ipntable.c b/ip/ipntable.c
> index 879626ee4f491..7be1f04d33d90 100644
> --- a/ip/ipntable.c
> +++ b/ip/ipntable.c
> @@ -633,7 +633,8 @@ static int ipntable_show(int argc, char **argv)
>   } else if (strcmp(*argv, "name") == 0) {
>   NEXT_ARG();
> 
> - strncpy(filter.name, *argv, sizeof(filter.name));
> + strncpy(filter.name, *argv, sizeof(filter.name) - 1);
> + filter.name[sizeof(filter.name) - 1] = '\0';

Why not check for overflow instead?
if (filter.name[sizeof(filter.name) - 1])
usage("filer name too long");

David