-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Use getrandom syscall for all Linux and Android targets. #45896
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Submodule liblibc
updated
29 files
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Sure, but that implies that
RandomState
would then be insecure. To be concrete on the problem here: ifgetrandom
withGRND_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 togetrandom
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
forHashMap
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.