-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Use getrandom syscall for all Linux and Android targets. #45896
Conversation
unsafe { | ||
libc::syscall(NR_GETRANDOM, buf.as_mut_ptr(), buf.len(), GRND_NONBLOCK) | ||
libc::syscall(libc::SYS_getrandom, buf.as_mut_ptr(), buf.len(), libc::GRND_NONBLOCK) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why libc::GRND_NONBLOCK
? Presumably users want this call to be blocking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GRND_NONBLOCK causes getrandom to return an error immediately rather than blocking indefinitely if the kernel randomness source isn't yet initialized. It only matters for things running around init time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The right behavior is to block indefinitely until the kernel randomness source is initialized. See, e.g., this paper. The vast majority of applications that need cryptographic randomness need it to actually be secure. The tiny minority of applications that don't can call getrandom
directly if they really want to. This is a secure-by-default vs insecure-by-default thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is not publicly exposed. It's only used by RandomState
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, Python 3.5 had some controversy about this: https://lwn.net/Articles/693189/
But Python 3.6 does default to blocking now: https://docs.python.org/3.6/library/os.html#os.urandom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is not publicly exposed. It's only used by
RandomState
.
Sure, but that implies that RandomState
would then be insecure. To be concrete on the problem here: if getrandom
with GRND_NONBLOCK
returns immediately because the entropy system isn't yet initialized, and then you fall back on reading from /dev/urandom
, it means that (barring the system getting initialized between the call to getrandom
and the read from /dev/urandom
), you'll be reading insecure entropy.
In fact, it won't just be that single read that will return insecure entropy, it could be all future reads. As section 3.1 of this paper (see the "Corollary" section under "Reality 1") describes, reading while the entropy system isn't yet initialized can actually allow an attacker to execute attacks that compromise the entropy system on an ongoing basis into the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is quite a bit of discussion about this here: #32953.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, fair enough. The TLDR of that discussion seems to be that using RandomState
for HashMap
s right after boot is problematic on embedded devices because of the blocking problem.
In keeping with the general trend of default-secure, perhaps we could have blocking be the default, but add a constructor that produces a RandomState
that doesn't block? Users that need it explicitly could call that constructor, but most users would get the secure option without having to do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't solve the underlying problem, which is that you don't have control of the construction of every HashMap in your dependency graph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point. I hadn't thought of that.
📌 Commit 62fce3b has been approved by |
⌛ Testing commit 62fce3b with merge e08637d6c1f60232536a0126699d9af4a7af6cf6... |
💔 Test failed - status-travis |
|
libc updated. |
@bors r=alexcrichton |
📌 Commit 8f2dfd1 has been approved by |
Use getrandom syscall for all Linux and Android targets. I suppose we can use it in all Linux and Android targets. In function `is_getrandom_available` is checked if the syscall is available (getrandom syscall was add in version 3.17 of Linux kernel), if the syscall is not available `fill_bytes` fallback to reading from `/dev/urandom`. Update libc to include getrandom related constants.
☀️ Test successful - status-appveyor, status-travis |
I suppose we can use it in all Linux and Android targets. In function
is_getrandom_available
is checked if the syscall is available (getrandom syscall was add in version 3.17 of Linux kernel), if the syscall is not availablefill_bytes
fallback to reading from/dev/urandom
.Update libc to include getrandom related constants.