[PATCH] sunrpc: use snprintf to guard against buffer overflow

[Philipp Tomsich]

On Thu, 3 Dec 2020 at 11:09, Florian Weimer <fweimer@redhat.com> wrote:
>
> * Philipp Tomsich:
>
> > GCC11 has improved detection of buffer overflows detectable through the analysis
> > of format strings and parameters, which identifies the following issue:
> >    netname.c:52:28: error: '%s' directive writing up to 255 bytes into a region
> >                            of size between 239 and 249 [-Werror=format-overflow=]
> >
> > This rewrites user2netname() to use snprintf to guard against overflows.
> >
> > ---
> >
> >  sunrpc/netname.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/sunrpc/netname.c b/sunrpc/netname.c
> > index 24ee519..62e644f 100644
> > --- a/sunrpc/netname.c
> > +++ b/sunrpc/netname.c
> > @@ -49,8 +49,10 @@ user2netname (char netname[MAXNETNAMELEN + 1], const uid_t uid,
> >    if ((strlen (dfltdom) + OPSYS_LEN + 3 + MAXIPRINT) > (size_t) MAXNETNAMELEN)
> >      return 0;
> >
> > -  sprintf (netname, "%s.%d@%s", OPSYS, uid, dfltdom);
> > -  i = strlen (netname);
> > +  i = snprintf (netname, MAXNETNAMELEN + 1, "%s.%d@%s", OPSYS, uid, dfltdom);
> > +  if (i > (size_t) MAXNETNAMELEN)
> > +    return 0;
> > +
>
> There's a strlen check right above the sprintf, and it looks like it
> would catch cases where we'd right too much into the buffer.  So this
> looks like a GCC 11 false positive to me.  Am I missing something?

I should maybe reword the commit message to clearly state that this does not
mitigate an exploitable overflow, but just addresses a language statement that
the static analyzer in GCC chokes on (for the obvious reason that format-string
is not checked against a ranger — although the "implied guarantee" on the length
of the dfltdom-string will not be easily understandable to a compiler).

> The switch to snprintf is reasonable (with the caveat that this code is
> in very, very deep maintenance mode), but I think you should replace the
> strlen check and also check for negative i.

I'll provide a V2.

Thanks,
Philipp.