From 5056133cc7d6c52033c25b2e1f7762bcafcce760 Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Fri, 8 Dec 2023 15:41:51 -0500 Subject: [PATCH] Avoid setting gen_type to -1 in dsa_gen_set_params gh_gen_type_common_set_params looks up a dsa contexts gen_type using name2id, but if it returns error, we inadvertently set gctx->gen_type to -1, which is an invalid value, which may lead to improper behavior in future calls, in the event that said future calls preform an operation of the form; if (gen_type == ) { do_stuff else { do_other_stuff } Technically it is not correct to continue with the operations on the gen context after failed parameters setting but this makes it more predictable. Fix it by assigning the result of a lookup to a stack variable, and only update gctx->gen_value if the lookup returns a non-failing value In leiu of testing this specific case, also add an ossl_assert in dsa_gen to validate the gen_val input prior to continuing, should other code points attempt to do the same thing Reviewed-by: Hugo Landau Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/22991) --- include/crypto/dsa.h | 5 ++++ include/openssl/dh.h | 6 ++++- providers/implementations/keymgmt/dh_kmgmt.c | 7 +++++- providers/implementations/keymgmt/dsa_kmgmt.c | 24 ++++++++++++++++++- 4 files changed, 39 insertions(+), 3 deletions(-) diff --git a/include/crypto/dsa.h b/include/crypto/dsa.h index 85d92a18cbffc..b08a42c7f2861 100644 --- a/include/crypto/dsa.h +++ b/include/crypto/dsa.h @@ -15,6 +15,11 @@ # include # include "internal/ffc.h" +/* + * DSA Paramgen types + * Note, adding to this list requires adjustments to various checks + * in dsa_gen range validation checks + */ #define DSA_PARAMGEN_TYPE_FIPS_186_4 0 /* Use FIPS186-4 standard */ #define DSA_PARAMGEN_TYPE_FIPS_186_2 1 /* Use legacy FIPS186-2 standard */ #define DSA_PARAMGEN_TYPE_FIPS_DEFAULT 2 diff --git a/include/openssl/dh.h b/include/openssl/dh.h index f1c0ed06b375a..97024929a40de 100644 --- a/include/openssl/dh.h +++ b/include/openssl/dh.h @@ -25,7 +25,11 @@ extern "C" { #include -/* DH parameter generation types used by EVP_PKEY_CTX_set_dh_paramgen_type() */ +/* + * DH parameter generation types used by EVP_PKEY_CTX_set_dh_paramgen_type() + * Note that additions/changes to this set of values requires corresponding + * adjustments to range checks in dh_gen() + */ # define DH_PARAMGEN_TYPE_GENERATOR 0 /* Use a safe prime generator */ # define DH_PARAMGEN_TYPE_FIPS_186_2 1 /* Use FIPS186-2 standard */ # define DH_PARAMGEN_TYPE_FIPS_186_4 2 /* Use FIPS186-4 standard */ diff --git a/providers/implementations/keymgmt/dh_kmgmt.c b/providers/implementations/keymgmt/dh_kmgmt.c index 183738b61ba87..82c3093b122c2 100644 --- a/providers/implementations/keymgmt/dh_kmgmt.c +++ b/providers/implementations/keymgmt/dh_kmgmt.c @@ -710,7 +710,12 @@ static void *dh_gen(void *genctx, OSSL_CALLBACK *osslcb, void *cbarg) if (gctx->group_nid != NID_undef) gctx->gen_type = DH_PARAMGEN_TYPE_GROUP; - /* Bounds check on context gen_type */ + /* + * Do a bounds check on context gen_type. Must be in range: + * DH_PARAMGEN_TYPE_GENERATOR <= gen_type <= DH_PARAMGEN_TYPE_GROUP + * Noted here as this needs to be adjusted if a new group type is + * added. + */ if (!ossl_assert((gctx->gen_type >= DH_PARAMGEN_TYPE_GENERATOR) && (gctx->gen_type <= DH_PARAMGEN_TYPE_GROUP))) { ERR_raise_data(ERR_LIB_PROV, ERR_R_INTERNAL_ERROR, diff --git a/providers/implementations/keymgmt/dsa_kmgmt.c b/providers/implementations/keymgmt/dsa_kmgmt.c index a89d20822bf12..3fdc78432b8e1 100644 --- a/providers/implementations/keymgmt/dsa_kmgmt.c +++ b/providers/implementations/keymgmt/dsa_kmgmt.c @@ -462,6 +462,7 @@ static int dsa_gen_set_params(void *genctx, const OSSL_PARAM params[]) { struct dsa_gen_ctx *gctx = genctx; const OSSL_PARAM *p; + int gen_type = -1; if (gctx == NULL) return 0; @@ -472,10 +473,18 @@ static int dsa_gen_set_params(void *genctx, const OSSL_PARAM params[]) p = OSSL_PARAM_locate_const(params, OSSL_PKEY_PARAM_FFC_TYPE); if (p != NULL) { if (p->data_type != OSSL_PARAM_UTF8_STRING - || ((gctx->gen_type = dsa_gen_type_name2id(p->data)) == -1)) { + || ((gen_type = dsa_gen_type_name2id(p->data)) == -1)) { ERR_raise(ERR_LIB_PROV, ERR_R_PASSED_INVALID_ARGUMENT); return 0; } + + /* + * Ony assign context gen_type if it was set by dsa_gen_type_name2id + * must be in range: + * DSA_PARAMGEN_TYPE_FIPS_186_4 <= gen_type <= DSA_PARAMGEN_TYPE_FIPS_DEFAULT + */ + if (gen_type != -1) + gctx->gen_type = gen_type; } p = OSSL_PARAM_locate_const(params, OSSL_PKEY_PARAM_FFC_GINDEX); if (p != NULL @@ -568,6 +577,19 @@ static void *dsa_gen(void *genctx, OSSL_CALLBACK *osslcb, void *cbarg) gctx->gen_type = (gctx->pbits >= 2048 ? DSA_PARAMGEN_TYPE_FIPS_186_4 : DSA_PARAMGEN_TYPE_FIPS_186_2); + /* + * Do a bounds check on context gen_type. Must be in range: + * DSA_PARAMGEN_TYPE_FIPS_186_4 <= gen_type <= DSA_PARAMGEN_TYPE_FIPS_DEFAULT + * Noted here as this needs to be adjusted if a new type is + * added. + */ + if (!ossl_assert((gctx->gen_type >= DSA_PARAMGEN_TYPE_FIPS_186_4) + && (gctx->gen_type <= DSA_PARAMGEN_TYPE_FIPS_DEFAULT))) { + ERR_raise_data(ERR_LIB_PROV, ERR_R_INTERNAL_ERROR, + "gen_type set to unsupported value %d", gctx->gen_type); + return NULL; + } + gctx->cb = osslcb; gctx->cbarg = cbarg; gencb = BN_GENCB_new();