[PATCH] staging: android: Replace strcpy with strlcpy

[PATCH] staging: android: Replace strcpy with strlcpy

[simran singhal]

Replace strcpy with strlcpy as strcpy does not check for buffer
overflow.
This is found using Flawfinder.

Signed-off-by: simran singhal <singhalsimran0@gmail.com>
---
 drivers/staging/android/ashmem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 7cbad0d..eb2f4ef 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
 	if (unlikely(asma->file))
 		ret = -EINVAL;
 	else
-		strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
+		strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
+			sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
 
 	mutex_unlock(&ashmem_mutex);
 	return ret;
-- 
2.7.4

Re: [Outreachy kernel] [PATCH] staging: android: Replace strcpy with strlcpy

[Julia Lawall]

On Sun, 12 Mar 2017, simran singhal wrote:

> Replace strcpy with strlcpy as strcpy does not check for buffer
> overflow.
> This is found using Flawfinder.
>
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> ---
>  drivers/staging/android/ashmem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index 7cbad0d..eb2f4ef 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
>  	if (unlikely(asma->file))
>  		ret = -EINVAL;
>  	else
> -		strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
> +		strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
> +			sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));

There is a parenthesis in the wrong place.

julia

>
>  	mutex_unlock(&ashmem_mutex);
>  	return ret;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170311204001.GA13301%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [PATCH] staging: android: Replace strcpy with strlcpy

[Al Viro]

On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
> Replace strcpy with strlcpy as strcpy does not check for buffer
> overflow.
> This is found using Flawfinder.
> 
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> ---
>  drivers/staging/android/ashmem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index 7cbad0d..eb2f4ef 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
>  	if (unlikely(asma->file))
>  		ret = -EINVAL;
>  	else
> -		strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
> +		strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
> +			sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));

Trivial C quiz: given

struct ashmem_area {
        char name[ASHMEM_FULL_NAME_LEN];
        struct list_head unpinned_list;
        struct file *file;
        size_t size;
        unsigned long prot_mask;
};
static int set_name(struct ashmem_area *asma, void __user *name)

what, in your opinion, would be
	1) type of asma->name
	2) type of asma->name + ASHMEM_NAME_PREFIX_LEN
	3) value of sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN)
As a bonus question,
	4) what is the value of this kind of patches?

<rot13 answers>
	1) NFUZRZ_SHYY_ANZR_YRA-ryrzrag neenl bs pune
	2) cbvagre gb pune
	3) fvmr bs n cbvagre
	4) fbpvbybtvpny - ernql-znqr vyyhfgengvbaf bs crevyf bs
pnetb phyg.

Re: [Outreachy kernel] [PATCH] staging: android: Replace strcpy with strlcpy

[Al Viro]

On Sat, Mar 11, 2017 at 09:47:30PM +0100, Julia Lawall wrote:
> 
> 
> On Sun, 12 Mar 2017, simran singhal wrote:
> 
> > Replace strcpy with strlcpy as strcpy does not check for buffer
> > overflow.
> > This is found using Flawfinder.
> >
> > Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> > ---
> >  drivers/staging/android/ashmem.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> > index 7cbad0d..eb2f4ef 100644
> > --- a/drivers/staging/android/ashmem.c
> > +++ b/drivers/staging/android/ashmem.c
> > @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
> >  	if (unlikely(asma->file))
> >  		ret = -EINVAL;
> >  	else
> > -		strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
> > +		strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
> > +			sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
> 
> There is a parenthesis in the wrong place.

Worse - moving parenthesis to just after asma->name would result in
interestingly bogus value (size + amount skipped instead of size -
amount skipped).

