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.

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 setting OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY for EC keys #277

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

manison
Copy link
Contributor

@manison manison commented Aug 21, 2023

This PR implements set_params method for EC keys that allows assigning OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY parameter. This parameter is required for EVP_PKEY_set1_encoded_public_key to function properly. This function is in turn used by CMS_decrypt_set1_pkey_and_peer so without this decrypting a CMS does not work at all.

This PR also adds a test to check CMS encrypt/decrypt using an EC key. Since decrypting a CMS uses CKM_ECDH1_DERIVE mechanism with shared data the test is being run on nss-softokn only. Although SoftHSM2 also supports ECDH1 derivation mechanism it currently (softhsm/SoftHSMv2#599) only does when no shared data is specified.

This test is only run on softokn since decrypting CMS encrypted with EC
keys requires the PKCS#11 token to support CKM_ECDH1_DERIVE mechanism
*with* shared data. SoftHSM2 support ECDH1 derivation mechanism with no
shared data only.

Signed-off-by: manison <manison@users.noreply.github.com>
@simo5
Copy link
Member

simo5 commented Aug 21, 2023

@manison this looks interesting.
I am surprised that the code expects public key data to already exist and be replaced.
How is the API actually work here?
What's in the key public attributes before the setting happens?

@manison
Copy link
Contributor Author

manison commented Aug 21, 2023

It's during CMS_Decrypt, it should be easily visible from the tcms test I added in this PR.

This is the backtrace:

pkcs11.dll!p11prov_obj_set_ec_encoded_public_key(p11prov_obj * key, const void * pubkey, unsigned __int64 pubkey_len)
pkcs11.dll!p11prov_ec_set_params(void * keydata, const ossl_param_st * params)
[Inline Frame] libcrypto3.dll!EVP_PKEY_set_params(evp_pkey_st *)
[Inline Frame] libcrypto3.dll!EVP_PKEY_set_octet_string_param(evp_pkey_st *)
libcrypto3.dll!EVP_PKEY_set1_encoded_public_key(evp_pkey_st * pkey, const unsigned char * pub, unsigned __int64 publen)
libcrypto3.dll!ecdh_cms_set_peerkey(evp_pkey_ctx_st * pubkey, X509_algor_st *)
libcrypto3.dll!ecdh_cms_decrypt(CMS_RecipientInfo_st * ri)
libcrypto3.dll!CMS_RecipientInfo_kari_decrypt(CMS_ContentInfo_st * cms, CMS_RecipientInfo_st * ri, CMS_RecipientEncryptedKey_st * rek)
[Inline Frame] libcrypto3.dll!cms_kari_set1_pkey_and_peer(CMS_ContentInfo_st *)
libcrypto3.dll!CMS_decrypt_set1_pkey_and_peer(CMS_ContentInfo_st * cms, evp_pkey_st * pk, x509_st * cert, x509_st * peer)
[Inline Frame] libcrypto3.dll!CMS_decrypt_set1_pkey(CMS_ContentInfo_st *)
libcrypto3.dll!CMS_decrypt(CMS_ContentInfo_st * cms, evp_pkey_st * pk, x509_st * cert, bio_st * dcont, bio_st * out, unsigned int flags)

The ecdh_cms_set_peerkey calls EVP_PKEY_copy_parameters so my guess is that they duplicate a key and then replace its public part?

@simo5
Copy link
Member

simo5 commented Aug 22, 2023

Ok, I see how this is messed up, but it is what it is.
What I need here before we can merge is a check that will refuse to set parameters if this is a key from the token. Parameter setting should be available only for generated keys.
We may need to add a boolean to the key structure to record this fact.

@manison
Copy link
Contributor Author

manison commented Aug 22, 2023

What I need here before we can merge is a check that will refuse to set parameters if this is a key from the token. Parameter setting should be available only for generated keys.

Agreed.

We may need to add a boolean to the key structure to record this fact.

Wouldn't it be enough to check the object handle and/or slot id?

@simo5
Copy link
Member

simo5 commented Aug 22, 2023

Wouldn't it be enough to check the object handle and/or slot id?

I guess that will work, yeah.

This is required for EVP_PKEY_set1_encoded_public_key (used by
CMS_decrypt_set1_pkey_and_peer for example).

Signed-off-by: manison <manison@users.noreply.github.com>
@manison manison force-pushed the feature/set-encoded-pubkey branch from ee39710 to 11d8f68 Compare August 28, 2023 11:32
@manison
Copy link
Contributor Author

manison commented Aug 28, 2023

I added the check to verify we only set public key to mock objects.

@simo5
Copy link
Member

simo5 commented Aug 28, 2023

Looks good

@simo5
Copy link
Member

simo5 commented Aug 28, 2023

Sadly we have some CI woes lately, but this looks good, so I'll merge and fix CI later.

@simo5 simo5 merged commit 49acfd0 into latchset:main Aug 28, 2023
@manison manison deleted the feature/set-encoded-pubkey branch August 28, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants