Hey Alex, Thanks a bunch for doing that research. On Tue, Mar 22, 2022 at 10:30 PM Alex Xu (Hello71) <alex_y_xu@xxxxxxxx> wrote: > Several programs use it for testing purposes, without writing any > entropy to /dev/random or /dev/urandom, including rauc, wireguard The WireGuard use case is sitting in my tree but unpushed, and indeed it's done as a hack. I have no qualms about changing that before pushing to net or net-next. > - kata-containers is a lightweight VM implementation. Its guest-side > agent offers a gRPC endpoint which will write the provided data to > /dev/random, then call RNDADDTOENTCNT with the length of the data, > then call RNDRESEEDRNG. Sounds like this usage is safe, and that this patch wouldn't really fix much there. > - aws-nitro-enclaves-sdk-c is an SDK for building lightweight VMs to be > used with AWS Nitro Enclaves. kmstool-enclave is a sample application > provided, which writes "up to 256 bytes" (from where?) to /dev/random, > then calls RNDADDTOENTCNT, then repeats the process until it reaches > 1024 bytes. Looks like another safe use case, which the patch here doesn't help. Actually this patch might _hurt_ that use case if some of those writes are short (less than 7 bytes or so). So that might be a data point that indicates we shouldn't merge this patch, and instead should go with the "do nothing" route. > - sandy-harris/maxwell is a "jitter entropy" daemon, similar to haveged. > It writes 4 bytes of "generated entropy" to /dev/random, then calls > RNDADDTOENTCNT, then repeats. Okay bingo. The existence of this means that this patch will definitely introduce a new vulnerability. It means that an attacker can brute force all of Sandy's (CC'd now) inputs 32 bits at a time. So I don't think I can merge this patch as-is. A potential fix for this would be to change: + if (unlikely(crng_init == 0 && !will_credit)) + crng_pre_init_inject(block, len, false); into + if (unlikely(crng_init == 0 && !will_credit && count >= 16 && capable(CAP_SYS_ADMIN))) + crng_pre_init_inject(block, len, false); Or something like that. But that doesn't account for the case where what's written to /dev/urandom is 300 bytes long but only has 3 bits of unknown data. So it's better as far as heuristics go, but it doesn't definitely solve the problem, which stems from the fact of separating entropy writing from entropy crediting into separate calls. (Plus as I already mentioned to David, the patch still wouldn't help the crng_init=1 case.) > - guix is, among other things, a "GNU/"Linux distribution. The provided > base services write the seed file to /dev/urandom, then call > RNDADDTOENTCNT, then write 512 bytes from /dev/hwrng to /dev/urandom, > then call RNDADDTOENTCNT, then "immediately" read 512 bytes from > /dev/urandom and write it to the seed file. On shutdown, 512 bytes are > read from /dev/urandom and written to the seed file. That also sounds like a safe usage of RNDADDTOENTCNT. > I don't have any particular expertise with the random subsystem or > conclusions to make from this data, but I hope this helps inform the > discussion. Very much so, thanks again. What I take away from your results is: - RNDADDTOENTCNT is in active use in a safe way. Sure, RNDADDENTROPY is still much better, but RNDADDTOENTCNT isn't entirely broken in the above configurations either. - This patch would make RNDADDTOENTCNT unsafe for some of the above configurations in a way that it currently isn't unsafe. - Plenty of things are seeding the RNG correctly, and buildroot's shell script is just "doing it wrong". On that last point, I should reiterate that buildroot's shell script still isn't actually initializing the RNG, despite what it says in its echo; there's never been a way to initialize the RNG from a shell script, without calling out to various special purpose ioctl-aware binaries. Jason
