rust-random / rand

A Rust library for random number generation.
https://crates.io/crates/rand
Other
1.61k stars 423 forks source link

Policy: "not a crypto library"? #1358

Open dhardy opened 7 months ago

dhardy commented 7 months ago

For the original post see below the line. Deliverables tracker:


Cross-posting from here for visibility — the book may document policy, but it affects the whole project.

I've seen a few projects use rand in security sensitive code. A reviewer may eventually point them to this warning in the book: https://github.com/rust-random/book/blame/master/src/guide-rngs.md#L263-L271 Inferring that rand does not provide cryptographically secure prngs and they should use a different random library.

Well,

Is this warning out of date?

Not really; we don't have a formal policy.

Instead Rand attempts to choose reasonable compromises between performance, security and usability for a general audience. It is not marketed as a cryptographic product, but is hopefully good enough not to be a major security flaw in case it is (mis)used as one.

I'm not sure if Rand should go beyond this, unless it were to move from a volunteer project to a professional one. That said, I do have some possible ideas for improvements:

See also https://github.com/rust-random/rand/issues/543, https://github.com/rust-random/rand/issues/882, https://github.com/rust-random/rand/issues/463

CC @vks @newpavlov @josephlr @joshlf @tarcieri

newpavlov commented 7 months ago

It may be worth to additionally mention that CSPRNGs like ChaCha8Rng lack key erasure. Maybe we should introduce an additional marker trait for it?

Use zeroize around ThreadRng state

Usually we have (disabled by default) feature which enables zeroization of sensitive structs. It makes sense to add such feature to rand and CSPRNG crates.

