Fortuna implementation issues

[cemeyer]

::reseed() should not be a public-facing API (easy to misuse).

::reseed() incorrectly implements SHA_d-256(). (It's missing zero blocks as initial input to the inner invocation of SHA256.) I don't believe this has any real security impact, it's just non-compliant with the spec.

Similarly, Pool incorrectly implements SHA_d-256() in the same way. I don't believe this has any impact on the security properties of Fortuna, it's just non-spec. The implementation I have worked on also has this bug, FWIW.

Both FortunaGenerator::reseed() and Pool::result() must explicitly zero the old hasher/self.state iff self.state = Sha256::new() is a pointer-assignment rather than overwrite-in-place (not sure of Rust semantics, but if it's like Java or C++, this is broken).

reseed and add_random_event() APIs must take mut inputs and cryptoutil::zero() them immediately after use to prevent leakage of RNG state. (I'm making the assumption that cryptoutil::zero cannot be optimized out, like explicit_bzero() in C. If that can be optimized out, well, it needs to be fixed.) If the SeedableRng::from_seed interface requires non-mut seed, a mut copy of seed can be made and passed in to reseed().

Repeating RNG state on fork is a pretty bad failure mode. I get that Rust stdlib does not expose fork, but your library may be used in programs that invoke fork and ideally your library is defensive against that possibility. I'm not super familiar with Rust — is there any way to detect the compilation environment (i.e., POSIX target platform) and register an atfork suicide of some kind (zeroing the generator state, at a minimum)? Some platforms support minherit() with INHERIT_ZERO to automatically zero out sensitive state on fork, although this may be difficult to use in Rust.

It is needlessly inefficient for ::generate_blocks() to recreate a new Aes instance every time it is invoked with the same key. AES key scheduling is slow; you can instead save the key state in the Fortuna object.

Additionally, care must be taken to explicitly zero Encryptor state when it is no longer used (maybe there is a destructor which does this, I did not look).

Similarly, in ::generate_random_data() (depending on Rust's semantics here — not sure if self.key = new_key is a pointer or array copy), one of new_key or the previous key array must be explicitly zeroed.

The assertion in ::generate_random_data() is incorrect in excluding rem bytes of out.len(). Just use out.len() directly.

Fortuna::last_reseed_time being a floating point number is kind of odd and unsuitable for embedded systems use. Not a bug per se just a weird design choice.

The second assertion in ::fill_bytes is redundant, or should be a compile time assertion.

Stylistic: in ::fill_bytes, the 32 in let mut hash = [0; (32 * NUM_POOLS)]; and subsequent computations should be length sizeof(u32) * sha2.STATE_LEN, although ideally the constant would be exported from the sha2 module with a clearer name, like SHA256_DIGEST_LENGTH. No functional difference.

::next_u32 should explicitly zero ret to avoid leaking a generated value.

[cemeyer]

See also #440 , and the non-dead project https://github.com/RustCrypto does not seem to have a Fortuna implementation in their "CSRNGs" repository.