Fix PRE_PARAMS_TO_CTRL translation

[ElMostafaIdrassi]

This PR fixes #20161.

It does that by addressing the following issues:

• ctx->p2 is NULL and is passed by value instead of by address to OSSL_PARAM_get_utf8_string. As a result, get_string_internal returns 0, leading to failure. By passing ctx->p2 by address, get_string_internal will allocate the needed string and make ctx->p2 point to it. Also, worth noting that the next call to OBJ_sn2nid in fix_ec_paramgen_curve_nid expects a const char * as argument, which means that ctx->p2 is expected to be a char *.
• ctx->ctrl_cmd is 0 in evp_pkey_ctx_setget_params_to_ctrl, As a result, the next call to EVP_PKEY_CTX_ctrl will fail: it calls pkey_ec_ctrl, and since ctx->ctrl_cmd is 0, it fails with -2 and raises EVP_R_COMMAND_NOT_SUPPORTED. By setting it to EVP_PKEY_CTRL_EC_PARAMGEN_CURVE_NID in fix_ec_paramgen_curve_nid, the call to EVP_PKEY_CTX_ctrl will succeed as expected.

[myrkr]

I had a similar problem with TLS and an EC engine loaded, where the following error was reported:

4020B8418B7F0000:error:0A080006:SSL routines:ssl_generate_pkey_group:EVP lib:ssl/s3_lib.c:4714:

The problem was happening with the curl library and preloading our EC engine.

This patch solved this problem but I fear that we are leaking the memory allocated by OSSL_PARAM_get_utf8_string.

This leak was fixed by adding the OPENSSL_free call as shown in this diff:

@@ -1155,9 +1155,12 @@ static int fix_ec_paramgen_curve_nid(enum state state,
 
     if (state == PRE_PARAMS_TO_CTRL) {
         ctx->p1 = OBJ_sn2nid(ctx->p2);
+        OPENSSL_free(ctx->p2);
         ctx->p2 = NULL;
     }
 
+    ctx->ctrl_cmd = EVP_PKEY_CTRL_EC_PARAMGEN_CURVE_NID;
+
     return ret;
 }
 

[ElMostafaIdrassi]

@myrkr That makes sense, thanks.
I think this should be generalized to all fix_X functions that have their translation->param_data_type set to OSSL_PARAM_UTF8_STRING and called with state being PRE_PARAMS_TO_CTRL.
For example, fix_kdf_type suffers from the same issue, where ctx->p2 should have been allocated by the calls to default_fixup_args->OSSL_PARAM_get_utf8_string->get_string_internal but is not being freed and is just NULLed out.

openssl/crypto/evp/ctrl_params_translate.c

