linux-kernel - Re: [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"

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: <CAAUqJDuDsgLZ_7i=knqFNkqWJn+G3FqE3Yv=RBLr27mBMJk1Cg@mail.gmail.com>
Date:   Wed, 30 Sep 2020 16:35:57 +0200
From:   Ondrej Mosnáček <omosnacek@...il.com>
To:     Colin King <colin.king@...onical.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Tianjia Zhang <tianjia.zhang@...ux.alibaba.com>,
        Waiman Long <longman@...hat.com>,
        kernel-janitors@...r.kernel.org, linux-crypto@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"

st 30. 9. 2020 o 15:04 Colin King <colin.king@...onical.com> napísal(a):
>
> From: Colin Ian King <colin.king@...onical.com>
>
> There is an off-by-one range check on the upper limit of
> index "no".  Fix this by changing the > comparison to >=

Note that this doesn't completely fix the bug though... (see below)

>
> Addresses-Coverity: ("Out-of-bounds read")
> Fixes: a8ea8bdd9df9 ("lib/mpi: Extend the MPI library")
> Signed-off-by: Colin Ian King <colin.king@...onical.com>
> ---
>
> resend to Cc linux-crypto
>
> ---
>  lib/mpi/mpiutil.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/mpi/mpiutil.c b/lib/mpi/mpiutil.c
> index 3c63710c20c6..632d0a4bf93f 100644
> --- a/lib/mpi/mpiutil.c
> +++ b/lib/mpi/mpiutil.c
> @@ -69,7 +69,7 @@ postcore_initcall(mpi_init);
>   */
>  MPI mpi_const(enum gcry_mpi_constants no)
>  {
> -       if ((int)no < 0 || no > MPI_NUMBER_OF_CONSTANTS)
> +       if ((int)no < 0 || no >= MPI_NUMBER_OF_CONSTANTS)
>                 pr_err("MPI: invalid mpi_const selector %d\n", no);

What the code does is it just logs an error if the value is out of
range, but then it happily continues dereferencing the array anyway...
In the original libgcrypt code [1] (which BTW needs this patch, too),
there is log_bug() instead of pr_err(), which doesn't just log the
error, but also abort()'s the program. BUG() would be the correct
kernel equivalent for log_bug(). It seems the whole kernel's MPI
library clone should be re-audited for other instances of pr_*()'s
that should in fact be BUG()'s (or even better, WARN_ONCE()'s with
proper error handling, but that might diverge the code from libgcrypt
too much...).

[1] https://github.com/gpg/libgcrypt/blob/9cd92ebae21900e54cc3d8b607c8ed1afbf2eb9b/mpi/mpiutil.c#L773

>         if (!constants[no])
>                 pr_err("MPI: MPI subsystem not initialized\n");
> --
> 2.27.0
>