Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user

[Kees Cook]

On Mon, Jan 9, 2017 at 12:56 PM, Kees Cook  wrote:
> On Mon, Jan 9, 2017 at 11:08 AM, Julia Lawall  wrote:
>>
>> On Mon, 9 Jan 2017, Vaishali Thakkar wrote:
>>
>>> Here, may be we should add few more lines from Pengfei's
>>> script to avoid th potential FPs.
>>
>> Which lines (I don't have it handy)?
>
> I'm going to compare
> https://github.com/wpengfei/double_fetch_cocci/blob/master/pattern_match_linux.cocci
> to my original one, add your improvements and see what I get...

Okay, I finally had time to look at this. Pengfei added two other
logical cases that should be checked for, IIUC:

1) destination alias checking (with assignment either before or after
the first copy_from_user):

struct thing object;
struct thing *pointer = 

copy_from_user(..., );
...
copy_from_user(..., pointer);

2) field writes (via . or ->, instead of short writes):

struct thing object;

copy_from_user(..., );
...
copy_from_user(..., );


It'd probably better to convert Pengfei's into being able to run under
the coccicheck target.

-Kees

-- 
Kees Cook
Nexus Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user

[Kees Cook]

On Mon, Jan 9, 2017 at 11:08 AM, Julia Lawall  wrote:
>
>
> On Mon, 9 Jan 2017, Vaishali Thakkar wrote:
>
>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote:
>> > I totally dropped the ball on this.  Many thanks to Vaishali for
>> > resurrecting it.
>> >
>> > Some changes are suggested below.
>> >
>> > On Tue, 26 Apr 2016, Kees Cook wrote:
>> >
>> > > This is usually a sign of a resized request. This adds a check for
>> > > potential races or confusions. The check isn't 100% accurate, so it
>> > > needs some manual review.
>> > >
>> > > Signed-off-by: Kees Cook 
>> > > ---
>> > >  scripts/coccinelle/tests/reusercopy.cocci | 36
>> > > +++
>> > >  1 file changed, 36 insertions(+)
>> > >  create mode 100644 scripts/coccinelle/tests/reusercopy.cocci
>> > >
>> > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci
>> > > b/scripts/coccinelle/tests/reusercopy.cocci
>> > > new file mode 100644
>> > > index ..53645de8ae95
>> > > --- /dev/null
>> > > +++ b/scripts/coccinelle/tests/reusercopy.cocci
>> > > @@ -0,0 +1,36 @@
>> > > +/// Recopying from the same user buffer frequently indicates a pattern 
>> > > of
>> > > +/// Reading a size header, allocating, and then re-reading an entire
>> > > +/// structure. If the structure's size is not re-validated, this can 
>> > > lead
>> > > +/// to structure or data size confusions.
>> > > +///
>> > > +// Confidence: Moderate
>> > > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2.
>> > > +// URL: http://coccinelle.lip6.fr/
>> > > +// Comments:
>> > > +// Options: -no_includes -include_headers
>> >
>> > The options could be: --no-include --include-headers
>> >
>> > Actually, Coccinelle supports both, but it only officially supports the
>> > -- versions.
>> >
>> > > +
>> > > +virtual report
>> > > +virtual org
>> >
>> > Add, the following for the *s:
>> >
>> > virtual context
>> >
>> > Then add the following rule:
>> >
>> > @ok@
>> > position p;
>> > expression src,dest;
>> > @@
>> >
>> > copy_from_user@p(, src, sizeof(dest))
>> >
>> > > +
>> > > +@cfu_twice@
>> > > +position p;
>> >
>> > Change this to:
>> >
>> > position p != ok.p;
>> >
>> > > +identifier src;
>> > > +expression dest1, dest2, size1, size2, offset;
>> > > +@@
>> > > +
>> > > +*copy_from_user(dest1, src, size1)
>> > > + ... when != src = offset
>> > > + when != src += offset
>>
>> Here, may be we should add few more lines from Pengfei's
>> script to avoid th potential FPs.
>
> Which lines (I don't have it handy)?

I'm going to compare
https://github.com/wpengfei/double_fetch_cocci/blob/master/pattern_match_linux.cocci
to my original one, add your improvements and see what I get...

-Kees

>
> julia
>
>>
>> > Add the following lines:
>> >
>> >  when != if (size2 > e1 || ...) { ... return ...; }
>> >  when != if (size2 > e1 || ...) { ... size2 = e2 ... }
>> >
>> > These changes drop cases where the last argument to copy_from_usr is the
>> > size of the first argument, which seems safe enough, and where there is a
>> > test on the size value that can either update it or abort the function.
>> > These changes only eliminate false positives, as far as I could tell.
>> >
>> > If it would be more convenient, I could just send the complete revised
>> > patch, or whatever seems convenient.
>>
>> I was also thinking that probably we should also add other user space memory
>> API functions. May be get_user and strncpy_from_user. Although I'm not sure
>> how common it is to find such patterns for both of these functions.
>>
>> > thanks,
>> > julia
>> >
>> > > +*copy_from_user@p(dest2, src, size2)
>> > > +
>> > > +@script:python depends on org@
>> > > +p << cfu_twice.p;
>> > > +@@
>> > > +
>> > > +cocci.print_main("potentially dangerous second copy_from_user()",p)
>> > > +
>> > > +@script:python depends on report@
>> > > +p << cfu_twice.p;
>> > > +@@
>> > > +
>> > > +coccilib.report.print_report(p[0],"potentially dangerous second
>> > > copy_from_user()")
>> > > --
>> > > 2.6.3
>> > >
>> > >
>> > > --
>> > > Kees Cook
>> > > Chrome OS & Brillo Security
>> > >
>> > ___
>> > Cocci mailing list
>> > Cocci@systeme.lip6.fr
>> > https://systeme.lip6.fr/mailman/listinfo/cocci
>> >
>>
>>



-- 
Kees Cook
Nexus Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user

[Julia Lawall]

On Mon, 9 Jan 2017, Vaishali Thakkar wrote:

> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote:
> > I totally dropped the ball on this.  Many thanks to Vaishali for
> > resurrecting it.
> >
> > Some changes are suggested below.
> >
> > On Tue, 26 Apr 2016, Kees Cook wrote:
> >
> > > This is usually a sign of a resized request. This adds a check for
> > > potential races or confusions. The check isn't 100% accurate, so it
> > > needs some manual review.
> > >
> > > Signed-off-by: Kees Cook 
> > > ---
> > >  scripts/coccinelle/tests/reusercopy.cocci | 36
> > > +++
> > >  1 file changed, 36 insertions(+)
> > >  create mode 100644 scripts/coccinelle/tests/reusercopy.cocci
> > >
> > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci
> > > b/scripts/coccinelle/tests/reusercopy.cocci
> > > new file mode 100644
> > > index ..53645de8ae95
> > > --- /dev/null
> > > +++ b/scripts/coccinelle/tests/reusercopy.cocci
> > > @@ -0,0 +1,36 @@
> > > +/// Recopying from the same user buffer frequently indicates a pattern of
> > > +/// Reading a size header, allocating, and then re-reading an entire
> > > +/// structure. If the structure's size is not re-validated, this can lead
> > > +/// to structure or data size confusions.
> > > +///
> > > +// Confidence: Moderate
> > > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2.
> > > +// URL: http://coccinelle.lip6.fr/
> > > +// Comments:
> > > +// Options: -no_includes -include_headers
> >
> > The options could be: --no-include --include-headers
> >
> > Actually, Coccinelle supports both, but it only officially supports the
> > -- versions.
> >
> > > +
> > > +virtual report
> > > +virtual org
> >
> > Add, the following for the *s:
> >
> > virtual context
> >
> > Then add the following rule:
> >
> > @ok@
> > position p;
> > expression src,dest;
> > @@
> >
> > copy_from_user@p(, src, sizeof(dest))
> >
> > > +
> > > +@cfu_twice@
> > > +position p;
> >
> > Change this to:
> >
> > position p != ok.p;
> >
> > > +identifier src;
> > > +expression dest1, dest2, size1, size2, offset;
> > > +@@
> > > +
> > > +*copy_from_user(dest1, src, size1)
> > > + ... when != src = offset
> > > + when != src += offset
>
> Here, may be we should add few more lines from Pengfei's
> script to avoid th potential FPs.

Which lines (I don't have it handy)?

julia

>
> > Add the following lines:
> >
> >  when != if (size2 > e1 || ...) { ... return ...; }
> >  when != if (size2 > e1 || ...) { ... size2 = e2 ... }
> >
> > These changes drop cases where the last argument to copy_from_usr is the
> > size of the first argument, which seems safe enough, and where there is a
> > test on the size value that can either update it or abort the function.
> > These changes only eliminate false positives, as far as I could tell.
> >
> > If it would be more convenient, I could just send the complete revised
> > patch, or whatever seems convenient.
>
> I was also thinking that probably we should also add other user space memory
> API functions. May be get_user and strncpy_from_user. Although I'm not sure
> how common it is to find such patterns for both of these functions.
>
> > thanks,
> > julia
> >
> > > +*copy_from_user@p(dest2, src, size2)
> > > +
> > > +@script:python depends on org@
> > > +p << cfu_twice.p;
> > > +@@
> > > +
> > > +cocci.print_main("potentially dangerous second copy_from_user()",p)
> > > +
> > > +@script:python depends on report@
> > > +p << cfu_twice.p;
> > > +@@
> > > +
> > > +coccilib.report.print_report(p[0],"potentially dangerous second
> > > copy_from_user()")
> > > --
> > > 2.6.3
> > >
> > >
> > > --
> > > Kees Cook
> > > Chrome OS & Brillo Security
> > >
> > ___
> > Cocci mailing list
> > Cocci@systeme.lip6.fr
> > https://systeme.lip6.fr/mailman/listinfo/cocci
> >
>
>
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user

[Vaishali Thakkar]

On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote:

I totally dropped the ball on this.  Many thanks to Vaishali for
resurrecting it.

Some changes are suggested below.

On Tue, 26 Apr 2016, Kees Cook wrote:


This is usually a sign of a resized request. This adds a check for
potential races or confusions. The check isn't 100% accurate, so it
needs some manual review.

Signed-off-by: Kees Cook 
---
 scripts/coccinelle/tests/reusercopy.cocci | 36 +++
 1 file changed, 36 insertions(+)
 create mode 100644 scripts/coccinelle/tests/reusercopy.cocci

diff --git a/scripts/coccinelle/tests/reusercopy.cocci 
b/scripts/coccinelle/tests/reusercopy.cocci
new file mode 100644
index ..53645de8ae95
--- /dev/null
+++ b/scripts/coccinelle/tests/reusercopy.cocci
@@ -0,0 +1,36 @@
+/// Recopying from the same user buffer frequently indicates a pattern of
+/// Reading a size header, allocating, and then re-reading an entire
+/// structure. If the structure's size is not re-validated, this can lead
+/// to structure or data size confusions.
+///
+// Confidence: Moderate
+// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2.
+// URL: http://coccinelle.lip6.fr/
+// Comments:
+// Options: -no_includes -include_headers


The options could be: --no-include --include-headers

Actually, Coccinelle supports both, but it only officially supports the
-- versions.


+
+virtual report
+virtual org


Add, the following for the *s:

virtual context

Then add the following rule:

@ok@
position p;
expression src,dest;
@@

copy_from_user@p(, src, sizeof(dest))


+
+@cfu_twice@
+position p;


Change this to:

position p != ok.p;


+identifier src;
+expression dest1, dest2, size1, size2, offset;
+@@
+
+*copy_from_user(dest1, src, size1)
+ ... when != src = offset
+ when != src += offset


Here, may be we should add few more lines from Pengfei's
script to avoid th potential FPs.


Add the following lines:

 when != if (size2 > e1 || ...) { ... return ...; }
 when != if (size2 > e1 || ...) { ... size2 = e2 ... }

These changes drop cases where the last argument to copy_from_usr is the
size of the first argument, which seems safe enough, and where there is a
test on the size value that can either update it or abort the function.
These changes only eliminate false positives, as far as I could tell.

If it would be more convenient, I could just send the complete revised
patch, or whatever seems convenient.


I was also thinking that probably we should also add other user space 
memory API functions. May be get_user and strncpy_from_user. Although 
I'm not sure how common it is to find such patterns for both of these 
functions.



thanks,
julia


+*copy_from_user@p(dest2, src, size2)
+
+@script:python depends on org@
+p << cfu_twice.p;
+@@
+
+cocci.print_main("potentially dangerous second copy_from_user()",p)
+
+@script:python depends on report@
+p << cfu_twice.p;
+@@
+
+coccilib.report.print_report(p[0],"potentially dangerous second 
copy_from_user()")
--
2.6.3


--
Kees Cook
Chrome OS & Brillo Security


___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci



___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci