On Tue, Dec 21, 2010 at 06:51:03PM +0100, Otto Moerbeek wrote: > On Tue, Dec 21, 2010 at 05:59:33PM +0100, Kurt Knochner wrote: > > OpenBSD uses arc4random() and arc4random_buf() all over the code to generate > > random numbers. This code is defined in src/sys/dev/rnd.c.
> > [arc4stir] initializes the RC4 context with some random data, gathered by > > system > > enthropy, that is mainly done by get_random_bytes(). > > > > ==> Bug #1 > > > > HOWEVER: Have a look at the buffer that's beeing used as a seed for the RC4 > > key setup. It's beeing filled with the random data, BUT at the beginning it > > will be filled with just the value of nanotime(). > > > > > nanotime((struct timespec *) buf); > > > len = sizeof(buf) - sizeof(struct timespec); > > > get_random_bytes(buf + sizeof (struct timespec), len); > > > len += sizeof(struct timespec); > > > > > > So, there is a lot of effort in get_random_bytes() to get "real random" data > > for the buffer and then the value of nanotime() is prepended to the buffer? > > That does not look right. Please consider: this buffer will be used as key > > for rc4_keysetup() and thus it should contain unrelated and unpredictable > > data. > > I don't know the answer to this question, but my guess is that the > buffer is filled by nanotime() to cover the case that > get_random_bytes() does not have enough entropy available, so at least > some non-constant data is used. Wouldn't it be better to XOR this into the data returned from get_random_bytes(), though? That'd get you optimal entropy in either case. Sample diff (which also removes the len variable): Index: rnd.c =================================================================== RCS file: /usr/cvs/src/src/sys/dev/rnd.c,v retrieving revision 1.104 diff -u -p -r1.104 rnd.c --- rnd.c 21 Nov 2010 22:58:40 -0000 1.104 +++ rnd.c 21 Dec 2010 18:19:35 -0000 @@ -779,13 +779,14 @@ get_random_bytes(void *buf, size_t nbyte static void arc4_stir(void) { - u_int8_t buf[256]; - int len; + u_int8_t buf[256]; + struct timespec ts; + int i; - nanotime((struct timespec *) buf); - len = sizeof(buf) - sizeof(struct timespec); - get_random_bytes(buf + sizeof (struct timespec), len); - len += sizeof(struct timespec); + get_random_bytes(buf, sizeof(buf)); + nanotime(&ts); + for (i = 0; i < sizeof(ts); i++) + buf[i] ^= ((uint8_t *) &ts)[i]; mtx_enter(&rndlock); if (rndstats.arc4_nstirs > 0) @@ -793,7 +794,7 @@ arc4_stir(void) rc4_keysetup(&arc4random_state, buf, sizeof(buf)); arc4random_count = 0; - rndstats.arc4_stirs += len; + rndstats.arc4_stirs += sizeof(buf); rndstats.arc4_nstirs++; /* > > ==> Bug #2 > > > > The function rc4_crypt() get's called as soon as rndstats.arc4_nstirs > 0. > > This will be the case whenever arc4_stir get's called the second time (by > > the timer reset - see above). > > > > > if (rndstats.arc4_nstirs > 0) > > > rc4_crypt(&arc4random_state, buf, buf, sizeof(buf)); > > > > > rc4_keysetup(&arc4random_state, buf, sizeof(buf)); > > > arc4random_count = 0; > > > rndstats.arc4_stirs += len; > > > rndstats.arc4_nstirs++; > > > > HOWEVER, right after the call of rc4_crypt(), we call rc4_keysetup() with > > the same 'arc4random_state'. This makes the call to rc4_crypt() useless, as > > the data structure will be overwritten again with the init data of the RC4 > > function. > > rc4_crypt() changes both the state and the contents of buf, since buf is > used both as source and destination. That buf is used by rc4_keysetup() > to create a new state. So indeed the state is overwritten, but the > contents of buf produced by rc4_crypt() is used to do that. Yes, that does seem right. By the way, the arc4random_count variable doesn't seem to be used; is there a reason not to garbage-collect it? Joachim