Hi Ted, On Tue, Mar 22, 2022 at 9:35 PM Theodore Ts'o <tytso@xxxxxxx> wrote: > One of the big issues with /dev/urandom writes is that *anyone*, > including malicious user space, can force specific bytes to be mixed > in. That's the source of the reluctance to immediate use inputs from > writes into /dev/[u]random until there is a chance for it to be mixed > in with other entropy which is hopefully not under the control of > malicious userspace. Right, sort of. Since we now always use a cryptographic hash function, we can haphazardly mix whatever any user wants, without too much concern. The issue is whether we _credit_ those bits. Were we to credit those bits, a malicious unpriv'd user could credit 248 bits of known input, and then bruteforce 8 bits of unknown input, and repeat, and in that way destroy the security of the thing. So, yea, the current reluctance does make sense. > Now, I recognize that things are a bit special in early boot, and if > we have a malicious script running in a systemd unit script, we might > as well go home. But something to consider is whether we want to do > soemthing special if the process writing to /dev/[u]random has > CAP_SYS_ADMIN, or some such. Exactly. So one way of approaching this would be to simply credit writes to /dev/urandom if it's CAP_SYS_ADMIN and maybe if also !crng_ready(), and just skip the crng_pre_init_inject() part that this current patch adds. I'll attach a sample patch of what this might look like at the end of this email. The problem with that approach, though, is that various userspaces might already write garbage into /dev/urandom, not expecting them to be credited -- for example, some userspace hardware configuration component that writes some serial number there. So I'm sort of hesitant to _change_ the behavior that we've had for so long. Another variation on that would be to do what this current patch does, but only crng_pre_init_inject() on CAP_SYS_ADMIN. But this has the same pitfall of only working as intended at cnrg_init=0 but not crng_init=1. That's better than nothing, but it's not perfect, and it introduces that problem with RNDADDTOENTCNT. Also, to echo the point I made in the email I just sent to David, this has _always_ been broken, and those shell scripts have _always_ been vulnerable. Maybe the kernel should fix that, but due to the ambiguity of the /dev/urandom write interface, maybe the best fix is actually in userspace itself, which means it'd work on old kernels too (which are rather common for the embedded devices that tend to have those types of shell scripts). Specifically, I'm talking about this fix I did for systemd: https://github.com/systemd/systemd/commit/da2862ef06f22fc8d31dafced6d2d6dc14f2ee0b And then the same fix that I just submitted this evening for Buildroot: https://lists.buildroot.org/pipermail/buildroot/2022-March/639359.html It seems like hashing the old seed together with the new one ought to be a good standard practice to mitigate against all sorts of bugs. > Yeah, no one should ever ver ever be using RNDADDTOENTCNT. It's an > ioctl which requires root privilegs, and if it breaks, you get to keep > both pieces. > > > And perhaps we might consider attempting to deprecate RNDADDTOENTCNT at > > some point in the future. > > That would be a good idea. :-) Oh cool, I'm glad you agree. Let's do that then. Have a preferred path? Maybe just a pr_once() saying not to use it? Jason --------8<------------------------------------------- The CAP_SYS_ADMIN idea, maybe not so good, but here for illustration: diff --git a/drivers/char/random.c b/drivers/char/random.c index 706f08edf0dc..d1dc46366647 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -1493,12 +1493,16 @@ static __poll_t random_poll(struct file *file, poll_table *wait) return mask; } -static int write_pool(const char __user *ubuf, size_t count) +static int write_pool(const char __user *ubuf, size_t count, bool credit_at_boot) { - size_t len; + size_t len, bits; int ret = 0; u8 block[BLAKE2S_BLOCK_SIZE]; + if (count > SIZE_MAX / 8) + return -EINVAL; + bits = count * 8; + while (count) { len = min(count, sizeof(block)); if (copy_from_user(block, ubuf, len)) { @@ -1511,6 +1515,9 @@ static int write_pool(const char __user *ubuf, size_t count) cond_resched(); } + if (credit_at_boot && !crng_ready() && capable(CAP_SYS_ADMIN)) + credit_entropy_bits(bits); + out: memzero_explicit(block, sizeof(block)); return ret; @@ -1521,7 +1528,7 @@ static ssize_t random_write(struct file *file, const char __user *buffer, { int ret; - ret = write_pool(buffer, count); + ret = write_pool(buffer, count, true); if (ret) return ret; @@ -1584,7 +1591,7 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg) return -EINVAL; if (get_user(size, p++)) return -EFAULT; - retval = write_pool((const char __user *)p, size); + retval = write_pool((const char __user *)p, size, false); if (retval < 0) return retval; credit_entropy_bits(ent_count);
