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

rand_core: incorrect check on buffer length when seeding RNGs #764

Merged
merged 5 commits into from
Feb 15, 2021

Conversation

dhardy
Copy link
Contributor

@dhardy dhardy commented Feb 14, 2021

Summary: rand_core::le::read_u32_into and read_u64_into have incorrect checks on the source buffer length, allowing the destination buffer to be under-filled.

Implications: some downstream RNGs, including Hc128Rng (but not the more widely used ChaCha*Rng), allow seeding using the SeedableRng::from_seed trait-function with too short keys.

Link: rust-random/rand#1096


I suspect I am being over-cautious by reporting this and it can be closed, but as you say, if in doubt...
Affected versions of rand_core have not (yet) been yanked.

@Shnatsel
Copy link
Member

I agree not giving enough entropy to an RNG that's supposed to be cryptographically secure deserves an advisory.

The advisory format has transitioned to markdown. See e.g. https://github.com/RustSec/advisory-db/blob/master/crates/rgb/RUSTSEC-2020-0029.md for the new template. The file should also have the .md extension.

I've looked at our README again and that's not made sufficiently clear, so I'll file an issue about that.

@dhardy
Copy link
Contributor Author

dhardy commented Feb 15, 2021

I agree not giving enough entropy to an RNG that's supposed to be cryptographically secure deserves an advisory.

That isn't the question. This issue doesn't make correct use of the RNGs in libraries unsafe (nor does the fix make incorrect usage such as using a seed from a weak source impossible), it's just one extra check. (Note the CryptoRng docs.)

@Shnatsel Shnatsel requested a review from tarcieri February 15, 2021 11:51
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