Allow generated EVP_PKEY to be used to verify operation #479

latal-1 · 2024-11-25T21:49:26Z

Description

Previously generated EVP_PKEY could be used for operations that only require a private key

Fixes #480

Checklist

Code modified for feature
Test suite updated with functionality tests
~~Test suite updated with negative tests~~
~~Documentation updated~~

Reviewer's checklist:

Any issues marked for closing are addressed
There is a test suite reasonably covering new functionality or modifications
This feature/change has adequate documentation added
Code conform to coding style that today cannot yet be enforced via the check style test
Commits have short titles and sensible commit messages
Coverity Scan has run if needed (code PR) and no new defects were found

simo5 · 2024-11-25T22:12:32Z

This proposed change has several problems.

First of all, could you open an issue describing what problem you encountered ?

Second, please do not mix extraneous changes (like the clang warning) or other reformatting with feature changes in the same commit.

Third, I do not understand the point of the code you are proposing.
It seems like you are generating a keypair, and then expecting to be able to use the key pair to verify a signature?

But this is failing because, due to how OpenSSL magaes key generation we can only return a single key type and you are left w/o the public key pair part.

If this is the case please clearly state it in the issue you need to open, because if that is the case I need to think about a different architecture, probably a new key abstraction that can hold a public and a private key pair and chose based on operation, in either case the kind of change you are proposing won't be acceptable, as it is a hack that falls short in other areas.

simo5 · 2024-11-25T22:13:04Z

Also a change like this must come with tests to be acceptable.

latal-1 · 2024-11-26T00:50:31Z

First of all, could you open an issue describing what problem you encountered ?

#480

src/objects.c

simo5

It is a good start, some minor changes are required.
that said there is more needed to make this properly functional.

src/objects.c

src/keymgmt.c

src/objects.c

EVP_PKEY_generate returns only one EVP_PKEY instance for private and public keys. This requires storing the public key object in the private key object in order to use it for operations that using the public key Signed-off-by: latal-1 <[email protected]>

Signed-off-by: latal-1 <[email protected]>

latal-1 · 2024-12-13T19:41:21Z

Also a change like this must come with tests to be acceptable.

Done

latal-1 · 2024-12-13T20:12:31Z

Sorry for a long time reaction
I can't handle some problems:

SIGSEGV in tsigver.c after move deletion from p11prov_rsa_free and p11prov_ec_free to p11prov_obj_free
pkcs11-provider:softokn / tls and pkcs11-provider:softokn / tlsfuzzer get error

Can someone solve these problems? Authorship of pull request and commits is not important for me

simo5 · 2024-12-13T20:27:57Z

src/keymgmt.c


 done:
    if (ret != CKR_OK) {
+        p11prov_obj_free(pub_key);


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.

simo5 · 2024-12-13T20:28:51Z

src/keymgmt.c

        p11prov_obj_free(priv_key);
-        priv_key = NULL;
+        pub_key = priv_key = NULL;


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

simo5 · 2024-12-13T20:30:12Z

src/objects.c

@@ -440,6 +440,9 @@ void p11prov_obj_free(P11PROV_OBJ *obj)
    if (obj == NULL) {
        return;
    }
+
+    p11prov_obj_free(obj->pub_key_obj);
+


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.

simo5 · 2024-12-13T20:35:59Z

src/signature.c

    sigctx->operation = operation;

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


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.

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.

simo5 · 2024-12-13T20:45:35Z

Note that DCO is missing in some commit and this will prevent merging until fixed (no exceptions)

I also recommend you rebase interactively and edit each commit by running make check-style-fix and then git add -u; git rebase --continue, as the style is broken in several places.

simo5

Please fix the errors I identified and all the CI failures regarding Copyright headers, commit signatures and styles.

We are almost there.

latal-1 force-pushed the add-pub-to-gen-key branch from becbf86 to e8144a2 Compare November 25, 2024 21:53

Jakuje reviewed Nov 26, 2024

View reviewed changes

src/objects.c Outdated Show resolved Hide resolved

latal-1 force-pushed the add-pub-to-gen-key branch from e8144a2 to bf702a5 Compare December 5, 2024 11:16

simo5 requested changes Dec 5, 2024

View reviewed changes

src/objects.c Outdated Show resolved Hide resolved

src/keymgmt.c Show resolved Hide resolved

src/keymgmt.c Outdated Show resolved Hide resolved

src/objects.c Show resolved Hide resolved

Kirill Ermoshin added 5 commits December 13, 2024 22:17

Use public key object from P11PROV_OBJ during generate and verify

6f1a787

Signed-off-by: latal-1 <[email protected]>

Remove reinitialization of P11PROV_OBJ some fields

57fb91a

Signed-off-by: latal-1 <[email protected]>

Reformat some files with clang-format

9fd45c3

Signed-off-by: latal-1 <[email protected]>

Add test on sign and verify with generated keys

f5f049d

Signed-off-by: latal-1 <[email protected]>

latal-1 force-pushed the add-pub-to-gen-key branch from bf702a5 to f5f049d Compare December 13, 2024 19:37

simo5 reviewed Dec 13, 2024

View reviewed changes

simo5 self-requested a review December 13, 2024 20:49

simo5 requested changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow generated EVP_PKEY to be used to verify operation #479

Allow generated EVP_PKEY to be used to verify operation #479

latal-1 commented Nov 25, 2024 •

edited

Loading

simo5 commented Nov 25, 2024

simo5 commented Nov 25, 2024

latal-1 commented Nov 26, 2024

simo5 left a comment

latal-1 commented Dec 13, 2024

latal-1 commented Dec 13, 2024

simo5 Dec 13, 2024

simo5 Dec 13, 2024

simo5 Dec 13, 2024

simo5 Dec 13, 2024 •

edited

Loading

simo5 Dec 13, 2024

simo5 commented Dec 13, 2024

simo5 left a comment

Allow generated EVP_PKEY to be used to verify operation #479

Are you sure you want to change the base?

Allow generated EVP_PKEY to be used to verify operation #479

Conversation

latal-1 commented Nov 25, 2024 • edited Loading

Description

Checklist

Reviewer's checklist:

simo5 commented Nov 25, 2024

simo5 commented Nov 25, 2024

latal-1 commented Nov 26, 2024

simo5 left a comment

Choose a reason for hiding this comment

latal-1 commented Dec 13, 2024

latal-1 commented Dec 13, 2024

simo5 Dec 13, 2024

Choose a reason for hiding this comment

simo5 Dec 13, 2024

Choose a reason for hiding this comment

simo5 Dec 13, 2024

Choose a reason for hiding this comment

simo5 Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

simo5 Dec 13, 2024

Choose a reason for hiding this comment

simo5 commented Dec 13, 2024

simo5 left a comment

Choose a reason for hiding this comment

latal-1 commented Nov 25, 2024 •

edited

Loading

simo5 Dec 13, 2024 •

edited

Loading