[Openvpn-devel] openssl: Replace not[Before/After] functions with get0 variants

Message ID 20190327215626.23502-1-rosenp@gmail.com
State New
Delegated to: Arne Schwabe
Headers show
Series
  • [Openvpn-devel] openssl: Replace not[Before/After] functions with get0 variants
Related show

Commit Message

Rosen Penev March 27, 2019, 9:56 p.m.
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>
---
 configure.ac                 |  2 ++
 src/openvpn/openssl_compat.h |  8 ++++++++
 src/openvpn/ssl_openssl.c    | 18 ++++++++++++------
 3 files changed, 22 insertions(+), 6 deletions(-)

Comments

Gert Doering March 28, 2019, 7:51 a.m. | #1
Hi,

On Wed, Mar 27, 2019 at 02:56:26PM -0700, Rosen Penev wrote:
> 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.

The Subject: line is a bit misleading - shouldn't this be more something
like "Avoid calling deprecated-API calls of OpenSSL 1.1" or similar?

(People *do* look at commit logs, and the current subject does not cover
making the SSL_library_init() call conditional, etc.)

On the patch itself, I have no strong opinion and would welcome a review
from Steffan or Arne :-) - the Subject: line I can fix at commit time.

gert
Arne Schwabe March 28, 2019, 8:45 a.m. | #2
> diff --git a/configure.ac b/configure.ac
> index dfb268ca..2617f344 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -922,6 +922,8 @@ if test "${with_crypto_library}" = "openssl"; then
>  			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..788843a2 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -89,6 +89,14 @@ EVP_MD_CTX_new(void)
>  }
>  #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
> +


This is fine.


>  #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


Please add a comment like
/* On OpenSSL 1.1.0 or above, then the library will initialize itself
automatically. */
Otherwise people will be very confused why this code is that way.


>      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
>  }

Same as above.

>  
> @@ -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;
> 


In general it be better split this patch into two: renaming the get/set
methods and removing the initialisation from OpenSSL >=1.1.0

Arne
Rosen Penev March 28, 2019, 9:36 a.m. | #3
On Thu, Mar 28, 2019 at 12:51 AM Gert Doering <gert@greenie.muc.de> wrote:
>
> Hi,
>
> On Wed, Mar 27, 2019 at 02:56:26PM -0700, Rosen Penev wrote:
> > 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.
>
> The Subject: line is a bit misleading - shouldn't this be more something
> like "Avoid calling deprecated-API calls of OpenSSL 1.1" or similar?
Yeah. The patch doesn't fully fix it though. I ended up with a
somewhat workable solution though.

https://github.com/neheb/openvpn/commit/945f190a1bfbde3d6bf11f5b576f5c9e5ec1b0f3

I can squash both of these so that they get the same Subject line.
>
> (People *do* look at commit logs, and the current subject does not cover
> making the SSL_library_init() call conditional, etc.)
>
> On the patch itself, I have no strong opinion and would welcome a review
> from Steffan or Arne :-) - the Subject: line I can fix at commit time.
>
> gert
> --
> "If was one thing all people took for granted, was conviction that if you
>  feed honest figures into a computer, honest figures come out. Never doubted
>  it myself till I met a computer with a sense of humor."
>                              Robert A. Heinlein, The Moon is a Harsh Mistress
>
> Gert Doering - Munich, Germany                             gert@greenie.muc.de

Patch

diff --git a/configure.ac b/configure.ac
index dfb268ca..2617f344 100644
--- a/configure.ac
+++ b/configure.ac
@@ -922,6 +922,8 @@  if test "${with_crypto_library}" = "openssl"; then
 			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..788843a2 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -89,6 +89,14 @@  EVP_MD_CTX_new(void)
 }
 #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;