From 2f362e99a1178263c7102474f0190836166f416d Mon Sep 17 00:00:00 2001 From: slontis Date: Thu, 26 Sep 2024 15:18:59 +1000 Subject: [PATCH] Fix bugs in ECDH cofactor FIPS indicator. The code was not detecting that the cofactor was set up correctly if OSSL_PKEY_PARAM_USE_COFACTOR_ECDH was set, resulting in an incorrect FIPS indicator error being triggered. Added a test for all possible combinations of a EVP_PKEY setting OSSL_PKEY_PARAM_USE_COFACTOR_ECDH and the derive context setting OSSL_EXCHANGE_PARAM_EC_ECDH_COFACTOR_MODE. This only affects the B & K curves (which have a cofactor that is not 1). Bug reported by @abkarcher Testing this properly, also detected a memory leak of privk when the FIPS indicator error was triggered (in the case where mode = 0 and use_cofactor was 1). Reviewed-by: Paul Dale Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/25548) --- .../implementations/exchange/ecdh_exch.c | 5 +- test/acvp_test.c | 59 +++++++++++++++++++ test/acvp_test.inc | 36 +++++++++++ 3 files changed, 99 insertions(+), 1 deletion(-) diff --git a/providers/implementations/exchange/ecdh_exch.c b/providers/implementations/exchange/ecdh_exch.c index ee56c1c26c..760ebc5190 100644 --- a/providers/implementations/exchange/ecdh_exch.c +++ b/providers/implementations/exchange/ecdh_exch.c @@ -539,6 +539,9 @@ int ecdh_plain_derive(void *vpecdhctx, unsigned char *secret, } } else { privk = pecdhctx->k; +#ifdef FIPS_MODULE + cofactor_approved = key_cofactor_mode; +#endif } #ifdef FIPS_MODULE @@ -551,7 +554,7 @@ int ecdh_plain_derive(void *vpecdhctx, unsigned char *secret, pecdhctx->libctx, "ECDH", "Cofactor", ossl_fips_config_ecdh_cofactor_check)) { ERR_raise(ERR_LIB_PROV, PROV_R_COFACTOR_REQUIRED); - return 0; + goto end; } } #endif diff --git a/test/acvp_test.c b/test/acvp_test.c index 1625cedc11..2cb1ae8d02 100644 --- a/test/acvp_test.c +++ b/test/acvp_test.c @@ -343,6 +343,63 @@ err: return ret; } + +static int ecdh_cofactor_derive_test(int tstid) +{ + int ret = 0; + const struct ecdh_cofactor_derive_st *t = &ecdh_cofactor_derive_data[tstid]; + unsigned char secret1[16]; + size_t secret1_len = sizeof(secret1); + const char *curve = "K-283"; /* A curve that has a cofactor that it not 1 */ + EVP_PKEY *peer1 = NULL, *peer2 = NULL; + EVP_PKEY_CTX *p1ctx = NULL; + OSSL_PARAM params[2], *prms = NULL; + int use_cofactordh = t->key_cofactor; + int cofactor_mode = t->derive_cofactor_mode; + + if (!TEST_ptr(peer1 = EVP_PKEY_Q_keygen(libctx, NULL, "EC", curve)) + || !TEST_ptr(peer2 = EVP_PKEY_Q_keygen(libctx, NULL, "EC", curve))) + goto err; + + params[1] = OSSL_PARAM_construct_end(); + + prms = NULL; + if (t->key_cofactor != COFACTOR_NOT_SET) { + params[0] = OSSL_PARAM_construct_int(OSSL_PKEY_PARAM_USE_COFACTOR_ECDH, + &use_cofactordh); + prms = params; + } + if (!TEST_int_eq(EVP_PKEY_set_params(peer1, prms), 1) + || !TEST_ptr(p1ctx = EVP_PKEY_CTX_new_from_pkey(libctx, peer1, NULL))) + goto err; + + prms = NULL; + if (t->derive_cofactor_mode != COFACTOR_NOT_SET) { + params[0] = OSSL_PARAM_construct_int(OSSL_EXCHANGE_PARAM_EC_ECDH_COFACTOR_MODE, + &cofactor_mode); + prms = params; + } + if (!TEST_int_eq(EVP_PKEY_derive_init_ex(p1ctx, prms), 1) + || !TEST_int_eq(EVP_PKEY_derive_set_peer(p1ctx, peer2), 1) + || !TEST_int_eq(EVP_PKEY_derive(p1ctx, secret1, &secret1_len), + t->expected)) + goto err; + + ret = 1; +err: + if (ret == 0) { + static const char *state[] = { "unset", "-1", "disabled", "enabled" }; + + TEST_note("ECDH derive() was expected to %s if key cofactor is" + "%s and derive mode is %s", t->expected ? "Pass" : "Fail", + state[2 + t->key_cofactor], state[2 + t->derive_cofactor_mode]); + } + EVP_PKEY_free(peer1); + EVP_PKEY_free(peer2); + EVP_PKEY_CTX_free(p1ctx); + return ret; +} + #endif /* OPENSSL_NO_EC */ #if !defined(OPENSSL_NO_DSA) || !defined(OPENSSL_NO_ECX) @@ -1688,6 +1745,8 @@ int setup_tests(void) ADD_ALL_TESTS(ecdsa_pub_verify_test, OSSL_NELEM(ecdsa_pv_data)); ADD_ALL_TESTS(ecdsa_siggen_test, OSSL_NELEM(ecdsa_siggen_data)); ADD_ALL_TESTS(ecdsa_sigver_test, OSSL_NELEM(ecdsa_sigver_data)); + ADD_ALL_TESTS(ecdh_cofactor_derive_test, + OSSL_NELEM(ecdh_cofactor_derive_data)); #endif /* OPENSSL_NO_EC */ #ifndef OPENSSL_NO_ECX diff --git a/test/acvp_test.inc b/test/acvp_test.inc index 7c98d9805a..67787f3740 100644 --- a/test/acvp_test.inc +++ b/test/acvp_test.inc @@ -45,6 +45,12 @@ struct ecdsa_sigver_st { int pass; }; +struct ecdh_cofactor_derive_st { + int derive_cofactor_mode; + int key_cofactor; + int expected; +}; + static const struct ecdsa_keygen_st ecdsa_keygen_data[] = { { "P-224" }, }; @@ -231,6 +237,36 @@ static const struct ecdsa_sigver_st ecdsa_sigver_data[] = { }, }; +/* + * FIPS EC DH key derivation requires the use of the cofactor if a curve has a + * cofactor that is not 1. The cofactor option is determined by either + * (1) The derive ctx using OSSL_EXCHANGE_PARAM_EC_ECDH_COFACTOR_MODE or via + * (2) The EVP_PKEY (used by the derive) using OSSL_PKEY_PARAM_USE_COFACTOR_ECDH + * Test all combinations of these. + * Notes: + * COFACTOR_MODE is -1 by default. (It can be -1, 0, or 1). + * OSSL_PKEY_PARAM_USE_COFACTOR_ECDH is 0 by default. (It can be 0 or 1) + * + * OSSL_PKEY_PARAM_USE_COFACTOR_ECDH is only used if the COFACTOR_MODE is -1. + * + * If the cofactor is not set by either then the derived is expected to fail. + */ +# define COFACTOR_NOT_SET -2 /* Use the default by not setting the param */ +static const struct ecdh_cofactor_derive_st ecdh_cofactor_derive_data[] = { + { COFACTOR_NOT_SET, COFACTOR_NOT_SET, 0 }, + { COFACTOR_NOT_SET, 0, 0 }, + { COFACTOR_NOT_SET, 1, 1 }, + { -1, COFACTOR_NOT_SET, 0 }, + { -1, 0, 0 }, + { -1, 1, 1 }, + { 0, COFACTOR_NOT_SET, 0 }, + { 0, 0, 0 }, + { 0, 1, 0 }, + { 1, COFACTOR_NOT_SET, 1 }, + { 1, 0, 1 }, + { 1, 1, 1 } +}; + #endif /* OPENSSL_NO_EC */ #ifndef OPENSSL_NO_ECX