rand: don't block before random pool is initialized #33086
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
More of a RFC than a real PR. |
| @@ -63,6 +65,11 @@ mod imp { | |||
| let err = errno() as libc::c_int; | |||
| if err == libc::EINTR { | |||
| continue; | |||
| } else if err == libc::EAGAIN { | |||
| let reader = File::open("/dev/urandom")?; | |||
There was a problem hiding this comment.
getrandom_fill_bytes doesn't return a Result, so ? won't work here.
|
Presumably this is "more of an RFC" because it could be quite controversial. If so, could you lay out the motivation and drawbacks, RFC-style (if shorter and more informally)? I suspect the main drawback is that this may draw from a PRNG that has not yet collected sufficient entropy, which would be bad for predictability reasons. As for motivation, I've not heard complaints about |
cardoe
force-pushed
the
non-blocking-rand-read
branch
from
f5a3ff6 to
eb79148
Compare
|
@rkruppe If you follow #32953 in turn docopt/docopt.rs#178 you'll see the reason is that for early boot I will also point out that the delay for an initialized random pool on some hardware and some platforms is not a non-trivial amount of time (order of minutes) so it precludes a number of Rust crates (docopt and regex are two I know about currently) from being used in Rust applications that will be used in early boot. As far as The argument against this change was that many users of HashMap do not have unbounded data sets and as a result are paying a penalty by using SipHash over FNV since FNV is more performant than SipHash (ignoring the random initializing penalty) and perhaps people should be educated about how to use HashMap for their use case and select the right hasher. The issue there is that people may not know how their crate will be used and could unintentionally expose their crate's users to a vulnerability. This argument appeared to be rejected in the favor of best effort which matches how systemd works. |
If we attempt a read with getrandom() on Linux the syscall can block before the random pool is initialized unless the GRND_NONBLOCK flag is passed. This flag causes getrandom() to instead return EAGAIN while the pool is uninitialized. To avoid downstream users of crate or std functionality that have no ability to avoid this blocking behavior this change causes Rust to read bytes from /dev/urandom while getrandom() would block and once getrandom() is available to use that. Fixes rust-lang#32953. Signed-off-by: Doug Goldstein <[email protected]>
cardoe
force-pushed
the
non-blocking-rand-read
branch
from
eb79148 to
f875dac
Compare
|
Thanks for the PR @cardoe! In terms of implementation I'd probably say that because I'm gonna tag this as cc @rust-lang/libs |
alexcrichton
added
the
T-libs-api
| let reader = File::open("/dev/urandom").expect("Unable to open /dev/urandom"); | ||
| let mut reader_rng = ReaderRng::new(reader); | ||
| reader_rng.fill_bytes(& mut v[read..]); | ||
| read += v.len() as usize; |
There was a problem hiding this comment.
Needs small comment with an explanation.
There was a problem hiding this comment.
Please let me know if 61cbd07 is sufficient and I will squash it in.
|
lgtm. I feel like the HashMap docs should say something about this, but I also don't want to make a big deal out of it. Nobody should not use HashMap because of this. Is this also happening in the public rand crate? |
|
"Use bad random numbers if we can't acquire good ones." |
|
@alexcrichton That would block the access to random numbers on systems with |
|
From http://www.2uo.de/myths-about-urandom/:
As I understand it, this pull request would mean that we sidestep the protection |
The random functions that HashMap use make no guarantees about the quality of random data so this documents that to the user so that they are aware. This was brought about by the change to the Linux random code to not block until the urandom pool was initialized to avoid users of crates that internally use HashMap being caught unaware and having their application block until the urandom pool is initialized. Signed-off-by: Doug Goldstein <[email protected]>
Add some comments so that people know why we are performing a fallback from getrandom() and what that fallback aims to achieve. Signed-off-by: Doug Goldstein <[email protected]>
|
@tbu- I'd recommend reading my long explanation that I gave @rkruppe above and followed the two referenced issues. I've rehashed the reasons and the options forward in ways I feel are pretty clear between both tickets and this PR. This PR implements the option that seemed favorable by the Rust maintainers. Obviously if a different option is deemed more favorable I will implement that as well. |
|
@alexcrichton I see two issues with always having /dev/urandom opened is we're consuming a fd from a process always which might not be desirable. And we're back to being dependent on having /dev/urandom be open-able which one of the reasons people tried to get away from using it, specifically the LibreSSL people. See https://lwn.net/Articles/606141/ for some context. |
|
@tbu- The only use for this Rng is keys for the HashMap hash function. |
|
@alexcrichton Does that reasoning make sense as to why we don't want to always have |
|
We might want to consider expanding our set of platform specific versions of this logic - we could use |
|
@bluss Fair enough. But it should probably still be pointed out that we use low-quality random numbers for the hash map if there aren't any high-quality random numbers available. I don't know how that affects the attack surface, but generally Rust had chosen the most secure variant that is still practical (for hashmaps), e.g. choosing a secure hasher over a fast one. |
|
@sfackler The code already does that. Look farther down in the source file. |
|
Oh great! |
|
@tbu- I added documentation to that effect in 121225f, are you not happy with the wording? The attack surface is not really affected because the only time a hash map is vulnerable is if the data set is unbounded. Most of the time applications that allow unbounded data sets (e.g. network daemons) are going to be started up after enough hardware (an interrupt source such as a network card). The result here is that it doesn't matter. Plus a weaker random value would still have to be guessed by someone over the network to be able to attack the hash map. Honestly I would recommend doing some reading about the attack surface of hash maps, what and how they need to be seeded to be secure in those cases, the Linux random number generator implementation and the use cases of how this code will be used because you're honestly spreading a little bit of FUD here. |
Yes, albeit a seemingly much bigger hammer than is necessary here for just a pair of keys to hash maps. In any case, though, the PR looks ok to me, just wanna make sure that others in @rust-lang/libs are also signed off. |
|
@cardoe Yes, thank you. |
rand: don't block before random pool is initialized If we attempt a read with getrandom() on Linux the syscall can block before the random pool is initialized unless the GRND_NONBLOCK flag is passed. This flag causes getrandom() to instead return EAGAIN while the pool is uninitialized. To avoid downstream users of crate or std functionality that have no ability to avoid this blocking behavior this change causes Rust to read bytes from /dev/urandom while getrandom() would block and once getrandom() is available to use that. Fixes #32953. Signed-off-by: Doug Goldstein <[email protected]>
|
I realize I'm a bit late on this, but I wanted to explain why I feel this is a poor decision, and one based on a false dichotomy. The problem as expressed is (to my understanding):
This is justified by that in the fallback case to However:
In essence, I feel that this change weakens EDIT: In addition, on the BSDs, |
|
Chiming in to heartily concur with @eternaleye 's post. Also imagine you are a new-to-rust developer and you read this patch's
Now, I either need to be concerned about DoS for my use case, or I do not. If I do care, how can I evaluate whether or not If the documentation were extended to spell out the conditions in which the entropy (and thus DoS) protection is weak, can I determine whether or not my use case is in that condition? (A panic is one clear way to determine if that's the case, and for the use cases that require If it's not possible to evaluate or determine when or how this DoS protection comes into effect, then isn't its value largely diminished? That's akin to ignoring if your HTTPS connection has a valid cert or not because "encryption may help prevent network eavesdroppers". Meanwhile, for the other use case of early startup, if DoS protection does not matter, then why use a library that sometimes provides that feature anyway, instead of a simpler alternative? |
|
@eternaleye this was why we concluded that librand should be documented as not being suitable outside the standard library. |
|
@alexcrichton: But that doesn't address my point: This change directly encourages using |
|
@eternaleye Many things rely on @nejucomo Hash tables that accept unbound data sets (where a user can put any data in) are vulnerable to DoS attacks if they are not seeded. While the original bug report asked for some crates to move away from using
You assume that every daemon is accepted unbounded data that is started before the random pool is initialized. You also assume that existing crate authors are aware of this and have the ability to avoid using Honestly I would love to see the people opposed to this change propose a solution that's acceptable to the Rust team because I would be willing to implement it. Just like I was willing to implement the solution that was requested of me initially. |
|
@cardoe: Many things use Early boot is a constrained environment that requires significant care; papering over " And yes, systemd does something I consider incorrect there, especially as it does allow unbounded data: |
|
@eternaleye Enough grandstanding and throwing out irrelevant examples. Propose an actual solution to the issue at hand and we can move forward. I am more than willing to implement another solution. |
|
@cardoe: I am not grandstanding, and I'm rather insulted by the implication. In addition, the examples are not irrelevant. Fundamentally, early boot is not a POSIX environment, and it makes no guarantees whatsoever of what is available.
In addition, while blocking when unable to get randomness makes Fundamentally, early boot is a Furthermore, this ignores that not all the world is Linux. Different systems have different behaviors in an early-boot environment. For instance, early boot BSDs block on |
Note that such design follows the "secure by default" principle. In particular, consider an application that invokes the |
Where in the world do you get this crazy notion from? I'd love to see some document to back this assertion up.
So you're arguing that Rust can not be used as an init system then? So Rust is acceptable to bare metal embedded systems and to fully stood up POSIX environments only.
That's absolutely absurd assertion to make. #![no_std] is clearly not for that use case to imply it is absolutely false. I encourage you to read the Rust book again. https://doc.rust-lang.org/book/no-stdlib.html The Rust developers pointed me in this direction because users have the ability to use a different hash function with @briansmith |
The first bullet of what I suggested was indeed effectively "revert the change." Such reversion is needed to undo the compatibility regression anyway. However, the second bullet of what I suggested was the necessary step to get the behavior you want: an opt-in mechanism that any application can make to get the behavior you want. |
|
As I've said on the previous issues. Provide another hasher function in the standard library and steer crate authors to the right implementation for the right use case. I get the "secure by default" attitude but people would similarly find it over the top to establish a full TLS connection and teardown for 2 bytes of data. Its all about the right tool for the right job. In most of the crates I see using |
As was pointed out by @alexcrichton that was discussed and the team decided to proceed forward. |
|
@cardoe: You are putting words in my mouth. Please stop that.
...the fact that various things required by POSIX can be absent during early boot?
No, I said what I meant and I meant what I said. Some crates are valid and some are not.
One can then load the individual crates that are usually bundled behind the
It's not black and white " Unless the full set of assumptions behind |
Where in libstd does it call out to the basename binary or open /dev/console? We're talking about a compiled program and not a shell environment. You're still discussing apples to oranges. |
|
I never said it did either. I said early boot is not a POSIX environment, and gave two examples I was able to find quickly, rather than spend hours wading through the spec for more targeted ones. |
|
@cardoe I think that you're overly aggressive about this. |
|
Sorry if I'm late to the party — I came here after reading the patch notes for 1.10 and being concerned about the notion about reverting to It seems to me this is a poor security tradeoff. Rust programs that are intended for early-boot environments are surely a somewhat-rare use case. In these cases, such binaries should be forced to opt-in to reduced security — in some cases it's probably not a big deal, but for others it can and will cause security issues. This isn't a small project that will barely see the light of day — as Rust maintainers, you're making security decisions and tradeoffs that affect all of your users. And trading away strong security by default to favor rare use cases (early-boot programs that are short-lived and/or have bounded datasets in HashMaps) does a disservice to the majority. |
|
@stouset I suggest filing a bug about it. Comments here will be ineffective. I'm also a bit worried about this, but it's important to remember that the only thing this affects is the order things are inserted into hash tables. That is, the only attack this could enable is hash table DOS. It's not a weakening of random numbers in Rust in general. |
|
Yes, I realize that's the case. This is, to be clear, a somewhat rare use-case that the overwhelming majority Rust developers will likely never encounter. |
Update HashMap docs regarding DoS protection Because of changes to how Rust acquires randomness HashMap is not guaranteed to be DoS resistant. This commit reflects these changes in the docs themselves and provides an alternative method to creating a hash that is resistant if needed. This fixes rust-lang#33817 and includes relevant information regarding changes made in rust-lang#33086
Update HashMap docs regarding DoS protection Because of changes to how Rust acquires randomness HashMap is not guaranteed to be DoS resistant. This commit reflects these changes in the docs themselves and provides an alternative method to creating a hash that is resistant if needed. This fixes rust-lang#33817 and includes relevant information regarding changes made in rust-lang#33086
If we attempt a read with getrandom() on Linux the syscall can block
before the random pool is initialized unless the GRND_NONBLOCK flag is
passed. This flag causes getrandom() to instead return EAGAIN while the
pool is uninitialized. To avoid downstream users of crate or std
functionality that have no ability to avoid this blocking behavior this
change causes Rust to read bytes from /dev/urandom while getrandom()
would block and once getrandom() is available to use that. Fixes #32953.
Signed-off-by: Doug Goldstein [email protected]