Fix assertions inside read_{u32,u64}_into

[tmandry]

Unless I'm mistaken the size multiplier was on the wrong side, making the assertions too permissive. This could have resulted in destination buffers not being fully populated.

[newpavlov]

No, the asserts are correct. &[u64] must be 8 times longer than &[u8], i.e. on each u64 we must have 8 bytes.

[tmandry]

dst here is the &[u64], and the length of src (bytes) is what's being multiplied by 8.

• If I have 16 bytes in src and 2 u64 in dst, this assert is checking that (16 * 8) >= 2. I'd expect equality here.
• If I have 2 bytes in src and 1 u64 in dst, this assert is checking that (2 * 8) >= 1, which is also true despite not having enough bytes to fill the destination.

That doesn't make sense to me.

[tmandry]

Also this code was changed from the macro:

macro_rules! read_slice {
    ($src:expr, $dst:expr, $size:expr, $which:ident) => {{
        assert_eq!($src.len(), $size * $dst.len());

        unsafe {
            ptr::copy_nonoverlapping(
                $src.as_ptr(),
                $dst.as_mut_ptr() as *mut u8,
                $src.len());
        }
        for v in $dst.iter_mut() {
            *v = v.$which();
        }
    }};
}

Note that $dst.len() is what gets multiplied by $size.

[newpavlov]

Oops, I was looking at the right side instead of the left one. :) Yes. you are right.

[dhardy]

Yikes, good catch! This potentially affects seeding of Xoshiro128++, 256++, PCG, and HC128 generators.

The HC128 generator is even declared as a crypto generator:

impl CryptoRng for Hc128Core {}

So, how do we handle this?

• We should probably push this as a hotfix even though it could be a breaking change
• We should yank other 0.6.x releases?
• We should publish a security advisory since it can theoretically allow cryptographic RNGs to be initialised with insufficient entropy? Note though this is not security hole in itself, it merely makes it easier for other crypto libs to do something stupid, so perhaps not.

Can I ask you to add a changelog entry and bump rand_core to v0.6.2 in this PR?

[tmandry]

Can I ask you to add a changelog entry and bump rand_core to v0.6.2 in this PR?

Sure, done.

[dhardy]

Fixed version published; thanks @tmandry. I'll let the Rust advisory team decide how to deal with the rest.

[plugwash]

Hi.

This was recently filed as a release critical bug in Debian. Debian testing/unstable has version 0.5.1 of rand_core and Debian stable has version 0.3.0 of rand_core. Wolfgang Silbermayr has looked at the code for 0.5.1 and I have looked at the code for both 0.3.0 and 0.5.1 (which appears to be identical). We have concluded that this is a 0.6.x specific issue but would like confirmation from those familiar with the code base.

[dhardy]

@plugwash that is correct (it is in the advisory notice).

[vks]

@dhardy Somehow this does not show on the rustsec.org website.