[Openvpn-devel,PATCHv2] openssl: Fix compilation without deprecated OpenSSL 1.1 APIs

Message ID 20190403225657.8003-1-rosenp@gmail.com
State Superseded
Delegated to: Arne Schwabe
Headers show
Series
  • [Openvpn-devel,PATCHv2] openssl: Fix compilation without deprecated OpenSSL 1.1 APIs
Related show

Commit Message

Rosen Penev April 3, 2019, 10:56 p.m.
EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were
replaced with _reset.

Also removed initialization with OpenSSL 1.1 as it is no longer needed and
causes compilation errors when disabling deprecated APIs.

Same with SSL_CTX_set_ecdh_auto as it got removed.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: Squashed previous patches together.
 configure.ac                 |  4 ++++
 src/openvpn/openssl_compat.h | 16 ++++++++++++++++
 src/openvpn/ssl_openssl.c    | 18 ++++++++++++------
 3 files changed, 32 insertions(+), 6 deletions(-)

Comments

Arne Schwabe May 8, 2019, 2:14 p.m. | #1
Am 04.04.19 um 00:56 schrieb Rosen Penev:
> EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were
> replaced with _reset.
> 
> Also removed initialization with OpenSSL 1.1 as it is no longer needed and
> causes compilation errors when disabling deprecated APIs.
> 
> Same with SSL_CTX_set_ecdh_auto as it got removed.

> +#if !defined(HAVE_EVP_CIPHER_CTX_INIT)
> +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset
> +#endif
> +
> +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP)
> +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset
> +#endif
> +

Generally we tried to avoid things like implementing the old API with
the new API but instead try to only use the current OpenSSL API and
implement it using the older API on older OpenSSL version. Is there way
for these functions to do that as well?

Arne
Arne Schwabe June 14, 2019, 10:38 a.m. | #2
Am 04.04.19 um 00:56 schrieb Rosen Penev:
> EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were
> replaced with _reset.
> 
> Also removed initialization with OpenSSL 1.1 as it is no longer needed and
> causes compilation errors when disabling deprecated APIs.
> 
> Same with SSL_CTX_set_ecdh_auto as it got removed.
> 

This gets kind of an ACK but needs some additional changes to be really
good.


>  
> +#if !defined(HAVE_EVP_CIPHER_CTX_INIT)
> +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset
> +#endif
> +
> +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP)
> +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset
> +#endif

These two keep the older API instead of switching to the new one, from
OpenSSL.

# if OPENSSL_API_COMPAT < 0x10100000L
#  define EVP_CIPHER_CTX_init(c)      EVP_CIPHER_CTX_reset(c)
#  define EVP_CIPHER_CTX_cleanup(c)   EVP_CIPHER_CTX_reset(c)
# endif

Since just using only the new API in this case does not really work I
think in case it would be better to rather always use
EVP_CIPHER_CTX_reset isntead of init and  have ifdefs in the 2-3 places
where we actually use EVP_CIPHER_CTX_cleanup so we can remove the old
API when we bump our minimum OpenSSL version (and find this thing easy
since it is an ifdef depending on the openssl version).

> +
> +#if !defined(HAVE_X509_GET0_NOTBEFORE)
> +#define X509_get0_notBefore X509_get_notBefore
> +#endif
> +
> +#if !defined(HAVE_X509_GET0_NOTAFTER)
> +#define X509_get0_notAfter X509_get_notAfter
> +#endif
> +
>  #if !defined(HAVE_HMAC_CTX_RESET)
>  /**
>   * Reset a HMAC context
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 8bcebac4..e41cafa5 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -76,12 +76,13 @@ int mydata_index; /* GLOBAL */
>  void
>  tls_init_lib(void)
>  {
> +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
>      SSL_library_init();
> -#ifndef ENABLE_SMALL
> +# ifndef ENABLE_SMALL

The space between # and ifndef looks wrong.


Arne
Matthias Andree July 12, 2019, 4:37 p.m. | #3
Am 14.06.19 um 12:38 schrieb Arne Schwabe:
>
>> -#ifndef ENABLE_SMALL
>> +# ifndef ENABLE_SMALL
> The space between # and ifndef looks wrong.

It's standard C. (Chapter 3.8 in the 1989/1990 standard, chapter 6.10 in
recent editions, I checked 1999 and 2017/2018, although worded in a
quite convoluted way) and I have yet to see a relevant C compiler that
would reject this.

