[PATCH v2] seedrng: limit poolsize to 256 bytes and document flock() and fsync() usage

[Emmanuel Deloget]

I'm not sure I have any right to jump into the conversation but here I am.

Le dim. 1 mai 2022 à 15:07, David Laight <David.Laight at aculab.com> a écrit :
>
> From: Jason A. Donenfeld
> > Sent: 30 April 2022 14:48
> >
> > Hi Denys,
> >
> > On Sat, Apr 30, 2022 at 3:12 PM Denys Vlasenko <vda.linux at googlemail.com> wrote:
> > > > +       /* The fsync() here is necessary for safety here, so that power being pulled
> > > > +        * at the wrong moment doesn't result in the seed being used twice by accident. */
> > > >         if ((unlink(filename) < 0 || fsync(dfd) < 0) && seed_len) {
> > > >                 bb_perror_msg("can't%s seed", " remove");
> > > >                 return -1;
> > >
> > > I don't understand the scenario mentioned here.
> > > Let's say we removed fsync() above.
> > > We read /var/lib/seedrng/seed.credit contents.
> > > We unlink() it, but this change does not go to disk yet.
> > > We seed the RNG.
> > > We recreate /var/lib/seedrng/seed.credit with new contents,
> > > but this change does not go to disk yet too.
> > > We exit.
> > > Only now, after we are done, RNG can be used for e.g. key generation.
> > >
> > > And only if the power gets pulled AFTER this key generation,
> > > and the write cached data is still not on the disk
> > > (for example, if you use ext4 or xfs, this won't happen since
> > > they synchronously wait for both O_TRUNC truncations
> > > and renames), the previous /var/lib/seedrng/seed.credit might
> > > still exist and might get reused on the next boot,
> > > and a new key can be generated from the same seed.
> > >
> > > Do you often pull power cords from machines you use for
> > > somewhat important crypto operations, such as generating keys?
> > > What are the chances that you also do it on a machine without RTC
> > > clock (which would otherwise supply randomness
> > > from the current time)?
> > >
> > > IOW: To me, this does not seem to be a realistic threat.
> > > It's a theoretical one: you can concoct a scenario where it might happen,
> > > but triggering it for real, even on purpose, would be difficult.
> >
> > Again, stop charging steadfastly toward creating vulnerabilities
> > because you don't understand things. The scenario is:
> >
> > - RNG is seeded and credited using file A.
> > - File A is unlinked but not fsync()d.
> > - TLS connection does something and a nonce is generated.
> > - System loses power and reboots.
> > - RNG is seeded and credited using same file A.
> > - TLS connection does something and the same nonce is generated,
> > resulting in catastrophic cryptographic failure.
> >
> > This is a big deal. Crediting seeds is not to be taken lightly.
> > Crediting file-based seeds is _only_ safe when they're never used
> > twice.
>
> Using the same file twice is better than having nothing at all.

I beg to differ, and especially on some embedded systems where the RNG
might be quite controllable by an attacker from the outside (mostly because
it lacks a lot of entropy crediting inputs, which is exactly the reason why we
need seedrng in the first place). This may lead to catastrophic cryptography
failures down the road.

> At least different systems use different values.
> Unless you have a remote 'dos' attack that can crash the system
> at exactly the right point in the boot sequence this is an
> entirely 'academic' error.

Having such a distinction is less helpful than one might think. First of all,
even an 'academic' vulnerability is a vulnerability. It means that basing any
embedded product on busybox's seedrng precludes said product from
being acceptable for any kind of security certification program. It doesn't
matter whether the vuln is exploitable or not at the time of certification - the
reasoning is that it's a known vuln, and it _might_ be exploitable in
the future.
Official bodies might be paranoïd here, but they have the last word.

> What is much more likely is that the file where the entropy
> is saved is just a memory overlay on top of a read-only image.
>
> That is much more likely for an embedded system than any of
> the 'failure' cases you've considered.

Given the right tools and how to use them, we developers have a duty
to use them correctly. I understand that it's not always the case, but this
is our responsibility - not the responsibility of the tool maker. If I design
a product without (at least) one way to permanently store a seed file
then I shall provide another way to seed the RNG.

Fortunately, a lot of recent SoC have a hardware RNG which will
participate to /dev/random early on.

> I also wonder how sane it is to do 'new_key = f(old_key)'.
> That doesn't seem significantly better than using the same key.

f() being a hash function with a salt + a (maybe not enough)
random value, I would think that new_key is going to be widely
different than old_key and more or less unpredictable, which is
what we want for a seed.

> For a really embedded system the only persistent storage
> could easily be a small serial EEPROM with a very limited
> number of write cycles.
> This requires special code to read/write and care to avoid
> hitting the write cycle count on a small number of memory cells.
> No amount of faffing about with filesystem accesses will
> help here at all.

As you noted by yourself, it requires special code to read/write, so
unless this use-case is fully supported in busybox (with special
implementation for open/read/write), I don't see why it should have
an impact on the security of _all_ other devices.

> There is also the case (that on my systems at least) udev
> initialisation reads from /dev/[u]random well before the S20
> script loads any saved entropy.
> I've not tried to find out what the value is used for.

I find at least one occurrence where the goal is to create a
random delay in udev-watch.c [1].

Anyway, and unless some weird security-oriented process decides
to read some random bytes without checking whether the random
pool in in a correct state (in which case I'm not sure you shall trust this
security-oriented process in the first place) the goal of seedrng is not
to protect all reads from /dev/urandom, but to make sure that we have
enough entropy to do the Real Work.

>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

It seems to me that one might want to add a
CONFIG_FAVOR_SIZE_OVER_SECURITY configuration flag. I wish to say
that I'm not sure it's a good idea. Having a small busybox is a good thing, but
having a too small busybox that breaks products because of bad security
choices is not.

Best regards,

-- Emmanuel Deloget

[1] https://github.com/systemd/systemd/blob/d9338387d94ad611233cf0753801eefccfda432a/src/udev/udev-watch.c#L102