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

[Jason A. Donenfeld]

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?

Jason