The # is a token by itself can can be separated from the other
preprocessing tokens on the same line by space and horizontal-tab.
Arne Schwabe July 12, 2019, 4:44 p.m. | #4
Am 12.07.19 um 18:37 schrieb Matthias Andree:
> Am 14.06.19 um 12:38 schrieb Arne Schwabe:
>>
>>> -#ifndef ENABLE_SMALL
>>> +# ifndef ENABLE_SMALL
>> The space between # and ifndef looks wrong.
> 
> It's standard C. (Chapter 3.8 in the 1989/1990 standard, chapter 6.10 in
> recent editions, I checked 1999 and 2017/2018, although worded in a
> quite convoluted way) and I have yet to see a relevant C compiler that
> would reject this.

I know :).

Looks "wrong" in the sense that it does not match the style we want to
have in openvpn.
Gert Doering July 12, 2019, 4:48 p.m. | #5
Hi,

On Fri, Jul 12, 2019 at 06:44:06PM +0200, Arne Schwabe wrote:
> Looks "wrong" in the sense that it does not match the style we want to
> have in openvpn.

Indeed.  I have seen and used "#   cpp-thing" in other projects, but
OpenVPN seems to have decided to not use space-indenting for nested
#if-things.

So, we'll keep this (and try to get rid of #ifdefs anyway :-) ).

gert
Rosen Penev July 12, 2019, 7:22 p.m. | #6
On Fri, Jun 14, 2019 at 3:38 AM Arne Schwabe <arne@rfc2549.org> wrote:
>
> Am 04.04.19 um 00:56 schrieb Rosen Penev:
> > EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were
> > replaced with _reset.
> >
> > Also removed initialization with OpenSSL 1.1 as it is no longer needed and
> > causes compilation errors when disabling deprecated APIs.
> >
> > Same with SSL_CTX_set_ecdh_auto as it got removed.
> >
>
> This gets kind of an ACK but needs some additional changes to be really
> good.
>
>
> >
> > +#if !defined(HAVE_EVP_CIPHER_CTX_INIT)
> > +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset
> > +#endif
> > +
> > +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP)
> > +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset
> > +#endif
>
> These two keep the older API instead of switching to the new one, from
> OpenSSL.
Yes I know. I feel that it's a cleaner solution as _init and _cleanup
both get defined to _reset.
>
> # if OPENSSL_API_COMPAT < 0x10100000L
> #  define EVP_CIPHER_CTX_init(c)      EVP_CIPHER_CTX_reset(c)
> #  define EVP_CIPHER_CTX_cleanup(c)   EVP_CIPHER_CTX_reset(c)
> # endif
>
> Since just using only the new API in this case does not really work I
> think in case it would be better to rather always use
> EVP_CIPHER_CTX_reset isntead of init and  have ifdefs in the 2-3 places
> where we actually use EVP_CIPHER_CTX_cleanup so we can remove the old
> API when we bump our minimum OpenSSL version (and find this thing easy
> since it is an ifdef depending on the openssl version).
OK. Will change.
>
> > +
> > +#if !defined(HAVE_X509_GET0_NOTBEFORE)
> > +#define X509_get0_notBefore X509_get_notBefore
> > +#endif
> > +
> > +#if !defined(HAVE_X509_GET0_NOTAFTER)
> > +#define X509_get0_notAfter X509_get_notAfter
> > +#endif
> > +
> >  #if !defined(HAVE_HMAC_CTX_RESET)
> >  /**
> >   * Reset a HMAC context
> > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> > index 8bcebac4..e41cafa5 100644
> > --- a/src/openvpn/ssl_openssl.c
> > +++ b/src/openvpn/ssl_openssl.c
> > @@ -76,12 +76,13 @@ int mydata_index; /* GLOBAL */
> >  void
> >  tls_init_lib(void)
> >  {
> > +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
> >      SSL_library_init();
> > -#ifndef ENABLE_SMALL
> > +# ifndef ENABLE_SMALL
>
> The space between # and ifndef looks wrong.
Will eliminate. Not sure exactly why I added the space ATM.
>
>
> Arne
>
Steffan Karger July 24, 2019, 10:28 a.m. | #7
Hi all,

On 14-06-19 12:38, Arne Schwabe wrote:
>> +#if !defined(HAVE_EVP_CIPHER_CTX_INIT)
>> +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset
>> +#endif
>> +
>> +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP)
>> +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset
>> +#endif
> 
> These two keep the older API instead of switching to the new one, from
> OpenSSL.
> 
> # if OPENSSL_API_COMPAT < 0x10100000L
> #  define EVP_CIPHER_CTX_init(c)      EVP_CIPHER_CTX_reset(c)
> #  define EVP_CIPHER_CTX_cleanup(c)   EVP_CIPHER_CTX_reset(c)
> # endif
> 
> Since just using only the new API in this case does not really work I
> think in case it would be better to rather always use
> EVP_CIPHER_CTX_reset isntead of init and  have ifdefs in the 2-3 places
> where we actually use EVP_CIPHER_CTX_cleanup so we can remove the old
> API when we bump our minimum OpenSSL version (and find this thing easy
> since it is an ifdef depending on the openssl version).

Why wouldn't using the new API work? _reset() is basically the new name
for _cleanup(), which some some actual cleanup + init.

As far as I can see it would work perfectly fine to use _reset() instead
of the _init/_cleanup calls everywhere. We never call _init on
uninitialized memory (which is the only case where _init() would work
while _cleanup() would fail).

All we'd have to do than is add something like

#ifndef HAVE_EVP_CIPHER_CTX_RESET
#define EVP_CIPHER_CTX_reset EVP_CIPHER_CTX_cleanup
#endif

to openssl_compat.h.

That way we would just use the new API everywhere, and can get rid of
the lines in the compat file once we drop support for OpenSSL 1.0.

Or am I missing something here?

-Steffan
Arne Schwabe July 24, 2019, 11:14 a.m. | #8
Am 24.07.19 um 12:28 schrieb Steffan Karger:
> Hi all,
> 
> On 14-06-19 12:38, Arne Schwabe wrote:
>>> +#if !defined(HAVE_EVP_CIPHER_CTX_INIT)
>>> +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset
>>> +#endif
>>> +
>>> +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP)
>>> +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset
>>> +#endif
>>
>> These two keep the older API instead of switching to the new one, from
>> OpenSSL.
>>
>> # if OPENSSL_API_COMPAT < 0x10100000L
>> #  define EVP_CIPHER_CTX_init(c)      EVP_CIPHER_CTX_reset(c)
>> #  define EVP_CIPHER_CTX_cleanup(c)   EVP_CIPHER_CTX_reset(c)
>> # endif
>>
>> Since just using only the new API in this case does not really work I
>> think in case it would be better to rather always use
>> EVP_CIPHER_CTX_reset isntead of init and  have ifdefs in the 2-3 places
>> where we actually use EVP_CIPHER_CTX_cleanup so we can remove the old
>> API when we bump our minimum OpenSSL version (and find this thing easy
>> since it is an ifdef depending on the openssl version).

Okay, I double checked this.


The new API drops EVP_CIPHER_CTX_cleanup, so we have do either ifdef the
EVP_CIPHER_CTX_cleanup calls or remove them completely.

Currently we only call the EVP_CIPHER_CTX_cleanup directly before
EVP_CIPHER_CTX_free.

Looking at the 1.0.2 openssl code:


void EVP_CIPHER_CTX_free(EVP_CIPHER_CTX *ctx)
{
   if (ctx) {
       EVP_CIPHER_CTX_cleanup(ctx);
      OPENSSL_free(ctx);
   }
}

it looks like the EVP_CIPHER_CTX_cleanup is implicit already, so we can
remove those call and drop the cipher_ctx_cleanup function and combine
cipher_ctx_cleanup and _free into one for mbed TLS.

Does that sound reasonble?

Arne

Patch

diff --git a/configure.ac b/configure.ac
index dfb268ca..891799ea 100644
--- a/configure.ac
+++ b/configure.ac
@@ -918,10 +918,14 @@  if test "${with_crypto_library}" = "openssl"; then
 			EVP_MD_CTX_new \
 			EVP_MD_CTX_free \
 			EVP_MD_CTX_reset \
+			EVP_CIPHER_CTX_init \
+			EVP_CIPHER_CTX_cleanup \
 			OpenSSL_version \
 			SSL_CTX_get_default_passwd_cb \
 			SSL_CTX_get_default_passwd_cb_userdata \
 			SSL_CTX_set_security_level \
+			X509_get0_notBefore \
+			X509_get0_notAfter \
 			X509_get0_pubkey \
 			X509_STORE_get0_objects \
 			X509_OBJECT_free \
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index a4072b9a..2453b85e 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -89,6 +89,22 @@  EVP_MD_CTX_new(void)
 }
 #endif
 
+#if !defined(HAVE_EVP_CIPHER_CTX_INIT)
+#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset
+#endif
+
+#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP)
+#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset
+#endif
+
+#if !defined(HAVE_X509_GET0_NOTBEFORE)
+#define X509_get0_notBefore X509_get_notBefore
+#endif
+
+#if !defined(HAVE_X509_GET0_NOTAFTER)
+#define X509_get0_notAfter X509_get_notAfter
+#endif
+
 #if !defined(HAVE_HMAC_CTX_RESET)
 /**
  * Reset a HMAC context
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 8bcebac4..e41cafa5 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -76,12 +76,13 @@  int mydata_index; /* GLOBAL */
 void
 tls_init_lib(void)
 {
+#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
     SSL_library_init();
-#ifndef ENABLE_SMALL
+# ifndef ENABLE_SMALL
     SSL_load_error_strings();
-#endif
+# endif
     OpenSSL_add_all_algorithms();
-
+#endif
     mydata_index = SSL_get_ex_new_index(0, "struct session *", NULL, NULL, NULL);
     ASSERT(mydata_index >= 0);
 }
@@ -89,9 +90,11 @@  tls_init_lib(void)
 void
 tls_free_lib(void)
 {
+#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
     EVP_cleanup();
-#ifndef ENABLE_SMALL
+# ifndef ENABLE_SMALL
     ERR_free_strings();
+# endif
 #endif
 }
 
@@ -541,7 +544,7 @@  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
         goto cleanup; /* Nothing to check if there is no certificate */
     }
 
-    ret = X509_cmp_time(X509_get_notBefore(cert), NULL);
+    ret = X509_cmp_time(X509_get0_notBefore(cert), NULL);
     if (ret == 0)
     {
         msg(D_TLS_DEBUG_MED, "Failed to read certificate notBefore field.");
@@ -551,7 +554,7 @@  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
         msg(M_WARN, "WARNING: Your certificate is not yet valid!");
     }
 
-    ret = X509_cmp_time(X509_get_notAfter(cert), NULL);
+    ret = X509_cmp_time(X509_get0_notAfter(cert), NULL);
     if (ret == 0)
     {
         msg(D_TLS_DEBUG_MED, "Failed to read certificate notAfter field.");
@@ -634,10 +637,13 @@  tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
     else
     {
 #if OPENSSL_VERSION_NUMBER >= 0x10002000L
+#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
+
         /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter
          * loading */
         SSL_CTX_set_ecdh_auto(ctx->ctx, 1);
         return;
+#endif
 #else
         /* For older OpenSSL we have to extract the curve from key on our own */
         EC_KEY *eckey = NULL;