Subject: bug#41456: fix cases where insecure randomness could be used

From: Taylor Hornby <[email-masked]>
Date: Fri, 22 May 2020 01:19:15 -0600

I reported a potential security bug on GitHub:
https://github.com/coreutils/coreutils/pull/32. To save you a click, I'll
copy-paste it here (for context this is on a PR with a fix):

Comment #1:

Apologies for submitting on GitHub, it's so much more convenient. I will
understand if no one sees this because I didn't follow the guidelines.

Justification:

   - The existing code is dangerous because it can silently fail to seed
   the random number generator securely, either when fopen() fails or when
   read() returns fewer bytes than requested, which can happen if the call
   is interrupted by an interrupt. This is important for utilities like
   shred where cryptographic-quality randomness is important.
   - I removed the bytes_bound stuff because it didn't seem necessary
   anywhere it was used, and if get_nonce is ever called with bytes_bound <
   bufsize, then part of ISAAC's initial state will contain
   timestamps/PIDs, so it will not be uniformly random. Usually, stream
   ciphers like ISAAC require their initial state to be uniformly random,
   otherwise there will be statistical biases in the early output.

I have not tested all the utilities this affects.

(Full disclosure is appropriate in this case because any damage has already
been done, fixing the problem in secret would not stop any attacks, but
disclosing might encourage users to stop using the dangerous code and
upgrade.)

Comment #2:

This is a more serious issue on Solars, which apparently has a blocking
/dev/random <https://icmconference.org/wp-content/uploads/G11b-Fenwick.pdf>
and NAME_OF_NONCE_DEVICE defaults to /dev/random (see gc-random.m4), or
when NAME_OF_NONCE_DEVICE is overriden to /dev/random with a configure flag
on a Linux system.

I ran some experiments on a Debian 9 box, and read() from /dev/random
frequently returns very few bytes, sometimes as few as just 6 bytes. This
means, ironically, if someone built the code with /dev/random thinking it
would be more secure, it's actually less secure, because read() will return
fewer bytes and then very little of the ISAAC seed will be random and most
of it will be timestamp/PID/uninitialized memory.

Regards,

-Taylor