From 3786d74868fe440250f902ce1a78974136ca9304 Mon Sep 17 00:00:00 2001 From: jwalch Date: Thu, 24 Sep 2020 11:43:06 -0400 Subject: [PATCH] en EVP_PKEY_CTX_set_rsa_keygen_pubexp() BIGNUM management Fixes #12635 As discussed in the issue, supporting the set0-like semantics long-term is not necessarily desirable, although necessary for short-term compatibility concerns. So I've deprecated the original method and added an equivalent that is explicitly labelled as set1. I tried to audit existing usages of the (now-deprecated) API and update them to use set1 if that appeared to align with their expectations. Reviewed-by: Matt Caswell Reviewed-by: Shane Lontis (Merged from https://github.com/openssl/openssl/pull/12917) --- CHANGES.md | 5 +++++ apps/genrsa.c | 2 +- crypto/evp/pmeth_lib.c | 1 + crypto/rsa/rsa_lib.c | 35 ++++++++++++++++++++++++++++++---- crypto/rsa/rsa_pmeth.c | 5 ++--- doc/man3/EVP_PKEY_CTX_ctrl.pod | 24 ++++++++++++++++++----- include/crypto/evp.h | 7 +++++++ include/openssl/rsa.h | 4 +++- test/acvp_test.c | 2 +- util/libcrypto.num | 3 ++- 10 files changed, 72 insertions(+), 16 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index c88629ebeb..595a7425ca 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -32,6 +32,11 @@ OpenSSL 3.0 *Richard Levitte* + * Deprecated EVP_PKEY_CTX_set_rsa_keygen_pubexp() & introduced + EVP_PKEY_CTX_set1_rsa_keygen_pubexp(), which is now preferred. + + *Jeremy Walch* + * Changed all "STACK" functions to be macros instead of inline functions. Macro parameters are still checked for type safety at compile time via helper inline functions. diff --git a/apps/genrsa.c b/apps/genrsa.c index 04315a559b..f471814e08 100644 --- a/apps/genrsa.c +++ b/apps/genrsa.c @@ -188,7 +188,7 @@ opthelp: BIO_printf(bio_err, "Error allocating RSA public exponent\n"); goto end; } - if (EVP_PKEY_CTX_set_rsa_keygen_pubexp(ctx, bn) <= 0) { + if (EVP_PKEY_CTX_set1_rsa_keygen_pubexp(ctx, bn) <= 0) { BIO_printf(bio_err, "Error setting RSA public exponent\n"); goto end; } diff --git a/crypto/evp/pmeth_lib.c b/crypto/evp/pmeth_lib.c index 656a0e065a..a3a65857b8 100644 --- a/crypto/evp/pmeth_lib.c +++ b/crypto/evp/pmeth_lib.c @@ -402,6 +402,7 @@ void EVP_PKEY_CTX_free(EVP_PKEY_CTX *ctx) #if !defined(OPENSSL_NO_ENGINE) && !defined(FIPS_MODULE) ENGINE_finish(ctx->engine); #endif + BN_free(ctx->rsa_pubexp); OPENSSL_free(ctx); } diff --git a/crypto/rsa/rsa_lib.c b/crypto/rsa/rsa_lib.c index 858d3d72c8..475fca0f89 100644 --- a/crypto/rsa/rsa_lib.c +++ b/crypto/rsa/rsa_lib.c @@ -1345,7 +1345,9 @@ int EVP_PKEY_CTX_set_rsa_keygen_bits(EVP_PKEY_CTX *ctx, int bits) return 1; } -int EVP_PKEY_CTX_set_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, BIGNUM *pubexp) +static int evp_pkey_ctx_set_rsa_keygen_pubexp_intern(EVP_PKEY_CTX *ctx, + BIGNUM *pubexp, + int copy) { OSSL_PARAM_BLD *tmpl; OSSL_PARAM *params; @@ -1362,9 +1364,15 @@ int EVP_PKEY_CTX_set_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, BIGNUM *pubexp) return -1; /* TODO(3.0): Remove this eventually when no more legacy */ - if (ctx->op.keymgmt.genctx == NULL) - return EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_RSA, EVP_PKEY_OP_KEYGEN, - EVP_PKEY_CTRL_RSA_KEYGEN_PUBEXP, 0, pubexp); + if (ctx->op.keymgmt.genctx == NULL) { + if (copy == 1) + pubexp = BN_dup(pubexp); + ret = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_RSA, EVP_PKEY_OP_KEYGEN, + EVP_PKEY_CTRL_RSA_KEYGEN_PUBEXP, 0, pubexp); + if ((copy == 1) && (ret <= 0)) + BN_free(pubexp); + return ret; + } if ((tmpl = OSSL_PARAM_BLD_new()) == NULL) return 0; @@ -1377,9 +1385,28 @@ int EVP_PKEY_CTX_set_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, BIGNUM *pubexp) ret = EVP_PKEY_CTX_set_params(ctx, params); OSSL_PARAM_BLD_free_params(params); + + /* + * Satisfy memory semantics for pre-3.0 callers of + * EVP_PKEY_CTX_set_rsa_keygen_pubexp(): their expectation is that input + * pubexp BIGNUM becomes managed by the EVP_PKEY_CTX on success. + */ + if ((copy == 0) && (ret > 0)) + ctx->rsa_pubexp = pubexp; + return ret; } +int EVP_PKEY_CTX_set_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, BIGNUM *pubexp) +{ + return evp_pkey_ctx_set_rsa_keygen_pubexp_intern(ctx, pubexp, 0); +} + +int EVP_PKEY_CTX_set1_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, BIGNUM *pubexp) +{ + return evp_pkey_ctx_set_rsa_keygen_pubexp_intern(ctx, pubexp, 1); +} + int EVP_PKEY_CTX_set_rsa_keygen_primes(EVP_PKEY_CTX *ctx, int primes) { OSSL_PARAM params[2], *p = params; diff --git a/crypto/rsa/rsa_pmeth.c b/crypto/rsa/rsa_pmeth.c index e899fbd605..261f347a2d 100644 --- a/crypto/rsa/rsa_pmeth.c +++ b/crypto/rsa/rsa_pmeth.c @@ -653,9 +653,8 @@ static int pkey_rsa_ctrl_str(EVP_PKEY_CTX *ctx, BIGNUM *pubexp = NULL; if (!BN_asc2bn(&pubexp, value)) return 0; - ret = EVP_PKEY_CTX_set_rsa_keygen_pubexp(ctx, pubexp); - if (ret <= 0) - BN_free(pubexp); + ret = EVP_PKEY_CTX_set1_rsa_keygen_pubexp(ctx, pubexp); + BN_free(pubexp); return ret; } diff --git a/doc/man3/EVP_PKEY_CTX_ctrl.pod b/doc/man3/EVP_PKEY_CTX_ctrl.pod index e5c187d950..7159d8885c 100644 --- a/doc/man3/EVP_PKEY_CTX_ctrl.pod +++ b/doc/man3/EVP_PKEY_CTX_ctrl.pod @@ -17,6 +17,7 @@ EVP_PKEY_CTX_set_rsa_pss_saltlen, EVP_PKEY_CTX_get_rsa_pss_saltlen, EVP_PKEY_CTX_set_rsa_keygen_bits, EVP_PKEY_CTX_set_rsa_keygen_pubexp, +EVP_PKEY_CTX_set1_rsa_keygen_pubexp, EVP_PKEY_CTX_set_rsa_keygen_primes, EVP_PKEY_CTX_set_rsa_mgf1_md_name, EVP_PKEY_CTX_set_rsa_mgf1_md, @@ -101,7 +102,7 @@ EVP_PKEY_CTX_set_kem_op int EVP_PKEY_CTX_set_rsa_pss_saltlen(EVP_PKEY_CTX *ctx, int saltlen); int EVP_PKEY_CTX_get_rsa_pss_saltlen(EVP_PKEY_CTX *ctx, int *saltlen); int EVP_PKEY_CTX_set_rsa_keygen_bits(EVP_PKEY_CTX *ctx, int mbits); - int EVP_PKEY_CTX_set_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, BIGNUM *pubexp); + int EVP_PKEY_CTX_set1_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, BIGNUM *pubexp); int EVP_PKEY_CTX_set_rsa_keygen_primes(EVP_PKEY_CTX *ctx, int primes); int EVP_PKEY_CTX_set_rsa_mgf1_md_name(EVP_PKEY_CTX *ctx, const char *mdname, const char *mdprops); @@ -177,6 +178,14 @@ EVP_PKEY_CTX_set_kem_op int EVP_PKEY_CTX_get1_id(EVP_PKEY_CTX *ctx, void *id); int EVP_PKEY_CTX_get1_id_len(EVP_PKEY_CTX *ctx, size_t *id_len); +Deprecated since OpenSSL 3.0, can be hidden entirely by defining +B with a suitable version value, see +L: + + #include + + int EVP_PKEY_CTX_set_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, BIGNUM *pubexp); + =head1 DESCRIPTION EVP_PKEY_CTX_ctrl() sends a control operation to the context I. The key @@ -282,10 +291,15 @@ The padding mode must already have been set to B. EVP_PKEY_CTX_set_rsa_keygen_bits() sets the RSA key length for RSA key generation to I. If not specified 2048 bits is used. -EVP_PKEY_CTX_set_rsa_keygen_pubexp() sets the public exponent value for RSA key -generation to I. Currently it should be an odd integer. The -I pointer is used internally by this function so it should not be -modified or freed after the call. If not specified 65537 is used. +EVP_PKEY_CTX_set1_rsa_keygen_pubexp() sets the public exponent value for RSA key +generation to the value stored in I. Currently it should be an odd +integer. In accordance with the OpenSSL naming convention, the I pointer +must be freed independently of the EVP_PKEY_CTX (ie, it is internally copied). +If not specified 65537 is used. + +EVP_PKEY_CTX_set_rsa_keygen_pubexp() does the same as +EVP_PKEY_CTX_set1_rsa_keygen_pubexp() except that there is no internal copy and +therefore I should not be modified or freed after the call. EVP_PKEY_CTX_set_rsa_keygen_primes() sets the number of primes for RSA key generation to I. If not specified 2 is used. diff --git a/include/crypto/evp.h b/include/crypto/evp.h index 835224a7aa..23990f651c 100644 --- a/include/crypto/evp.h +++ b/include/crypto/evp.h @@ -95,6 +95,13 @@ struct evp_pkey_ctx_st { void *data; /* Indicator if digest_custom needs to be called */ unsigned int flag_call_digest_custom:1; + /* + * Used to support taking custody of memory in the case of a provider being + * used with the deprecated EVP_PKEY_CTX_set_rsa_keygen_pubexp() API. This + * member should NOT be used for any other purpose and should be removed + * when said deprecated API is excised completely. + */ + BIGNUM *rsa_pubexp; } /* EVP_PKEY_CTX */ ; #define EVP_PKEY_FLAG_DYNAMIC 1 diff --git a/include/openssl/rsa.h b/include/openssl/rsa.h index 140c0d4412..24b2a7eb55 100644 --- a/include/openssl/rsa.h +++ b/include/openssl/rsa.h @@ -132,7 +132,9 @@ int EVP_PKEY_CTX_set_rsa_pss_saltlen(EVP_PKEY_CTX *ctx, int saltlen); int EVP_PKEY_CTX_get_rsa_pss_saltlen(EVP_PKEY_CTX *ctx, int *saltlen); int EVP_PKEY_CTX_set_rsa_keygen_bits(EVP_PKEY_CTX *ctx, int bits); -int EVP_PKEY_CTX_set_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, BIGNUM *pubexp); +DEPRECATEDIN_3_0(int EVP_PKEY_CTX_set_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, + BIGNUM *pubexp)) +int EVP_PKEY_CTX_set1_rsa_keygen_pubexp(EVP_PKEY_CTX *ctx, BIGNUM *pubexp); int EVP_PKEY_CTX_set_rsa_keygen_primes(EVP_PKEY_CTX *ctx, int primes); int EVP_PKEY_CTX_set_rsa_pss_keygen_saltlen(EVP_PKEY_CTX *ctx, int saltlen); diff --git a/test/acvp_test.c b/test/acvp_test.c index de1a2e1fbc..74e2eace7e 100644 --- a/test/acvp_test.c +++ b/test/acvp_test.c @@ -1119,7 +1119,7 @@ static int rsa_keygen_test(int id) || !TEST_int_gt(EVP_PKEY_keygen_init(ctx), 0) || !TEST_true(EVP_PKEY_CTX_set_params(ctx, params)) || !TEST_true(EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, tst->mod)) - || !TEST_true(EVP_PKEY_CTX_set_rsa_keygen_pubexp(ctx, e_bn)) + || !TEST_true(EVP_PKEY_CTX_set1_rsa_keygen_pubexp(ctx, e_bn)) || !TEST_int_gt(EVP_PKEY_keygen(ctx, &pkey), 0) || !TEST_true(pkey_get_bn_bytes(pkey, OSSL_PKEY_PARAM_RSA_TEST_P1, &p1, &p1_len)) diff --git a/util/libcrypto.num b/util/libcrypto.num index db4a1aab2d..7a0965ba72 100644 --- a/util/libcrypto.num +++ b/util/libcrypto.num @@ -4984,7 +4984,8 @@ OSSL_CMP_MSG_read ? 3_0_0 EXIST::FUNCTION:CMP OSSL_CMP_MSG_write ? 3_0_0 EXIST::FUNCTION:CMP EVP_PKEY_gen ? 3_0_0 EXIST::FUNCTION: EVP_PKEY_CTX_set_rsa_keygen_bits ? 3_0_0 EXIST::FUNCTION:RSA -EVP_PKEY_CTX_set_rsa_keygen_pubexp ? 3_0_0 EXIST::FUNCTION:RSA +EVP_PKEY_CTX_set_rsa_keygen_pubexp ? 3_0_0 EXIST::FUNCTION:DEPRECATEDIN_3_0,RSA +EVP_PKEY_CTX_set1_rsa_keygen_pubexp ? 3_0_0 EXIST::FUNCTION:RSA EVP_PKEY_CTX_set_rsa_keygen_primes ? 3_0_0 EXIST::FUNCTION:RSA NCONF_new_with_libctx ? 3_0_0 EXIST::FUNCTION: CONF_modules_load_file_with_libctx ? 3_0_0 EXIST::FUNCTION: -- 2.39.2