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

Implement PCT for EDDSA #23408

Closed
wants to merge 8 commits into from
Closed

Conversation

0140454
Copy link
Contributor

@0140454 0140454 commented Jan 28, 2024

According to FIPS 140-3 IG 10.3.A Additonal Comment 1, a PCT shall be performed consistent with the intended use of the keys.

This commit implements PCT for EDDSA via performing sign and verify operations after key generated.

Checklist
  • documentation is added or updated
  • tests are added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 28, 2024
@t8m
Copy link
Member

t8m commented Jan 29, 2024

There is a PR #22112 that implements the POST for EdDSA.

Also we would need a signed CLA to be able to accept this contribution.

@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Jan 29, 2024
@t8m t8m requested a review from slontis January 29, 2024 15:29
@0140454
Copy link
Contributor Author

0140454 commented Jan 29, 2024

Yes, I notice there is a PR for EDDSA self-test. Therefore, I just implement PCT here.

I will send a signed CLA as soon as possible.

@0140454
Copy link
Contributor Author

0140454 commented Jan 30, 2024

A signed ICAL have been sent to legal@opensslfoundation.org with title ICLA for PR 23408.

By the way, I also notice there is a PR to add EDDSA support to EVP_PKEY_sign/verify.
However, since there is no EVP_PKEY instance can be used in EVP_PKEY_sign/verify when generating key, I use EVP_SIGNATURE to perform signing and verifying in this PR.

OSSL_SELF_TEST_onbegin(st, OSSL_SELF_TEST_TYPE_PCT,
OSSL_SELF_TEST_DESC_PCT_EDDSA);

alg = EVP_SIGNATURE_fetch(ecx->libctx, instance, NULL);
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 look at other places that do this, they all avoid the overhead of doing the fetch by calling directly into low level API's. Its bad enough that we need to do this in the first place :). So crypto/ecx.h would be better to use for this.
Could you also add a comment block to the top of the function that references the relevant section in the FIPS 140-3 IG that forces us to do this..

Copy link
Member

Choose a reason for hiding this comment

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

Does it need to do sign + verify or is just doing a KAT sufficient for this purpose?

Copy link
Contributor Author

@0140454 0140454 Jan 30, 2024

Choose a reason for hiding this comment

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

If you look at other places that do this, they all avoid the overhead of doing the fetch by calling directly into low level API's. Its bad enough that we need to do this in the first place :). So crypto/ecx.h would be better to use for this.

Yes, I noticed that RSA and ECDSA use low-level API like ECDSA_do_sign to do this, but didn't aware there are ossl_ed25519_sign and other APIs for EDDSA.

I will try to change the implementation using them. Thank you.

Could you also add a comment block to the top of the function that references the relevant section in the FIPS 140-3 IG that forces us to do this..

Of course, I will add it in next commit.

Does it need to do sign + verify or is just doing a KAT sufficient for this purpose?

The addition comment in IG says

Though not a CAST, a pairwise consistency test (PCT) shall be conducted for every generated
public and private key pair for the applicable approved algorithm ... shall be performed.

If at the time a PCT on a key pair is performed it is known whether the keys will be used in a key
agreement scheme, digital signature algorithm or to perform a key transport, then the PCT shall be
performed consistent with the intended use of the keys ..., even if the underlying standard does not
require a PCT.

... The PCT shall be performed either when keys are generated/imported, prior to the first
exportation, or prior to the first operational use (if not exported before the first use).

In my opinion, I think it is needed to do sign + verify for every generated key, just like DSA, ECDSA and RSA.

If there is a worry about it, I may ask our lab to confirm it.

Copy link
Member

Choose a reason for hiding this comment

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

As we cant use X25519/X448 in FIPS 140-3 currently, then we know that the keygen result can only be used for signature purposes.. Until that changes we can only do the sign test here :)

Copy link
Member

Choose a reason for hiding this comment

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

The question is whether this has to be part of the key generation algorithm or if it would be sufficient to make mandatory that an application uses EVP_PKEY_pairwise_check() before the first use of the keypair. However please note that EVP_PKEY_pairwise_check() does not do a PCT for EdDSA keys - it just regenerates the public key from the private key and compares the result with the original public key. So we would have to change the pairwise check anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question is whether this has to be part of the key generation algorithm or if it would be sufficient to make mandatory that an application uses EVP_PKEY_pairwise_check() before the first use of the keypair.

After confirming with our lab, the lab states that PCT should be performed automatically as soon as the key is generated. Therefore, it should be a part of the key generation.

