[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHb2+K5CYJfp236MPYM=eLepUdFh+A9q42DA+reAg+Kuy2zsNQ@mail.gmail.com>
Date: Sun, 16 Feb 2025 21:03:52 +0300
From: Anton Moryakov <ant.v.moryakov@...il.com>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] src: string: Replace unsafe strcpy with strncat in strcat()
Thanks for the feedback!
вс, 16 февр. 2025 г. в 21:02, Rich Felker <dalias@...c.org>:
> On Sun, Feb 16, 2025 at 08:39:27PM +0300, Anton Moryakov wrote:
> > Static analyzer reported:
> > PROC_USE.VULNERABLE Use of vulnerable function 'strcpy' at strcat.c:5.
> This function is unsafe, use strncpy instead.
> >
> > Corrections explained:
> > Replaced the vulnerable function strcpy with strncat in strcat()
> > to prevent potential buffer overflows.
> >
> > Previous code:
> > strcpy(dest + strlen(dest), src);
> >
> > New code:
> > strncat(dest, src, strlen(src));
> >
> > This improves security but does not guarantee full buffer safety.
> > For complete protection, dest_size should be explicitly checked
> >
> > Triggers found by static analyzer Svace.
> >
> > Signed-off-by: Anton Moryakov <ant.v.moryakov@...il.com>
> >
> > ---
> > src/string/strcat.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/string/strcat.c b/src/string/strcat.c
> > index 33f749b1..d341facf 100644
> > --- a/src/string/strcat.c
> > +++ b/src/string/strcat.c
> > @@ -2,6 +2,6 @@
> >
> > char *strcat(char *restrict dest, const char *restrict src)
> > {
> > - strcpy(dest + strlen(dest), src);
> > + strncat(dest, src, strlen(src));
> > return dest;
> > }
> > --
> > 2.30.2
>
> This change makes utterly no sense. strncat is only a bounded copy if
> the argument n is the remaining buffer space in dest. However, the
> value you're passing is the length of src, which makes it functionally
> equivalent to an unbounded copy, except that it gratuitously traverses
> src twice rather than once in order to compute a length that's never
> used.
>
> If you would pay a little bit of attention to what the code is doing
> rather than what the static analyzer said, this file is the
> implementation of strcat, which is inherently only safe when you know
> the length of src is less than the remaining space in dest. There is
> nothing that can be done to make the implementation safer than the
> contract itself.
>
> Rich
>
Content of type "text/html" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
