On Sat, Jul 30, 2022 at 06:40:21PM +1000, Damien Miller wrote:
> 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);
Replace += with =. With that fixed, OK visa@
