OpenSSL: remove unreachable call to SSL_CTX_get0_privatekey()
PATCH 1/1 OpenSSL: remove unreachable call to SSL_CTX_get0_privatekey()
From: Antonio Quartulli <[email protected]...>
In tls_ctx_load_ecdh_params() the SSL_CTX_get0_privatekey() function is invoked only when "OPENSSL_VERSION_NUMBER >= 0x10002000L" and curve_name is NULL.
However, under the very same conditions the code flow will lead to an earlier return, thus never reaching the invocation of SSL_CTX_get0_privatekey().
Restructure the surrounding code in order to make the if/else block a bit easier to read and get rid of the unreachable invocation.
---
Note: I believe that code sections like this, trying to deal with different library versions, should be cleaned up by moving the various #ifs to header files, instead of having them in the code. However, this should be done in a later (and bigger) cleanup.
src/openvpn/ssl_openssl.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)
src/openvpn/ssl_openssl.c +9 -14
@@ -484,15 +484,7 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name/* Generate a new ECDH key for each SSL session (for non-ephemeral ECDH) */ SSL_CTX_set_options (ctx->ctx,SSL_OP_SINGLE_ECDH_USE ); -#if OPENSSL_VERSION_NUMBER >=0x10002000L -/* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter loading */ -if (NULL == curve_name) - { -SSL_CTX_set_ecdh_auto (ctx->ctx,1 ); -return ; - } -#endif -/* For older OpenSSL, we'll have to do the parameter loading on our own */ +if (curve_name !=NULL ) {/* Use user supplied curve if given */
@@ -501,14 +493,17 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name }else { -/* Extract curve from key */ +#if OPENSSL_VERSION_NUMBER >=0x10002000L +/* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter +* loading */ +SSL_CTX_set_ecdh_auto (ctx->ctx,1 ); +return ; +#else +/* For older OpenSSL we have to extract the curve from key on our own */ EC_KEY *eckey =NULL ;const EC_GROUP *ecgrp =NULL ;EVP_PKEY *pkey =NULL ; -#if OPENSSL_VERSION_NUMBER >=0x10002000L && !defined(LIBRESSL_VERSION_NUMBER ) - pkey =SSL_CTX_get0_privatekey (ctx->ctx); -#else /* Little hack to get private key ref from SSL_CTX, yay OpenSSL... */ SSL *ssl =SSL_new (ctx->ctx);if (!ssl)
@@ -517,7 +512,6 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name } pkey =SSL_get_privatekey (ssl);SSL_free (ssl); -#endif msg (D_TLS_DEBUG ,"Extracting ECDH curve from private key" );
@@ -526,6 +520,7 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name { nid =EC_GROUP_get_curve_name (ecgrp); } +#endif }/* Translate NID back to name , just for kicks */
Signature · 1 line
2.13.3
Hi,
Quoted 14 lines · Antonio Quartulli
From: Antonio Quartulli <[email protected]...>
In
tls_ctx_load_ecdh_params()theSSL_CTX_get0_privatekey()function is invoked only when "OPENSSL_VERSION_NUMBER>=0x10002000L" andcurve_nameis NULL.However, under the very same conditions the code flow will lead to an earlier return, thus never reaching the invocation of
SSL_CTX_get0_privatekey().Restructure the surrounding code in order to make the if/else block a bit easier to read and get rid of the unreachable invocation.
Quoted 7 lines
Signed-off-by Antonio Quartulli <[email protected]...>---
Note: I believe that code sections like this, trying to deal with different library versions, should be cleaned up by moving the various #ifs to header files, instead of having them in the code. However, this should be done in a later (and bigger) cleanup.
I'm not so sure that would improve things, but I'm happy to review a patch to be convinced otherwise ;-)
Quoted 56 lines
src/openvpn/ssl_openssl.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)src/openvpn/ssl_openssl.c +8
-14@@ -484,15 +484,7 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name/* Generate a new ECDH key for each SSL session (for non-ephemeral ECDH) */ SSL_CTX_set_options (ctx->ctx,SSL_OP_SINGLE_ECDH_USE ); -#if OPENSSL_VERSION_NUMBER >=0x10002000L -/* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter loading */ -if (NULL == curve_name) - { -SSL_CTX_set_ecdh_auto (ctx->ctx,1 ); -return ; - } -#endif -/* For older OpenSSL, we'll have to do the parameter loading on our own */ +if (curve_name !=NULL ) {/* Use user supplied curve if given */ @@ -501,14 +493,17 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name }else { -/* Extract curve from key */ +#if OPENSSL_VERSION_NUMBER >=0x10002000L +/* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter +* loading */ +SSL_CTX_set_ecdh_auto (ctx->ctx,1 ); +return ; +#else +/* For older OpenSSL we have to extract the curve from key on our own */ EC_KEY *eckey =NULL ;const EC_GROUP *ecgrp =NULL ;EVP_PKEY *pkey =NULL ; -#if OPENSSL_VERSION_NUMBER >=0x10002000L && !defined(LIBRESSL_VERSION_NUMBER ) - pkey =SSL_CTX_get0_privatekey (ctx->ctx); -#else /* Little hack to get private key ref from SSL_CTX, yay OpenSSL... */ SSL *ssl =SSL_new (ctx->ctx);if (!ssl)@@ -517,7 +512,6 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name } pkey =SSL_get_privatekey (ssl);SSL_free (ssl); -#endif msg (D_TLS_DEBUG ,"Extracting ECDH curve from private key" );
src/openvpn/ssl_openssl.c tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name · line 526
Quoted 7 lines
src/openvpn/ssl_openssl.c +1
-0@@ -526,6 +520,7 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name { nid =EC_GROUP_get_curve_name (ecgrp); } +#endif }/* Translate NID back to name , just for kicks */
ACK. Patch looks good, and this still works as expected with both OpenSSL 1.0.1 and 1.0.2 (both #if branches).
-Steffan
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Your patch has been applied to the following branches
commit 5b004f99d069fe0238aacbb0b3288872a4d7ae17 (master) commit a0ee61b31ced8c49ed3926adaf8c42dca4702b49 (release/2.4)
Author: Antonio Quartulli Date: Wed Aug 9 15:42:37 2017 +0800
OpenSSL: remove unreachable call to SSL_CTX_get0_privatekey()
Signed-off-by: Antonio Quartulli <[email protected]...>
Acked-by: Steffan Karger <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg15186.html
Signed-off-by: David Sommerseth <[email protected]...>- --
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBCgAGBQJZjg/NAAoJEIbPlEyWcf3y1F4QAJyow7lPlSCgFUc5Yq7Yh5jy de+FtUTELHC9lostek1wGDEQc4xloAQKMUaJRRAMUAGOOpI7Td+rbRZ+MqkUg975 dvPnJloFAFdzwVHcRjfhS/fCZ78jAmgosCXwcz4kLj+TR6ZaYRDnlIbKV+KrNISD gMXoQnkaGcQRNkYIJJYIJ/13Eqh46Es2+O5UjSy3m/giSOfhOvy71o58WC4RwD3x jxsjObiR4tCFEj1VbqtL8zUc6e1YkBCHmLNMcDKIiHo7tPRxo6U2FfHq0KwFqo2R ZYn2VlzOeG332F33jqtXAUKPNLP9KrFIiFXeL++1jJWSwUoPlcW0oEBXclu6rUGG w+DiucOp/ZwI4sir1i7aZbZwqK1L9RIT9C0DI25bJ5BgukxIyz+zBR0IvcOtPnST OAvnbqM89uwf2KagJ0SPrfqNcGJoQ8jkJV2tm0UeNQGM+BB3fmEzRN+e82f7prbW 5rUy20NChdJc8LYNBO0jpfrabUSLQNSTbToEC6/pcG4CbLvVH6f+wTsmJVWzvcWB fGpHaBIyZ/emKJSorMByxYXFQRgOmN+i8LG3B3nEPM9ylQ0XKF0lfHdQq/AbAFdW PqcH4SEgSlXtP/FbVBydPbBKFZhtbePttfCQOtP6DdazSXUlOF2TztbwOghoWW7T 1FWTnln2Xy23ADls0Axm
=oLsD
-----END PGP SIGNATURE-----