[openssl-dev] [openssl.org #4235] AutoReply: Crash on ssleay_rand_bytes - global variable is not protected

Alexandre PAQUE via RT rt at openssl.org
Thu Jan 14 15:26:42 UTC 2016


I'm not sure that the issue is the lock on  the "state" variable because
this is a char array with a fixed size. No dynamic allocations and free.

However, the crash is on this part of code:

​
 MD_Update(&m, &(state[0]), k); ​





On 14 January 2016 at 14:37, The default queue via RT <rt at openssl.org>
wrote:

>
> Greetings,
>
> This message has been automatically generated in response to the
> creation of a trouble ticket regarding:
>         "Crash on ssleay_rand_bytes - global variable is not protected",
> a summary of which appears below.
>
> There is no need to reply to this message right now.  Your ticket has been
> assigned an ID of [openssl.org #4235].
>
> Please include the string:
>
>          [openssl.org #4235]
>
> in the subject line of all future correspondence about this issue. To do
> so,
> you may reply to this message.
>
>                         Thank you,
>                         rt at openssl.org
>
> -------------------------------------------------------------------------
> *​Context:​*
>
> openssl 1.0.2e
> Windows 2012
> gsoap 2.8.7 - WSSE
> Application multithread
>
>
> *crash dump: *
>
> d:\opensource\openssl-1.0.2e\crypto\sha\sha_locl.h (320):
> sha1_block_data_order
> d:\opensource\openssl-1.0.2e\tmp32\md32_common.h (343): SHA1_Update
> d:\opensource\openssl-1.0.2e\crypto\evp\m_sha1.c (78): update
> d:\opensource\openssl-1.0.2e\crypto\rand\md_rand.c (496): ssleay_rand_bytes
> d:\opensource\openssl-1.0.2e\crypto\rand\md_rand.c (544):
> ssleay_rand_pseudo_bytes
> d:\dev\3rdparty\gsoap-2.8\gsoap\plugin\wsseapi.cpp (3545): calc_nonce
> d:\dev\3rdparty\gsoap-2.8\gsoap\plugin\wsseapi.cpp (1718):
> soap_wsse_add_UsernameTokenDigest
>
> *Analyze:*
>
> I think this issue occurs because of the global static variable "state" is
> not protected into the functions (md_rand.c).
>
> sample:
>
> int ssleay_rand_bytes(unsigned char *buf, int num, int pseudo, int lock)
> {
> ...
>
> *if (lock)*
> *        CRYPTO_w_lock(CRYPTO_LOCK_RAND);*
>
> *    /* prevent ssleay_rand_bytes() from trying to obtain the lock again
> */*
> *    CRYPTO_w_lock(CRYPTO_LOCK_RAND2);*
> *    CRYPTO_THREADID_current(&locking_threadid);*
> *    CRYPTO_w_unlock(CRYPTO_LOCK_RAND2);*
> *    crypto_lock_rand = 1;*
>
>     if (!initialized) {
>         RAND_poll();
>         initialized = 1;
>     }
>
>     if (!stirred_pool)
>         do_stir_pool = 1;
>
>     ok = (entropy >= ENTROPY_NEEDED);
>     if (!ok) {
>         /*
>          * If the PRNG state is not yet unpredictable, then seeing the PRNG
>          * output may help attackers to determine the new state; thus we
> have
>          * to decrease the entropy estimate. Once we've had enough initial
>          * seeding we don't bother to adjust the entropy count, though,
>          * because we're not ambitious to provide *information-theoretic*
>          * randomness. NOTE: This approach fails if the program forks
> before
>          * we have enough entropy. Entropy should be collected in a
> separate
>          * input pool and be transferred to the output pool only when the
>          * entropy limit has been reached.
>          */
>         entropy -= num;
>         if (entropy < 0)
>             entropy = 0;
>     }
>
>     if (do_stir_pool) {
>         /*
>          * In the output function only half of 'md' remains secret, so we
>          * better make sure that the required entropy gets 'evenly
>          * distributed' through 'state', our randomness pool. The input
>          * function (ssleay_rand_add) chains all of 'md', which makes it
> more
>          * suitable for this purpose.
>          */
>
>         int n = STATE_SIZE;     /* so that the complete pool gets accessed
> */
>         while (n > 0) {
> #if MD_DIGEST_LENGTH > 20
> # error "Please adjust DUMMY_SEED."
> #endif
> #define DUMMY_SEED "...................." /* at least MD_DIGEST_LENGTH */
>             /*
>              * Note that the seed does not matter, it's just that
>              * ssleay_rand_add expects to have something to hash.
>              */
>             ssleay_rand_add(DUMMY_SEED, MD_DIGEST_LENGTH, 0.0);
>             n -= MD_DIGEST_LENGTH;
>         }
>         if (ok)
>             stirred_pool = 1;
>     }
>
>     st_idx = state_index;
>     st_num = state_num;
>     md_c[0] = md_count[0];
>     md_c[1] = md_count[1];
>     memcpy(local_md, md, sizeof md);
>
>     state_index += num_ceil;
>     if (state_index > state_num)
>         state_index %= state_num;
>
>     /*
>      * state[st_idx], ..., state[(st_idx + num_ceil - 1) % st_num] are now
>      * ours (but other threads may use them too)
>      */
>
>     md_count[0] += 1;
>
>     /* before unlocking, we must clear 'crypto_lock_rand' */
>     crypto_lock_rand = 0;
>  *   if (lock)*
> *        CRYPTO_w_unlock(CRYPTO_LOCK_RAND);*
>
>     while (num > 0) {
>         /* num_ceil -= MD_DIGEST_LENGTH/2 */
>         j = (num >= MD_DIGEST_LENGTH / 2) ? MD_DIGEST_LENGTH / 2 : num;
>         num -= j;
>         MD_Init(&m);
> #ifndef GETPID_IS_MEANINGLESS
>         if (curr_pid) {         /* just in the first iteration to save time
> */
>             MD_Update(&m, (unsigned char *)&curr_pid, sizeof curr_pid);
>             curr_pid = 0;
>         }
> #endif
>         MD_Update(&m, local_md, MD_DIGEST_LENGTH);
>         MD_Update(&m, (unsigned char *)&(md_c[0]), sizeof(md_c));
>
> #ifndef PURIFY                  /* purify complains */
>         /*
>          * The following line uses the supplied buffer as a small source of
>          * entropy: since this buffer is often uninitialised it may cause
>          * programs such as purify or valgrind to complain. So for those
>          * builds it is not used: the removal of such a small source of
>          * entropy has negligible impact on security.
>          */
>         MD_Update(&m, buf, j);
> #endif
>
> // no lock to protect "state" global variable
>
>         k = (st_idx + MD_DIGEST_LENGTH / 2) - st_num;
>         if (k > 0) {
>             MD_Update(&m, &(state[st_idx]), MD_DIGEST_LENGTH / 2 - k);
>          *
> ​​
>  MD_Update(&m, &(state[0]), k); // CRASH*
>         } else
>             MD_Update(&m, &(state[st_idx]), MD_DIGEST_LENGTH / 2);
>         MD_Final(&m, local_md);
>
>         for (i = 0; i < MD_DIGEST_LENGTH / 2; i++) {
>             /* may compete with other threads */
>             state[st_idx++] ^= local_md[i];
>             if (st_idx >= st_num)
>                 st_idx = 0;
>             if (i < j)
>                 *(buf++) = local_md[i + MD_DIGEST_LENGTH / 2];
>         }
>     }
>
>     MD_Init(&m);
>     MD_Update(&m, (unsigned char *)&(md_c[0]), sizeof(md_c));
>     MD_Update(&m, local_md, MD_DIGEST_LENGTH);
> *    if (lock)*
> *        CRYPTO_w_lock(CRYPTO_LOCK_RAND);*
>     MD_Update(&m, md, MD_DIGEST_LENGTH);
>     MD_Final(&m, md);
>  *   if (lock)*
> *        CRYPTO_w_unlock(CRYPTO_LOCK_RAND);*
>
>     EVP_MD_CTX_cleanup(&m);
>
>
>
> Best Regards,
> Alex
>
>



More information about the openssl-dev mailing list