Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ffi): strict the length of rng seed bytes by hashing the digest #93

Open
wants to merge 17 commits into
base: straka/arkworks_minus_cli
Choose a base branch
from

Conversation

hanyunx
Copy link
Contributor

@hanyunx hanyunx commented Apr 27, 2022

The uncertainty of the length of the seed bytes provided could trigger panic in the program. So this change enforces the strict length of 32 bytes of seeds - by hashing whatever amount of data provided into a 32-bytes seed.

Also some minor changes like comments typo.

@hanyunx hanyunx requested a review from mstraka100 April 27, 2022 19:16
@nategraf
Copy link
Contributor

A couple of things here:

  • I don't think we can actually change the RNG used in the blind function right away without breaking Valora. I'd recommend we implement this now, but use the deprecated method until we can check what impact this change will have on them. (It may not actually be a concern, but my current understanding is that it is)
    // Creates a PRNG for use in deterministic blinding of messages or in key generation from the array
    // of seeds provided as input.
    fn get_rng(seeds: &[&[u8]]) -> impl RngCore {
    let mut outer = Blake2s256::new();
    outer.update("Celo POPRF WASM RNG Seed");
    for seed in seeds.iter() {
    outer.update(Blake2s256::digest(seed));
    }
    let seed = outer.finalize();
    ChaChaRng::from_seed(seed.into())
    }
    // TODO(victor) Remove this when it is no longer used.
    fn get_rng_deprecated(digest: &[u8]) -> impl RngCore {
    let seed = from_slice(digest);
    ChaChaRng::from_seed(seed)
    }
    fn from_slice(bytes: &[u8]) -> [u8; 32] {
    let mut array = [0; 32];
    let bytes = &bytes[..array.len()]; // panics if not enough data
    array.copy_from_slice(bytes);
    array
    }
  • We should try to start reducing the number of dependencies for these modules so they are not quite so big to include in mobile and web apps, where these are intended to be used. In celo-poprf-rs I implemented the get_rng function using the DirectHasher we already use elsewhere. I'd recommend we do that here so we can remove the blake2 dependencies. https://github.com/celo-org/celo-poprf-rs/blob/a5ea39fa2e5e282c5bd8ba0b333a5ea4e92755a5/src/ffi/wasm.rs#L509-L523
  • We currently implement the get_rng function both in ffi.rs and wasm.rs. Probably best to extract it to lib.rs.

Copy link
Contributor

@nategraf nategraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I left a couple of comments to look at, in addition to getting CI to pass, before this is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants