Fwd: mpi_set_secure leads to heap corruption

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Fwd: mpi_set_secure leads to heap corruption

Andreas Metzler-3
Hello,

this is http://bugs.debian.org/866964 submitted by Mark Wooding
<[hidden email]> against libgcrypt 1.7.6-1.7.8:

-------------------------------------------------------------
The function `mpi_set_secure' is used by `gcry_mpi_set_flag' to convert
an integer so as to use `secure' (i.e., locked, non-swappable) memory.
[...]
The code allocates enough secure memory for the active limbs, copies
them from the existing buffer, and stores a pointer to the new buffer --
all without reducing the separate count of the number of allocated
limbs.  In particular, when the securified integer is freed,
`_gcry_mpi_free' calls `_gcry_mpi_free_limb_space' to release the limb
buffer, giving it the allocated size, and the latter attempts to zeroize
the storage, leading to a heap corruption.

The patch fixes the problem.  I've not thought deeply about the
performance effects: maybe it'd be better to allocate the same total
limb buffer rather than just the active size, but this patch is simple
and obviously right.

diff --git a/mpi/mpiutil.c b/mpi/mpiutil.c
index 6dee0b9..2a32d26 100644
--- a/mpi/mpiutil.c
+++ b/mpi/mpiutil.c
@@ -260,6 +260,7 @@ mpi_set_secure( gcry_mpi_t a )
   MPN_COPY( bp, ap, a->nlimbs );
   a->d = bp;
   _gcry_mpi_free_limb_space (ap, a->alloced);
+  a->alloced = a->nlimbs;
 }
-------------------------------------------------------------

cu Andreas

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fwd: mpi_set_secure leads to heap corruption

NIIBE Yutaka
Hello,
(Cc-ed to the bug report BTS)

Thank you for forwarding the bug report.

Fixed both for master and LIBGCRYPT-1-7-BRANCH.

master:
https://git.gnupg.org/cgi-bin/gitweb.cgi?p=libgcrypt.git;a=commit;h=5feaf1cc8f22c1f8d19a34850d86fe190f1432e2

LIBGCRYPT-1-7-BRANCH:
https://git.gnupg.org/cgi-bin/gitweb.cgi?p=libgcrypt.git;a=commit;h=a195d7346a8006f3b6fb77ccd6df8e91833d2b5a

> The patch fixes the problem.  I've not thought deeply about the
> performance effects: maybe it'd be better to allocate the same total
> limb buffer rather than just the active size, but this patch is simple
> and obviously right.

Yes.  While the patch is right, I followed the suggestion for less
surprise.

While there is the API, I don't know the real use case.  So, I did
search:

    https://codesearch.debian.net/search?q=mpi_set_flag.*GCRYMPI_FLAG_SECURE

and seccure-0.5_1 has use cases.  Since all use cases are gcry_mpi_scan
then gcry_mpi_set_flag, I think that those cases are safe for heap
corruption.

==================================================
Commit: a195d7346a8006f3b6fb77ccd6df8e91833d2b5a

mpi: Fix mpi_set_secure.

* mpi/mpiutil.c (mpi_set_secure): Allocate by ->alloced.

--

The code was simply wrong.  The question is if (1) it allocates
(possibly) more or (2) modifi ->alloced.  The choice is (1).

Because we have routines of mpi_set_cond and mpi_swap_cond which
assume no change for the allocated length of limbs, no surprise is
better.  See _gcry_mpi_ec_mul_point for concrete example for those
routines.  That's for constant-time computation.

Debian-bug-id: 866964
Suggested-by: Mark Wooding <[hidden email]>
Signed-off-by: NIIBE Yutaka <[hidden email]>

(backport from master commit:
5feaf1cc8f22c1f8d19a34850d86fe190f1432e2)

1 file changed, 1 insertion(+), 1 deletion(-)
mpi/mpiutil.c | 2 +-

modified   mpi/mpiutil.c
@@ -256,7 +256,7 @@ mpi_set_secure( gcry_mpi_t a )
       gcry_assert (!ap);
       return;
     }
-  bp = mpi_alloc_limb_space (a->nlimbs, 1);
+  bp = mpi_alloc_limb_space (a->alloced, 1);
   MPN_COPY( bp, ap, a->nlimbs );
   a->d = bp;
   _gcry_mpi_free_limb_space (ap, a->alloced);

--

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fwd: mpi_set_secure leads to heap corruption

Werner Koch
On Tue,  4 Jul 2017 03:05, [hidden email] said:

> Yes.  While the patch is right, I followed the suggestion for less
> surprise.

The reason why it was falsely allocated as nlimbs is likely to save on
secure memory.  Now that we auto-grow the secure memory this is not
needed and thus this simple and correct fix is sufficient.


Salam-Shalom,

   Werner


--
Die Gedanken sind frei.  Ausnahmen regelt ein Bundesgesetz.

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

attachment0 (233 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fwd: mpi_set_secure leads to heap corruption

Andreas Metzler-3
In reply to this post by NIIBE Yutaka
On 2017-07-04 NIIBE Yutaka <[hidden email]> wrote:
[...]
> Fixed both for master and LIBGCRYPT-1-7-BRANCH.
[...]
> While there is the API, I don't know the real use case.  So, I did
> search:

>     https://codesearch.debian.net/search?q=mpi_set_flag.*GCRYMPI_FLAG_SECURE

> and seccure-0.5_1 has use cases.  Since all use cases are gcry_mpi_scan
> then gcry_mpi_set_flag, I think that those cases are safe for heap
> corruption.

Thanks. Supersonic fix + checking for amount of actual breakage. :-)

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fwd: mpi_set_secure leads to heap corruption

Mark Wooding
In reply to this post by Andreas Metzler-3
NIIBE Yutaka <[hidden email]> writes:

> Thank you for forwarding the bug report.
>
> Fixed both for master and LIBGCRYPT-1-7-BRANCH.

Thanks.

> Yes.  While the patch is right, I followed the suggestion for less
> surprise.

Fair enough.

> While there is the API, I don't know the real use case.  So, I did
> search:
>
>     https://codesearch.debian.net/search?q=mpi_set_flag.*GCRYMPI_FLAG_SECURE
>
> and seccure-0.5_1 has use cases.  Since all use cases are
> gcry_mpi_scan then gcry_mpi_set_flag, I think that those cases are
> safe for heap corruption.

Alas not.  I found this bug because seccure-0.5_1 broke on amd64 (and I
couldn't mount my backup disks again until I fixed it).  What happened
is that `gcry_mpi_scan' returned a bignum with alloced = 5 and nlimbs =
4; zeroizing the limb vector clobbered the secure-memory pool structure
in a way I didn't investigate too carefully, but the result was that
`mb_get_new' thought that the pool was full and `gcry_malloc_secure'
failed.  As far as I can make out, `seccure-decrypt' can't decrypt
anything at all on amd64.

-- [mdw]

_______________________________________________
Gcrypt-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gcrypt-devel
Loading...