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

[David Holland <dholland-bugs%netbsd.org@localhost>]

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

From: David Holland <dholland-bugs%netbsd.org@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: lib/25367: arc4random state is shared across forks
Date: Fri, 30 May 2014 08:19:43 +0000

 here's another copy of the mail above (that got sent to the wrong
 place) in which the patch isn't base64'd:
 
    ------
 
 From: Roy Marples <roy%marples.name@localhost>
 To: gnats%netbsd.org@localhost
 Subject: Re: lib/25367: arc4random state is shared across forks
 Date: Sat, 24 May 2014 23:07:07 +0100
 
 Hi
 
 I bumped into this as well.
 I looked over the FreeBSD thread regarding potential speed issues and despite
 any concerns they have actually merged the change in.
 Attached is a patch to -current which does two things.
 
 1) Re-stirs if the pid changes
 2) Re-stirs after swallowing 1600000 bytes (which also addresses PR/45952)
 
 I've been running this on my laptop for a while with great success and unless
 anyone objects I'll commit it shortly.
 
 Thanks
 
 Roy
 
 ? test
 ? test.c
 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   24 May 2014 21:40:13 -0000
 @@ -49,11 +49,11 @@ __weak_alias(arc4random_uniform,_arc4ran
  #endif
  
  struct arc4_stream {
 -      uint8_t stirred;
 -      uint8_t pad;
        uint8_t i;
        uint8_t j;
        uint8_t s[(uint8_t)~0u + 1u];   /* 256 to you and me */
 +      size_t count;
 +      pid_t stir_pid;
        mutex_t mtx;
  };
  
 @@ -67,7 +67,7 @@ struct arc4_stream {
                        mutex_unlock(&(rs)->mtx);      \
        }
  #else
 -#define LOCK(rs) 
 +#define LOCK(rs)
  #define UNLOCK(rs)
  #endif
  
 @@ -78,7 +78,7 @@ struct arc4_stream {
  #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 };
 +              .count = 0, .stir_pid = 0, .mtx = MUTEX_INITIALIZER };
  
  #undef S
  #undef S4
 @@ -87,20 +87,10 @@ static struct arc4_stream rs = { .i = 0x
  #undef S256
  
  static inline void arc4_addrandom(struct arc4_stream *, u_char *, int);
 -static __noinline void arc4_stir(struct arc4_stream *);
 +static __noinline void arc4_stir(struct arc4_stream *, pid_t);
  static inline uint8_t arc4_getbyte(struct arc4_stream *);
  static inline uint32_t arc4_getword(struct arc4_stream *);
  
 -static inline int
 -arc4_check_init(struct arc4_stream *as)
 -{
 -      if (__predict_true(rs.stirred))
 -              return 0;
 -
 -      arc4_stir(as);
 -      return 1;
 -}
 -
  static inline void
  arc4_addrandom(struct arc4_stream *as, u_char *dat, int datlen)
  {
 @@ -117,7 +107,7 @@ arc4_addrandom(struct arc4_stream *as, u
  }
  
  static __noinline void
 -arc4_stir(struct arc4_stream *as)
 +arc4_stir(struct arc4_stream *as, pid_t pid)
  {
        int rdat[32];
        int mib[] = { CTL_KERN, KERN_URND };
 @@ -146,10 +136,24 @@ 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.)
         */
 -      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;
 +      as->stir_pid = pid;
 +}
 +
 +static inline void
 +arc4_stir_if_needed(struct arc4_stream *as, size_t len)
 +{
 +      pid_t pid;
 +
 +      pid = getpid();
 +      if (as->count <= len || as->stir_pid != pid)
 +              arc4_stir(as, pid);
 +      else
 +              as->count -= len;
  }
  
  static __inline uint8_t
 @@ -169,6 +173,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 +181,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,16 +192,18 @@ arc4_getword(struct arc4_stream *as)
  void
  arc4random_stir(void)
  {
 +
        LOCK(&rs);
 -      arc4_stir(&rs);
 +      arc4_stir(&rs, getpid());
        UNLOCK(&rs);
  }
  
  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 +214,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 +228,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 +271,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