Failing unit tests after adding public key check to pkey_ec_derive()

Thu Dec 31 22:12:25 UTC 2020

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 <matt at openssl.org>
Reviewed-by: Kurt Roeckx <kurt at 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 <patrick at 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.lontis at oracle.com>
> Date:   Sat Mar 23 13:12:08 2019 +1000
>
>     ec key validation checks updated
>
>     Reviewed-by: Nicola Tuveri <nic.tuv at gmail.com>
>     Reviewed-by: Matt Caswell <matt at 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>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mta.openssl.org/pipermail/openssl-users/attachments/20201231/6d94852a/attachment.html>