Add EDDSA FIPS self tests. #22112

slontis · 2023-09-15T06:16:26Z

See FIPS 140-3 IG Section 10.3.A Part 11
Indicates ECDSA requires a sign and verify test.
Note 11 states that HashEdDSA is not required to be tested if PureEdDSA is tested. Note 12 indicates that both ED25519 and X448 need to be tested.

Since ED uses the oneshot interface, additional API's needed to be exposed to the FIPS provider using #ifdef FIPS_MODULE.

Changed ED25518 and ED448 to use fips=true in the FIPS provider. Updated documentation for provider lists for EDDSA.

Checklist

documentation is added or updated
tests are added or updated

slontis · 2023-09-15T06:18:53Z

crypto/evp/digest.c

@@ -86,7 +85,6 @@ static int evp_md_ctx_reset_ex(EVP_MD_CTX *ctx, int keep_fetched)
        EVP_PKEY_CTX_free(ctx->pctx);


Without this change we get a memory leak.. since the digestsign interface uses ctx->pctx

Line 565 of self_test_kats.c..

I think you meant that last comment to go against the other thread about the init field in st_kat_sign_st... :-)

providers/fips/self_test_data.inc

slontis · 2023-09-15T08:26:54Z

As this is FIPS related it is probably not required for OpenSSL 3.2 (unless 3.2 is going to become FIPS validated).

paulidale

We could consider this for 3.1 if we end up validating a version other than 3.1.2.

paulidale · 2023-09-15T09:17:23Z

OMC: should we including this in the 3.1.x validation?

paulidale · 2023-09-27T06:31:47Z

IMO: this is a working group discussion.
I'm in favour of 3.1 getting this.
I'm unsure about 3.0.

mattcaswell · 2023-10-25T08:23:31Z

There is a conflict in this PR.

Also, there is a hold on this PR, but the question seems to be about 3.1. Is this going into master, and should it be there for the beta?

t8m · 2023-10-25T08:48:09Z

I think we could give an post-beta exception if it would be approved to go in 3.1 by OMC.

Otherwise it can as well wait for 3.3.

t-j-h · 2023-10-25T09:29:52Z

This is a feature addition - this is not in the fips provider and it is being added.
It is not a simple addition of a "test".

As such it has to wait for 3.3.

paulidale · 2023-10-25T10:10:48Z

I think this is a question for WG.

The 140-3 submission is long delayed and we've still got a possibility of including EdDSA.
We know that EdDSA works, so it's really just changing "no" to "yes" in a few places and adding this test.

The 140-3 submission requires other changes in addition.

t8m · 2023-11-15T09:34:33Z

OMC: This can go into master branch. Not in 3.1.

t8m · 2023-11-16T17:40:29Z

@slontis please rebase

slontis · 2023-11-17T09:25:27Z

rebased

t8m · 2023-11-20T09:12:19Z

CI is relevant

t8m · 2023-11-20T09:14:42Z

What about, instead of using the DigestSign API for the self test, adding the EDDSA support to the EVP_PKEY_sign()/verify()?

See FIPS 140-3 IG Section 10.3.A Part 11 Indicates ECDSA requires a sign and verify test. Note 11 states that HashEdDSA is not required to be tested if PureEdDSA is tested. Note 12 indicates that both ED25519 and X448 need to be tested. Since ED uses the oneshot interface, additional API's needed to be exposed to the FIPS provider using #ifdef FIPS_MODULE. Changed ED25518 and ED448 to use fips=true in the FIPS provider. Updated documentation for provider lists for EDDSA.

slontis · 2024-02-26T04:59:15Z

rebased again

0140454 · 2024-02-26T05:12:38Z

providers/fips/self_test_data.inc

+        "ED448",
+        NULL,
+        ed448_key,
+        NULL, 0, NULL, 0, NULL, 0,


Hi,

When I pick and test this commit, I found it should be as the following, or it will failed on self-tests.

ITM(sig_kat_entropyin), ITM(sig_kat_nonce), ITM(sig_kat_persstr),

Therefore, I would like to know whether it is a typo here, or do I misunderstand something?

Thank you.

I will have to reorder the code since this doesnt print anything when it fails here.

EDDSA works without the entropy.

I found that it failed in set_kat_drbg.

Since EDDSA works without the entropy, we may need to skip the KAT setup for it?

yes - I need to shift this inside the function so that it runs the self test callback with an error set..

slontis · 2024-02-26T05:54:13Z

It looks like the tests fail here - so I will fix it..

slontis · 2024-02-26T07:36:46Z