Folks, blind changes in name of security are seriously counterproductive;
fortunately, in this particular case overflow prevention is taken care
of by earlier code (source of strcpy is a local array of size that
isn't enough to cause trouble and it is NUL-terminated), so that
particular strlcpy() is simply pointless, but if not for that...
Variant with sizeof(asma->name) + ASHMEM_NAME_PREFIX_LEN would've
invited an overflow *and* made it harder to spot in the future -
"it uses strlcpy, no worries about overflows here"...

Re: [PATCH] staging: android: Replace strcpy with strlcpy

[Dan Carpenter]

On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
> Replace strcpy with strlcpy as strcpy does not check for buffer
> overflow.
> This is found using Flawfinder.
> 
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> ---
>  drivers/staging/android/ashmem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index 7cbad0d..eb2f4ef 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
>  	if (unlikely(asma->file))
>  		ret = -EINVAL;
>  	else
> -		strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
> +		strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
> +			sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This isn't right.

Also please do some analysis to see if it's a real bug or a false
positive.  It is a false positive in this case.

regards,
dan carpenter

Re: [PATCH] staging: android: Replace strcpy with strlcpy

[SIMRAN SINGHAL]

On Mon, Mar 13, 2017 at 6:11 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
>> Replace strcpy with strlcpy as strcpy does not check for buffer
>> overflow.
>> This is found using Flawfinder.
>>
>> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
>> ---
>>  drivers/staging/android/ashmem.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
>> index 7cbad0d..eb2f4ef 100644
>> --- a/drivers/staging/android/ashmem.c
>> +++ b/drivers/staging/android/ashmem.c
>> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
>>       if (unlikely(asma->file))
>>               ret = -EINVAL;
>>       else
>> -             strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
>> +             strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
>> +                     sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This isn't right.
>
> Also please do some analysis to see if it's a real bug or a false
> positive.  It is a false positive in this case.
>

Dan,
I have already sent v3 of this in which I have used:
sizeof(asma->name) - ASHMEM_NAME_PREFIX_LEN

Thanks!
Simran

> regards,
> dan carpenter
>

Re: [PATCH] staging: android: Replace strcpy with strlcpy

[Dan Carpenter]

On Mon, Mar 13, 2017 at 06:17:22PM +0530, SIMRAN SINGHAL wrote:
> On Mon, Mar 13, 2017 at 6:11 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
> >> Replace strcpy with strlcpy as strcpy does not check for buffer
> >> overflow.
> >> This is found using Flawfinder.
> >>
> >> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> >> ---
> >>  drivers/staging/android/ashmem.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> >> index 7cbad0d..eb2f4ef 100644
> >> --- a/drivers/staging/android/ashmem.c
> >> +++ b/drivers/staging/android/ashmem.c
> >> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
> >>       if (unlikely(asma->file))
> >>               ret = -EINVAL;
> >>       else
> >> -             strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
> >> +             strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
> >> +                     sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This isn't right.
> >
> > Also please do some analysis to see if it's a real bug or a false
> > positive.  It is a false positive in this case.
> >
> 
> Dan,
> I have already sent v3 of this in which I have used:
> sizeof(asma->name) - ASHMEM_NAME_PREFIX_LEN

Yeah.  I saw that.  It's fine, I suppose but you should have done more
analysis to see if it was a real bug like Al and Greg suggested.  The
changelog should say something like:

"The destination buffer is 12345 bytes long but we're copying a 10000
character string so it can overflow."  Occasionally, I will fudge a
little bit on these changelogs to say that I have looked every where to
determine the size of the source buffer and can't figure it out so this
change makes it easier to audit.  But I try to figure it out generally.

Really tools should be able to show that this code is safe.  They
currently don't so far as I know, but they should.  It's a matter of
waiting a year for Smatch to improve.

regards,
dan carpenter

Re: [PATCH] staging: android: Replace strcpy with strlcpy

[SIMRAN SINGHAL]

On Mon, Mar 13, 2017 at 6:27 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Mar 13, 2017 at 06:17:22PM +0530, SIMRAN SINGHAL wrote:
>> On Mon, Mar 13, 2017 at 6:11 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > On Sun, Mar 12, 2017 at 02:10:01AM +0530, simran singhal wrote:
>> >> Replace strcpy with strlcpy as strcpy does not check for buffer
>> >> overflow.
>> >> This is found using Flawfinder.
>> >>
>> >> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
>> >> ---
>> >>  drivers/staging/android/ashmem.c | 3 ++-
>> >>  1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
>> >> index 7cbad0d..eb2f4ef 100644
>> >> --- a/drivers/staging/android/ashmem.c
>> >> +++ b/drivers/staging/android/ashmem.c
>> >> @@ -548,7 +548,8 @@ static int set_name(struct ashmem_area *asma, void __user *name)
>> >>       if (unlikely(asma->file))
>> >>               ret = -EINVAL;
>> >>       else
>> >> -             strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
>> >> +             strlcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
>> >> +                     sizeof(asma->name + ASHMEM_NAME_PREFIX_LEN));
>> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> > This isn't right.
>> >
>> > Also please do some analysis to see if it's a real bug or a false
>> > positive.  It is a false positive in this case.
>> >
>>
>> Dan,
>> I have already sent v3 of this in which I have used:
>> sizeof(asma->name) - ASHMEM_NAME_PREFIX_LEN
>
> Yeah.  I saw that.  It's fine, I suppose but you should have done more
> analysis to see if it was a real bug like Al and Greg suggested.  The
> changelog should say something like:
>
> "The destination buffer is 12345 bytes long but we're copying a 10000
> character string so it can overflow."  Occasionally, I will fudge a
> little bit on these changelogs to say that I have looked every where to
> determine the size of the source buffer and can't figure it out so this
> change makes it easier to audit.  But I try to figure it out generally.
>
> Really tools should be able to show that this code is safe.  They
> currently don't so far as I know, but they should.  It's a matter of
> waiting a year for Smatch to improve.
>

Thanks!

Will keep this in mind.

> regards,
> dan carpenter
>