After looking at the HRR issue a little bit deeper, I think I'm running into an issue that was fixed by this commit ( 166c0b98fd6e8b1bb341397642527a9396468f6c):
Don't generate an unnecessary Diffie-Hellman key in TLS 1.3 clients. tls_parse_stoc_key_share was generating a new EVP_PKEY public/private keypair and then overrides it with the server public key, so the generation was a waste anyway. Instead, it should create a parameters-only EVP_PKEY. (This is a consequence of OpenSSL using the same type for empty key, empty key with key type, empty key with key type + parameters, public key, and private key. As a result, it's easy to mistakenly mix such things up, as happened here.) Reviewed-by: Matt Caswell <m...@openssl.org> Reviewed-by: Kurt Roeckx <k...@roeckx.be> (Merged from #9445) Because the TLS 1.3 client was generating a key in order to set the parameters prior to setting the public key, a stale private key was left over that didn't match the public key that was retrieved from the server. Applying this change to the OpenSSL 1.1.1 codebase fixed the ec_key_simple_check_key:invalid private key issue. I'm still a bit baffled by the issue in test_evp. Patrick On Tue, Dec 29, 2020 at 2:20 PM Patrick Jakubowski <patr...@qumulo.com> wrote: > Hi all, > > I've been tasked with making some modifications to OpenSSL 1.1.1 in order > to bring it into compliance with FIPS 140-2. One of the items on the to-do > list was to implement the required key agreement scheme assurances > specified in NIST SP.800-56Ar3 Section 9. This involves performing some > validation on the public key provided via the EVP_PKEY_derive() call. > > To that end, I backported this patch which purports to implement the > required validation in EC_KEY_check_key(): > > commit 5173cdde7d758824e6a07f2a6c6808b254602e11 > Author: Shane Lontis <shane.lon...@oracle.com> > Date: Sat Mar 23 13:12:08 2019 +1000 > > ec key validation checks updated > > Reviewed-by: Nicola Tuveri <nic....@gmail.com> > Reviewed-by: Matt Caswell <m...@openssl.org> > (Merged from https://github.com/openssl/openssl/pull/8564) > > I then added a call to EC_KEY_check_key in pkey_ec_derive() to validate > the public key, like so: > > diff --git a/crypto/ec/ec_pmeth.c b/crypto/ec/ec_pmeth.c > index 5bee031b92..84d8eb5d95 100644 > --- a/crypto/ec/ec_pmeth.c > +++ b/crypto/ec/ec_pmeth.c > @@ -163,6 +163,14 @@ static int pkey_ec_derive(EVP_PKEY_CTX *ctx, unsigned > char *key, size_t *keylen) > > eckey = dctx->co_key ? dctx->co_key : ctx->pkey->pkey.ec; > > + /* > + * Check the validity of the received public key as required by NIST > + * SP.800-56Ar3 Section 9 > + */ > + ret = EC_KEY_check_key(ctx->peerkey->pkey.ec); > + if (ret <= 0) > + return ret; > + > if (!key) { > const EC_GROUP *group; > group = EC_KEY_get0_group(eckey); > > Adding this check causes several unexpected unit test failures, which I > was hoping someone could help me with. The first category of failure seems > to be with TLS 1.3 tests that exercise the HelloRetryRequest (HRR) > functionality. Unfortunately, I'm not terribly familiar with the TLS > protocol, so my understanding here is limited. It seems like the unit tests > induce this HRR condition by calling SSL_set1_groups_list("P-256") while > providing an RSA private key. I'm not exactly sure of the effect of this > change, but here is an example test failure: > > ok 22 - test_early_data_skip > # Subtest: test_early_data_skip_hrr > 1..3 > # ERROR: (bool) 'SSL_write_ex(clientssl, MSG2, strlen(MSG2), &written) > == true' failed @ test/sslapitest.c:2620 > # false > # 139755660280960:error:1010207B:elliptic curve > routines:ec_key_simple_check_key:invalid private key:crypto/ec/ec_key.c:406: > # 139755660280960:error:1424E044:SSL routines:ssl_derive:internal > error:ssl/s3_lib.c:4781: > not ok 1 - iteration 1 > > There are a couple of other tests that use SSL_set1_groups_list to do a > similar thing and fail in a similar way. > > Additionally, there is another test in test_evp whose failure I don't > quite understand. The test involves calling EVP_PKEY_derive() with the > ALICE_zero_secp112r2 and BOB_zero_secp112r2_PUB keys from > test/recipes/30-test_evp_data/evppkey_ecc.txt. It appears to have been > added by commit 5d92b853f6b875ba8d1a1b51b305f14df5adb8aa as a regression > test for a change to the GFp ladder algorithm. > > The test failure looks like this: > > # Starting "zero x-coord regression tests" tests at line 4536 > # INFO: @ test/evp_test.c:2320 > # ../../test/recipes/30-test_evp_data/evppkey_ecc.txt:4670: Source of > above error; unexpected error DERIVE_ERROR > # 140441081306112:error:10102082:elliptic curve > routines:ec_key_simple_check_key:wrong order:crypto/ec/ec_key.c:382: > > My question is: is there something invalid about adding this call to > EC_KEY_check_key() to pkey_ec_derive() or are these failures benign and > indications that the tests need to be changed? I'm particularly concerned > about the TLS 1.3 HRR tests as I want to make sure I haven't somehow broken > the TLS protocol. > > FWIW, I see a similar check to the one I added in the DH shared secret > derivation codepath. > > Thank you for any insight you can bring to bear! > > > -- > *Patrick Jakubowski* > > *Member of Technical Staff* > *___________________________________* > *Qumulo, Inc.* > *World's First Data-Aware Scale-Out NAS* > > Twitter <https://twitter.com/qumulo?lang=en> /// LinkedIn > <https://www.linkedin.com/company/2533593?trk=tyah&trkInfo=idx%3A3-1-6%2CtarId%3A1426264780078%2Ctas%3Aqumul> > /// Facebook <https://www.facebook.com/Qumulo> /// YouTube > <https://www.youtube.com/channel/UCe6kSaQmqlCzpNd-tWIP9pQ/feed> > -- *Patrick Jakubowski* *Member of Technical Staff* *___________________________________* *Qumulo, Inc.* *World's First Data-Aware Scale-Out NAS* Twitter <https://twitter.com/qumulo?lang=en> /// LinkedIn <https://www.linkedin.com/company/2533593?trk=tyah&trkInfo=idx%3A3-1-6%2CtarId%3A1426264780078%2Ctas%3Aqumul> /// Facebook <https://www.facebook.com/Qumulo> /// YouTube <https://www.youtube.com/channel/UCe6kSaQmqlCzpNd-tWIP9pQ/feed>
