[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250216180638.GI10433@brightrain.aerifal.cx>
Date: Sun, 16 Feb 2025 13:06:38 -0500
From: Rich Felker <dalias@...c.org>
To: Anton Moryakov <ant.v.moryakov@...il.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH] src: string: Replace unsafe strcpy with strncat
in strcat()
On Sun, Feb 16, 2025 at 09:03:52PM +0300, Anton Moryakov wrote:
> Thanks for the feedback!
I'm not sure this is relevant, but in case it is:
https://www.openwall.com/lists/musl/2024/10/19/3
> вс, 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
> >
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.
