-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Check if sys/random.h is required for getentropy. #10301
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
Conversation
|
utACK (can someone on MacOSX 10.12+ test this please?) |
|
I tested this on MacOSX 10.12.4 myself, should we have a runtime /dev/random fallback for getentropy like we do for the linux SYS_getrandom? |
As I understand it, MacOSX guarantees that anything compiled with the MacOSX 10.12 SDK, which is necessary to get autotools to detect this in the first place doesn't work on earlier versions of the OS. In which case this would be unnecessary. Feel free to correct me though @jonasschnelli @theuni. |
|
@laanwj macOS uses weak imports for new symbols. They're fine to use as long as they're checked at runtime. This is how we're able to build with 10.11 (soon 10.12), but remain back-compat to 10.8. See an example of runtime handling at getentropy does not exist in the 10.11 sdk. So I'm assuming that it is a weak import introduced in 10.12. Meaning that this would (I believe) blow up at runtime on <= 10.11 when built with 10.12. |
|
@theuni I'm unsure whether your comment above means that this PR is fine or the opposite. |
|
@sipa Sorry for being unclear. Cautiously, the opposite. I believe that this PR would cause runtime crashes when building with SDK >= 10.12, and running on < 10.12. We definitely need someone to test that scenario before merge, as 10.12 will likely be the SDK we use for 0.15. |
|
@theuni so this means we need to do a runtime check and fallback like we do for SYS_getrandom? Something like this maybe? if (getentropy != NULL) {
if (getentropy(ent32, NUM_OS_RANDOM_BYTES) != 0) {
RandFailure();
}
} else {
GetDevURandom(ent32);
} |
|
@jameshilliard Yes. According to the apple docs it would look something like: #if defined(HAVE_GETENTROPY_RAND)
#if defined(MAC_OSX)
if (getentropy != NULL) {
getentropy(foo, bar);
} else {
// fallback
}
#else
getentropy(foo, bar);
#endif
#endif |
|
I've added a runtime fallback but I don't have an OSX system with anything older than 10.12 to test against. |
|
Apparently the Solaris version of getentropy is not safe to use directly for cryptographic functions unlike OpenBSD and OSX and getrandom should be used there instead.
|
Great, that's what we're doing. |
|
I'll give it a test (build with 10.12 and run it own 10.8). Will report soon. And I agree with @theuni (Using getentropy – which way introduced in 10.12 would crash in < 10.12). Here's the 10.12 manpage for genentropy for the records: |
configure.ac
Outdated
| [ AC_MSG_RESULT(no)] | ||
| ) | ||
|
|
||
| AC_MSG_CHECKING(for getentropy rand) |
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.
nit: for getentropy rand sounds after we are looking for a new identifier/function. I recommend to use for getentropy via random.h.
Most of the OSes warn against using kernel entropy directly for cryptographic primitives. For example OpenBSD: The foremost reason for this is because of the performance characteristics of calling the kernel for every 256 bytes of random data that is needed, the functionality is not set up for high-bandwidth randomness. Solaris' reason is more interesting
] In any case we're fine - we always combine the output with that of the OpenSSL random number generator, the OS entropy is added as an extra safety measure. |
| */ | ||
| #if defined(MAC_OSX) | ||
| // Fallback for OSX < 10.12 | ||
| if (&getentropy != NULL) { |
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.
It seems harmless to do the &getentropy != NULL check for other platforms too (avoiding the MAC_OSX specific code).
src/random.cpp
Outdated
| * The call cannot return less than the requested number of bytes. | ||
| */ | ||
| #if defined(MAC_OSX) | ||
| // Fallback for OSX < 10.12 |
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.
Keeping this comment would make sense (so those reading the code understand why the check is there)
|
This has a lot of discussion but not really (ut)ACKs. Is the current approach ok, anything left to do here? |
|
This is no longer relevant after #10855 (only use getentropy on openbsd) |
|
That's intentional, getentropy is only to be used on OpenBSD as there it's a system call. On OSX and Linux it's emulation of something at most so there it's not used. Edit: hrmph, on so OSX 10.12 it's a system call too, so in principle it could be used, but you'd have to ask @theuni. |
Yeah, that was the main purpose of this PR. |
|
Reopening then, I had understood from the title that it just adds a check, which would no longer be relevant when the function is only used on obsd. |
Nah, FIPS 140-2 processing means standard mandated low qualityness. :P Basically they do post processing to make sure they don't return all zeros or whatnot, which means (negligibly) lower entropy. The original motivation is to make certain kinds of hardware RNG failures fail secure instead of fail to making your keys all zeros; but then people go cargo-cult applying these tests on far end of cryptographic whitening, where they make no sense. In our case we use it to feed our own entropy pool, so that processing isn't useful for us (to the extent that it's useful anywhere). |
e4b5567 to
ee2d10a
Compare
|
I've modified this to check for OSX explicitly. Should I try and make this more universal or is that good enough for now? |
|
I think this is ok as-is. It'd be nice to add a little refactor on top (later) to:
|
|
utACK ee2d10a. I'm a little paranoid that an overly clever linker might try to optimize this check out of lto builds, but the crash would be obvious in that case. |
ee2d10a Check if sys/random.h is required for getentropy on OSX. (James Hilliard) Pull request description: This should check and include sys/random.h if required for osx as mentioned [here](#9821 (comment)). Tree-SHA512: e9491f67f2e8b2e6bcdbcbb8063295e844d5627daf5336e3e17b4a8027d888fa65a08e4580a745abdc35ffd8d86b4fc7434daaac172c4a06ab7566a2ed0bfb92
ee2d10a Check if sys/random.h is required for getentropy on OSX. (James Hilliard) Pull request description: This should check and include sys/random.h if required for osx as mentioned [here](bitcoin#9821 (comment)). Tree-SHA512: e9491f67f2e8b2e6bcdbcbb8063295e844d5627daf5336e3e17b4a8027d888fa65a08e4580a745abdc35ffd8d86b4fc7434daaac172c4a06ab7566a2ed0bfb92
This should check and include sys/random.h if required for osx as mentioned here.