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

[Jason A. Donenfeld]

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.

> > @@ -190,6 +192,8 @@ int seedrng_main(int argc UNUSED_PARAM, char *argv[])
> >         if (mkdir(seed_dir, 0700) < 0 && errno != EEXIST)
> >                 bb_perror_msg_and_die("can't %s seed directory", "create");
> >         dfd = open(seed_dir, O_DIRECTORY | O_RDONLY);
> > +       /* The flock() here is absolutely necessary, as the consistency of this
> > +        * program breaks down with concurrent uses. */
> >         if (dfd < 0 || flock(dfd, LOCK_EX) < 0)
> >                 bb_perror_msg_and_die("can't %s seed directory", "lock");
>
> The locking is notoriously not reliable across networked filesystems,
> and people often find more reliable ways to ensure safety wrt concurrency.
>
> E.g. renaming the file before use (rename is atomic even on NFS).
>
> Or, for example, what if we open  /var/lib/seedrng/seed.credit,
> then try to unlink it. if unlink fails with ENOENT, this means we have
> a concurrent user. Thus, we bail out with an error message.
> Would this work?

No, because a concurrent user might have replaced seed.credit at just
the wrong moment:

readfile()
                       readfile()
unlink() = success
createnewseed()
                       unlink() = success

>
> >         printf("Saving %u bits of %screditable seed for next boot\n", (unsigned)new_seed_len * 8, new_seed_creditable ? "" : "non-");
> >         fd = open(NON_CREDITABLE_SEED_NAME, O_WRONLY | O_CREAT | O_TRUNC, 0400);
> > +       /* The fsync() here is necessary to ensure the data is written to disk before
> > +        * we attempt to make it creditable. */
> >         if (fd < 0 || full_write(fd, new_seed, new_seed_len) != (ssize_t)new_seed_len || fsync(fd) < 0) {
> >                 bb_perror_msg("can't%s seed", " write");
> >                 return program_ret | (1 << 4);
>
> Are you worrying that /var/lib/seedrng/seed.no-credit can be renamed to
> /var/lib/seedrng/seed.credit (and this metadata change can hit the disk)
> before its _contents_ is flushed to disk?

Yes. Some file systems will do this.

Jason