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

Policy: "not a crypto library"? #1358

Closed
2 of 5 tasks
dhardy opened this issue Dec 2, 2023 · 16 comments · Fixed by #1514
Closed
2 of 5 tasks

Policy: "not a crypto library"? #1358

dhardy opened this issue Dec 2, 2023 · 16 comments · Fixed by #1514
Labels
E-question Participation: opinions wanted

Comments

@dhardy
Copy link
Member

dhardy commented Dec 2, 2023

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,

  • Rand is not in any way certified for cryptographic security.
  • SeedableRng::from_entropy, OsRng and the getrandom crate attempt to offer strong seeding from an external source
  • CryptoRng and CryptoBlockRng are type-level documentation that implementers are supposed to be cryptographically secure (unpredictable given past outputs), but don't require any direct protection of their state (e.g. I don't think our implementations attempt to zero their state on destruction)
  • ThreadRng builds on the above and includes periodic reseeding and periodic fork-detection, but this may be insufficient in some cases (e.g. perhaps we should add a method to force immediate fork-detection for use before key-generation or even allow bypassing its buffer for when secrecy is more important than performance).

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:

  • Use zeroize around ThreadRng state
  • Add fn rand::secret_rng to access the inner RNG (bypass ThreadRng's cache)

See also #543, #882, #463

CC @vks @newpavlov @josephlr @joshlf @tarcieri

@newpavlov
Copy link
Member

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
Copy link
Contributor

dcmiddle commented Dec 9, 2023

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] rust-random/book#57 (comment)

@dhardy
Copy link
Member Author

dhardy commented Dec 9, 2023

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:

  • We could just not have any form of fork protection, but at some point...
  • Realised that adding periodic fork-detection and reseeding is practically free...
  • However this falls short of the type of immediate detection-and-reseed that would be expected in an entropy source intended for security applications.

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:

  • 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.
  • Continue to compromise, offering some level of security while still advertising that this is "not a crypto library"?
  • Become "a crypto library"?

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
Copy link

nstilt1 commented Dec 17, 2023

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
Copy link
Member Author

dhardy commented Dec 18, 2023

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:

  • If used for security-conscious applications, rand::thread_rng should put security first (especially by checking for fork on every usage, not only when generating the next block). However, this would be a significant change of purpose for something originally intended to be fast and good enough for things like hash function randomisation (where the main risk is DoS). ...
  • ... largely, this comes down to being clear what purposes a thing is fit for. (Though as said above, it might get mis-used anyway.)
  • Offering opt-in via feature flags might seem like the best of both worlds, but it's a hazard and in some ways the worst of both (might get forgotten/missed; no fine-grained control over usage).
  • Who stands behind the rand lib? Currently we're clear that we're a community project that provides no guarantees. We don't even ask for donations, and one of the reasons I would not be comfortable doing so is that where people give, they might expect something in return. ...
  • ... In other words, should we look for a company willing to stand behind the project and offer some level of verification? (This would necessarily have an impact on the nature of the project, likely with reduced interest in accepting PRs from the community.)

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
Copy link

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
Copy link
Contributor

dcmiddle commented Dec 18, 2023

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
Copy link

nstilt1 commented Dec 20, 2023

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
Copy link
Member Author

dhardy commented Feb 1, 2024

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] rust-random/book#57 (comment)

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
Copy link
Member Author

dhardy commented Apr 29, 2024

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).

  • Should we add an RNG with forward secrecy? I'm inclined to recommend usage of OsRng where it matters, but not too knowledgeable on the topic. (As I see, it's a trade-off between performance and damage limitation, where OsRng is the slowest/most secure option and something like ChaCha12Rng is at the other end of what can still claim to be a secure generator. A key-erasure CSPRNG would presumably be somewhere in between.)
  • Should we add a marker trait for this capability?

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
Copy link
Member

@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

  • rand::OsRng.try_fill_bytes
  • ring::rand::SystemRandom::new().fill
  • getrandom::getrandom

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
Copy link
Member

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

@dhardy
Copy link
Member Author

dhardy commented May 2, 2024

@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
Copy link
Member

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
Copy link

tarcieri commented May 2, 2024

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:

@dhardy
Copy link
Member Author

dhardy commented Oct 16, 2024

I propose closing this by better documenting what the library is not: #1514.

Regarding sub-issues:

  • zeroize may eventually be supported for StdRng and might clear ThreadRng's memory on (thread) exit, but we can't guarantee the latter.
  • ThreadRng uses a block to cache results, potentially leaving copies of generated output (and future outputs) in memory for a while. Not doing this in general would have a significant performance penalty, and wouldn't exactly help anyway considering ThreadRng's core is only reseeded rarely, hence it makes more sense to use OsRng / getrandom where forward secrecy or in-memory-security is important. README: rand is not a crypto library #1514 adds documentation to this effect.

dhardy added a commit that referenced this issue Oct 28, 2024
Closes #1358 by documenting what Rand is not.

Co-authored-by: Dan <dan.middleton@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants