* [PATCH] sunrpc: use snprintf to guard against buffer overflow
@ 2020-12-02 19:04 Philipp Tomsich
2020-12-03 10:09 ` Florian Weimer
0 siblings, 1 reply; 5+ messages in thread
From: Philipp Tomsich @ 2020-12-02 19:04 UTC (permalink / raw)
To: libc-alpha; +Cc: 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;
+
if (netname[i - 1] == '.')
netname[i - 1] = '\0';
return 1;
--
1.8.3.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sunrpc: use snprintf to guard against buffer overflow
2020-12-02 19:04 [PATCH] sunrpc: use snprintf to guard against buffer overflow Philipp Tomsich
@ 2020-12-03 10:09 ` Florian Weimer
2020-12-03 10:15 ` Philipp Tomsich
2020-12-03 15:43 ` Martin Sebor
0 siblings, 2 replies; 5+ messages in thread
From: Florian Weimer @ 2020-12-03 10:09 UTC (permalink / raw)
To: Philipp Tomsich; +Cc: libc-alpha, Martin Sebor
* 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
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sunrpc: use snprintf to guard against buffer overflow
2020-12-03 10:09 ` Florian Weimer
@ 2020-12-03 10:15 ` Philipp Tomsich
2020-12-03 10:19 ` Florian Weimer
2020-12-03 15:43 ` Martin Sebor
1 sibling, 1 reply; 5+ messages in thread
From: Philipp Tomsich @ 2020-12-03 10:15 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha, Martin Sebor
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sunrpc: use snprintf to guard against buffer overflow
2020-12-03 10:15 ` Philipp Tomsich
@ 2020-12-03 10:19 ` Florian Weimer
0 siblings, 0 replies; 5+ messages in thread
From: Florian Weimer @ 2020-12-03 10:19 UTC (permalink / raw)
To: Philipp Tomsich; +Cc: libc-alpha, Martin Sebor
* 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
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sunrpc: use snprintf to guard against buffer overflow
2020-12-03 10:09 ` Florian Weimer
2020-12-03 10:15 ` Philipp Tomsich
@ 2020-12-03 15:43 ` Martin Sebor
1 sibling, 0 replies; 5+ messages in thread
From: Martin Sebor @ 2020-12-03 15:43 UTC (permalink / raw)
To: Florian Weimer, Philipp Tomsich; +Cc: Martin Sebor, libc-alpha
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-03 15:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-02 19:04 [PATCH] sunrpc: use snprintf to guard against buffer overflow Philipp Tomsich
2020-12-03 10:09 ` Florian Weimer
2020-12-03 10:15 ` Philipp Tomsich
2020-12-03 10:19 ` Florian Weimer
2020-12-03 15:43 ` Martin Sebor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).