Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow generated EVP_PKEY to be used to verify operation #479

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Allow generated EVP_PKEY to be used to verify operation #479

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b9df2d
Add public key object pointer to P11PROV_OBJ
Dec 5, 2024
6f1a787
Use public key object from P11PROV_OBJ during generate and verify
Dec 5, 2024
57fb91a
Remove reinitialization of P11PROV_OBJ some fields
Dec 5, 2024
9fd45c3
Reformat some files with clang-format
Dec 5, 2024
f5f049d
Add test on sign and verify with generated keys
Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/keymgmt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,14 +482,19 @@ static int p11prov_common_gen(struct key_generator *ctx,
}

ret = p11prov_merge_pub_attrs_into_priv(pub_key, priv_key);
simo5 marked this conversation as resolved.
Show resolved Hide resolved
if (ret != CKR_OK) {
goto done;
}

ret = p11prov_set_pub(priv_key, pub_key);

done:
if (ret != CKR_OK) {
p11prov_obj_free(pub_key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong now because the next line will already free the pub_key you set on it.
After you set the pub key on the priv key you need to set pub_key = NULL;

This is may be one of the segfault issue, because you are causing a double-free here.

p11prov_obj_free(priv_key);
priv_key = NULL;
pub_key = priv_key = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This become useless once you correctly zero the pub_key right after you pass ownership of it to the priv_key object.

}
p11prov_return_session(session);
p11prov_obj_free(pub_key);
*key = priv_key;
return ret;
}
Expand Down
3 changes: 3 additions & 0 deletions src/objects.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,9 @@ void p11prov_obj_free(P11PROV_OBJ *obj)
if (obj == NULL) {
return;
}

p11prov_obj_free(obj->pub_key_obj);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, all freeing MUST happen after the refcounting reaches 0.
Move it after the following block and you'll solve most of your segfaults.

if (__atomic_sub_fetch(&obj->refcnt, 1, __ATOMIC_SEQ_CST) != 0) {
P11PROV_debug("object free: reference held");
return;
Expand Down
16 changes: 10 additions & 6 deletions src/signature.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,11 +686,7 @@ static CK_RV p11prov_sig_op_init(void *ctx, void *provkey, CK_FLAGS operation,
return ret;
}

sigctx->key = p11prov_obj_ref(key);
if (sigctx->key == NULL) {
return CKR_KEY_NEEDED;
}
class = p11prov_obj_get_class(sigctx->key);
class = p11prov_obj_get_class(key);
switch (operation) {
case CKF_SIGN:
if (class != CKO_PRIVATE_KEY) {
Expand All @@ -699,17 +695,25 @@ static CK_RV p11prov_sig_op_init(void *ctx, void *provkey, CK_FLAGS operation,
break;
case CKF_VERIFY:
if (class != CKO_PUBLIC_KEY) {
return CKR_KEY_TYPE_INCONSISTENT;
key = p11prov_get_pub(key);
if (key == NULL) {
return CKR_KEY_TYPE_INCONSISTENT;
}
}
break;
default:
return CKR_GENERAL_ERROR;
}
sigctx->key = p11prov_obj_ref(key);
if (sigctx->key == NULL) {
return CKR_KEY_NEEDED;
}
sigctx->operation = operation;

if (digest) {
ret = p11prov_digest_get_by_name(digest, &sigctx->digest);
if (ret != CKR_OK) {
p11prov_obj_free(sigctx->key);
Copy link
Member

@simo5 simo5 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, if you free the key here you also need to NULL the pointer or we will get a double free in p11prov_sig_freectx(), but you do not have to null anything here exactly because p11prov_sig_freectx() already takes care of this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are concerned about someone potentially calling the init operation multiple times, just check if the sigctx->key is not NULL at the start of the function and immediately error if that is the case. But there is no need to add that, it is an application error to try to initialize multiple times and the worst case is some memory leakage.

ERR_raise(ERR_LIB_PROV, PROV_R_INVALID_DIGEST);
return ret;
}
Expand Down