Skip to content

Conversation

@jameshilliard
Copy link
Contributor

This should check and include sys/random.h if required for osx as mentioned here.

@fanquake fanquake added the macOS label Apr 30, 2017
@laanwj
Copy link
Member

laanwj commented May 1, 2017

utACK (can someone on MacOSX 10.12+ test this please?)

@jameshilliard
Copy link
Contributor Author

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?

@laanwj
Copy link
Member

laanwj commented May 1, 2017

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.

@theuni
Copy link
Member

theuni commented May 1, 2017

@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 MacNotificationHandler::hasUserNotificationCenterSupport.

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.

@sipa
Copy link
Member

sipa commented May 1, 2017

@theuni I'm unsure whether your comment above means that this PR is fine or the opposite.

@theuni
Copy link
Member

theuni commented May 2, 2017

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

@jameshilliard
Copy link
Contributor Author

jameshilliard commented May 2, 2017

@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);
}

@theuni
Copy link
Member

theuni commented May 2, 2017

@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

@jameshilliard
Copy link
Contributor Author

I've added a runtime fallback but I don't have an OSX system with anything older than 10.12 to test against.

@jameshilliard
Copy link
Contributor Author

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.

On Solaris the output of getentropy(2) is entropy and should not be used where randomness is needed, in particular it must not be used where an IV or nonce is needed when calling a cryptographic operation. It is intended only for seeding a user space RBG (Random Bit Generator) system. More specifically the data returned by getentropy(2) has not had the required FIPS 140-2 processing for the DRBG applied to it.

@sipa
Copy link
Member

sipa commented May 2, 2017

It is intended only for seeding a user space RBG (Random Bit Generator) system.

Great, that's what we're doing.

@jonasschnelli
Copy link
Contributor

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:








GETENTROPY(2)               BSD System Calls Manual              GETENTROPY(2)

NAME
     getentropy -- get entropy

SYNOPSIS
     #include <sys/random.h>

     int
     getentropy(void *buf, size_t buflen);

DESCRIPTION
     getentropy() fills a buffer with random data, which can be used as input
     for process-context pseudorandom generators like arc4random(3).

     The maximum buffer size permitted is 256 bytes.  If buflen exceeds this,
     an error of EIO will be indicated.

     getentropy() should be used as a replacement for random(4) when random
     data derived directly from the kernel random byte generator is required.
     Unlike the random(4) pseudo-devices, it is not vulnerable to file
     descriptor exhaustion attacks and is available when sandboxed or in a
     chroot, making it more reliable for security-critical applications.

     However, it should be noted that getentropy() is primarily intended for
     use in the construction and seeding of userspace PRNGs like arc4random(3)
     or CC_crypto(3).  Clients who simply require random data should use
     arc4random(3), CCRandomGenerateBytes() from CC_crypto(3), or
     SecRandomCopyBytes() from the Security framework instead of getentropy()
     or random(4)

RETURN VALUES
     Upon successful completion, the value 0 is returned; otherwise the
     value -1 is returned and the global variable errno is set to indicate the
     error.

ERRORS
     getentropy() will succeed unless:

     [EINVAL]           The buf parameter points to an invalid address.

     [EIO]              Too many bytes requested, or some other fatal error
                        occurred.

SEE ALSO
     arc4random(3) CC_crypto(3) random(4)

HISTORY
     The getentropy() function appeared in OSX 10.12

BSD                             October 2 2015                             BSD

configure.ac Outdated
[ AC_MSG_RESULT(no)]
)

AC_MSG_CHECKING(for getentropy rand)
Copy link
Contributor

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.

@laanwj
Copy link
Member

laanwj commented May 3, 2017

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.

Most of the OSes warn against using kernel entropy directly for cryptographic primitives. For example OpenBSD:

     getentropy() is not intended for regular code; please use the
     arc4random(3) family of functions instead.

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
[

More specifically the data returned by getentropy(2) has not had the required FIPS 140-2 processing for the DRBG applied to it.

]
This seems to imply that the function returns lower-quality instead of higher-quality entropy?!

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

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

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)

@laanwj
Copy link
Member

laanwj commented Jun 7, 2017

This has a lot of discussion but not really (ut)ACKs. Is the current approach ok, anything left to do here?

@laanwj
Copy link
Member

laanwj commented Jul 25, 2017

This is no longer relevant after #10855 (only use getentropy on openbsd)

@laanwj laanwj closed this Jul 25, 2017
@jameshilliard
Copy link
Contributor Author

@laanwj It seems #10855 would not result in getentropy being included on systems like OSX, wouldn't this still be needed?

@laanwj
Copy link
Member

laanwj commented Jul 26, 2017

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.

@jameshilliard
Copy link
Contributor Author

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.

@laanwj laanwj reopened this Jul 26, 2017
@laanwj
Copy link
Member

laanwj commented Jul 26, 2017

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.
Needs rebase.

@gmaxwell
Copy link
Contributor

This seems to imply that the function returns lower-quality instead of higher-quality entropy?!

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

@jameshilliard
Copy link
Contributor Author

I've modified this to check for OSX explicitly. Should I try and make this more universal or is that good enough for now?

@theuni
Copy link
Member

theuni commented Jul 31, 2017

I think this is ok as-is. It'd be nice to add a little refactor on top (later) to:

  • early-return on success, and have a single fall-back for /dev/urandom
  • Move detection out to a separate function so that we can log what's in use.

@theuni
Copy link
Member

theuni commented Jul 31, 2017

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.

@laanwj laanwj merged commit ee2d10a into bitcoin:master Aug 7, 2017
laanwj added a commit that referenced this pull request Aug 7, 2017
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Aug 2, 2019
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants