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.
|
|
||
| P11PROV_debug("Refresh object %p", obj); | ||
|
|
||
| if (obj->handle_owner) { |
There was a problem hiding this comment.
This would require us to know that the owner was refreshed before this object?
| 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?
| 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.
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: