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

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

[Anton Moryakov]

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

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

[Rich Felker]

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

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

[Anton Moryakov]

[-- 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 --]

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

[Rich Felker]

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@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
> >

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

[Anton Moryakov]

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