Re: lib/25367: arc4random state is shared across forks

[Roy Marples <roy%marples.name@localhost>]

The following reply was made to PR lib/25367; it has been noted by GNATS.

From: Roy Marples <roy%marples.name@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: apb%cequrux.com@localhost, David Laight <david%l8s.co.uk@localhost>,
 dholland-bugs%netbsd.org@localhost, Joerg Sonnenberger 
<joerg%britannica.bec.de@localhost>
Subject: Re: lib/25367: arc4random state is shared across forks
Date: Sat, 31 May 2014 01:15:34 +0100

 Thanks Joerg!
 I learned a new function :)
 
 Posted inline (gnats sucks) is a new patch using pthread_atfork.
 However, our man page and online documentation I can find is sadly 
 lacking in a few places so I'm not entirely sure the patch is correct as 
 no locking/unlocking is done at fork time and I don't know if it's 
 needed or not. Not really a threading expert :)
 
 Saying that, this patch has been running on my server and workstation 
 for a few days now with no adverse effects.
 Comments welcome.
 Roy
 
 Index: gen/arc4random.c
 ===================================================================
 RCS file: /cvsroot/src/lib/libc/gen/arc4random.c,v
 retrieving revision 1.21
 diff -u -p -r1.21 arc4random.c
 --- gen/arc4random.c    17 Oct 2013 23:56:17 -0000      1.21
 +++ gen/arc4random.c    31 May 2014 00:03:55 -0000
 @@ -33,6 +33,7 @@ __RCSID("$NetBSD: arc4random.c,v 1.21 20
   #include "namespace.h"
   #include "reentrant.h"
   #include <fcntl.h>
 +#include <pthread.h>
   #include <stdlib.h>
   #include <unistd.h>
   #include <sys/types.h>
 @@ -49,11 +50,12 @@ __weak_alias(arc4random_uniform,_arc4ran
   #endif
 
   struct arc4_stream {
 -       uint8_t stirred;
 +       uint8_t inited;
          uint8_t pad;
          uint8_t i;
          uint8_t j;
          uint8_t s[(uint8_t)~0u + 1u];   /* 256 to you and me */
 +       size_t count;
          mutex_t mtx;
   };
 
 @@ -67,7 +69,7 @@ struct arc4_stream {
                          mutex_unlock(&(rs)->mtx);      \
          }
   #else
 -#define LOCK(rs)
 +#define LOCK(rs)
   #define UNLOCK(rs)
   #endif
 
 @@ -77,8 +79,8 @@ struct arc4_stream {
   #define S64(n) S16(n), S16(n + 16), S16(n + 32), S16(n + 48)
   #define S256 S64(0), S64(64), S64(128), S64(192)
 
 -static struct arc4_stream rs = { .i = 0xff, .j = 0, .s = { S256 },
 -               .stirred = 0, .mtx = MUTEX_INITIALIZER };
 +static struct arc4_stream rs = { .inited = 0, .i = 0xff, .j = 0, .s = { 
 S256 },
 +               .count = 0, .mtx = MUTEX_INITIALIZER };
 
   #undef S
   #undef S4
 @@ -91,14 +93,22 @@ static __noinline void arc4_stir(struct
   static inline uint8_t arc4_getbyte(struct arc4_stream *);
   static inline uint32_t arc4_getword(struct arc4_stream *);
 
 -static inline int
 +static void
 +arc4_forked(void)
 +{
 +
 +       /* Reset the counter to a force new stir after forking */
 +       rs.count = 0;
 +}
 +
 +static inline void
   arc4_check_init(struct arc4_stream *as)
   {
 -       if (__predict_true(rs.stirred))
 -               return 0;
 
 -       arc4_stir(as);
 -       return 1;
 +       if (!__predict_true(as->inited)) {
 +               as->inited = 1;
 +               pthread_atfork(NULL, NULL, arc4_forked);
 +       }
   }
 
   static inline void
 @@ -124,6 +134,8 @@ arc4_stir(struct arc4_stream *as)
          size_t len;
          size_t i, j;
 
 +       arc4_check_init(as);
 +         */
 -       for (j = 0; j < __arraycount(as->s) * 4; j++)
 +       for (j = 0; j < __arraycount(as->s) * sizeof(uint32_t); j++)
                  arc4_getbyte(as);
 
 -       as->stirred = 1;
 +       /* Stir again after swallowing 1600000 bytes or if the pid 
 changes */
 +       as->count = 1600000;
 +}
 +
 +static inline void
 +arc4_stir_if_needed(struct arc4_stream *as, size_t len)
 +{
 +
 +       if (as->count <= len)
 +               arc4_stir(as);
 +       else
 +               as->count -= len;
   }
 
   static __inline uint8_t
 @@ -169,6 +192,7 @@ arc4_getbyte_ij(struct arc4_stream *as,
   static inline uint8_t
   arc4_getbyte(struct arc4_stream *as)
   {
 +
          return arc4_getbyte_ij(as, &as->i, &as->j);
   }
 
 @@ -176,6 +200,7 @@ static inline uint32_t
   arc4_getword(struct arc4_stream *as)
   {
          uint32_t val;
 +
          val = arc4_getbyte(as) << 24;
          val |= arc4_getbyte(as) << 16;
          val |= arc4_getbyte(as) << 8;
 @@ -186,6 +211,7 @@ arc4_getword(struct arc4_stream *as)
   void
   arc4random_stir(void)        UNLOCK(&rs);
 @@ -194,8 +220,9 @@ arc4random_stir(void)
   void
   arc4random_addrandom(u_char *dat, int datlen)
   {
 +
          LOCK(&rs);
 -       arc4_check_init(&rs);
 +       arc4_stir_if_needed(&rs, datlen);
          arc4_addrandom(&rs, dat, datlen);
          UNLOCK(&rs);
   }
 @@ -206,7 +233,7 @@ arc4random(void)
          uint32_t v;
 
          LOCK(&rs);
 -       arc4_check_init(&rs);
 +       arc4_stir_if_needed(&rs, sizeof(v));
          v = arc4_getword(&rs);
          UNLOCK(&rs);
          return v;
 @@ -220,7 +247,7 @@ arc4random_buf(void *buf, size_t len)
          uint8_t i, j;
 
          LOCK(&rs);
 -       arc4_check_init(&rs);
 +       arc4_stir_if_needed(&rs, len);
 
          /* cache i and j - compiler can't know 'buf' doesn't alias them 
 */
          i = rs.i;
 @@ -263,7 +290,7 @@ arc4random_uniform(uint32_t upper_bound)
          min = (0xFFFFFFFFU - upper_bound + 1) % upper_bound;
 
          LOCK(&rs);
 -       arc4_check_init(&rs);
 +       arc4_stir_if_needed(&rs, sizeof(r));
 
          /*
           * This could theoretically loop forever but each retry has
 
   {
 +
          LOCK(&rs);
          arc4_stir(&rs);
 
          /*
           * This code once opened and read /dev/urandom on each
           * call.  That causes repeated rekeying of the kernel stream
 @@ -146,10 +158,21 @@ arc4_stir(struct arc4_stream *as)
           * paper "Weaknesses in the Key Scheduling Algorithm of RC4"
           * by Fluher, Mantin, and Shamir.  (N = 256 in our case.)