fix md_rand.c locking bugs

diff --git a/CHANGES b/CHANGES
index 9ae334fad649df39ffc37903deee29d474c979de..f284da3696d10c53f66a79d73f41730a354d6d3e 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -11,6 +11,19 @@
          *) applies to 0.9.6a (/0.9.6b) and 0.9.7
          +) applies to 0.9.7 only
 
+  *) Move 'if (!initialized) RAND_poll()' into regions protected by
+     CRYPTO_LOCK_RAND.  This is not strictly necessary, but avoids
+     having multiple threads calling RAND_poll() concurrently.
+     [Bodo Moeller]
+
+  *) In crypto/rand/md_rand.c, replace 'add_do_not_lock' flag by a
+     combination of a flag and a thread ID variable.
+     Otherwise while one thread is in ssleay_rand_bytes (which sets the
+     flag), *other* threads can enter ssleay_add_bytes without obeying
+     the CRYPTO_LOCK_RAND lock (and may even illegaly release the lock
+     that they do not hold after the first thread unsets add_do_not_lock).
+     [Bodo Moeller]
+
   +) Implement binary inversion algorithm for BN_mod_inverse in addition
      to the algorithm using long divison.  The binary algorithm can be
      used only if the modulus is odd.  On 32-bit systems, it is faster

diff --git a/crypto/rand/md_rand.c b/crypto/rand/md_rand.c
index 98c6a39f5d0e624cfbb57b7d27362eba99e28f5a..c8728a07bf6827e9f8eb6f7ffe89c6c1bff7cb9f 100644 (file)
--- a/crypto/rand/md_rand.c
+++ b/crypto/rand/md_rand.c
@@ -141,10 +141,11 @@ static long md_count[2]={0,0};
 static double entropy=0;
 static int initialized=0;
 
-/* This should be set to 1 only when ssleay_rand_add() is called inside
-   an already locked state, so it doesn't try to lock and thereby cause
-   a hang.  And it should always be reset back to 0 before unlocking. */
-static int add_do_not_lock=0;
+static unsigned int crypto_lock_rand = 0; /* may be set only when a thread
+                                           * holds CRYPTO_LOCK_RAND
+                                           * (to prevent double locking) */
+static unsigned long locking_thread = 0; /* valid iff crypto_lock_rand is set */
+
 
 #ifdef PREDICT
 int rand_predictable=0;
@@ -191,6 +192,7 @@ static void ssleay_rand_add(const void *buf, int num, double add)
        long md_c[2];
        unsigned char local_md[MD_DIGEST_LENGTH];
        MD_CTX m;
+       int do_not_lock;
 
        /*
         * (Based on the rand(3) manpage)
@@ -207,7 +209,10 @@ static void ssleay_rand_add(const void *buf, int num, double add)
          * hash function.
         */
 
-       if (!add_do_not_lock) CRYPTO_w_lock(CRYPTO_LOCK_RAND);
+       /* check if we already have the lock */
+       do_not_lock = crypto_lock_rand && (locking_thread == CRYPTO_thread_id());
+
+       if (!do_not_lock) CRYPTO_w_lock(CRYPTO_LOCK_RAND);
        st_idx=state_index;
 
        /* use our own copies of the counters so that even
@@ -239,7 +244,7 @@ static void ssleay_rand_add(const void *buf, int num, double add)
 
        md_count[1] += (num / MD_DIGEST_LENGTH) + (num % MD_DIGEST_LENGTH > 0);
 
-       if (!add_do_not_lock) CRYPTO_w_unlock(CRYPTO_LOCK_RAND);
+       if (!do_not_lock) CRYPTO_w_unlock(CRYPTO_LOCK_RAND);
 
        for (i=0; i<num; i+=MD_DIGEST_LENGTH)
                {
@@ -281,7 +286,7 @@ static void ssleay_rand_add(const void *buf, int num, double add)
                }
        memset((char *)&m,0,sizeof(m));
 
-       if (!add_do_not_lock) CRYPTO_w_lock(CRYPTO_LOCK_RAND);
+       if (!do_not_lock) CRYPTO_w_lock(CRYPTO_LOCK_RAND);
        /* Don't just copy back local_md into md -- this could mean that
         * other thread's seeding remains without effect (except for
         * the incremented counter).  By XORing it we keep at least as
@@ -292,7 +297,7 @@ static void ssleay_rand_add(const void *buf, int num, double add)
                }
        if (entropy < ENTROPY_NEEDED) /* stop counting when we have enough */
            entropy += add;
