Destroy stored imported public keys on free - #751
Conversation
When an imported public key is used in a token operation, the provider stores it on the token as a session object. The object was never destroyed, so every unique imported public key leaked one object on the long lived login session. Mark objects stored by the provider and destroy their handle when the object is freed. Objects created from a pool copy borrow the handle and keep a reference on the owning object, so the stored object is only destroyed when the last object using it goes away. Refreshing an imported object now stores the key again instead of searching the token for an object that cannot be found there. Fixes openssl-projects#739 Signed-off-by: Jakub Zelenka <jakub.openssl@gmail.com>
simo5
left a comment
simo5
left a comment
There was a problem hiding this comment.
Some questions that should be answered before changing the names.
Overall I understand the intention and makes sense to me, but I wonder if "imported" is needed at all.
| if (src->handle_owner || src->imported) { | ||
| /* keep the owner alive as the handle is destroyed on its free */ | ||
| dst->handle_owner = p11prov_obj_ref_no_cache( | ||
| src->handle_owner ? src->handle_owner : src); |
There was a problem hiding this comment.
bring this out as a proper if/else, please
| struct p11prov_obj { | ||
| P11PROV_CTX *ctx; | ||
| bool raf; /* re-init after fork */ | ||
| bool imported; |
There was a problem hiding this comment.
I think the proper name for this should be either "ephemeral" or "session_obj", "imported" is ambiguous because I can import and store in the token database.
There was a problem hiding this comment.
Actually now that I think of it, what is the difference between this "imported" bool set to true and cka_token below set to false?
There was a problem hiding this comment.
Hmm, couldn't this result in attempt to destroy objects that are not imported potentially causing some mix up. In other word aren't there cases where cka_token is false but imported is also false. It looked to me there are few suspicious places where this could happen but will need to verify it properly if you are not sure?
There was a problem hiding this comment.
well any object that has cka_token = false, is a session object and therefore ephemeral in nature. What I am trying to understand here is if there is a case where the distinction between:
- generated as a session object by pkcs11-provider
vs - imported as as session object by ask of openssl
is an important question for this functionality.
If you do not have any reference anymore to an object do we still need to retain it in the session at all if we generated it?
Generally these would be ephemeral symmetric keys that are not "recalled" once the EVP_PKEY object is destroyed, so I can't think of a case where the distinction matters for the purpose of retaining versus destroying the object.
There was a problem hiding this comment.
I will check this out when I have more time for this and verify it.
| dst->slotid = src->slotid; | ||
| dst->handle = src->handle; | ||
| dst->handle = p11prov_obj_get_handle(src); | ||
| if (src->handle_owner || src->imported) { |
There was a problem hiding this comment.
Looking at this code TBH I feel like I want to do away with p11prov_obj_copy_key_data() but the incoming key comes from openssl so it is not really possible, sigh ...
That said, "handle_owner" is really not very clear, because what matters here is that you want to keep a reference to the original object so that it is not deleted. So I think this field is probably better called "ref_obj" or maybe "ref_orig_obj" to make it clear this is a reference being hold.
There was a problem hiding this comment.
Yeah your name makes more sense. Will change it.
|
Thanks for the review. I was just looking into it again and not sure if the logic is actually right for cached objects. Will need to verify it. Also there might be a race in pool_find that this could make worse so will need to check that properly too. |
2 participants
Description
When an imported public key is used in a token operation, the provider stores it on the token as a session object. The object was never destroyed, so every unique imported public key leaked one object on the long lived login session.
Mark objects stored by the provider and destroy their handle when the object is freed. Objects created from a pool copy borrow the handle and keep a reference on the owning object, so the stored object is only destroyed when the last object using it goes away. Refreshing an imported object now stores the key again instead of searching the token for an object that cannot be found there.
Fixes #739
Checklist
Reviewer's checklist: