Don't hold CRYPTO_LOCK_RSA during time-consuming operations.

diff --git a/CHANGES b/CHANGES
index fecbe653fa8895df372abd34c67cc83a369d2023..c63a2dfcc9285852c902761bcaccd79320d92b3f 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -9,13 +9,13 @@
      [Bodo Moeller; problem reported by Eric Day <eday@concentric.net>]
 
   *) In RSA_eay_public_{en,ed}crypt and RSA_eay_mod_exp (rsa_eay.c),
-     obtain lock CRYPTO_LOCK_RSA before creating BN_MONT_CTX
-     structures and setting rsa->_method_mod_{n,p,q}.
+     obtain lock CRYPTO_LOCK_RSA before setting rsa->_method_mod_{n,p,q}.
 
      (RSA objects have a reference count access to which is protected
      by CRYPTO_LOCK_RSA [see rsa_lib.c, s3_srvr.c, ssl_cert.c, ssl_rsa.c],
      so they are meant to be shared between threads.)
-     [patch submitted by "Reddie, Steven" <Steven.Reddie@ca.com>]
+     [Bodo Moeller, Geoff Thorpe; original patch submitted by
+     "Reddie, Steven" <Steven.Reddie@ca.com>]
 
   *) Make mkdef.pl parse some of the ASN1 macros and add apropriate
      entries for variables.

