V0.5.1

[nmahendru]

securely zeroize the underlying object.

This is just best effort.
No one can guarantee zeroizing as the underlying devices/OS may still cover this up.

[survived]

Hi @nmahendru, good PR! Just left a few comments about it.

[survived]

It's never used in the PR. Maybe remove it?

[nmahendru]

good point! I missed that.

[survived]

Casting and writing to a raw pointer seems pretty dangerous. Can mpz_limbs_write function replace pointer casting? It provides access to the internal limbs.

[nmahendru]

Good question!
I will add a comment here as to why this.
the mpz_limbs_write is not doing compiler fencing(optimizations by compilers might get rid of the code or delay it) which is what we want to do a best effort securely wipe off the underlying limb array.
Also:

1. In gmp source the mp_limb_t is defined as unsigned long so c_int will never be more than that on any platform.
2. Memory for the array is guaranteed to be allocated by gmp in limb sizes. So we can be sure that we are not writing what we should not.

Also I think we should be having something in the gmp library itself to do this kind of a thing. I will try to do a PR for them and if that is accepted, We can remove it here.

[nmahendru]

Actually I looked the source of mpz_limbs_write just now and looks like it makes a new alloc and does not return the internal pointer. That is probably not what we want.

#include "gmp-impl.h"

mp_ptr
mpz_limbs_write (mpz_ptr x, mp_size_t n)
{
  ASSERT (n > 0);
  return MPZ_NEWALLOC (x, n);
}

[survived]

looks like it makes a new alloc

As I understand, it will make new alloc only if n > size(x), according to macro definition:

#define MPZ_REALLOC(x, n) \
  (ABSIZ(x) >= (n) ? PTR(x) : (_mpz_realloc ((x),(n)), PTR (x)))

#define MPZ_NEWALLOC MPZ_REALLOC

mpz_limbs_write is not doing compiler fencing

Maybe I missing something, but I thought that mpz_limbs_write just returns a pointer to underlying limb array, which should be the same thing as casting pointer (you can still use std::ptr::write_volatile and atomic::{fence, compiler_fence}), but this function is provided by API, so any further changes in internal representation of GMP number will not affect this code.

However, I do not believe that internal representation of GMP number might be changed, I think it's okay to leave pointer casting here.

[nmahendru]

@survived I think the serde changes which are in the crates.io package were for some reason not in master on this repo. so they are also getting added when I rebased. hope that is okie ?

or I can prepare another PR.

[survived]

You're right, it would be better to add serialization support here!

Technically rebased changes will work, but I have some thoughts about it, I believe that it can be improved in this crate.

My suggestions:

1. There's serde_json dependency, although it is only used in tests, so it can be moved to [dev-dependencies]

2. serde & serde_derive dependencies that can be combined:

   serde = { version = "1.0", features = ["derive"] }

3. There's serde_support feature, though it can be removed. Serde support can be turned on/off by toggling serde optional dependency, e.g.:

   #[cfg(feature="serde")]
   impl Serialize for Mpz {
       fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
           where
               S: Serializer,
       {
           serializer.serialize_str(&self.to_str_radix(HEX_RADIX))
       }
   }

   This code will be compiled if serde dependency is chosen and thrown out otherwise.

or I can prepare another PR.

It's more convenient to have different features in separated PRs, but if you're more comfortable with keeping them in one place, I'm good with that.

[nmahendru]

Hi @survived . I have incorporated all but suggestion 3 from your comments.

[survived]

Good! I'm merging. Zeroizing is must have here, it's a good contribution into security of upstream libraries 🙂

[ggutoski]

This merge has broken curv tag v0.2.6 https://github.com/ZenGo-X/curv/releases/tag/v0.2.6

[survived]

@ggutoski, just pushed a fix