Originally found in 1.0.0j. Did not check versions between 1.0.0j and
1.0.1c since it is still present in the current version.
I am also worried about lines 139-143:
139: if (key->pkey != NULL)
140: {
141: CRYPTO_add(&key->pkey->references, 1,
CRYPTO_LOCK_EVP_PKEY);
142: return key->pkey;
143: }
Assume thread 1 runs up to line 140 and some other thread does
EVP_PKEY_free() on the key before thread 1 can get the lock starting in
line 141. Am I overseeing some protection ?
Also note the same problem in lines 175-184:
175: CRYPTO_w_lock(CRYPTO_LOCK_EVP_PKEY);
176: if (key->pkey)
177: {
178: EVP_KEY_free(ret);
179: ret = key->pkey;
180: }
181: else
182: key->pkey = ret;
183: CRYPTO_w_unlock(CRYPTO_LOCK_EVP_PKEY);
184: CRYPTO_add(&ret->references, 1, CRYPTO_LOCK_EVP_PKEY);
Assume thread 1 goes up until and finishes line 183, then thread 2 does
EVP_PKEY_free() on key.
I would have supplied patch suggestions for those places as well but I
am not aware of an existing openssl technique of manipulating the
reference counter *without* aquiring a lock, in case we already have a
lock of the correct type.
Regards,
Thomas
diff --git a/crypto/asn1/x_pubkey.c b/crypto/asn1/x_pubkey.c
index 627ec87..d8422ca 100644
--- a/crypto/asn1/x_pubkey.c
+++ b/crypto/asn1/x_pubkey.c
@@ -133,6 +133,7 @@ error:
EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key)
{
EVP_PKEY *ret=NULL;
+ EVP_PKEY *pktmp=NULL;
if (key == NULL) goto error;
@@ -175,7 +176,7 @@ EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key)
CRYPTO_w_lock(CRYPTO_LOCK_EVP_PKEY);
if (key->pkey)
{
- EVP_PKEY_free(ret);
+ pktmp=ret;
ret = key->pkey;
}
else
@@ -183,6 +184,8 @@ EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key)
CRYPTO_w_unlock(CRYPTO_LOCK_EVP_PKEY);
CRYPTO_add(&ret->references, 1, CRYPTO_LOCK_EVP_PKEY);
+ if (pktmp)
+ EVP_KEY_free(pktmp);
return ret;
error:
