[PATCH] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[PATCH] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[Calvince Otieno]

strncpy() function is actively dangerous to use since it may not
NUL-terminate the destination string, resulting in potential memory
content exposures, unbounded reads, or crashes. strcpy() performs
no bounds checking on the destination buffer. The safe replacement
is strscpy() which is specific to the Linux kernel.

Signed-off-by: Calvince Otieno <calvncce@gmail.com>
---
 drivers/staging/wlan-ng/prism2fw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wlan-ng/prism2fw.c b/drivers/staging/wlan-ng/prism2fw.c
index 5d03b2b9aab4..57a99dd12143 100644
--- a/drivers/staging/wlan-ng/prism2fw.c
+++ b/drivers/staging/wlan-ng/prism2fw.c
@@ -725,7 +725,7 @@ static int plugimage(struct imgchunk *fchunk, unsigned int nfchunks,
 
 		if (j == -1) {	/* plug the filename */
 			memset(dest, 0, s3plug[i].len);
-			strncpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);
+			strscpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);
 		} else {	/* plug a PDR */
 			memcpy(dest, &pda->rec[j]->data, s3plug[i].len);
 		}

Re: [PATCH] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[Bagas Sanjaya]

On 12/10/2023 12:27, Calvince Otieno wrote:
>  		if (j == -1) {	/* plug the filename */
>  			memset(dest, 0, s3plug[i].len);
> -			strncpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);
> +			strscpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);

Is this strscpy() behavior same as previous strncpy()?

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[Calvince Otieno]

Yes, strscpy() has the same behavior as strncpy(). It is preferred to
strncpy() since it always returns
a valid string, and doesn't unnecessarily force the tail of the
destination buffer to be zeroed.

On Thu, Oct 12, 2023 at 12:17 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On 12/10/2023 12:27, Calvince Otieno wrote:
> >               if (j == -1) {  /* plug the filename */
> >                       memset(dest, 0, s3plug[i].len);
> > -                     strncpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);
> > +                     strscpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);
>
> Is this strscpy() behavior same as previous strncpy()?
>
> --
> An old man doll... just what I always wanted! - Clara
>

Re: [PATCH] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[Dan Carpenter]

This is a long email, because I have seen a bunch of people sending
strscpy() fixes and I want to explain how to write those correctly.

On Thu, Oct 12, 2023 at 08:27:49AM +0300, Calvince Otieno wrote:
> strncpy() function is actively dangerous to use since it may not
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is kind of an over statement.  This code has a " - 1" which
ensures that there is a NUL terminator.

> NUL-terminate the destination string, resulting in potential memory
> content exposures, unbounded reads, or crashes. strcpy() performs
> no bounds checking on the destination buffer.

This strcpy() sentence is unrelated to the commit.  No one was
considering using strcpy().

> The safe replacement
> is strscpy() which is specific to the Linux kernel.
> 

When you're writing the commit message, instead of talking about vague
theoretical stuff, what we want to know is the information specific to
the code you are checking.  In *this code* will the resulting string
be NUL terminated?  The other danger that strscpy() is designed to avoid
is a read overflow.  Is PRISM2_USB_FWFILE NULL terminated?  The
potential problem with strscpy() is that it does not pad the rest of the
string with zeroes and strncpy() will.  Maybe it looks something like
this:

	char buf[16];

	strscpy(buf, src, sizeof(buf));
	copy_to_user(user_pointer, buf, sizeof(buf));

If "src" is less than 15 characters long then the last characters are
a stack information leak.  So we need that analysis as well.

But then there is just the other regular string copy stuff you should
review as well.  How big is the dest buffer?  Where does s3plug[i].len
come from?  How do we know it's valid?

Sometimes these questions are quite difficult to answer, but it's not
clear from your commit message that you have tried to look for the
answers.  The question that is 100% necessary to answer is about padding
because we want to avoid introducing information leaks (security bugs).

> Signed-off-by: Calvince Otieno <calvncce@gmail.com>
> ---
>  drivers/staging/wlan-ng/prism2fw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wlan-ng/prism2fw.c b/drivers/staging/wlan-ng/prism2fw.c
> index 5d03b2b9aab4..57a99dd12143 100644
> --- a/drivers/staging/wlan-ng/prism2fw.c
> +++ b/drivers/staging/wlan-ng/prism2fw.c
> @@ -725,7 +725,7 @@ static int plugimage(struct imgchunk *fchunk, unsigned int nfchunks,
>  
>  		if (j == -1) {	/* plug the filename */
>  			memset(dest, 0, s3plug[i].len);

Pay attention to this line.

> -			strncpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);
> +			strscpy(dest, PRISM2_USB_FWFILE, s3plug[i].len - 1);

Alright, so your code has a bug where you kept the " - 1".  When you
introduce a bug, then you should always assume that other people have
probably made the same mistake.

The simplest approach is to do a:

	git grep strscpy | grep " - 1"

But the better approach would be to write a Smatch or Coverity check to
prevent these in the future.  I will add this to my lore todo list:

KTODO: write a Smatch check for strscpy(dest, src, len - 1);

regards,
dan carpenter

Re: [PATCH] staging/wlan-ng: remove strcpy() use in favor of strscpy()

[Dan Carpenter]

On Thu, Oct 12, 2023 at 01:03:40PM +0300, Dan Carpenter wrote:
> The simplest approach is to do a:
> 
> 	git grep strscpy | grep " - 1"
> 
> But the better approach would be to write a Smatch or Coverity check to
> prevent these in the future.

I meant Coccinelle not Coverity.  Duh...

Also btw, sometimes we want to keep the "don't necessarily terminate the
string behavior".  That's rare and ugly, but it does exist.

regards,
dan carpenter