sunrpc: use snprintf to guard against buffer overflow

  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(-)
  

* 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?

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.

Thanks,
Florian
  

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.
  

* Philipp Tomsich:

>> 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.

Looking forward to it.  You can probably expand the definition of OPSYS
and remove the now-unused macros, too.

Thanks,
Florian
  

On 12/3/20 3:09 AM, Florian Weimer via Libc-alpha 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?

There has been a change to the warning but I don't see it in
my x86_64 build with the latest GCC/Glibc.  Compiling with
-ftree-dump-strlen=/dev/stdout prints the details for each
directive.  That should help explain how the warning came
up with the number.  My result looks like this:

netname.c:52: sprintf: objsize = 256, fmtstr = "%s.%d@%s"
   Directive 1 at offset 0: "%s"
     Result: 4, 4, 4, 4 (4, 4, 4, 4)
   Directive 2 at offset 2: ".", length = 1
     Result: 1, 1, 1, 1 (5, 5, 5, 5)
   Directive 3 at offset 3: "%d"
     Result: 1, 1, 11, 11 (6, 6, 16, 16)
   Directive 4 at offset 5: "@", length = 1
     Result: 1, 1, 1, 1 (7, 7, 17, 17)
   Directive 5 at offset 6: "%s"
     Result: 0, 237, 237, 237 (7, 244, 254, 254)
   Directive 6 at offset 8: "", length = 1

If you see the warning on a target other that x86_64 I'll be
happy to look into it if you can send me the translation unit
(and the target) so I can easily reproduce it on my end.

Martin

> 
> 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.
> 
> Thanks,
> Florian
>