[Openvpn-devel] Fix OpenSSL 1.1.1 not using auto ecliptic curve selection

Message ID 20200328040858.16505-1-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel] Fix OpenSSL 1.1.1 not using auto ecliptic curve selection | expand

Commit Message

Arne Schwabe March 27, 2020, 5:08 p.m. UTC
Commit 8a01147ff attempted to avoid calling the deprecated/noop
operation SSL_CTX_set_ecdh_auto by surrounding it with #ifdef.
Unfortunately, that change also made the return; that would exit
the function no longer being compiled when using OpenSSL 1.1.0+.
As consequence OpenVPN with OpenSSL 1.1.0+ would always set
secp384r1 as ecdh curve unless otherwise specified by ecdh

This patch restores the correct/previous behaviour.
---
 src/openvpn/ssl_openssl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Antonio Quartulli April 14, 2020, 11:50 p.m. UTC | #1
Hi,

On 28/03/2020 05:08, Arne Schwabe wrote:
> Commit 8a01147ff attempted to avoid calling the deprecated/noop
> operation SSL_CTX_set_ecdh_auto by surrounding it with #ifdef.
> Unfortunately, that change also made the return; that would exit
> the function no longer being compiled when using OpenSSL 1.1.0+.
> As consequence OpenVPN with OpenSSL 1.1.0+ would always set
> secp384r1 as ecdh curve unless otherwise specified by ecdh
> 
> This patch restores the correct/previous behaviour.
> ---
>  src/openvpn/ssl_openssl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 3f0031ff..4b5ca214 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -678,8 +678,11 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
>          /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter
>           * loading */
>          SSL_CTX_set_ecdh_auto(ctx->ctx, 1);
> -        return;
> +
> +        /* OpenSSL 1.1.0 and newer have always ecdh auto loading enabled,
> +         * so do nothing */
>  #endif
> +        return;

my eyes want to fall when seeing this ifdef jungle...but that's another
topic.

The change makes sense, because for ossl >= 1.1.0 we only want to omit
the call to SSL_CTX_set_ecdh_auto() [no-op since 1.1.0], but the
codeflow should remain the same.

>  #else
>          /* For older OpenSSL we have to extract the curve from key on our own */
>          EC_KEY *eckey = NULL;
> 


Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering April 15, 2020, 12:15 a.m. UTC | #2
Your patch has been applied to the master and release/2.4 branch (bugfix).

I've changed the title from "ecliptic" to "elliptic" curves, though :)

Haven't tested, but have stared at the actual change and the surrounding
code a bit, and hope that we can remove 1.0.x support soon... :-)

commit d8ac887c6b1b57a1953ab62058b4aed5d8c11f65 (master)
commit 5ee76a8fab0411c7529c8da9f40ad386433d9a0c (release/2.4)
Author: Arne Schwabe
Date:   Sat Mar 28 05:08:58 2020 +0100

     Fix OpenSSL 1.1.1 not using auto ecliptic curve selection

     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20200328040858.16505-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19630.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 3f0031ff..4b5ca214 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -678,8 +678,11 @@  tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
         /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter
          * loading */
         SSL_CTX_set_ecdh_auto(ctx->ctx, 1);
-        return;
+
+        /* OpenSSL 1.1.0 and newer have always ecdh auto loading enabled,
+         * so do nothing */
 #endif
+        return;
 #else
         /* For older OpenSSL we have to extract the curve from key on our own */
         EC_KEY *eckey = NULL;