Add fn rand::secret_rng to access the inner RNG (bypass ThreadRng's cache)

I don't think it's a particularly useful addition.

dcmiddle commented 7 months ago

Thanks for the quick responses to the issue!

I've pointed a few colleagues here in the hopes that they might help contribute patches to address documented improvement ideas like zeroize above.

Similarly if there's more to be said about "It's more of a best-effort thing, we don't spend the resources in verifying our implementations that a crypto library might."[1] it may be possible to recruit those resources from security organizations.. but only if that's a relevant goal for the project.

[1] https://github.com/rust-random/book/issues/57#issuecomment-1837138065

dhardy commented 7 months ago

FYI this just came up: #1362. It appears to me that fork reseeding works exactly as advertised, but that someone mis-understood what those advertisements are.

To recap:

Does this mean that a compromise is worse than not having fork-detection at all? Should we just remove all documentation of this capability?

This argument could be taken further: periodic reseeding (without fork) might be a useful feature in some contexts, but from a security point of view it means almost nothing: if the seeds were ever insecure or state was ever leaked then the system may already be compromised.


So, should:

It seems to be a slippery slope all the way (even from the initial position of "not a crypto library" due to mis-usages), thus perhaps the only logical solution is to put security first. However this is currently just a community volunteer project; from this position we cannot offer any form of guarantees.

nstilt1 commented 6 months ago

I'm not going to lie... I was planning on mis-using rand_chacha v0.3.1 in security contexts before I found out that it doesn't use zeroize. What got me into it was this (sort of) misleading statement from the StdRng docs:

For a secure reproducible generator, we recommend use of the rand_chacha crate directly.

If there is a common desire to become a "crypto library," I think the first step would be to create a Crypto Tracker issue of sorts that outlines the main changes that need to be made for each RNG or trait. I can try to tackle some issues though if y'all want a hand. And if it is okay to change SeedableRng, it might be beneficial for the seeds to be passed by reference. Or maybe there could be a way for just CryptoRngs to have seeds passed by reference.

dhardy commented 6 months ago

Thanks for the feedback @nstilt1.

And if it is okay to change SeedableRng, it might be beneficial for the seeds to be passed by reference.

To avoid unnecessary copies? I think changing SeedableRng is fine (we're going to cause some breakage anyway). That does beg the question of whether it should take a &Self::Seed or &[u8], but that's a topic for elsewhere (#1285).

I think the first step would be to create a Crypto Tracker issue of sorts that outlines the main changes that need to be made for each RNG or trait.

Good plan. But before jumping to that, my main concerns at this point are:

I've been pondering this over the last couple of weeks, and while we likely should clarify docs I'm less sure about other changes. But, I would like to hear others' opinions, especially that of frequent contributors or reviewers (but also of users).

tarcieri commented 6 months ago

I will note that @RustCrypto definitely stands behind our crates like chacha20 as being production-ready for cryptographic applications.

The chacha20 crate has supported zeroization since its very first release back in 2019. Hopefully soon, we'll support the rand_core API again.

dcmiddle commented 6 months ago

Yes #1362 is a helpful issue for considering this one. It illustrates that it is a pretty advanced user who knows to look for a fork issue, and to read the documentation detail related to it, and then interpret it correctly.

As a user voice...

So, Should: Rand explicitly be for non-cryptographic only? This was roughly the initial intention, though we expected some people might still mis-use the library for security critical applications.

Especially for new rust developers, rand is going to be the first place they go for a random regardless of the usage. https://doc.rust-lang.org/book/ch02-00-guessing-game-tutorial.html?highlight=random#generating-a-secret-number

I don't think rand can cede (very intentional pun) secure usages.

[Rand] Continue to compromise, offering some level of security while still advertising that this is "not a crypto library"?

To live in the gray area more effectively, I'd recommend making the warning more prominent.

Moving "not a crypto library" into "what rand is not" https://github.com/rust-random/rand/blob/master/README.md?plain=1#L29

The wording you see when you first encounter the crate includes words like secure, cryptographic, and correctness. https://github.com/rust-random/rand/blob/master/README.md?plain=1#L15 https://github.com/rust-random/rand/blob/master/README.md?plain=1#L19 https://github.com/rust-random/rand/blob/master/README.md?plain=1#L33

https://docs.rs/rand/0.8.5/src/rand/rngs/mod.rs.html#53-62

Become "a crypto library"?

As someone getting something for free, yes! :-D But considering the costs...

Volunteers: If someone has spare time to help out would it be better to direct that effort to rand or ring?

Funding: There are organizations that sponsor security work. In this case I don't know what would be material, ~but I am looking into options.~ The Open Source Security Foundation (OpenSSF) has a grant program called alpha-omega: https://alpha-omega.dev/grants/how-to-apply/

nstilt1 commented 6 months ago

Since I've been working on ChaCha, I've noticed that one of the rustdoc comments was incorrect because of some security/performance choices that split away from BlockRng (the comment in question just said that the implementation uses BlockRng, and I had to remove the comment).

Of the choices, there was zeroize for the BlockRng's results, but I have found another potential performance improvement for Block RNGs. It should be possible to have a common fill_bytes() method, and then a function signature for a generate as unsafe fn generate(dest_ptr: *mut u8, num_blocks: usize). One benefit of this is that using fill_bytes() to fill an array with more than BLOCK_SIZE * n bytes, most of, if not all of the bytes could be written directly to the array, skipping the buffer. It mainly depends on the RNG's index and the size of the destination array.

The num_blocks parameter is especially useful when there are SIMD backends that are capable of generating more than 1 block at a time. It can also assist with the use of hardware-dependent buffer sizes by calling self.core.generate(self.buffer.as_mut_ptr() as *mut u8, BUF_BLOCKS);.

Edit: Well, it turns out that it is fairly difficult to make a variable-sized buffer at runtime for x86/x86_64 instead of compile time.

dhardy commented 5 months ago

thread_rng

Fork protection

Quite likely the best option here is to not attempt to provide any fork protection at all (or at least to remove mention of this in the docs). See #1377 #1379.

Zeroize

thread_rng has a lot of potentially-sensitive memory: the RNG core + 256-byte buffer on each thread.

Usage of zeroize is not sufficient to guarantee erasure of this memory since thread-local destructors are not guaranteed to run. Usage of a static item wouldn't solve this either: "Static items do not call drop at the end of the program."

Thus, while it may still be worth adding a zeroize feature, it won't guarantee erasure of sensitive memory.

Mini-conclusion (thread_rng)

The best solution may be to stay roughly where we are already: a fast unpredictable thread_rng without formal security guarantees and without following all best practices. In that case, the main outcome of this issue is that we need to document this better.

Secure sub-set?

If there is a common desire to become a "crypto library," I think the first step would be to create a Crypto Tracker issue of sorts that outlines the main changes that need to be made for each RNG or trait.

If we did, then according to the above conclusion, thread_rng would be excluded from the "secure content", as would many other parts of the library (e.g. RngCore without CryptoRng, seed_from_u64).

Similarly if there's more to be said about "It's more of a best-effort thing, we don't spend the resources in verifying our implementations that a crypto library might."[1] it may be possible to recruit those resources from security organizations.. but only if that's a relevant goal for the project.

[1] https://github.com/rust-random/book/issues/57#issuecomment-1837138065

This would require a significant effort — but potentially that effort could be covered through grants, sponsorship and/or support contracts, assuming a suitable company is interested in taking on this role.

Should we take any steps in this direction?

I'm not really sure what the outcomes would be. Security review of rand_chacha? A new secure_rng? An organisation able to provide support contracts?

dhardy commented 2 months ago

I added a tracker to the top of this post.

The book has been updated to clarify what rand attempts to provide (without substantial changes). Comments here please.

I don't think we should track zeroize here; it's already a feature of chacha20 and we already plan to switch to that (#934).

@newpavlov mentioned:

It may be worth to additionally mention that CSPRNGs like ChaCha8Rng lack key erasure. Maybe we should introduce an additional marker trait for it?

We briefly mention forward secrecy in the book; namely that none of our RNGs provide it (except OsRng which is stateless).

Without persuasion to the contrary, my answer to both of the above is no.


My conclusion: we can close this issue now; #934 already tracks the only remaining change I'd like to see.

josephlr commented 2 months ago

@dhardy I agree with the points above. Basically, If you only need random bytes for cryptography, you probably don't need to depend on rand.

If you want secure random bytes with acceptable performance, you can call

All of these are essentially identical, and all protect against forking and state exposure (by having no state).

If you want a faster userspace CSPRNG, use the external chacha20 crate and explicitly manage the state yourself.

newpavlov commented 2 months ago

I think it's reasonable for users to use ThreadRng (and ReseedingRng in general) in cryptographic code.

dhardy commented 2 months ago

@newpavlov I would say that it depends on requirements: if forward secrecy or minimal risk of exposed state is a requirement, then it's probably better to use getrandom directly.

Some people might expect that a CSPRNG is forward-secure; none of our generators are designed with this requirement in mind (nor is ReseedingRng).

newpavlov commented 2 months ago

Yes, but my point is that in many cases (e.g. for nonce generation) forward secrecy is not important and it's fine to use ThreadRng there.

tarcieri commented 2 months ago

The current state of affairs is several crypto libraries document usage examples which use rand::thread_rng for all sorts of sensitive CryptoRng use cases including key generation. See e.g: