On Fri, 29 Jul 2022, Theo de Raadt wrote:
> The question is what _rs_random_u32() will do when it calls
> _rs_stir_if_needed().
>
> There is one potential problem. lib/libcrypto/arc4random/*.h contains
> portable wrappers for _rs_forkdetect(), which actually do things.
> memset(rs, 0, sizeof(*rs)) will trash the rs state. Let's imagine a
> "fork" has happened same time that bytes run out.
>
> _rs_stir()
> ...
> rs->rs_count = REKEY_BASE;
> _rs_random_u32 -> _rs_stir_if_needed -> _rs_forkdetect
> - all rs fields are zero'd with memset
> - _rs_forkdetect returns
>
> back in _rs_stir_if_needed,
> - if (!rs || rs->rs_count <= len)
> _rs_stir();
>
>
> So it will recurse once (only once, because a 2nd fork cannot happen).
> But this is fragile.
>
> Alternatives are to get the value direct from getentropy -- with
> KEYSZ + IVSZ + 4 maybe? Or fetch a value for this random bias early
> and track it in rs, but don't damage it in the memset? Or split
> _rs_random_u32() so that a sub-function of it may collect these 4
> keystream bytes without the _rs_stir_if_needed/_rs_rekey checks?
I don't see how a fork could trash these - do you mean one that
happened in a thread or a signal handler? AFAIK arc4random() isn't
safe in these contexts right now, even without fork().
Anyway, this version invokes the chacha context directly so there's
not possibility of _rs_stir() reentrance. It is still not safe against
something clobbering rsx concurrently (but neither is the existing
code).
Index: crypt/arc4random.c
===================================================================
RCS file: /cvs/src/lib/libc/crypt/arc4random.c,v
retrieving revision 1.56
diff -u -p -r1.56 arc4random.c
--- crypt/arc4random.c 28 Feb 2022 21:56:29 -0000 1.56
+++ crypt/arc4random.c 30 Jul 2022 08:38:44 -0000
@@ -49,6 +49,8 @@
#define BLOCKSZ 64
#define RSBUFSZ (16*BLOCKSZ)
+#define REKEY_BASE (1024*1024) /* NB. should be a power of 2 */
+
/* Marked MAP_INHERIT_ZERO, so zero'd out in fork children. */
static struct _rs {
size_t rs_have; /* valid bytes at end of rs_buf */
@@ -86,6 +88,7 @@ static void
_rs_stir(void)
{
u_char rnd[KEYSZ + IVSZ];
+ uint32_t rekey_fuzz = 0;
if (getentropy(rnd, sizeof rnd) == -1)
_getentropy_fail();
@@ -100,7 +103,10 @@ _rs_stir(void)
rs->rs_have = 0;
memset(rsx->rs_buf, 0, sizeof(rsx->rs_buf));
- rs->rs_count = 1600000;
+ /* rekey interval should not be predictable */
+ chacha_encrypt_bytes(&rsx->rs_chacha, (uint8_t *)&rekey_fuzz,
+ (uint8_t *)&rekey_fuzz, sizeof(rekey_fuzz));
+ rs->rs_count += REKEY_BASE + (rekey_fuzz % REKEY_BASE);
}
static inline void
