On Fri, Jul 29, 2022 at 06:56:08AM -0600, Theo de Raadt wrote: > Visa Hankala <v...@hankala.org> wrote: > > > On Thu, Jul 28, 2022 at 11:00:12AM +1000, Damien Miller wrote: > > > + rs->rs_count = REKEY_BASE; > > > + /* rekey interval should not be predictable */ > > > + _rs_random_u32(&rekey_fuzz); > > > + rs->rs_count += rekey_fuzz % REKEY_BASE; > > > > The randomization looks good. > > > > However, might it cause a problem (in the future) that _rs_random_u32() > > calls _rs_stir_if_needed()? rs_count has a largish value so a recursive > > invocation of _rs_stir() should not happen, but anyway. > > 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?
_rs_stir() clears rs_buf, so a rekey is needed if the fuzz value is taken from the keystream. Another option is to move the _rs_stir_if_needed() calls from _rs_random_u32() and _rs_random_buf() to arc4random() and arc4random_buf(). The latter two are the subsystem's entry points. Taking the fuzz value directly from getentropy() would be a clear approach that does not add odd hoops, though some might feel it uneconomic use of system entropy. ;)
