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: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/4328)

diff --git a/crypto/rand/drbg_lib.c b/crypto/rand/drbg_lib.c
index 0042a931b0a1291ebf64bc29f88faa896ef01487..c471b6eda4e9ae221e1dd5cc92dd63c1f34e97f5 100644 (file)
--- a/crypto/rand/drbg_lib.c
+++ b/crypto/rand/drbg_lib.c
@@ -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.
  *
  */

diff --git a/crypto/rand/rand_lib.c b/crypto/rand/rand_lib.c
index 6f8deca1f9ff16e3aa72df7c14af90ad14ec339c..a290e5c1a2558f2aefaed658cbc78758e0f3674b 100644 (file)
--- a/crypto/rand/rand_lib.c
+++ b/crypto/rand/rand_lib.c
@@ -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)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index da74e96ecb35854de67322f8335bbffe71dd87e6..ce45a916133f76e3cd43b827a12daa50c2e9785c 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -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)
-        return RAND_DRBG_generate(s->drbg, rnd, size, 0, NULL, 0);
+    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);
 }