Had to do a slight change since a failure in the entropy setup was not being reported as a failure by the self test callback (since it was outside the onstart()/onend() callbacks.

openssl-machine · 2024-03-28T00:09:09Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

openssl-machine · 2024-04-28T00:09:20Z

This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago

mattcaswell · 2024-05-01T15:47:13Z

providers/fips/self_test_kats.c

+    paramsinit = OSSL_PARAM_BLD_to_param(bldinit);
+
+    fromctx = EVP_PKEY_CTX_new_from_name(libctx, t->algorithm, "");
+    if (fromctx == NULL\


Why the trailing \ here?

mattcaswell · 2024-05-01T15:47:42Z

providers/fips/self_test_kats.c

+    mctx = EVP_MD_CTX_new();
+    if (mctx == NULL
+        || EVP_DigestSignInit_ex(mctx, NULL, NULL, libctx, NULL,
+                                 pkey, paramsinit)<= 0)


nit: missing space before <=

mattcaswell · 2024-05-01T15:48:43Z

providers/fips/self_test_kats.c

@@ -697,13 +786,16 @@ static int self_test_signatures(OSSL_SELF_TEST *st, OSSL_LIB_CTX *libctx)

    for (i = 0; ret && i < (int)OSSL_NELEM(st_kat_sign_tests); ++i) {
        t = st_kat_sign_tests + i;
-        if (!set_kat_drbg(libctx, t->entropy, t->entropy_len,
-                          t->nonce, t->nonce_len, t->persstr, t->persstr_len))
-            return 0;


Why is this no longer needed?

openssl-machine · 2024-06-01T00:08:53Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

slontis added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Sep 15, 2023

slontis commented Sep 15, 2023

View reviewed changes

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 15, 2023

mattcaswell reviewed Sep 15, 2023

View reviewed changes

providers/fips/self_test_data.inc Show resolved Hide resolved

mattcaswell approved these changes Sep 15, 2023

View reviewed changes

mattcaswell removed the approval: otc review pending This pull request needs review by an OTC member label Sep 15, 2023

paulidale approved these changes Sep 15, 2023

View reviewed changes

paulidale added the hold: need omc decision The OMC needs to make a decision label Sep 15, 2023

t8m added the triaged: feature The issue/pr requests/adds a feature label Sep 15, 2023

t8m added hold: need rebase The pull request needs to be rebased and removed hold: need omc decision The OMC needs to make a decision labels Nov 15, 2023

slontis force-pushed the fips_eddsa_cast branch from 8017a4e to 6510ad3 Compare November 17, 2023 09:25

t8m removed the approval: ready to merge The 24 hour grace period has passed, ready to merge label Nov 20, 2023

t8m removed the hold: need rebase The pull request needs to be rebased label Nov 20, 2023

jvdsn mentioned this pull request Jan 9, 2024

Add EdDSA support to EVP_PKEY_sign/verify #23240

Open

2 tasks

slontis mentioned this pull request Jan 17, 2024

EdDSA algorithm has been added (deterministic and prehash version) openssl/project#337

Open

t8m mentioned this pull request Jan 29, 2024

Implement PCT for EDDSA #23408

Closed

2 tasks

slontis force-pushed the fips_eddsa_cast branch from 6510ad3 to be68df3 Compare February 26, 2024 04:57

slontis added the approval: done This pull request has the required number of approvals label Feb 26, 2024

0140454 reviewed Feb 26, 2024

View reviewed changes

slontis added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member and removed approval: done This pull request has the required number of approvals labels Feb 26, 2024

fixup! Add EDDSA FIPS self tests.

97f048b

slontis requested review from mattcaswell and paulidale February 26, 2024 07:34

t8m mentioned this pull request Apr 11, 2024

fips: Edwards curves are approved for signatures only #24093

Closed

2 tasks

mattcaswell requested changes May 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EDDSA FIPS self tests. #22112

Add EDDSA FIPS self tests. #22112

slontis commented Sep 15, 2023

slontis Sep 15, 2023

slontis Sep 15, 2023

mattcaswell Sep 15, 2023

slontis commented Sep 15, 2023

paulidale left a comment

paulidale commented Sep 15, 2023

paulidale commented Sep 27, 2023

mattcaswell commented Oct 25, 2023

t8m commented Oct 25, 2023

t-j-h commented Oct 25, 2023

paulidale commented Oct 25, 2023

t8m commented Nov 15, 2023

t8m commented Nov 16, 2023

slontis commented Nov 17, 2023

t8m commented Nov 20, 2023

t8m commented Nov 20, 2023

slontis commented Feb 26, 2024

0140454 Feb 26, 2024

slontis Feb 26, 2024

slontis Feb 26, 2024

0140454 Feb 26, 2024

slontis Feb 26, 2024

slontis commented Feb 26, 2024

slontis commented Feb 26, 2024

openssl-machine commented Mar 28, 2024

openssl-machine commented Apr 28, 2024

mattcaswell May 1, 2024

mattcaswell May 1, 2024

mattcaswell May 1, 2024

openssl-machine commented Jun 1, 2024

		@@ -86,7 +85,6 @@ static int evp_md_ctx_reset_ex(EVP_MD_CTX *ctx, int keep_fetched)
		EVP_PKEY_CTX_free(ctx->pctx);

Add EDDSA FIPS self tests. #22112

Are you sure you want to change the base?

Add EDDSA FIPS self tests. #22112

Conversation

slontis commented Sep 15, 2023

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slontis commented Sep 15, 2023

paulidale left a comment

Choose a reason for hiding this comment

paulidale commented Sep 15, 2023

paulidale commented Sep 27, 2023

mattcaswell commented Oct 25, 2023

t8m commented Oct 25, 2023

t-j-h commented Oct 25, 2023

paulidale commented Oct 25, 2023

t8m commented Nov 15, 2023

t8m commented Nov 16, 2023

slontis commented Nov 17, 2023

t8m commented Nov 20, 2023

t8m commented Nov 20, 2023

slontis commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slontis commented Feb 26, 2024

slontis commented Feb 26, 2024

openssl-machine commented Mar 28, 2024

openssl-machine commented Apr 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openssl-machine commented Jun 1, 2024