Good day Jason,
On Wed, Apr 20, 2022 at 6:29 PM Jason A. Donenfeld <[email protected]> wrote:
> Hi Bernhard,
>
> On Wed, Apr 20, 2022 at 3:55 PM Bernhard Reutner-Fischer
> <[email protected]> wrote:
> > I've applied this v9 now, thanks for the patch and thanks a lot for your
> > patience!
>
> Excellent! Thank you. Feel free to CC me on other things you have
> planned there -- happy to review.
Looking at the current git.
In read_new_seed(), if getrandom(GRND_NONBLOCK) reads
less than len bytes:
static int read_new_seed(uint8_t *seed, size_t len, bool *is_creditable)
{
ssize_t ret;
*is_creditable = false;
ret = getrandom(seed, len, GRND_NONBLOCK);
if (ret == (ssize_t)len) {
*is_creditable = true;
return 0;
}
if (ret < 0 && errno == ENOSYS) {
.fd = open("/dev/random", O_RDONLY),
...
*is_creditable = poll(&random_fd, 1, 0) == 1;
close(random_fd.fd);
} else if (getrandom(seed, len, GRND_INSECURE) == (ssize_t)len)
return 0;
the code reads GRND_INSECURE... overwriting possibly
up to len-1 useful, and probably more securely random bytes?
if (open_read_close("/dev/urandom", seed, len) == (ssize_t)len)
return 0;
And here again, if we got less than len bytes, we overwrite them again?
I'm not saying it's a bug, I'm asking whether it's a decision
to have simple, but suboptimal code, or is this consideration overlooked?
if ((unlink(filename) < 0 || fsync(dfd) < 0) && seed_len) {
bb_perror_msg("can't%s seed", " remove");
return -1;
}
Why can't the entire above if() be replaced with xunlink(filename) ?
if (dfd < 0 || flock(dfd, LOCK_EX) < 0)
bb_perror_msg_and_die("can't %s seed directory", "lock");
People will repeatedly try to remove this flock(),
let's add a comment which explains, in detail, what bad things can happen
if it is removed.
Can we replace all [s]size_t's with ints/unsigneds? We do not expect
random pools anywhere near 4 gigabytes...
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
