[openssl.org #2905] Double locking bug added in openssl-1.0.0h crypto/asn1/x_pubkey.c

[Richard Skinner via RT]

The following code added in 1.0.0h causes CRYPTO_LOCK_EVP_PKEY lock to be 
requested twice with no intervening unlock when there is a race between 2  or 
more threads to create the EVP_PKEY associated with the X509_PUBKEY. The second 
lock request occurs in EVP_PKEY_free().

crypto/asn1/x_pubkey.c:174

        /* Check to see if another thread set key->pkey first */
        CRYPTO_w_lock(CRYPTO_LOCK_EVP_PKEY);
        if (key->pkey)
                {
                EVP_PKEY_free(ret);
                ret = key->pkey;
                }
        else
                key->pkey = ret;
        CRYPTO_w_unlock(CRYPTO_LOCK_EVP_PKEY);
        CRYPTO_add(&ret->references, 1, CRYPTO_LOCK_EVP_PKEY);

In the case where the user supplied locking mechanism uses non-recursive 
mutexes, this will cause the second lock request (within the call to 
EVP_PKEY_free) to either deadlock or simply fail (if deadlock checking has been 
implemented).
In the case where the second lock fails, the CRYPTO_LOCK_EVP_PKEY will be 
unlocked on return from EVP_PKEY_free and the call to CRYPTO_w_unlock is a 
no-op.
I have attached a patch file which should apply cleanly to 1.0.0h/i/j (`patch 
-p0 < patch.txt` from the openssl base directory). The patch moves the call to 
EVP_PKEY_free outside the CRYPTO_LOCK_EVP_PKEY locking section - though I'm in 
two minds as to whether a better fix wouldn't be just to remove the new calls 
to CRYPTO_x_lock/unlock as these appear to be used to protect access to 
key->pkey which isn't deemed necessary elsewhere in the same function and 
wasn't deemed necessary in the original code prior to 1.0.0h.


MARKITSERV DISCLAIMER: This email and any files transmitted with it are 
confidential and intended solely for the use of the individual or entity to 
whom they are addressed. If you have received this email in error, please 
notify us immediately and delete the email and any attachments from your 
system. The recipient should check this email and any attachments for the 
presence of viruses.  MarkitSERV accepts no liability for any damage caused by 
any virus transmitted by this email, makes no warranty as to the accuracy or 
completeness of this email and hereby disclaims any liability of any kind for 
the information contained herein.  For full details about MarkitSERV, its 
offerings and legal terms and conditions, please see MarkitSERV's website at 
http://www.markitserv.com.


--- crypto/asn1/x_pubkey.c      2012-02-28 14:47:25.000000000 +0000
+++ /d/dev/openssl/crypto/asn1/x_pubkey.c       2012-10-30 16:18:15.571215400 
+0000
@@ -133,6 +133,7 @@
 EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key)
        {
        EVP_PKEY *ret=NULL;
+    EVP_PKEY *freeval=NULL;

 
        if (key == NULL) goto error;
 
@@ -175,13 +176,15 @@
        CRYPTO_w_lock(CRYPTO_LOCK_EVP_PKEY);
        if (key->pkey)
                {
-               EVP_PKEY_free(ret);
+        freeval = ret;

                ret = key->pkey;
                }
        else
                key->pkey = ret;
        CRYPTO_w_unlock(CRYPTO_LOCK_EVP_PKEY);
        CRYPTO_add(&ret->references, 1, CRYPTO_LOCK_EVP_PKEY);
+    if (freeval)

+        EVP_PKEY_free(freeval);

 
        return ret;