On Fri, Apr 29, 2022 at 7:05 PM Jason A. Donenfeld <ja...@zx2c4.com> wrote:
> Rather than having getrandom() be called in a loop that handles EINTR --
> which would require more code bloat -- we just limit the maximum seed
> size to 256 bytes, which the kernel guarantees won't be interrupted.
> Additionally document the flock() and fsync() usage so that somebody
> doesn't remove it. Apparently busybox developers like to remove things
> they don't understand with no regards to security implications, so Denys
> suggested I leave some comments here.
>
> Cc: Denys Vlasenko <vda.li...@googlemail.com>
> Cc: Bernhard Reutner-Fischer <rep.dot....@gmail.com>
> Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
> ---
> util-linux/seedrng.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/util-linux/seedrng.c b/util-linux/seedrng.c
> index c42274759..5ee6197e3 100644
> --- a/util-linux/seedrng.c
> +++ b/util-linux/seedrng.c
> @@ -56,7 +56,7 @@
>
> enum {
> MIN_SEED_LEN = SHA256_OUTSIZE,
> - MAX_SEED_LEN = 512
> + MAX_SEED_LEN = 256 /* Maximum size of getrandom() call without EINTR.
> */
> };
>
> static size_t determine_optimal_seed_len(void)
> @@ -142,6 +142,8 @@ static int seed_from_file_if_exists(const char *filename,
> int dfd, bool credit,
> bb_perror_msg("can't%s seed", " read");
> return -1;
> }
> + /* 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.
> @@ -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?
> 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?
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
