linux-kernel - Re: [PATCH] coccicheck: add a test for repeat copy_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1612271916250.2001@hadrien>
Date:   Tue, 27 Dec 2016 19:21:56 +0100 (CET)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     Kees Cook <keescook@...omium.org>
cc:     linux-kernel@...r.kernel.org,
        Gilles Muller <Gilles.Muller@...6.fr>,
        Nicolas Palix <nicolas.palix@...g.fr>,
        Michal Marek <mmarek@...e.com>,
        Pengfei Wang <wpengfeinudt@...il.com>, cocci@...teme.lip6.fr,
        Vaishali Thakkar <vthakkar1994@...il.com>
Subject: Re: [PATCH] coccicheck: add a test for repeat copy_from_user

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 <keescook@...omium.org>
> ---
>  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 000000000000..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(&dest, src, sizeof(dest))

> +
> +@..._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

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.

thanks,
julia

> +*copy_from_user@p(dest2, src, size2)
> +
> +@...ipt:python depends on org@
> +p << cfu_twice.p;
> +@@
> +
> +cocci.print_main("potentially dangerous second copy_from_user()",p)
> +
> +@...ipt: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
>