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
