Skip to content

Commit

Permalink
Avoid setting gen_type to -1 in dh_gen_common_set_params
Browse files Browse the repository at this point in the history
gh_gen_type_common_set_params looks up a dh contexts gen_type using
name2id, but if it returns error, we set gctx->gen_type to -1, which
is an invalid value, which may lead to undefined behavior in
future calls, in the event that said future calls preform an operation
of the form;
if (gen_type == <VALID VALUE>) {
        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 dh_gen
to validate the gen_val input prior to continuing, should other code
points attempt to do the same thing

Reviewed-by: Hugo Landau <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#22991)
  • Loading branch information
nhorman authored and t8m committed Dec 14, 2023
1 parent 260d972 commit b697864
Showing 1 changed file with 13 additions and 1 deletion.
14 changes: 13 additions & 1 deletion providers/implementations/keymgmt/dh_kmgmt.c
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* internal use.
*/
#include "internal/deprecated.h"
#include "internal/common.h"

#include <string.h> /* strcmp */
#include <openssl/core_dispatch.h>
Expand Down Expand Up @@ -524,6 +525,7 @@ static int dh_gen_common_set_params(void *genctx, const OSSL_PARAM params[])
{
struct dh_gen_ctx *gctx = genctx;
const OSSL_PARAM *p;
int gen_type = -1;

if (gctx == NULL)
return 0;
Expand All @@ -533,11 +535,13 @@ static int dh_gen_common_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 =
|| ((gen_type =
dh_gen_type_name2id_w_default(p->data, gctx->dh_type)) == -1)) {
ERR_raise(ERR_LIB_PROV, ERR_R_PASSED_INVALID_ARGUMENT);
return 0;
}
if (gen_type != -1)
gctx->gen_type = gen_type;
}
p = OSSL_PARAM_locate_const(params, OSSL_PKEY_PARAM_GROUP_NAME);
if (p != NULL) {
Expand Down Expand Up @@ -706,6 +710,14 @@ 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 */
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,
"gen_type set to unsupported value %d", gctx->gen_type);
return NULL;
}

/* For parameter generation - If there is a group name just create it */
if (gctx->gen_type == DH_PARAMGEN_TYPE_GROUP
&& gctx->ffc_params == NULL) {
Expand Down

0 comments on commit b697864

Please sign in to comment.