prng: support getrandom & getentropy #1367
Conversation
|
What platforms have arc4random but not getrandom? I'd rather not add support for arc4 which has known weaknesses |
|
WASI (standardized WASM interface) doesn't have getrandom() unfortunately. it has getentropy(). i can swap the define preference so getrandom() comes first. |
|
Preferring getrandom over arc4random sounds like a good move -- how does that help with getentropy, though? |
done
it's orthogonal of course. I was just noting that WASI supports arc4random & getentropy, but not getrandom. I can drop arc4random & add getentropy if you want as it still makes things work for me (just tested). |
|
If that works, that sounds preferable. getrandom and getentropy are both reasonable modern APIs. |
|
ok, it'll take me a little bit to adjust to getentropy in the general case. the documented API says it operates on max size of 256, and while all the calls in mosh production code are well below that, src/tests/encrypt-decrypt.cc grabs a couple KB :). |
|
okidoki, this version supports getrandom first, then getentropy, then /dev/urandom. while the getentropy code is annoying due to the loop, a spot check on x86_64 shows it's only a few bytes larger than getrandom when compiled with -O2. |
| } | ||
|
|
||
| #if defined(HAVE_GETRANDOM) | ||
| if ( getrandom( dest, size, 0 ) != static_cast<ssize_t>( size ) ) { |
There was a problem hiding this comment.
Reading https://manpages.debian.org/trixie/manpages-dev/getrandom.2.en.html#Interruption_by_a_signal_handler I think we should use the same loop for getrandom as we do for getentropy. It looks like otherwise we might get EINTR, which I believe would have been handled for /dev/urandom and getentropy otherwise.
There was a problem hiding this comment.
i'm not sure std::basic_istream read() handles signals for you either tbh
not that this is an argument against being defensive against EINTR
There was a problem hiding this comment.
Fair point, but given you already wrote the code, may as well reuse it?
There was a problem hiding this comment.
hmm, if we want to be defensive against EINTR, should we do it comprehensively ? from a quick scan of the codebase, i think the only time we register signal handlers is in the select wrapper. so if we leverage SA_RESTART, then the kernel should take care of this transparently for us normally.
--- a/src/util/select.h
+++ b/src/util/select.h
@@ -108,7 +108,7 @@ public:
/* Register a handler, which will only be called when pselect()
is interrupted by a (possibly queued) signal. */
struct sigaction sa;
- sa.sa_flags = 0;
+ sa.sa_flags = SA_RESTART;
sa.sa_handler = &handle_signal;
fatal_assert( 0 == sigfillset( &sa.sa_mask ) );
fatal_assert( 0 == sigaction( signum, &sa, NULL ) );
There was a problem hiding this comment.
SA_RESTART is probably worthwhile. I will note that https://man7.org/linux/man-pages/man7/signal.7.html says it's only "some" syscalls, but getrandom and most file IO are on there. But on the BSD side, FreeBSD https://man.freebsd.org/cgi/man.cgi?query=signal at leaste claims that getentropy is not on the list. But it also doesn't seem to list EINTR as a valid return, so maybe that's fine?
vapier
changed the title
| } | ||
|
|
||
| #if defined(HAVE_GETRANDOM) | ||
| if ( getrandom( dest, size, 0 ) != static_cast<ssize_t>( size ) ) { |
There was a problem hiding this comment.
Fair point, but given you already wrote the code, may as well reuse it?
If the C library supports these random functions, use them directly instead of reading the /dev/urandom file. This makes life easier on platforms that don't have /dev/urandom (like WASM).
3 participants
If the C library supports these random functions, use them directly instead of reading the /dev/urandom file. This makes life easier on platforms that don't have /dev/urandom (like WASM).