Hi Denys, Nice to hear from you.
On Wed, Apr 27, 2022 at 06:15:50PM +0200, Denys Vlasenko wrote: > In read_new_seed(), if getrandom(GRND_NONBLOCK) reads > less than len bytes: > the code reads GRND_INSECURE... overwriting possibly > up to len-1 useful, and probably more securely random bytes? > 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? GRND_NONBLOCK, GRND_INSECURE, and /dev/urandom all return the same bytes. The difference, for the purposes of this function, is that which one is used plays a part in whether the seed is marked as creditable. The issue here, therefore, is that if getrandom(GRND_NONBLOCK) returns a short read, then we'll wind up using GRND_INSECURE or /dev/urandom and thus not credit the seed, when in reality we _could have_ gotten away with crediting the seed, if we had just tried again after a short read. For that reason, the original seedrng repo has a getrandom_full() function: <https://git.zx2c4.com/seedrng/tree/seedrng.c#n223>, which loops on EINTR, but I assumed that Bernhard would go nuts if I added more code to the busybox implementation. So the question is, how bad is this actually? And do we want to mitigate it? getrandom() will return a short read under the following circumstances: 1) On kernels < 5.18: a) The length must be > 256, b) A signal must be pending, and c) The task must be marked as needing to be rescheduled. 2) On kernels >= 5.18: a) The length must be > 4096, and b) A signal must be pending. Furthermore, on kernels < 5.18, length is always 512, and on kernels >= 5.18, length is always 32. That means there is no problem on kernels >= 5.18, and on kernels < 5.18, it is only a problem if both a signal is pending and the task is marked as needing to be rescheduled. And given the short lifetime of the process, it seems implausible that the process would be marked as needing to be rescheduled during its execution. So I think this is _probably_ fine. But if we did want to mitigate it somehow, we have a few options: 1) Pull in my getrandom_full() function. Pros: "complete" solution. Cons: code size increases. 2) Accept short reads, so long as we've read more than 32 bytes. Pros: not awful. Cons: caveat (1a) above only applies if the supplied length is <= 256, so this might not actually work in all cases. Also means we have to pass that length back which might increase code size. 3) Limit the poolsize to 256 bytes (by changing the MAX_SEED_LEN enum value to 256 instead of 512). Pros: works well. Cons: none that are practical? Do you have a preference? I'd lean toward 3, 1, or doing nothing, but happy to hear your input. > 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) ? It cannot be replaced with that. The fsync() must happen before other operations move forward, and exiting the program isn't necessarily the correct course of action. > 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. Fine with me (if you want to CC me on a patch to review etc). > Can we replace all [s]size_t's with ints/unsigneds? We do not expect > random pools anywhere near 4 gigabytes... Probably that's fine. Is the advantage to tossing out consistent types worth it though? Does this actually save space? Since [s]size_t is usually the word size, won't codegen not really change much? Jason _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