So we would have to change the pairwise check anyway.

Please let me confirm it again, so I should replace callings of ossl_ed*_public_from_private with ecd_keygen_pairwise_test (maybe rename to a more common name) in ecx_key_pairwise_check for types ECX_KEY_TYPE_ED25519 and ECX_KEY_TYPE_ED448. Is it right?

Copy link
Member

Choose a reason for hiding this comment

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

Please let me confirm it again, so I should replace callings of ossl_ed*_public_from_private with ecd_keygen_pairwise_test (maybe rename to a more common name) in ecx_key_pairwise_check for types ECX_KEY_TYPE_ED25519 and ECX_KEY_TYPE_ED448. Is it right?

I would do that at least within the FIPS_MODULE ifdefs. @slontis What is your opinion? IMO the current pairwise check for EdDSA keys is not FIPS compliant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The commit pushed.

I make it run sign + verify operations in FIPS provider and original operation in default provider.

@t8m t8m added approval: review pending This pull request needs review by a committer tests: exempted The PR is exempt from requirements for testing labels Jan 31, 2024
@t8m
Copy link
Member

t8m commented Jan 31, 2024

CI is relevant

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jan 31, 2024
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Please add to doc/man7/OSSL_PROVIDER-FIPS.pod also

@@ -756,6 +867,23 @@ static int ecx_key_pairwise_check(const ECX_KEY *ecx, int type)
case ECX_KEY_TYPE_X448:
ossl_x448_public_from_private(pub, ecx->privkey);
break;
default:
Copy link
Member

Choose a reason for hiding this comment

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

Is the validate required to use the same test? (Not sure they need to be the same,
especially since at some point the X25519/X448 might be allowed, so the PCT in the keygen could be either in that case sign or encrypt.) @t8m do you have any opinion on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since X25519/X448 currently is not approved but allowed algorithm, I think it may be not required to use the same test.

Copy link
Member

Choose a reason for hiding this comment

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

IMO the validate() call should do the same as required for FIPS at least in the FIPS_MODULE case. So it covers the - PCT validation of the keypair on import case. I do not see any problem with doing the existing thing for the X* keys and doing this PCT for ED* keys in validate().

@t8m t8m added the approval: otc review pending This pull request needs review by an OTC member label Feb 2, 2024
@0140454
Copy link
Contributor Author

0140454 commented Feb 22, 2024

  • Change author information
  • Push a commit to add EDDSA test to test/recipes/30-test_pairwise_fail.t

@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Feb 23, 2024
@0140454 0140454 requested review from t8m and slontis February 23, 2024 17:20
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Just one minor nit

test/recipes/30-test_pairwise_fail.t Outdated Show resolved Hide resolved
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

OK if CI passes

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Feb 25, 2024
@0140454
Copy link
Contributor Author

0140454 commented Feb 25, 2024

Hi @t8m ,

The address_ub_sanitizer job failed unexpectedly and I cannot reproduce it on my machine.

My steps:

  1. Execute git submodule update --init --depth 1 fuzz/corpora
  2. Start ubuntu:22.04 container and execute the following commands inside.
  3. ./config --banner=Configured --debug enable-asan enable-ubsan enable-rc5 enable-md2 enable-ec_nistp_64_gcc_128 enable-fips
  4. make -s -j$(nproc)
  5. make test OPENSSL_TEST_RAND_ORDER=1708857275 TESTS="test_sslapi", where 1708857275 is got from CI logs.

Is it possible to get more information about failure?

@t8m
Copy link
Member

t8m commented Feb 25, 2024

The CI failure is unrelated

@0140454
Copy link
Contributor Author

0140454 commented Feb 26, 2024

Rebased dev branch onto current master branch.

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 26, 2024
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Feb 27, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Mar 1, 2024

Squashed and merged to the master branch. Thank you for your contribution.

@t8m t8m closed this Mar 1, 2024
openssl-machine pushed a commit that referenced this pull request Mar 1, 2024
According to FIPS 140-3 IG 10.3.A Additonal Comment 1, a PCT shall be
performed consistent with the intended use of the keys.

This commit implements PCT for EDDSA via performing sign and verify
operations after key generated.

Also use the same pairwise test logic in EVP_PKEY_keygen and
EVP_PKEY_pairwise_check for EDDSA in FIPS_MODULE.

Add OSSL_SELF_TEST_DESC_PCT_EDDSA to OSSL_PROVIDER-FIPS page.

Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23408)
@0140454 0140454 deleted the feat-eddsa-pct branch March 1, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants