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

[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