Lines 895 to 909 in ae08ed0

	if ((ret = default_fixup_args(state, translation, ctx)) <= 0)
	return ret;
	if ((state == POST_CTRL_TO_PARAMS && ctx->action_type == GET)
	|| (state == PRE_PARAMS_TO_CTRL && ctx->action_type == SET)) {
	ctx->p1 = ret = -1;
	/* Convert KDF type strings to numbers */
	for (; kdf_type_map->kdf_type_str != NULL; kdf_type_map++)
	if (OPENSSL_strcasecmp(ctx->p2, kdf_type_map->kdf_type_str) == 0) {
	ctx->p1 = kdf_type_map->kdf_type_num;
	ret = 1;
	break;
	}
	ctx->p2 = NULL;

What do you think about this ?

[myrkr]

Some functions (including fix_cipher_md) use a different approach, temporarily letting ctx->p2 point to ctx->name_buffer. Unfortunatly, I have no idea which approach is the best one in this case.

[ElMostafaIdrassi]

I think we're only interested in the case where state is PRE_PARAMS_TO_CTRL, ctx->action_type is SET and translation->param_data_type is OSSL_PARAM_UTF8_STRING, because that's the only case where default_fixup_args actually calls OSSL_PARAM_get_utf8_string, which calls get_string_internal, which allocates and set ctx->p2.

For example, for fix_cipher_md, ctx->p2 has been set by default_fixup_args and is being passed to get_algo_by_name where the result is being set in ctx->p2 itself. I think that, for these kind of fix_X function, we can introduce an intermediate variable to hold the result of get_algo_by_name, free the original ctx->p2 before setting its new value, something like:

void* alg = (void *)get_algo_by_name(ctx->pctx->libctx, ctx->p2);
OPENSSL_free(ctx->p2);
ctx->p2 = alg;

[ElMostafaIdrassi]

I've added code that frees the memory allocated in ctx->p2 by OSSL_PARAM_get_utf8_string only for when state is PRE_PARAMS_TO_CTRL, ctx->action_type is SET and translation->param_data_type is OSSL_PARAM_UTF8_STRING. This should fix the memory leak and also avoid freeing ctx->p2 when it is not supposed to be freed.

[openssl-machine]

This PR has the label 'hold: cla required' and is stale: it has not been updated in 30 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html

[openssl-machine]

This PR has the label 'hold: cla required' and is stale: it has not been updated in 61 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html

[dylanweber]

@ElMostafaIdrassi Can you please complete & submit the contributor license agreement form so this pull request can get merged? This fix is essential to my workflow.

[ElMostafaIdrassi]

@dylanweber Sure, I believe I can do that during this week, but we still need someone to review the changes.

[ElMostafaIdrassi]

CLA was signed, can someone please review this PR ?

[levitte]

So I wonder, why doesn't OSSL_PARAM_OCTET_STRING get the same treatment?

[ElMostafaIdrassi]

IMO, and I could totally be wrong, but it seems to me that the reason why OSSL_PARAM_OCTET_STRING does not get the same treatment (but should) is because all the translations related to it in evp_pkey_ctx_translations do no have a fix_up_args function (they are all NULL), unlike OSSL_PARAM_UTF8_STRING. Therefore, default_fixup_args is never called for OSSL_PARAM_OCTET_STRING and the case where ctx->p2 is NULL never happens for OSSL_PARAM_OCTET_STRING, unlike OSSL_PARAM_UTF8_STRING.

I think that if there was a translation for OSSL_PARAM_OCTET_STRING that had a non NULL fix_up_args which called default_fixup_args, we'd be getting the same errors as the ctx->p2 is not passed by address, meaning the fix up function would not be able to allocate it properly.

As a side note, if you look for OSSL_PARAM_get_octet_string / OSSL_PARAM_get_utf8_string invocations in openssl code, you'll see that they always get called with the variable in question being passed via it address in val, unlike default_fixup_args where ctx->p2 is getting passed via its value. This is why I went for the same thing in here.

[levitte]

Point is that conceptually, there's not much difference between OSSL_PARAM_OCTET_STRING and OSSL_PARAM_UTF8_STRING. They both indicate strings, the only "real" difference being their intent, and how they are normally displayed to humans.

Thanks for the detailed feedback, though. It's possible that I missed a detail in addressing between different callbacks when I put together this module. (like I said, it is a tricky module that takes a lot of historical cases into account)

[levitte]

I understand that there's something that didn't work quite right... but it seems to me that this solution creates an allocation for one case, and forces all other cases to discard an allocation they never needed. Doesn't look quite right.

Mind you, this translation module is tricky, 'cause it seems that some ctrl functions were a bit of special cases. It shows.

I wonder, do you have a small program that demonstrates the issue and that you'd be willing to share? I'd like to have a closer look at what's going on.

[ElMostafaIdrassi]

Sure, here is the test EC engine I first encountered the issue with. There is a build script there, alongside the configuration file. I have explained the steps to reproduce the issue in here, with some extra details concerning the route that failed.

[levitte]

Thank you

[levitte]

I may actually have an alternative fix that's a bit less invasive. Are you willing to try it out, @ElMostafaIdrassi?

[ElMostafaIdrassi]

Closing this in favor of #20780.