Fix EC PKCS#11 object leak with OpenSSL < 3 and improve RSA reference counting#593
Conversation
|
@olszomal Please make sure this fix for unsupported OpenSSL versions does not negatively impact the supported versions, which are far more important. |
|
@olszomal btw I re-ran the |
|
So far I reviewed ae5e3d7. The commit description is fairly complicated, while the proposed change simply refactors |
|
I have verified that the proposed changes do not negatively affect the supported OpenSSL versions. There is still a memory leak in |
@mtrojnar Thank you! I renamed the function as you suggested, happy to attempt to reword the commit as well to make it more clear what the change is. The other change in that commit is that a single RSA object will always hold a single reference to a PKCS#11 object, while before this commit each call to |
mtrojnar
left a comment
mtrojnar
left a comment
There was a problem hiding this comment.
Good. Now git rebase to get rid of the Merge branch 'master' into improve-object-ref-counting commit, git rebase -i to squash the fixes/improvements into the commit with RSA fixes (you may use opportunity to improve commit description), and git push --force to push a clean pull request with only two commits.
|
@hazefully ping |
When an ECDSA private key is retrieved via the pkcs11_get_key function with OpenSSL versions < 3, a new reference to the underlying EC_KEY object is created and assigned to a new EVP_PKEY which is returned from the function. However, the reference counter of the PKCS11_OBJECT_private object attached to the EC_KEY is also incremented, which means that a single EC_KEY object ends up holding two references to the attached PKCS11_OBJECT_private, which leads to the reference count of the PKCS11_OBJECT_private object never reaching zero even when all the EVP_PKEY objects that reference them are freed. This doesn't happen with OpenSSL versions >= 3.0 as it creates a duplicate EC_KEY object instead of assigning the same object. This commit changes the logic in the pkcs11_get_key function for EC keys to stop incrementing the PKCS11_OBJECT_private reference count when assigning the EC_KEY object, to ensure that a single EC_KEY object always holds a single reference to the attached PKCS11_OBJECT_private. This leads to the correct clean up of the PKCS11_OBJECT_private objects and PKCS#11 slot objects.
This commit refactors the pkcs11_rsa function, used in operations on RSA EVP_PKEYs, to return a new reference to the underlying RSA object instead of a pointer to an existing reference. This avoids having to make assumptions about the reference count of the underlying RSA object of an EVP_PKEY in pkcs11_rsa. To reflect the fact that the returned reference must be freed after use, the function is renamed to pkcs11_get1_rsa following the conventions of OpenSSL functions. In addition, this commit ensures that the reference count of a PKCS11_OBJECT_private object that is attached to an RSA object is incremented only once for each RSA object. This assures that a PKCS11_OBJECT_private object can be freed once all RSA objects that reference it are freed.
hazefully
force-pushed
the
improve-object-ref-counting
branch
from
bfe59c7 to
cb7009a
Compare
|
@mtrojnar I did a rebase and attempted to reword the commit messages to make it more clear what they are trying to achieve, let me know if that looks good to you now |
|
@hazefully Thank you for your contribution. |
3 participants
Problem
When using an ECDSA private key with OpenSSL versions < 3, the
PKCS11_OBJECT_privateobject and attachedPKCS11_SLOT_privateend up leaking and not being cleaned up, even with all instances of theEVP_PKEYbeing freed. This can be seen when theec-check-privkey.softhsmtest is run with valgrind (copying relevant leaks only as there are a lot of leaks related todlopen) :A side effect of this is that
CloseAllSessionsis not called during the engine cleanup as the related slot's reference count never reaches zero:The same problem doesn't happen with OpenSSL 3.0.17, as it follows a different code path:
This issue is very similar to what used to happen with RSA keys, which was fixed in #555.
Changes
7ed4afc fixes the code path in
pkcs11_get_keyfor OpenSSL versions < 3 to skip incrementing the PKCS11_OBJECT_private object's reference counter when assigning the underlying EC_KEY object which already holds a reference to the same PKCS#11 object. This stops the memory leak and causesCloseAllSessionsto get called as expected:In addition, ae5e3d7 improves the reference counting of RSA objects and attached PKCS#11 objects to remove the assumption made in
pkcs11_rsain p11_rsa.c about the OpenSSL's EVP_PKEY reference counter. See the commit message for more details about the changes. This commit doesn't re-introduce the leak fixed in #555 with either OpenSSL 1.1.1 or OpenSSL 3.0.17: