Hi Kevin, On 2026-04-24T13:15:52+0800, Kevin J. McCarthy wrote: > Use getrandom() if available, or else arc4random_buf(). Only fall > back to using the built-in PRNG if those aren't found on the system. > > For the sizes of data mutt typically requests, getrandom() should > never fail, but nonetheless add code to make it retry on interrupt, > and fall back to the built-in PRNG only if it has to. > > FreeBSD and OpenBSD have rewritten arc4random() to use ChaCha20, which > should provide more than sufficient randomness. The call has no > return value and always succeeds, so there is no error handling > written for it. > > Many thanks to Werner Koch, Greg KH, Alejandro Colomar, and Kurt > Hackenberg for their feedback. > --- > > This version switches to using ssize_t for the length parameter and > result from getrandom(), and thus doesn't have any unsafe casts. > > It doesn't pass GRND_NONBLOCK. Nonetheless, it checks for EINTR, > as well as checks for the result size. The man page notes: > > The user of getrandom() must always check the return value, to determine > whether either an error occurred or fewer bytes than requested were > returned. In the case where GRND_RANDOM is not specified and size is > less than or equal to 256, a return of fewer bytes than requested should > never happen, but the careful programmer will check for this anyway! > > I said I was going to take out the fallback to the built-in PRNG, but I > decided to keep it anyways, for other errors (aside from EINTR, and > EGAIN which is obviated by the lack of GRND_NONBLOCK). > > configure.ac | 4 +++ > mutt_random.c | 70 +++++++++++++++++++++++++++++++++++++++++++-------- > mutt_random.h | 3 +-- > 3 files changed, 64 insertions(+), 13 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 5b455aff..456b916a 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -423,6 +423,10 @@ AC_CHECK_TYPE(ssize_t, [], > > AC_CHECK_FUNCS(fgetpos memmove memccpy setegid srand48 strerror) > > +dnl random data functions > +AC_CHECK_HEADERS(sys/random.h) > +AC_CHECK_FUNCS(getrandom arc4random_buf) > + > AC_REPLACE_FUNCS([setenv strcasecmp strdup strsep strtok_r wcscasecmp]) > AC_REPLACE_FUNCS([strcasestr mkdtemp]) > > diff --git a/mutt_random.c b/mutt_random.c > index 52ba44e5..6e95f6bf 100644 > --- a/mutt_random.c > +++ b/mutt_random.c > @@ -25,25 +25,38 @@ > > #include <fcntl.h> > #include <string.h> > -#include <sys/time.h> > -#include <sys/types.h> > -#include <unistd.h> > +#ifdef HAVE_SYS_TIME_H > + #include <sys/time.h> > +#endif
Hmmmm, this is interesting. We didn't have that header guarded by the existence check, and so far, nobody has complained about it. This file was unconditionally compiled, AFAICS, so if it would fail, we would have noticed. The include has been unguarded since it was added 6 years ago, in 8ccd96db (2020-09-14; "Implement LFRS113 PRNG functions"). <sys/time.h> was standardized in POSIX.1-2001. And well before that, 4.3BSD-Reno already had <sys/time.h>, with the file first added in 1983 (although 4.3BSD-Reno was released later in 1990). <https://www.tuhs.org/cgi-bin/utree.pl?file=4.3BSD-Reno/src/sys/sys/time.h> Because every modern BSD is descendant of 4.3BSD-Reno, we know the file is present in every version of FreeBSD/NetBSD/OpenBSD. glibc also has the file since the very first commit. musl libc also has the file since the very first commit. I think we can assume the header exists in every Unix-like system. And checking gnulib documentation, it seems that the only platform that lacks this header file was MSVC 14. Is mutt being built there? If not, we could maybe remove that check in master everywhere. Anyway, this is off-topic for this patch. > +#ifdef HAVE_SYS_RANDOM_H > + #include <sys/random.h> > +#endif > +#include <errno.h> > + > +/* Built-in LRFS113 PRNG code and variables. > + * > + * It isn't used on OpenBSD, where only arc4random_buf() is available, > + * so undef it for that case. > + */ > +#if defined(HAVE_GETRANDOM) || !defined(HAVE_ARC4RANDOM_BUF) > > static uint32_t z[4]; /* Keep state for LFRS113 PRNG */ > static int rand_bytes_produced = 0; > static time_t time_last_reseed = 0; > > -void mutt_random_bytes(char *random_bytes, int length_requested) > +static void prng_reseed(void); > + > +static void prng_random_bytes(char *random_bytes, size_t length_requested) > { > /* Reseed every day or after more than a 100000 random bytes produced */ > if (time(NULL) - time_last_reseed > 86400 || rand_bytes_produced > 100000) > - mutt_reseed(); > + prng_reseed(); > > uint32_t b; > > /* The loop below is our implementation of the LFRS113 PRNG algorithm by > * Pierre L'Ecuyer */ > - for (int i = length_requested; i > 0;) > + for (size_t i = length_requested; i > 0;) > { > b = ((z[0] << 6) ^ z[0]) >> 13; > z[0] = ((z[0] & 4294967294U) << 18) ^ b; > @@ -56,17 +69,17 @@ void mutt_random_bytes(char *random_bytes, int > length_requested) > > b = z[0] ^ z[1] ^ z[2] ^ z[3]; > > - if (--i >= 0) random_bytes[i] = (b >> 24) & 0xFF; > - if (--i >= 0) random_bytes[i] = (b >> 16) & 0xFF; > - if (--i >= 0) random_bytes[i] = (b >> 8) & 0xFF; > - if (--i >= 0) random_bytes[i] = (b) & 0xFF; > + if (i > 0) random_bytes[--i] = (b >> 24) & 0xFF; > + if (i > 0) random_bytes[--i] = (b >> 16) & 0xFF; > + if (i > 0) random_bytes[--i] = (b >> 8) & 0xFF; > + if (i > 0) random_bytes[--i] = (b) & 0xFF; > } > rand_bytes_produced += length_requested; > > return; > } > > -void mutt_reseed(void) > +static void prng_reseed(void) > { > uint32_t t[4]; /* Temp seed values from /dev/urandom */ > char computer_says_no = TRUE; /* Whether /dev/urandom was usable */ > @@ -105,6 +118,41 @@ void mutt_reseed(void) > for (int i = 0; i <= 3; i++) > z[i] ^= t[i]; > } > +#endif /* defined(HAVE_GETRANDOM) || !defined(HAVE_ARC4RANDOM_BUF) */ > + > +/* Generate length_requested random bytes of data */ > +void mutt_random_bytes(char *random_bytes, size_t length_requested) > +{ > +#if defined(HAVE_GETRANDOM) > + ssize_t requested = length_requested; > + ssize_t result; > + > + while (requested > 0) > + { > + do > + { > + result = getrandom(random_bytes, requested, 0); > + } while ((result == -1) && (errno == EINTR)); > + > + /* On some other errno, fall back to the PRNG */ > + if (result == -1) > + { > + prng_random_bytes(random_bytes, requested); > + requested = 0; > + } > + else if (result > 0) > + { > + random_bytes += result; > + requested -= result; > + } > + } This seems better than v1. > + > +#elif defined(HAVE_ARC4RANDOM_BUF) > + arc4random_buf(random_bytes, length_requested); Should we prefer arc4random_buf(3) over getrandom(2) if it's available? It's certainly simpler, and thus less prone to subtle bugs. Anyway: Reviewed-by: Alejandro Colomar <[email protected]> Have a lovely day! Alex > +#else > + prng_random_bytes(random_bytes, length_requested); > +#endif > +} > > /* Generate and Base64 encode 96 random bits and fill the buffer > output_B64 with the result. */ > diff --git a/mutt_random.h b/mutt_random.h > index ed9fdf29..0417e0d6 100644 > --- a/mutt_random.h > +++ b/mutt_random.h > @@ -25,6 +25,5 @@ typedef union random64 > } RANDOM64; > > void mutt_base64_random96(char output_B64[static 17]); > -void mutt_random_bytes(char *random_bytes, int length_requested); > -void mutt_reseed(void); > +void mutt_random_bytes(char *random_bytes, size_t length_requested); > #endif > -- > 2.53.0 > -- <https://www.alejandro-colomar.es>
signature.asc
Description: PGP signature
