Messages in this thread | | | Date | Fri, 14 Dec 2007 14:13:05 -0600 | From | Matt Mackall <> | Subject | Re: /dev/urandom uses uninit bytes, leaks user data |
| |
On Fri, Dec 14, 2007 at 11:34:09AM -0800, John Reiser wrote: > xfer_secondary_pool() in drivers/char/random.c tells add_entropy_words() > to use uninitialized tmp[] whenever bytes is not a multiple of 4. > Besides being unfriendly to automated dynamic checkers, this is a > potential leak of user data into the output stream. When called from > extract_entropy_user, then uninit tmp[] can capture leftover data > from a previous copy_from_user().
Yes, we use uninitialized data. But it's not a leak in any useful sense. To the extent the previous data is secret, this actually improves our entropy.
It's getting folded into the random number pool, where it will be impossible to recover it unless you already know what was in the pool. And if you know what's in the pool, you've already broken into the kernel.
But I'm sympathetic to making Valgrind happy.
> --- ./drivers/char/random.c.orig 2007-12-14 11:06:03.000000000 -0800 > +++ ./drivers/char/random.c 2007-12-14 11:06:57.000000000 -0800 > @@ -708,7 +708,19 @@ > > bytes=extract_entropy(r->pull, tmp, bytes, > random_read_wakeup_thresh / 8, rsvd); > - add_entropy_words(r, tmp, (bytes + 3) / 4); > + /* > + * 2007-12-13 (valgrind/memcheck) Do not use undefined bytes. > + * Avoid info leak when called from extract_entropy_user: > + * uninit tmp[] can have data from previous copy_from_user(). > + * Instead: fill last word using first bytes. > + */ > + { > + __u8 *src = (__u8 *)&tmp[0]; > + __u8 *dst = bytes + src; > + for (; 0!=(3 & bytes); ++bytes) > + *dst++ = *src++; > + }
That's hideous. How about a memset instead:
/* clear uninitialized bytes at the end to make valgrind happy */ memset((char *)tmp + bytes, 0, -bytes & 3);
Also, don't bother putting dates or the like in comments. We've got a version control system for that.
> + add_entropy_words(r, tmp, bytes>>2);
And that change is broken..
> credit_entropy_store(r, bytes*8);
..because it makes this line wrong. We have to add precisely the number of bytes returned by extract_entropy to keep the books balanced.
-- Mathematics is the supreme nostalgia of our time.
| |