diff --git a/crypto/rsa/rsa_eay.c b/crypto/rsa/rsa_eay.c
index f92a3022cb457399efbd18b0929a16c2cbc40e2d..35db9e5687ce1e69047ccb5a2bf2d8de26b249ae 100644 (file)
--- a/crypto/rsa/rsa_eay.c
+++ b/crypto/rsa/rsa_eay.c
@@ -141,26 +141,28 @@ static int RSA_eay_public_encrypt(int flen, const unsigned char *from,
        
        if ((rsa->_method_mod_n == NULL) && (rsa->flags & RSA_FLAG_CACHE_PUBLIC))
                {
-               CRYPTO_w_lock(CRYPTO_LOCK_RSA);
-               if (rsa->_method_mod_n == NULL)
+               BN_MONT_CTX* bn_mont_ctx;
+               if ((bn_mont_ctx=BN_MONT_CTX_new()) == NULL)
+                       goto err;
+               if (!BN_MONT_CTX_set(bn_mont_ctx,rsa->n,ctx))
                        {
-                       BN_MONT_CTX* bn_mont_ctx;
-                       if ((bn_mont_ctx=BN_MONT_CTX_new()) == NULL)
-                               {
-                               CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
-                               goto err;
-                               }
-                       if (!BN_MONT_CTX_set(bn_mont_ctx,rsa->n,ctx))
+                       BN_MONT_CTX_free(bn_mont_ctx);
+                       goto err;
+                       }
+               if (rsa->_method_mod_n == NULL) /* other thread may have finished first */
+                       {
+                       CRYPTO_w_lock(CRYPTO_LOCK_RSA);
+                       if (rsa->_method_mod_n == NULL)
                                {
-                               BN_MONT_CTX_free(bn_mont_ctx);
-                               CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
-                               goto err;
+                               rsa->_method_mod_n = bn_mont_ctx;
+                               bn_mont_ctx = NULL;
                                }
-                       rsa->_method_mod_n = bn_mont_ctx;
+                       CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
                        }
-               CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
+               if (bn_mont_ctx)
+                       BN_MONT_CTX_free(bn_mont_ctx);
                }
-
+               
        if (!meth->bn_mod_exp(&ret,&f,rsa->e,rsa->n,ctx,
                rsa->_method_mod_n)) goto err;
 
@@ -393,26 +395,28 @@ static int RSA_eay_public_decrypt(int flen, const unsigned char *from,
        /* do the decrypt */
        if ((rsa->_method_mod_n == NULL) && (rsa->flags & RSA_FLAG_CACHE_PUBLIC))
                {
-               CRYPTO_w_lock(CRYPTO_LOCK_RSA);
-               if (rsa->_method_mod_n == NULL)
+               BN_MONT_CTX* bn_mont_ctx;
+               if ((bn_mont_ctx=BN_MONT_CTX_new()) == NULL)
+                       goto err;
+               if (!BN_MONT_CTX_set(bn_mont_ctx,rsa->n,ctx))
                        {
-                       BN_MONT_CTX* bn_mont_ctx;
-                       if ((bn_mont_ctx=BN_MONT_CTX_new()) == NULL)
-                               {
-                               CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
-                               goto err;
-                               }
-                       if (!BN_MONT_CTX_set(bn_mont_ctx,rsa->n,ctx))
+                       BN_MONT_CTX_free(bn_mont_ctx);
+                       goto err;
+                       }
+               if (rsa->_method_mod_n == NULL) /* other thread may have finished first */
+                       {
+                       CRYPTO_w_lock(CRYPTO_LOCK_RSA);
+                       if (rsa->_method_mod_n == NULL)
                                {
-                               BN_MONT_CTX_free(bn_mont_ctx);
-                               CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
-                               goto err;
+                               rsa->_method_mod_n = bn_mont_ctx;
+                               bn_mont_ctx = NULL;
                                }
-                       rsa->_method_mod_n = bn_mont_ctx;
+                       CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
                        }
-               CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
+               if (bn_mont_ctx)
+                       BN_MONT_CTX_free(bn_mont_ctx);
                }
-
+               
        if (!meth->bn_mod_exp(&ret,&f,rsa->e,rsa->n,ctx,
                rsa->_method_mod_n)) goto err;
 
@@ -462,48 +466,53 @@ static int RSA_eay_mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa)
                {
                if (rsa->_method_mod_p == NULL)
                        {
-                       CRYPTO_w_lock(CRYPTO_LOCK_RSA);
-                       if (rsa->_method_mod_p == NULL)
+                       BN_MONT_CTX* bn_mont_ctx;
+                       if ((bn_mont_ctx=BN_MONT_CTX_new()) == NULL)
+                               goto err;
+                       if (!BN_MONT_CTX_set(bn_mont_ctx,rsa->p,ctx))
                                {
-                               BN_MONT_CTX* bn_mont_ctx;
-                               if ((bn_mont_ctx=BN_MONT_CTX_new()) == NULL)
-                                       {
-                                       CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
-                                       goto err;
-                                       }
-                               if (!BN_MONT_CTX_set(bn_mont_ctx,rsa->p,ctx))
+                               BN_MONT_CTX_free(bn_mont_ctx);
+                               goto err;
+                               }
+                       if (rsa->_method_mod_p == NULL) /* other thread may have finished first */
+                               {
+                               CRYPTO_w_lock(CRYPTO_LOCK_RSA);
+                               if (rsa->_method_mod_p == NULL)
                                        {
-                                       BN_MONT_CTX_free(bn_mont_ctx);
-                                       CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
-                                       goto err;
+                                       rsa->_method_mod_p = bn_mont_ctx;
+                                       bn_mont_ctx = NULL;
                                        }
-                               rsa->_method_mod_p = bn_mont_ctx;
-                               }
                                CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
+                               }
+                       if (bn_mont_ctx)
+                               BN_MONT_CTX_free(bn_mont_ctx);
                        }
+
                if (rsa->_method_mod_q == NULL)
                        {
-                       CRYPTO_w_lock(CRYPTO_LOCK_RSA);
-                       if (rsa->_method_mod_q == NULL)
+                       BN_MONT_CTX* bn_mont_ctx;
+                       if ((bn_mont_ctx=BN_MONT_CTX_new()) == NULL)
+                               goto err;
+                       if (!BN_MONT_CTX_set(bn_mont_ctx,rsa->q,ctx))
                                {
-                               BN_MONT_CTX* bn_mont_ctx;
-                               if ((bn_mont_ctx=BN_MONT_CTX_new()) == NULL)
-                                       {
-                                       CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
+                               BN_MONT_CTX_free(bn_mont_ctx);
                                        goto err;
-                                       }
-                               if (!BN_MONT_CTX_set(bn_mont_ctx,rsa->q,ctx))
+                               }
+                       if (rsa->_method_mod_q == NULL) /* other thread may have finished first */
+                               {
+                               CRYPTO_w_lock(CRYPTO_LOCK_RSA);
+                               if (rsa->_method_mod_q == NULL)
                                        {
-                                       BN_MONT_CTX_free(bn_mont_ctx);
-                                       CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
-                                       goto err;
+                                       rsa->_method_mod_q = bn_mont_ctx;
+                                       bn_mont_ctx = NULL;
                                        }
-                               rsa->_method_mod_q = bn_mont_ctx;
+                               CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
                                }
-                       CRYPTO_w_unlock(CRYPTO_LOCK_RSA);
+                       if (bn_mont_ctx)
+                               BN_MONT_CTX_free(bn_mont_ctx);
                        }
                }
-
+               
        if (!BN_mod(&r1,I,rsa->q,ctx)) goto err;
        if (!meth->bn_mod_exp(&m1,&r1,rsa->dmq1,rsa->q,ctx,
                rsa->_method_mod_q)) goto err;