FFC cleanups

Discovered during coverage testing.

Remove unneccesary check when using ossl_dh_get0_params() and
ossl_dsa_get0_params(). These point to addresses and can not fail
for any existing calls.

Make dsa keygen tests only available in the FIPS module - as they are
not used in the default provider.

Change ossl_ffc_set_digest() to return void as it cannot fail.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20359)

diff --git a/crypto/dh/dh_backend.c b/crypto/dh/dh_backend.c
index 726843fd30cd64d19dcb503b70b911baef82e961..abc66a5b309116dff6c2fa9f7b8e7805b768a029 100644 (file)
--- a/crypto/dh/dh_backend.c
+++ b/crypto/dh/dh_backend.c
@@ -31,13 +31,7 @@
 static int dh_ffc_params_fromdata(DH *dh, const OSSL_PARAM params[])
 {
     int ret;
-    FFC_PARAMS *ffc;
-
-    if (dh == NULL)
-        return 0;
-    ffc = ossl_dh_get0_params(dh);
-    if (ffc == NULL)
-        return 0;
+    FFC_PARAMS *ffc = ossl_dh_get0_params(dh);
 
     ret = ossl_ffc_params_fromdata(ffc, params);
     if (ret)

diff --git a/crypto/dsa/dsa_key.c b/crypto/dsa/dsa_key.c
index e8c8359634157be68d4f9149506f63aadc902932..7fc762880bed256e9181987d26e35e274a49832b 100644 (file)
--- a/crypto/dsa/dsa_key.c
+++ b/crypto/dsa/dsa_key.c
@@ -28,8 +28,7 @@
 # define MIN_STRENGTH 80
 #endif
 
-static int dsa_keygen(DSA *dsa, int pairwise_test);
-static int dsa_keygen_pairwise_test(DSA *dsa, OSSL_CALLBACK *cb, void *cbarg);
+static int dsa_keygen(DSA *dsa);
 
 int DSA_generate_key(DSA *dsa)
 {
@@ -37,7 +36,7 @@ int DSA_generate_key(DSA *dsa)
     if (dsa->meth->dsa_keygen != NULL)
         return dsa->meth->dsa_keygen(dsa);
 #endif
-    return dsa_keygen(dsa, 0);
+    return dsa_keygen(dsa);
 }
 
 int ossl_dsa_generate_public_key(BN_CTX *ctx, const DSA *dsa,
@@ -59,6 +58,7 @@ err:
     return ret;
 }
 
+#ifdef FIPS_MODULE
 /*
  * Refer: FIPS 140-3 IG 10.3.A Additional Comment 1
  * Perform a KAT by duplicating the public key generation.
@@ -107,7 +107,44 @@ err:
     return ret;
 }
 
-static int dsa_keygen(DSA *dsa, int pairwise_test)
+/*
+ * FIPS 140-2 IG 9.9 AS09.33
+ * Perform a sign/verify operation.
+ */
+static int dsa_keygen_pairwise_test(DSA *dsa, OSSL_CALLBACK *cb, void *cbarg)
+{
+    int ret = 0;
+    unsigned char dgst[16] = {0};
+    unsigned int dgst_len = (unsigned int)sizeof(dgst);
+    DSA_SIG *sig = NULL;
+    OSSL_SELF_TEST *st = NULL;
+
+    st = OSSL_SELF_TEST_new(cb, cbarg);
+    if (st == NULL)
+        goto err;
+
+    OSSL_SELF_TEST_onbegin(st, OSSL_SELF_TEST_TYPE_PCT,
+                           OSSL_SELF_TEST_DESC_PCT_DSA);
+
+    sig = DSA_do_sign(dgst, (int)dgst_len, dsa);
+    if (sig == NULL)
+        goto err;
+
+    OSSL_SELF_TEST_oncorrupt_byte(st, dgst);
+
+    if (DSA_do_verify(dgst, dgst_len, sig, dsa) != 1)
+        goto err;
+
+    ret = 1;
+err:
+    OSSL_SELF_TEST_onend(st, ret);
+    OSSL_SELF_TEST_free(st);
+    DSA_SIG_free(sig);
+    return ret;
+}
+#endif /* FIPS_MODULE */
+
+static int dsa_keygen(DSA *dsa)
 {
     int ok = 0;
     BN_CTX *ctx = NULL;
@@ -151,12 +188,9 @@ static int dsa_keygen(DSA *dsa, int pairwise_test)
     dsa->priv_key = priv_key;
     dsa->pub_key = pub_key;
 
-#ifdef FIPS_MODULE
-    pairwise_test = 1;
-#endif /* FIPS_MODULE */
-
     ok = 1;
-    if (pairwise_test) {
+#ifdef FIPS_MODULE
+    {
         OSSL_CALLBACK *cb = NULL;
         void *cbarg = NULL;
 
@@ -173,6 +207,7 @@ static int dsa_keygen(DSA *dsa, int pairwise_test)
             return ok;
         }
     }
+#endif
     dsa->dirty_cnt++;
 
  err:
@@ -184,39 +219,3 @@ static int dsa_keygen(DSA *dsa, int pairwise_test)
 
     return ok;
 }
-
-/*
- * FIPS 140-2 IG 9.9 AS09.33
- * Perform a sign/verify operation.
- */
-static int dsa_keygen_pairwise_test(DSA *dsa, OSSL_CALLBACK *cb, void *cbarg)
-{
-    int ret = 0;
-    unsigned char dgst[16] = {0};
-    unsigned int dgst_len = (unsigned int)sizeof(dgst);
-    DSA_SIG *sig = NULL;
-    OSSL_SELF_TEST *st = NULL;
-
-    st = OSSL_SELF_TEST_new(cb, cbarg);
-    if (st == NULL)
-        goto err;
-
-    OSSL_SELF_TEST_onbegin(st, OSSL_SELF_TEST_TYPE_PCT,
-                           OSSL_SELF_TEST_DESC_PCT_DSA);
-
-    sig = DSA_do_sign(dgst, (int)dgst_len, dsa);
-    if (sig == NULL)
-        goto err;
-
-    OSSL_SELF_TEST_oncorrupt_byte(st, dgst);
-
-    if (DSA_do_verify(dgst, dgst_len, sig, dsa) != 1)
-        goto err;
-
-    ret = 1;
-err:
-    OSSL_SELF_TEST_onend(st, ret);
-    OSSL_SELF_TEST_free(st);
-    DSA_SIG_free(sig);
-    return ret;
-}

diff --git a/crypto/dsa/dsa_lib.c b/crypto/dsa/dsa_lib.c
index 333885a01a5013de23f0336df7352904168a4f93..bcfd467b1c1253967d3c28276b9dd9f9f6d81abc 100644 (file)
--- a/crypto/dsa/dsa_lib.c
+++ b/crypto/dsa/dsa_lib.c
@@ -345,13 +345,7 @@ FFC_PARAMS *ossl_dsa_get0_params(DSA *dsa)
 int ossl_dsa_ffc_params_fromdata(DSA *dsa, const OSSL_PARAM params[])
 {
     int ret;
-    FFC_PARAMS *ffc;
-
-    if (dsa == NULL)
-        return 0;
-    ffc = ossl_dsa_get0_params(dsa);
-    if (ffc == NULL)
-        return 0;
+    FFC_PARAMS *ffc = ossl_dsa_get0_params(dsa);
 
     ret = ossl_ffc_params_fromdata(ffc, params);
     if (ret)

diff --git a/crypto/ffc/ffc_backend.c b/crypto/ffc/ffc_backend.c
index 01982bd6afe1aa777f0e00b125c66c4e34e411a9..954efb27bbc4510fa6b5c03e966c5befe90a75e7 100644 (file)
--- a/crypto/ffc/ffc_backend.c
+++ b/crypto/ffc/ffc_backend.c
@@ -24,9 +24,6 @@ int ossl_ffc_params_fromdata(FFC_PARAMS *ffc, const OSSL_PARAM params[])
     BIGNUM *p = NULL, *q = NULL, *g = NULL, *j = NULL;
     int i;
 
-    if (ffc == NULL)
-        return 0;
-
     prm  = OSSL_PARAM_locate_const(params, OSSL_PKEY_PARAM_GROUP_NAME);
     if (prm != NULL) {
         /*
@@ -76,9 +73,8 @@ int ossl_ffc_params_fromdata(FFC_PARAMS *ffc, const OSSL_PARAM params[])
     }
     prm  = OSSL_PARAM_locate_const(params, OSSL_PKEY_PARAM_FFC_SEED);
     if (prm != NULL) {
-        if (prm->data_type != OSSL_PARAM_OCTET_STRING)
-            goto err;
-        if (!ossl_ffc_params_set_seed(ffc, prm->data, prm->data_size))
+        if (prm->data_type != OSSL_PARAM_OCTET_STRING
+            || !ossl_ffc_params_set_seed(ffc, prm->data, prm->data_size))
             goto err;
     }
     prm  = OSSL_PARAM_locate_const(params, OSSL_PKEY_PARAM_FFC_VALIDATE_PQ);
@@ -113,10 +109,8 @@ int ossl_ffc_params_fromdata(FFC_PARAMS *ffc, const OSSL_PARAM params[])
                 goto err;
             props = p1->data;
         }
-        if (!ossl_ffc_set_digest(ffc, prm->data, props))
-            goto err;
+        ossl_ffc_set_digest(ffc, prm->data, props);
     }
-
     ossl_ffc_params_set0_pqg(ffc, p, q, g);
     ossl_ffc_params_set0_j(ffc, j);
     return 1;

diff --git a/crypto/ffc/ffc_params.c b/crypto/ffc/ffc_params.c
index 647d356eca7fca33ca54861f9cf75c893e55c89d..54068cbd9efbbb68f2a59a377d154d10377fb00b 100644 (file)
--- a/crypto/ffc/ffc_params.c
+++ b/crypto/ffc/ffc_params.c
@@ -75,9 +75,6 @@ void ossl_ffc_params_set0_j(FFC_PARAMS *d, BIGNUM *j)
 int ossl_ffc_params_set_seed(FFC_PARAMS *params,
                              const unsigned char *seed, size_t seedlen)
 {
-    if (params == NULL)
-        return 0;
-
     if (params->seed != NULL) {
         if (params->seed == seed)
             return 1;
@@ -125,11 +122,10 @@ void ossl_ffc_params_enable_flags(FFC_PARAMS *params, unsigned int flags,
         params->flags &= ~flags;
 }
 
-int ossl_ffc_set_digest(FFC_PARAMS *params, const char *alg, const char *props)
+void ossl_ffc_set_digest(FFC_PARAMS *params, const char *alg, const char *props)
 {
     params->mdname = alg;
     params->mdprops = props;
-    return 1;
 }
 
 int ossl_ffc_params_set_validate_params(FFC_PARAMS *params,
@@ -214,9 +210,6 @@ int ossl_ffc_params_todata(const FFC_PARAMS *ffc, OSSL_PARAM_BLD *bld,
 {
     int test_flags;
 
-    if (ffc == NULL)
-        return 0;
-
     if (ffc->p != NULL
         && !ossl_param_build_set_bn(bld, params, OSSL_PKEY_PARAM_FFC_P, ffc->p))
         return 0;

diff --git a/include/internal/ffc.h b/include/internal/ffc.h
index d203ebc73e0c2dbe7a358b4702c6e918ca88b99c..3a6d9f67bb423f0233a046bd979540ac0e3f2562 100644 (file)
--- a/include/internal/ffc.h
+++ b/include/internal/ffc.h
@@ -132,7 +132,7 @@ void ossl_ffc_params_set_h(FFC_PARAMS *params, int index);
 void ossl_ffc_params_set_flags(FFC_PARAMS *params, unsigned int flags);
 void ossl_ffc_params_enable_flags(FFC_PARAMS *params, unsigned int flags,
                                   int enable);
-int ossl_ffc_set_digest(FFC_PARAMS *params, const char *alg, const char *props);
+void ossl_ffc_set_digest(FFC_PARAMS *params, const char *alg, const char *props);
 
 int ossl_ffc_params_set_validate_params(FFC_PARAMS *params,
                                         const unsigned char *seed,

diff --git a/providers/implementations/keymgmt/dh_kmgmt.c b/providers/implementations/keymgmt/dh_kmgmt.c
index 2ca12df44253f64024557a135b68f1d278eb2a60..1de182ce6f32d3dabce243d670477c6cddc3a0a5 100644 (file)
--- a/providers/implementations/keymgmt/dh_kmgmt.c
+++ b/providers/implementations/keymgmt/dh_kmgmt.c
@@ -735,10 +735,8 @@ static void *dh_gen(void *genctx, OSSL_CALLBACK *osslcb, void *cbarg)
         } else if (gctx->hindex != 0) {
             ossl_ffc_params_set_h(ffc, gctx->hindex);
         }
-        if (gctx->mdname != NULL) {
-            if (!ossl_ffc_set_digest(ffc, gctx->mdname, gctx->mdprops))
-                goto end;
-        }
+        if (gctx->mdname != NULL)
+            ossl_ffc_set_digest(ffc, gctx->mdname, gctx->mdprops);
         gctx->cb = osslcb;
         gctx->cbarg = cbarg;
         gencb = BN_GENCB_new();

diff --git a/providers/implementations/keymgmt/dsa_kmgmt.c b/providers/implementations/keymgmt/dsa_kmgmt.c
index 881680c0857e52ee411c370a9846c5662a9c22ce..24316028b5b5cd87fb822ca89a064a2499db9978 100644 (file)
--- a/providers/implementations/keymgmt/dsa_kmgmt.c
+++ b/providers/implementations/keymgmt/dsa_kmgmt.c
@@ -587,10 +587,9 @@ static void *dsa_gen(void *genctx, OSSL_CALLBACK *osslcb, void *cbarg)
     } else if (gctx->hindex != 0) {
         ossl_ffc_params_set_h(ffc, gctx->hindex);
     }
-    if (gctx->mdname != NULL) {
-        if (!ossl_ffc_set_digest(ffc, gctx->mdname, gctx->mdprops))
-            goto end;
-    }
+    if (gctx->mdname != NULL)
+        ossl_ffc_set_digest(ffc, gctx->mdname, gctx->mdprops);
+
     if ((gctx->selection & OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS) != 0) {
 
          if (ossl_dsa_generate_ffc_parameters(dsa, gctx->gen_type,

diff --git a/test/evp_extra_test2.c b/test/evp_extra_test2.c
index 74a86c14e612f6d56efda8bd0db6eb13db30e0fd..94ab698c592ac404218730618a7903688418525a 100644 (file)
--- a/test/evp_extra_test2.c
+++ b/test/evp_extra_test2.c
@@ -388,6 +388,61 @@ static int test_dh_paramgen(void)
     return ret;
 }
 
+static int set_fromdata_string(EVP_PKEY_CTX *ctx, const char *name, char *value)
+{
+    int ret;
+    OSSL_PARAM params[2];
+    EVP_PKEY *pkey = NULL;
+
+    if (EVP_PKEY_fromdata_init(ctx) != 1)
+        return -1;
+    params[0] = OSSL_PARAM_construct_utf8_string(name, value, 0);
+    params[1] = OSSL_PARAM_construct_end();
+    ret = EVP_PKEY_fromdata(ctx, &pkey, EVP_PKEY_KEY_PARAMETERS, params);
+    EVP_PKEY_free(pkey);
+    return ret;
+}
+
+static int set_fromdata_uint(EVP_PKEY_CTX *ctx, const char *name)
+{
+    int ret;
+    unsigned int tmp = 0;
+    OSSL_PARAM params[2];
+    EVP_PKEY *pkey = NULL;
+
+    if (EVP_PKEY_fromdata_init(ctx) != 1)
+        return -1;
+    params[0] = OSSL_PARAM_construct_uint(name, &tmp);
+    params[1] = OSSL_PARAM_construct_end();
+    ret = EVP_PKEY_fromdata(ctx, &pkey, EVP_PKEY_KEY_PARAMETERS, params);
+    EVP_PKEY_free(pkey);
+    return ret;
+}
+
+static int test_dh_paramfromdata(void)
+{
+    EVP_PKEY_CTX *ctx = NULL;
+    int ret = 0;
+
+    /* Test failure paths for FFC - mainly due to setting the wrong param type */
+    ret = TEST_ptr(ctx = EVP_PKEY_CTX_new_from_name(mainctx, "DH", NULL))
+          && TEST_int_eq(set_fromdata_uint(ctx, OSSL_PKEY_PARAM_GROUP_NAME), 0)
+          && TEST_int_eq(set_fromdata_string(ctx, OSSL_PKEY_PARAM_GROUP_NAME, "bad"), 0)
+          && TEST_int_eq(set_fromdata_string(ctx, OSSL_PKEY_PARAM_FFC_P, "bad"), 0)
+          && TEST_int_eq(set_fromdata_string(ctx, OSSL_PKEY_PARAM_FFC_GINDEX, "bad"), 0)
+          && TEST_int_eq(set_fromdata_string(ctx, OSSL_PKEY_PARAM_FFC_PCOUNTER, "bad"), 0)
+          && TEST_int_eq(set_fromdata_string(ctx, OSSL_PKEY_PARAM_FFC_COFACTOR, "bad"), 0)
+          && TEST_int_eq(set_fromdata_string(ctx, OSSL_PKEY_PARAM_FFC_H, "bad"), 0)
+          && TEST_int_eq(set_fromdata_uint(ctx, OSSL_PKEY_PARAM_FFC_SEED), 0)
+          && TEST_int_eq(set_fromdata_string(ctx, OSSL_PKEY_PARAM_FFC_VALIDATE_PQ, "bad"), 0)
+          && TEST_int_eq(set_fromdata_string(ctx, OSSL_PKEY_PARAM_FFC_VALIDATE_G, "bad"), 0)
+          && TEST_int_eq(set_fromdata_string(ctx, OSSL_PKEY_PARAM_FFC_VALIDATE_LEGACY, "bad"), 0)
+          && TEST_int_eq(set_fromdata_uint(ctx, OSSL_PKEY_PARAM_FFC_DIGEST), 0);
+
+    EVP_PKEY_CTX_free(ctx);
+    return ret;
+}
+
 #endif
 
 #ifndef OPENSSL_NO_EC
@@ -1247,6 +1302,7 @@ int setup_tests(void)
 #ifndef OPENSSL_NO_DH
     ADD_TEST(test_dh_tofrom_data_select);
     ADD_TEST(test_dh_paramgen);
+    ADD_TEST(test_dh_paramfromdata);
 #endif
     ADD_TEST(test_rsa_tofrom_data_select);