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