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

Some fixes for EdDSA signatures + test coverage #497

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

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Jan 9, 2025

Description

There was a logic error in creating the eddsa signature params for pkcs11, that when OSSL_SIGNATURE_PARAM_INSTANCE OSSL_PARAM was not provided, the pkcs11 parameters were not created and Ed448 operations were failing.

For some reason, the pkcs11 parameters are required for the Ed448 even if no prehashing nor context is used.

Additionally, if only the context is provided by OpenSSL/caller, we also need to provide the correct parameters, which was not handled before.

Unfortunately, this is not the issue I was after during last days.

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

@Jakuje Jakuje requested a review from simo5 January 9, 2025 18:07
Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

I think the fix needs changes, but it is correct in abstract

ret = EVP_DigestSignFinal(sign_md[c], sig, &size);
if (mdname == NULL) {
ret =
EVP_DigestSign(sign_md[c], sig, &size, data, sizeof(data));
Copy link
Member

Choose a reason for hiding this comment

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

Given we do not really need multiple calls, would it mak sense to always just use the one shot call for all cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this would simplify things. I can change it as I hope no algorithm depends on having update.


/* PKCS #11 parameters are mandatory for Ed448 key type anyway */
if (size == ED448_BIT_SIZE) {
sigctx->use_eddsa_params = CK_TRUE;
Copy link
Member

Choose a reason for hiding this comment

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

I do not think this is correct, rather we should select an instance, presumably based on the default and bit lengths and then call through the same code at lines 2322 onward.

What are you doing here is effectively setting

    sigctx->use_eddsa_params = CK_TRUE;
    sigctx->eddsa_params.phFlag = CK_FALSE;

by omitting to set the phFlag.

I think the correct fix here involves spinning off the instance selection to a helper function, and if no params are set, calling it with "Ed25519" is size == ED25519_BIT_SIZE and with "Ed448" if ED448_BIT_SIZE

An option is to also pre-set instance constants, then if there are params they will overwrite the instance variable with whatever came from the OSSL_SIGNATURE_PARAM_INSTANCE

Once all params are handled, unconditionally call the cose that sets the various flags based on instance name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, check the table in the pkcs#11 specs:
https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/os/pkcs11-curr-v3.0-os.html#_Toc30061191

What are you doing here is effectively setting

this is intentional. The phFlag is false in this default default. I can set it explicitly if it will make it more clear

Missing instance name on the openssl level results in Ed448/Ed25519. I find it overcomplicated going through the openssl instance if we do not have any from the caller. The code from lines 2322 just really sets use_eddsa_params and phFlag. Moreover we return on the line 2311 when there are no parameters provided.

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