[PATCH] random: Don't reset crng_init_cnt on urandom_read()

[PATCH] random: Don't reset crng_init_cnt on urandom_read()

[Jann Horn]

At the moment, urandom_read() (used for /dev/urandom) resets crng_init_cnt
to zero when it is called at crng_init<2. This is inconsistent: We do it
for /dev/urandom reads, but not for the equivalent
getrandom(GRND_INSECURE).

(And worse, as Jason pointed out, we're only doing this as long as
maxwarn>0.)

crng_init_cnt is only read in crng_fast_load(); it is relevant at
crng_init==0 for determining when to switch to crng_init==1 (and where in
the RNG state array to write).

As far as I understand:

 - crng_init==0 means "we have nothing, we might just be returning the same
   exact numbers on every boot on every machine, we don't even have
   non-cryptographic randomness; we should shove every bit of entropy we
   can get into the RNG immediately"
 - crng_init==1 means "well we have something, it might not be
   cryptographic, but at least we're not gonna return the same data every
   time or whatever, it's probably good enough for TCP and ASLR and stuff;
   we now have time to build up actual cryptographic entropy in the input
   pool"
 - crng_init==2 means "this is supposed to be cryptographically secure now,
   but we'll keep adding more entropy just to be sure".

The current code means that if someone is pulling data from /dev/urandom
fast enough at crng_init==0, we'll keep resetting crng_init_cnt, and we'll
never make forward progress to crng_init==1. It seems to be intended to
prevent an attacker from bruteforcing the contents of small individual RNG
inputs on the way from crng_init==0 to crng_init==1, but that's misguided;
crng_init==1 isn't supposed to provide proper cryptographic security
anyway, RNG users who care about getting secure RNG output have to wait
until crng_init==2.

This code was inconsistent, and it probably made things worse - just get
rid of it.

Signed-off-by: Jann Horn <jannh@google.com>
---
 drivers/char/random.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 605969ed0f96..3c3f5c62378c 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1816,7 +1816,6 @@ urandom_read_nowarn(struct file *file, char __user *buf, size_t nbytes,
 static ssize_t
 urandom_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
 {
-	unsigned long flags;
 	static int maxwarn = 10;
 
 	if (!crng_ready() && maxwarn > 0) {
@@ -1824,9 +1823,6 @@ urandom_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
 		if (__ratelimit(&urandom_warning))
 			pr_notice("%s: uninitialized urandom read (%zd bytes read)\n",
 				  current->comm, nbytes);
-		spin_lock_irqsave(&primary_crng.lock, flags);
-		crng_init_cnt = 0;
-		spin_unlock_irqrestore(&primary_crng.lock, flags);
 	}
 
 	return urandom_read_nowarn(file, buf, nbytes, ppos);

base-commit: c9e6606c7fe92b50a02ce51dda82586ebdf99b48
-- 
2.34.1.448.ga2b2bfdf31-goog

Re: [PATCH] random: Don't reset crng_init_cnt on urandom_read()

[Jason A. Donenfeld]

On Mon, Jan 3, 2022 at 4:59 PM Jann Horn <jannh@google.com> wrote:
> This code was inconsistent, and it probably made things worse - just get
> rid of it.

Rather than adding crng_init_cnt=0 if crng_init<1 to extract_crng_user
and get_random_bytes, getting rid of it like this seems probably okay
and makes the model simpler. I'll apply this. Thank you.

Jason

Re: [PATCH] random: Don't reset crng_init_cnt on urandom_read()

[Theodore Ts'o]

On Mon, Jan 03, 2022 at 05:03:57PM +0100, Jason A. Donenfeld wrote:
> On Mon, Jan 3, 2022 at 4:59 PM Jann Horn <jannh@google.com> wrote:
> > This code was inconsistent, and it probably made things worse - just get
> > rid of it.
> 
> Rather than adding crng_init_cnt=0 if crng_init<1 to extract_crng_user
> and get_random_bytes, getting rid of it like this seems probably okay
> and makes the model simpler. I'll apply this. Thank you.

Ack.  It does mean that we're making a choice that an attacker who is
carrying out a incremental state tracking attack on the CRNG will make
/dev/urandom (and getrandom) to make the crng emit "less secure" in
the period when crng_init is > 0 and < 2.  On the other hand, this
allows us to get to the state of crng_init=2 faster, where as before,
the attacker could delay getting us to the state crng_init=1 forever,
where reads from /dev/urandom would be hence be insecure forever (and
getrandom() would block forever).


						- Ted

Re: [PATCH] random: Don't reset crng_init_cnt on urandom_read()

[Jason A. Donenfeld]

On Mon, Jan 3, 2022 at 5:39 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Jan 03, 2022 at 05:03:57PM +0100, Jason A. Donenfeld wrote:
> > On Mon, Jan 3, 2022 at 4:59 PM Jann Horn <jannh@google.com> wrote:
> > > This code was inconsistent, and it probably made things worse - just get
> > > rid of it.
> >
> > Rather than adding crng_init_cnt=0 if crng_init<1 to extract_crng_user
> > and get_random_bytes, getting rid of it like this seems probably okay
> > and makes the model simpler. I'll apply this. Thank you.
>
> Ack.  It does mean that we're making a choice that an attacker who is
> carrying out a incremental state tracking attack on the CRNG will make
> /dev/urandom (and getrandom) to make the crng emit "less secure" in
> the period when crng_init is > 0 and < 2.  On the other hand, this
> allows us to get to the state of crng_init=2 faster, where as before,
> the attacker could delay getting us to the state crng_init=1 forever,
> where reads from /dev/urandom would be hence be insecure forever (and
> getrandom() would block forever).

Right. I had a few early drafts of this commit where I was trying to
protect the 0->1 transition from being bruteforced with a trickle of
entropy, and Jann's offline comment was something along the lines of,
"why do we actually care about crng_init==1? it's not secure anyway,"
which seems compelling. Plus, as you point out, letting anything reset
crng_init_cnt (like /dev/urandom reads) means unprivileged userspace
can delay crng_init==2, which seems like a bigger deal.

Jason