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
Reviewed-by: Kurt Roeckx
(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
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
> Date: Sat Mar 23 13:12:08 2019 +1000
>
> ec key validation checks updated
>
> Reviewed-by: Nicola Tuveri
> Reviewed-by: Matt Caswell
> (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 have
