From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 759AB2DF8B for ; Sun, 16 Feb 2025 19:02:42 +0100 (CET) Received: (qmail 19719 invoked by uid 550); 16 Feb 2025 18:02:37 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com x-ms-reactions: disallow Received: (qmail 19685 invoked from network); 16 Feb 2025 18:02:37 -0000 Date: Sun, 16 Feb 2025 13:02:28 -0500 From: Rich Felker To: Anton Moryakov Cc: musl@lists.openwall.com Message-ID: <20250216180227.GG10433@brightrain.aerifal.cx> References: <20250216173927.712148-1-ant.v.moryakov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250216173927.712148-1-ant.v.moryakov@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] src: string: Replace unsafe strcpy with strncat in strcat() 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 > > --- > 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