random: use arc4random on OpenBSD #24238

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202202-random-use_arc4random_on_OpenBSD changing 2 files +8 −19

theStack commented at 3:20 PM on February 2, 2022: member
Inspired by a discussion on obtaining randomness on various OSes in a secp256k1 PR (https://github.com/bitcoin-core/secp256k1/pull/748#discussion_r524605472, see also https://bitcoincore.reviews/libsecp256k1-748), I think it makes sense to follow best practices and use arc4random_buf rather than getentropy on OpenBSD in our random module.

The getentropy(2) man page states:
```
getentropy() is not intended for regular code; please use the
arc4random(3) family of functions instead.
```
The arc4random(3) man page states:
```
Use of these functions is encouraged for almost all random number
consumption because the other interfaces are deficient in either quality,
portability, standardization, or availability.
```
On the linked PR discussion worries about using RC4 internally has been expressed (see https://security.stackexchange.com/questions/85601/is-arc4random-secure-enough/172905#172905), but this would only affect users of OpenBSD <5.5, using a version that was released more than 8 years ago.

random: use arc4random on OpenBSD

Following best practices on OpenBSD. The getentropy(2) man page states:
"getentropy() is not intended for regular code;
 please use the arc4random(3) family of functions instead."

5cd15ffdce

in src/random.cpp:308 in 5cd15ffdce outdated

 304 | @@ -305,16 +305,14 @@ void GetOSRand(unsigned char *ent32)
 305 |              RandFailure();
 306 |          }
 307 |      }
 308 | -#elif defined(HAVE_GETENTROPY) && defined(__OpenBSD__)

MarcoFalke commented at 4:12 PM on February 2, 2022:

nit: I think you can remove HAVE_GETENTROPY now?

theStack commented at 4:27 PM on February 2, 2022:

Good idea, done.

build: remove unneeded getentropy detection (HAVE_GETENTROPY) 0c49e52b22
theStack commented at 4:31 PM on February 2, 2022: member

Added another commit that removed the now unused getentropy detection (HAVE_GETENTROPY), as suggested by MarcoFalke. To my understanding, the inclusion of <unistd.h> within the pre-processor condition "defined(HAVE_GETENTROPY_RAND) && defined(MAC_OSX)" could also be removed, but I'm not completely sure and don't have that OS available to test. Maybe a MacOS user could comment on this.
DrahtBot added the label Build system on Feb 2, 2022
kristapsk commented at 5:06 PM on February 2, 2022: contributor

Concept ACK
laanwj commented at 6:57 PM on February 2, 2022: member

Concept ACK. I'm surprised getentropy was only used on OpenBSD. Clearly, if we use OS-specific functions on other OSes, and already have a OpenBSD specific path, then it also makes sense to use the suggested function.

but this would only affect users of OpenBSD <5.5, using a version that was released more than 8 years ago.

No need to worry about that. For the BSDs we tend to support the last major release, at most. This is the minimum level of support given how little the maintainers use them and get around to test on them. Also this is additional randomness. If it happens to have somewhat less entropy, it's no big deal.
gruve-p commented at 7:38 PM on February 2, 2022: contributor

Concept ACK
laanwj commented at 7:35 PM on February 9, 2022: member

Tested ACK 0c49e52b22be1baa8d51670e4f3c437fd3c0baa7 I verified that arc4random_buf (buf=?, n=32) is being called on OpenBSD 7.0, and the buffer is filled with different random bytes every time.
laanwj merged this on Feb 10, 2022
laanwj closed this on Feb 10, 2022
theStack deleted the branch on Feb 10, 2022
sidhujag referenced this in commit ba3551db7a on Feb 10, 2022
DrahtBot locked this on Feb 10, 2023

Contributors

Labels