Re: [musl] [PATCH] src: string: Replace unsafe strcpy with strncat in strcat()

[Anton Moryakov <ant.v.moryakov@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]

Thanks for the feedback!


вс, 16 февр. 2025 г. в 21:02, Rich Felker <dalias@libc.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@gmail.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
>

[-- Attachment #2: Type: text/html, Size: 2770 bytes --]