-       if (!add_do_not_lock) CRYPTO_w_unlock(CRYPTO_LOCK_RAND);
+       if (!do_not_lock) CRYPTO_w_unlock(CRYPTO_LOCK_RAND);
        
 #if !defined(OPENSSL_THREADS) && !defined(OPENSSL_SYS_WIN32)
        assert(md_c[1] == md_count[1]);
@@ -347,14 +352,18 @@ static int ssleay_rand_bytes(unsigned char *buf, int num)
         * global 'md'.
         */
 
-       if (!initialized)
-               RAND_poll();
-
        CRYPTO_w_lock(CRYPTO_LOCK_RAND);
-       add_do_not_lock = 1;    /* Since we call ssleay_rand_add while in
-                                  this locked state. */
 
-       initialized = 1;
+       /* prevent ssleay_rand_bytes() from trying to obtain the lock again */
+       crypto_lock_rand = 1;
+       locking_thread = CRYPTO_thread_id();
+
+       if (!initialized)
+               {
+               RAND_poll();
+               initialized = 1;
+               }
+       
        if (!stirred_pool)
                do_stir_pool = 1;
        
@@ -418,8 +427,9 @@ static int ssleay_rand_bytes(unsigned char *buf, int num)
 
        md_count[0] += 1;
 
-       add_do_not_lock = 0;    /* If this would ever be forgotten, we can
-                                  expect any evil god to eat our souls. */
+       /* before unlocking, we must clear 'crypto_lock_rand' */
+       crypto_lock_rand = 0;
+       locking_thread = 0;
        CRYPTO_w_unlock(CRYPTO_LOCK_RAND);
 
        while (num > 0)
@@ -498,14 +508,37 @@ static int ssleay_rand_pseudo_bytes(unsigned char *buf, int num)
 static int ssleay_rand_status(void)
        {
        int ret;
+       int do_not_lock;
 
+       /* check if we already have the lock
+        * (could happen if a RAND_poll() implementation calls RAND_status()) */
+       do_not_lock = crypto_lock_rand && (locking_thread == CRYPTO_thread_id());
+       
+       if (!do_not_lock)
+               {
+               CRYPTO_w_lock(CRYPTO_LOCK_RAND);
+               
+               /* prevent ssleay_rand_bytes() from trying to obtain the lock again */
+               crypto_lock_rand = 1;
+               locking_thread = CRYPTO_thread_id();
+               }
+       
        if (!initialized)
+               {
                RAND_poll();
+               initialized = 1;
+               }
 
-       CRYPTO_w_lock(CRYPTO_LOCK_RAND);
-       initialized = 1;
        ret = entropy >= ENTROPY_NEEDED;
-       CRYPTO_w_unlock(CRYPTO_LOCK_RAND);
 
+       if (!do_not_lock)
+               {
+               /* before unlocking, we must clear 'crypto_lock_rand' */
+               crypto_lock_rand = 0;
+               locking_thread = 0;
+               
+               CRYPTO_w_unlock(CRYPTO_LOCK_RAND);
+               }
+       
        return ret;
        }

diff --git a/crypto/rand/rand_unix.c b/crypto/rand/rand_unix.c
index 63b5efc32ca2d93c0610b72915f4644034681a99..c491280b1f0d1137d4a52675af408190409c407e 100644 (file)
--- a/crypto/rand/rand_unix.c
+++ b/crypto/rand/rand_unix.c
@@ -228,8 +228,9 @@ int RAND_poll(void)
 
 #if defined(DEVRANDOM) || defined(DEVRANDOM_EGD)
        return 1;
-#endif
+#else
        return 0;
+#endif
 }
 
 #endif