[PATCH] urandom-seed: use seedrng for seeding the random number generator

[Etienne Champetier]

Hi Jason,

Le lun. 4 avr. 2022 à 14:39, Jason A. Donenfeld <Jason at zx2c4.com> a écrit :
>
> Hey Etienne,
>
> On Tue, Mar 29, 2022 at 7:21 AM Jason A. Donenfeld <Jason at zx2c4.com> wrote:
> >
> > Hi Etienne,
> >
> > On Tue, Mar 29, 2022 at 1:06 AM Etienne Champetier
> > <champetier.etienne at gmail.com> wrote:
> > > > Oh that's an interesting set of considerations and it's possible I
> > > > didn't understand some aspect of this. Most OSes should call seedrng
> > > > once at boot and once at shutdown.
> > >
> > > As routers are always on devices, it's rare to have clean shutdown.
> > > Personally, my routers boot after an upgrade or after a power loss,
> > > so they almost never shutdown properly.
> >
> > That's a good point indeed.
> >
> > > > 1) read seed into memory, delete seed from disk, write into rng &
> > > > credit if good seed, write new seed to disk; repeat at shutdown/some
> > > > other time
> > > > 2) read seed into memory, write into rng w/o crediting, re-use the
> > > > same seed next boot
> > >
> > > Before this patch we had 2 and users could opt-in to renew seed on
> > > each boot, so closer to 1.
> >
> > I guess the issue is that the implementation of (1) was somewhat
> > non-optimal, but not exactly catastrophic either.
> >
> > > Looking at random.c, I would love add_device_randomness() behavior.
> > > Maybe it was already answered on LKML,
> > > but why can't writes to /dev/urandom from a process with CAP_SYS_ADMIN
> > > be mixed in right away a la add_device_randomness() without being credited ?
> > > This would not init the RNG faster, but this would make early
> > > /dev/urandom reads "safer".
> >
> > add_device_randomness() does not mix in immediately. It goes into the
> > entropy pool, but that doesn't get extracted into a new key until the
> > next reseeding. It does get mixed in directly for crng_init=0, but not
> > for crng_init=1 or crng_init=2, which is a big gap. Making
> > /dev/urandom writes behave like that for crng_init=0 doesn't address
> > the crng_init=1 and crng_init=2 cases, unfortunately. The bigger
> > problem, though, is that some users of /dev/urandom credit the entropy
> > via the RNDADDTOENTCNT ioctl _afterwards_. If we mixed it directly in,
> > then programs with the pattern of write 4 bytes, credit 32 bits,
> > writes 4 bytes, credit 32 bits, etc could have those 4 written bytes
> > brute forced each time in what's called a "premature next". For that
> > reason the key is only modified when 256 bits have accumulated first.
> >
> > > I'm fine with writing on each boot, but as we can't rely on shutdown,
> > > what we could do with the seeds:
> > > 1) load seed.no-credit, leave it on disk
> > > 2) mv seed.credit seed.no-credit && load seed.no-credit (and credit it)
> > > 3) read from getrandom a new seed.credit
> > >
> > > This would allow to always keep a seed on disk, only use seed.credit once,
> > > and actually write seed.credit.
> > > I would get rid of the whole hashing part as all our seeds would come
> > > from getrandom().
> >
> > If possible, it's better to not leave a seed on disk after using it,
> > even if not credited. If that's the only entropy, it's better to
> > "forget" it after use, so that you can't compromise past secrets. At
> > the very least, if you have poor entropy, you can replace the seed
> > with HASH(seed), so at least it ratchets forward. Another thing to
> > consider is that if you _do_ credit it, that'll initialize the RNG, so
> > getrandom() automatically works without blocking. These two
> > observations have lead to seedrng's current scheme, where the sequence
> > is:
> >
> > - load
> > - delete
> > - seed & credit, or seed & don't credit, depending
> > - save new seed, which may be creditable or not, depending on whether
> > previous things made the rng init
> >
> > It sounds like maybe a modification of your suggestion might be to make this:
> >
> > - load
> > - delete
> > - seed & credit, or seed & don't credit, depending
> > - save new seed using getrandom(0), so that it's always creditable
> >
> > Would that satisfy your concerns? Or are you also trying to preserve a
> > mode where the filesystem doesn't need to be written to on each boot?
> >
> >
> >
> > > /var is a symlink to /tmp
> >
> > Oh, then in these cleanups, we should change that /tmp/run to /var/run
> > just to be more "correct".
> >
> > >
> > > > Is there a different place for it that would be good?
> > >
> > > Maybe we can leave it in etc and just make sure to exclude it from backups
> >
> > That seems like a good course of action.
> >
> > If you have a firm idea of what you want this to look like, would you
> > like to send a series and I'll take a look?
>
> I never heard back from you, but all the concerns you raised strike me
> as kind of important. Did you intend to move forward with those? Or
> should I just send a revert for this whole thing, so that you can
> address it some other time?

I meant to work on it last week but had 0 time, hopefully this week
will be less busy :(
Master is not less secure than before, so let's wait a bit to revert

Best
Etienne

>
> Jason