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@xxxxxxxxxxx>
Reviewed-by: Kurt Roeckx <kurt@xxxxxxxxx>
(Merged from #9445)
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@xxxxxxxxxxx>
Reviewed-by: Kurt Roeckx <kurt@xxxxxxxxx>
(Merged from #9445)
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@xxxxxxxxxx> 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@xxxxxxxxxx>
Date: Sat Mar 23 13:12:08 2019 +1000
ec key validation checks updated
Reviewed-by: Nicola Tuveri <nic.tuv@xxxxxxxxx>
Reviewed-by: Matt Caswell <matt@xxxxxxxxxxx>
(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 1There 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!
--
--
