Commit 2139145b authored by Benjamin Kaduk's avatar Benjamin Kaduk Committed by Ben Kaduk
Browse files

Add missing RAND_DRBG locking 



The drbg's lock must be held across calls to RAND_DRBG_generate()
to prevent simultaneous modification of internal state.

This was observed in practice with simultaneous SSL_new() calls attempting
to seed the (separate) per-SSL RAND_DRBG instances from the global
rand_drbg instance; this eventually led to simultaneous calls to
ctr_BCC_update() attempting to increment drbg->bltmp_pos for their
respective partial final block, violating the invariant that bltmp_pos < 16.
The AES operations performed in ctr_BCC_blocks() makes the race window
quite easy to trigger.  A value of bltmp_pos greater than 16 induces
catastrophic failure in ctr_BCC_final(), with subtraction overflowing
and leading to an attempt to memset() to zero a very large range,
which eventually reaches an unmapped page and segfaults.

Provide the needed locking in get_entropy_from_parent(), as well as
fixing a similar issue in RAND_priv_bytes().  There is also an
unlocked call to RAND_DRBG_generate() in ssl_randbytes(), but the
requisite serialization is already guaranteed by the requirements on
the application's usage of SSL objects, and no further locking is
needed for correct behavior.  In that case, leave a comment noting
the apparent discrepancy and the reason for its safety (at present).

Reviewed-by: default avatarPaul Dale <paul.dale@oracle.com>
Reviewed-by: default avatarKurt Roeckx <kurt@roeckx.be>
Reviewed-by: default avatarRich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4328)
parent e0b625f9

crypto/rand/drbg_lib.c

+8 −0
Original line number Diff line number Diff line
@@ -116,6 +116,8 @@ void RAND_DRBG_free(RAND_DRBG *drbg) 
/*

 * Instantiate |drbg|, after it has been initialized.  Use |pers| and

 * |perslen| as prediction-resistance input.

 *

 * Requires that drbg->lock is already locked for write, if non-null.

 */

int RAND_DRBG_instantiate(RAND_DRBG *drbg,

                          const unsigned char *pers, size_t perslen)
@@ -185,6 +187,8 @@ end: 


/*

 * Uninstantiate |drbg|. Must be instantiated before it can be used.

 *

 * Requires that drbg->lock is already locked for write, if non-null.

 */

int RAND_DRBG_uninstantiate(RAND_DRBG *drbg)

{
@@ -197,6 +201,8 @@ int RAND_DRBG_uninstantiate(RAND_DRBG *drbg) 


/*

 * Reseed |drbg|, mixing in the specified data

 *

 * Requires that drbg->lock is already locked for write, if non-null.

 */

int RAND_DRBG_reseed(RAND_DRBG *drbg,

                     const unsigned char *adin, size_t adinlen)
@@ -349,6 +355,8 @@ int rand_drbg_restart(RAND_DRBG *drbg, 
 * to or if |prediction_resistance| is set.  Additional input can be

 * sent in |adin| and |adinlen|.

 *

 * Requires that drbg->lock is already locked for write, if non-null.

 *

 * Returns 1 on success, 0 on failure.

 *

 */

crypto/rand/rand_lib.c

+15 −2
Original line number Diff line number Diff line
@@ -155,12 +155,20 @@ size_t rand_drbg_get_entropy(RAND_DRBG *drbg, 
        if (buffer != NULL) {

            size_t bytes = 0;



            /* Get entropy from parent, include our state as additional input */

            /*

             * Get random from parent, include our state as additional input.

             * Our lock is already held, but we need to lock our parent before

             * generating bits from it.

             */

            if (drbg->parent->lock)

                CRYPTO_THREAD_write_lock(drbg->parent->lock);

            if (RAND_DRBG_generate(drbg->parent,

                                   buffer, bytes_needed,

                                   0,

                                   (unsigned char *)drbg, sizeof(*drbg)) != 0)

                bytes = bytes_needed;

            if (drbg->parent->lock)

                CRYPTO_THREAD_unlock(drbg->parent->lock);



            entropy_available = RAND_POOL_add_end(pool, bytes, 8 * bytes);

        }
@@ -626,6 +634,7 @@ int RAND_priv_bytes(unsigned char *buf, int num) 
{

    const RAND_METHOD *meth = RAND_get_rand_method();

    RAND_DRBG *drbg;

    int ret;



    if (meth != RAND_OpenSSL())

        return RAND_bytes(buf, num);
@@ -634,7 +643,11 @@ int RAND_priv_bytes(unsigned char *buf, int num) 
    if (drbg == NULL)

        return 0;



    return RAND_DRBG_generate(drbg, buf, num, 0, NULL, 0);

    /* We have to lock the DRBG before generating bits from it. */

    CRYPTO_THREAD_write_lock(drbg->lock);

    ret = RAND_DRBG_generate(drbg, buf, num, 0, NULL, 0);

    CRYPTO_THREAD_unlock(drbg->lock);

    return ret;

}



int RAND_bytes(unsigned char *buf, int num)

ssl/ssl_lib.c

+15 −2
Original line number Diff line number Diff line
@@ -5127,7 +5127,20 @@ uint32_t SSL_get_max_early_data(const SSL *s) 


int ssl_randbytes(SSL *s, unsigned char *rnd, size_t size)

{

    if (s->drbg != NULL)

    if (s->drbg != NULL) {

        /*

         * Currently, it's the duty of the caller to serialize the generate

         * requests to the DRBG. So formally we have to check whether

         * s->drbg->lock != NULL and take the lock if this is the case.

         * However, this DRBG is unique to a given SSL object, and we already

         * require that SSL objects are only accessed by a single thread at

         * a given time. Also, SSL DRBGs have no child DRBG, so there is

         * no risk that this DRBG is accessed by a child DRBG in parallel

         * for reseeding.  As such, we can rely on the application's

         * serialization of SSL accesses for the needed concurrency protection

         * here.

         */

         return RAND_DRBG_generate(s->drbg, rnd, size, 0, NULL, 0);

    }

    return RAND_bytes(rnd, (int)size);

}