The default RNG of Builders isn't cryptographically secure

[sgeisler]

DynamicHoneyBadgerBuilder and HoneyBadgerBuilder use ThreadRng as their default RNG which isn't cryptographically secure according to the rand 0.4.2 docs. Updating to a more recent version of rand would probably fix this problem.

The scariest part of the 0.4.2 ThreadRng is:

/// After generating a certain amount of randomness, the RNG will reseed itself
/// from the operating system or, if the operating system RNG returns an error,
/// a seed based on the current system time.

[mbr]

This bit is a bit tricky; I believe we used to use OsRng before, the thread_rngs crept back in when adding in builders. Checking the impact, rand 0.4.5 (the highest version we can use that is not 0.5), any thread_rng is a reseeding RNG, using a StdRng as the actual RNG, which uses an IsaacWord RNG on every platform. Certainly not ideal.

The scary part is indeed scary, luckily on Linux, I am not aware of any actual circumstance where /dev/urandom stops working.

Historically, all cryptography (e.g. threshold_crypto) used their own OsRng instances, which defaulted to OsRng. In production use we usually explicitly set the RNG though. As opposed to the externally audited threshold_crypto crate, hbbft has not been published on crates.io yet.

Regarding newer versions of rand, we did try to migrate to rand 0.5 when that was the latest version, specifically to be able to use the CryptoRng trait, but this failed because the underlying pairing crate has not kept up yet and we have not been able to get anything merged into it yet (@afck has an outstanding PR, for example). In their defense, I don't know any crate that has breaking changes at a rate as high as the rand crate though, evidenced by the fact that as of this writing, it's already on 0.6 =).

There's no excuse for the defaults not being as secure as possible though, so I will be putting in a PR soon to fix this across the board for good. We do have some other issues that interfaces that are strictly used for unit-testing only (such as the random() implementations on secret keys) are public, which should also be addressed.

Thank you very much for reporting this issue!

[mbr]

Fixed in HydraBadger as of now: poanetwork/hydrabadger@e5b9c8c

[sgeisler]

Thank you for your fast response :)

[mbr]

This has been thoroughly fixed with #357. RNGs are now firmly in the user's hands.