From 55399eedf41266dcbcac31b8d7f59214ae8ab736 Mon Sep 17 00:00:00 2001 From: Hayden Roche Date: Mon, 12 Sep 2022 15:46:49 -0700 Subject: [PATCH] Fix some issues discovered when running ruby OpenSSL gem tests. - If length param to rand bytes function is 0, do nothing and return success. - In we_dh_compute_key_int, if DH_get0_priv_key returns NULL, error out. The lack of NULL check here was causing a seg fault. - Add support for the dh_paramgen_prime_len control string in DH code. - Receiving EVP_PKEY_CTRL_DH_PARAMGEN_GENERATOR should return an error, since we don't support setting the generator. - Add a DH control command unit test. - Move EVP_PKEY_CTRL_DH_PAD definition into openssl_bc.h so that unit tests have access to it. Add comment noting which OpenSSL version introduced this macro. See ZD #14805. --- include/wolfengine/we_openssl_bc.h | 5 ++ src/we_dh.c | 36 ++++++++---- src/we_random.c | 64 +++++++++++---------- test/test_dh.c | 91 ++++++++++++++++++++++++++++++ test/unit.c | 1 + test/unit.h | 1 + 6 files changed, 155 insertions(+), 43 deletions(-) diff --git a/include/wolfengine/we_openssl_bc.h b/include/wolfengine/we_openssl_bc.h index a76a30b..784e39e 100644 --- a/include/wolfengine/we_openssl_bc.h +++ b/include/wolfengine/we_openssl_bc.h @@ -200,6 +200,11 @@ size_t EC_POINT_point2buf(const EC_GROUP *group, const EC_POINT *point, #define EVP_PKEY_ECDH_KDF_X9_63 EVP_PKEY_ECDH_KDF_X9_62 #endif +#ifndef EVP_PKEY_CTRL_DH_PAD +/* First defined in OPENSSL_VERSION_NUMBER == 0x10101001L. */ +#define EVP_PKEY_CTRL_DH_PAD (EVP_PKEY_ALG_CTRL + 16) +#endif + WOLFENGINE_LOCAL const BIGNUM *DH_get0_p(const DH *dh); WOLFENGINE_LOCAL const BIGNUM *DH_get0_g(const DH *dh); WOLFENGINE_LOCAL const BIGNUM *DH_get0_q(const DH *dh); diff --git a/src/we_dh.c b/src/we_dh.c index 2aad3b4..49fe7d9 100644 --- a/src/we_dh.c +++ b/src/we_dh.c @@ -31,10 +31,6 @@ * local one. */ -#ifndef EVP_PKEY_CTRL_DH_PAD -#define EVP_PKEY_CTRL_DH_PAD (EVP_PKEY_ALG_CTRL + 16) -#endif - #define DEFAULT_PRIME_LEN 1024 /** @@ -567,6 +563,7 @@ static int we_dh_compute_key_int(we_Dh *engineDh, unsigned char *secret, unsigned char *privBuf = NULL; int privLen = 0; unsigned int secLen = 0; + const BIGNUM* privBn; WOLFENGINE_ENTER(WE_LOG_KE, "we_dh_compute_key_int"); WOLFENGINE_MSG_VERBOSE(WE_LOG_KE, "ARGS [engineDh = %p, secret = %p, " @@ -600,7 +597,15 @@ static int we_dh_compute_key_int(we_Dh *engineDh, unsigned char *secret, if (ret == 1) { /* Convert our private key to a byte array. */ - ret = we_dh_bignum_to_bin(DH_get0_priv_key(dh), &privBuf, &privLen); + privBn = DH_get0_priv_key(dh); + if (privBn == NULL) { + WOLFENGINE_ERROR_MSG(WE_LOG_KE, "Private key is NULL. Can't create " + "DH shared secret."); + ret = 0; + } + else { + ret = we_dh_bignum_to_bin(DH_get0_priv_key(dh), &privBuf, &privLen); + } } if (ret == 1) { @@ -1057,6 +1062,7 @@ static int we_dh_pkey_ctrl(EVP_PKEY_CTX *ctx, int type, int num, void *ptr) "setting the generator when generating DH params"); /* wolfCrypt doesn't allow setting the generator when generating * DH params. */ + ret = 0; break; case EVP_PKEY_CTRL_DH_PAD: dh->pad = num; @@ -1086,7 +1092,8 @@ static int we_dh_pkey_ctrl(EVP_PKEY_CTX *ctx, int type, int num, void *ptr) * Extra operations for working with DH. * Supported operations include: * - "dh_param": set the named parameters. - * - "pad": pad out secret to input length. + * - "dh_pad": pad out secret to input length. + * - "dh_paramgen_prime_len": set the length of the prime, "p." * * @param ctx [in] Public key context of operation. * @param type [in] Type of operation to perform. @@ -1113,7 +1120,7 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type, if (ret == 1) { /* Set named DH parameters. */ - if (XSTRNCMP(type, "dh_param", 9) == 0) { + if (XSTRCMP(type, "dh_param") == 0) { #ifndef HAVE_WC_DHSETNAMEDKEY const DhParams *params = NULL; #else @@ -1122,7 +1129,7 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type, #ifdef HAVE_PUBLIC_FFDHE #ifdef HAVE_FFDHE_2048 - if (XSTRNCMP(value, "ffdhe2048", 10) == 0) { + if (XSTRCMP(value, "ffdhe2048") == 0) { WOLFENGINE_MSG(WE_LOG_KE, "Setting named parameters: ffdhe2048"); #ifndef HAVE_WC_DHSETNAMEDKEY @@ -1134,7 +1141,7 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type, else #endif #ifdef HAVE_FFDHE_3072 - if (XSTRNCMP(value, "ffdhe3072", 10) == 0) { + if (XSTRCMP(value, "ffdhe3072") == 0) { WOLFENGINE_MSG(WE_LOG_KE, "Setting named parameters: ffdhe3072"); #ifndef HAVE_WC_DHSETNAMEDKEY @@ -1146,7 +1153,7 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type, else #endif #ifdef HAVE_FFDHE_4096 - if (XSTRNCMP(value, "ffdhe4096", 10) == 0) { + if (XSTRCMP(value, "ffdhe4096") == 0) { WOLFENGINE_MSG(WE_LOG_KE, "Setting named parameters: ffdhe4096"); #ifndef HAVE_WC_DHSETNAMEDKEY @@ -1180,7 +1187,8 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type, rc = wc_DhSetNamedKey(&dh->key, params); #endif if (rc != 0) { - WOLFENGINE_ERROR_MSG(WE_LOG_KE, "Failed set parameters"); + WOLFENGINE_ERROR_MSG(WE_LOG_KE, "Failed to set " + "parameters."); ret = 0; } } @@ -1189,9 +1197,13 @@ static int we_dh_pkey_ctrl_str(EVP_PKEY_CTX *ctx, const char *type, } } /* Set padding requirement for secret output. */ - else if (XSTRNCMP(type, "dh_pad", 7) == 0) { + else if (XSTRCMP(type, "dh_pad") == 0) { dh->pad = XATOI(value); } + else if (XSTRCMP(type, "dh_paramgen_prime_len") == 0) { + ret = we_dh_pkey_ctrl(ctx, EVP_PKEY_CTRL_DH_PARAMGEN_PRIME_LEN, + XATOI(value), NULL); + } else { /* Unsupported control type. */ XSNPRINTF(errBuff, sizeof(errBuff), "Unsupported ctrl string %s", diff --git a/src/we_random.c b/src/we_random.c index 057c157..e049c16 100644 --- a/src/we_random.c +++ b/src/we_random.c @@ -358,43 +358,45 @@ static int we_rand_bytes(unsigned char *buf, int num) WOLFENGINE_MSG_VERBOSE(WE_LOG_RNG, "ARGS [buf = %p, num = %d]", buf, num); -#ifndef WE_SINGLE_THREADED - rc = wc_LockMutex(we_rng_mutex); - if (rc != 0) { - WOLFENGINE_ERROR_FUNC(WE_LOG_RNG, "wc_LockMutex", rc); - ret = 0; - } - else -#endif - { - /* Use global random to generator pseudo-random data. */ - rc = wc_RNG_GenerateBlock(we_rng, buf, num); + if (num > 0) { + #ifndef WE_SINGLE_THREADED + rc = wc_LockMutex(we_rng_mutex); if (rc != 0) { - WOLFENGINE_ERROR_FUNC(WE_LOG_RNG, "wc_RNG_GenerateBlock", rc); + WOLFENGINE_ERROR_FUNC(WE_LOG_RNG, "wc_LockMutex", rc); ret = 0; } - #ifndef WE_STATIC_WOLFSSL - /* Mix global seed if RAND_add() or RAND_seed() has been called. */ - if (ret == 1 && haveSeed) { - ret = we_rand_mix_seed(buf, num, we_seed, sizeof(we_seed)); - if (ret != 1) { - WOLFENGINE_ERROR_MSG(WE_LOG_RNG, "we_rand_mix_seed with global " - "seed failed"); + else + #endif + { + /* Use global random to generator pseudo-random data. */ + rc = wc_RNG_GenerateBlock(we_rng, buf, num); + if (rc != 0) { + WOLFENGINE_ERROR_FUNC(WE_LOG_RNG, "wc_RNG_GenerateBlock", rc); + ret = 0; } - } - /* Mix in weak entropy. */ - if (ret == 1) { - ret = we_rand_add_weak_entropy(buf, num); - if (ret != 1) { - WOLFENGINE_ERROR_MSG(WE_LOG_RNG, "we_rand_mix_seed with " - "weak entropy failed"); + #ifndef WE_STATIC_WOLFSSL + /* Mix global seed if RAND_add() or RAND_seed() has been called. */ + if (ret == 1 && haveSeed) { + ret = we_rand_mix_seed(buf, num, we_seed, sizeof(we_seed)); + if (ret != 1) { + WOLFENGINE_ERROR_MSG(WE_LOG_RNG, "we_rand_mix_seed with " + "global seed failed"); + } } - } - #endif /* !WE_STATIC_WOLFSSL */ + /* Mix in weak entropy. */ + if (ret == 1) { + ret = we_rand_add_weak_entropy(buf, num); + if (ret != 1) { + WOLFENGINE_ERROR_MSG(WE_LOG_RNG, "we_rand_mix_seed with " + "weak entropy failed"); + } + } + #endif /* !WE_STATIC_WOLFSSL */ - #ifndef WE_SINGLE_THREADED - wc_UnLockMutex(we_rng_mutex); - #endif + #ifndef WE_SINGLE_THREADED + wc_UnLockMutex(we_rng_mutex); + #endif + } } WOLFENGINE_LEAVE(WE_LOG_RNG, "we_rand_bytes", ret); diff --git a/test/test_dh.c b/test/test_dh.c index 7725638..cb61836 100644 --- a/test/test_dh.c +++ b/test/test_dh.c @@ -426,6 +426,97 @@ int test_dh_pkey(ENGINE* e, void* data) return err; } +int test_dh_ctrl(ENGINE* e, void* data) +{ + int err; + EVP_PKEY_CTX* ctx = NULL; + int primeLengths[] = {1024, 2048, 3072}; + word32 i; + + (void)data; + + err = (ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_DH, e)) == NULL; + if (err == 0){ + err = EVP_PKEY_paramgen_init(ctx) <= 0; + } + + for (i = 0; err == 0 && i < sizeof(primeLengths)/sizeof(*primeLengths); + ++i) { + /* Set valid prime lengths. */ + err = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_DH, EVP_PKEY_OP_PARAMGEN, + EVP_PKEY_CTRL_DH_PARAMGEN_PRIME_LEN, primeLengths[i], + NULL) <= 0; + } + if (err == 0) { + /* Set an invalid prime length. */ + err = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_DH, EVP_PKEY_OP_PARAMGEN, + EVP_PKEY_CTRL_DH_PARAMGEN_PRIME_LEN, 512, NULL) > 0; + } + if (err == 0) { + /* Set a valid prime length via control string. */ + err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_paramgen_prime_len", "2048") <= 0; + } + + if (err == 0) { + /* Set the generator (not supported by wolfCrypt). */ + err = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_DH, EVP_PKEY_OP_PARAMGEN, + EVP_PKEY_CTRL_DH_PARAMGEN_GENERATOR, 2, NULL) > 0; + } + + if (err == 0) { + /* Zero pad secret. */ + err = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_DH, EVP_PKEY_OP_PARAMGEN, + EVP_PKEY_CTRL_DH_PAD, 1, NULL) <= 0; + } + if (err == 0) { + /* Same as above but using a control string. */ + err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_pad", "1") <= 0; + } + + if (err == 0) { + /* Set peer key for shared secret. No-op internally. */ + err = EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_DH, EVP_PKEY_OP_PARAMGEN, + EVP_PKEY_CTRL_PEER_KEY, 0, NULL) <= 0; + } + +#ifdef HAVE_PUBLIC_FFDHE + /* Set parameters to different named parameter sets. */ + #ifdef HAVE_FFDHE_2048 + if (err == 0) { + err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe2048") <= 0; + } + #else + if (err == 0) { + err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe2048") > 0; + } + #endif /* HAVE_FFDHE_2048 */ + + #ifdef HAVE_FFDHE_3072 + if (err == 0) { + err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe3072") <= 0; + } + #else + if (err == 0) { + err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe3072") > 0; + } + #endif /* HAVE_FFDHE_3072 */ + + #ifdef HAVE_FFDHE_4096 + if (err == 0) { + err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe4096") <= 0; + } + #else + if (err == 0) { + err = EVP_PKEY_CTX_ctrl_str(ctx, "dh_param", "ffdhe4096") > 0; + } + #endif /* HAVE_FFDHE_4096 */ +#endif /* HAVE_PUBLIC_FFDHE */ + + EVP_PKEY_CTX_free(ctx); + + return err; +} + #endif /* WE_HAVE_EVP_PKEY */ #endif /* WE_HAVE_DH */ diff --git a/test/unit.c b/test/unit.c index b3ec741..928df64 100644 --- a/test/unit.c +++ b/test/unit.c @@ -153,6 +153,7 @@ TEST_CASE test_case[] = { #ifdef WE_HAVE_EVP_PKEY TEST_DECL(test_dh_pgen_pkey, NULL), TEST_DECL(test_dh_pkey, NULL), + TEST_DECL(test_dh_ctrl, NULL), #if !defined(WE_SINGLE_THREADED) && defined(_WIN32) TEST_DECL(test_dh_key_gen_multithreaded, NULL), #endif /* !WE_SINGLE_THREADED && _WIN32 */ diff --git a/test/unit.h b/test/unit.h index 1aefaa6..af22d8c 100644 --- a/test/unit.h +++ b/test/unit.h @@ -249,6 +249,7 @@ int test_dh(ENGINE *e, void *data); #ifdef WE_HAVE_EVP_PKEY int test_dh_pgen_pkey(ENGINE *e, void *data); int test_dh_pkey(ENGINE *e, void *data); +int test_dh_ctrl(ENGINE *e, void *data); #if !defined(WE_SINGLE_THREADED) && defined(_WIN32) int test_dh_key_gen_multithreaded(ENGINE *e, void *data); #endif /* !WE_SINGLE_THREADED && _WIN32 */