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 <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/25548)

diff --git a/providers/implementations/exchange/ecdh_exch.c b/providers/implementations/exchange/ecdh_exch.c
index ee56c1c26c6e1ed4e85e9339636ec8dbb7ee1147..760ebc51908cc0ad7164a85f4faf989de2f43fa9 100644 (file)
--- 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 1625cedc114daf6ad74acf94d60e13841614ddfc..2cb1ae8d023164de785cf7085e3f3e107d59e7c4 100644 (file)
--- 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 7c98d9805aa46b75d35912658bc89fef211668c0..67787f3740bb311e46dfd752714264088e21c377 100644 (file)
--- 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