Add support for OpenSSL 1.1.x

From: Emmanuel Deloget <[email protected]...>

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including SSL_CTX. We have to use the defined functions to do so.

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                 |  9 ++++++
 src/openvpn/openssl_compat.h | 74 ++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    | 13 +++++---
 3 files changed, 91 insertions(+), 5 deletions(-)
 create mode 100644 src/openvpn/openssl_compat.h

configure.ac +9 -0

index b29f8b410dfb69bce1145c3bb4a1ba011f0636ec..5fe5d6046ceafa2b577296af772c347ac2ad8039 100644
@@ -898,6 +898,15 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 		[have_crypto_aead_modes="no"; break]
 	)
 
+	AC_CHECK_FUNCS(
+		[ \
+			SSL_CTX_get_default_passwd_cb \
+			SSL_CTX_get_default_passwd_cb_userdata \
+		],
+		,
+		[]
+	)
+
 	CFLAGS="${saved_CFLAGS}"
 	LIBS="${saved_LIBS}"
 

src/openvpn/openssl_compat.h new file +74 -0

new file mode 100644
index 0000000000000000000000000000000000000000..59bad9ff24d10b358419d345181a0e2e52a0c662
@@ -0,0 +1,74 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single TCP/UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2002-2017 OpenVPN Technologies, Inc. <[email protected]...>
+ *  Copyright (C) 2010-2017 Fox Crypto B.V. <[email protected]...>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program (see the file COPYING included with this
+ *  distribution); if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+/**
+ * @file OpenSSL compatibility stub
+ *
+ * This file provide compatibility stubs for the OpenSSL libraries
+ * prior to version 1.1. This version introduces many changes in the
+ * library interface, including the fact that various objects and
+ * structures are not fully opaque.
+ */
+
+#ifndef OPENSSL_COMPAT_H_
+#define OPENSSL_COMPAT_H_
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include <openssl/ssl.h>
+
+#if !defined(HAVE_SSL_CTX_GET_DEFAULT_PASSWD_CB_USERDATA)
+/**
+ * Fetch the default password callback user data from the SSL context
+ *
+ * @param ctx                SSL context
+ * @return                   The password callback user data
+ */
+static inline void *
+SSL_CTX_get_default_passwd_cb_userdata(SSL_CTX *ctx)
+{
+    return ctx ? ctx->default_passwd_callback_userdata : NULL;
+}
+#endif
+
+#if !defined(HAVE_SSL_CTX_GET_DEFAULT_PASSWD_CB)
+/**
+ * Fetch the default password callback from the SSL context
+ *
+ * @param ctx                SSL context
+ * @return                   The password callback
+ */
+static inline pem_password_cb *
+SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx)
+{
+    return ctx ? ctx->default_passwd_callback : NULL;
+}
+#endif
+
+#endif /* OPENSSL_COMPAT_H_ */

src/openvpn/ssl_openssl.c +6 -3

index abf69c91a60910e450ae6d2d49ea7e5b1cd3a535..39e92f8cdae52d54d0ad95a9362e4e0e1b2289f4 100644
@@ -45,6 +45,7 @@
 #include "ssl_backend.h"
 #include "ssl_common.h"
 #include "base64.h"
+#include "openssl_compat.h"
 
 #ifdef ENABLE_CRYPTOAPI
 #include "cryptoapi.h"

@@ -658,7 +659,8 @@ tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,
         {
             for (i = 0; i < sk_X509_num(ca); i++)
             {
-                if (!X509_STORE_add_cert(ctx->ctx->cert_store,sk_X509_value(ca, i)))
+                X509_STORE *cert_store = SSL_CTX_get_cert_store(ctx->ctx);
+                if (!X509_STORE_add_cert(cert_store,sk_X509_value(ca, i)))
                 {
                     crypto_msg(M_FATAL,"Cannot add certificate to certificate chain (X509_STORE_add_cert)");
                 }

@@ -760,8 +762,9 @@ tls_ctx_load_cert_file_and_copy(struct tls_root_ctx *ctx,
         goto end;
     }
 
-    x = PEM_read_bio_X509(in, NULL, ctx->ctx->default_passwd_callback,
-                          ctx->ctx->default_passwd_callback_userdata);
+    x = PEM_read_bio_X509(in, NULL,
+                          SSL_CTX_get_default_passwd_cb(ctx->ctx),
+                          SSL_CTX_get_default_passwd_cb_userdata(ctx->ctx));
     if (x == NULL)
     {
         SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_FILE, ERR_R_PEM_LIB);

From: Emmanuel Deloget <[email protected]...>

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including SSL_CTX. We have to use the defined functions to do so.

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                 |  9 ++++++
 src/openvpn/openssl_compat.h | 74 ++++++++++++++++++++++++++++++++++++++++++++

This patch brings two problems outside the "OpenSSL functionality" part.

- openssl_compat.h is not included in the built tarballs, so mingw builds fail (and "builds for anyone building from tarballs" would break) -> findable by running "make distcheck"

- configure.ac does something to CentOS 6 / RHEL 6 which makes configure explode:

...
checking for linux/if_tun.h... yes
checking tap-windows.h usability... no
checking tap-windows.h presence... no
checking for tap-windows.h... no
checking whether TUNSETPERSIST is declared... yes checking for setcon in -lselinux... yes
checking for pam_start in -lpam... yes
checking for PKCS11_HELPER... no
./configure: line 21440: syntax error near unexpected token fi' ./configure: line 21440: fi'

Hi,

Quoted 6 lines · Gert Doering

This patch brings two problems outside the "OpenSSL functionality" part.

- openssl_compat.h is not included in the built tarballs, so mingw builds fail (and "builds for anyone building from tarballs" would break) -> findable by running "make distcheck"

This has been fixed & pushed (so we can build windows snapshots again).

Quoted 14 lines

- configure.ac does something to CentOS 6 / RHEL 6 which makes configure explode:

...
checking for linux/if_tun.h... yes
checking tap-windows.h usability... no
checking tap-windows.h presence... no
checking for tap-windows.h... no
checking whether TUNSETPERSIST is declared... yes checking for setcon in -lselinux... yes
checking for pam_start in -lpam... yes
checking for PKCS11_HELPER... no
./configure: line 21440: syntax error near unexpected token fi' ./configure: line 21440: fi'

This still needs investigation.

Quoted 15 lines

Quoted 14 lines

- configure.ac does something to CentOS 6 / RHEL 6 which makes configure explode:

...
checking for linux/if_tun.h... yes
checking tap-windows.h usability... no
checking tap-windows.h presence... no
checking for tap-windows.h... no
checking whether TUNSETPERSIST is declared... yes checking for setcon in -lselinux... yes
checking for pam_start in -lpam... yes
checking for PKCS11_HELPER... no
./configure: line 21440: syntax error near unexpected token fi' ./configure: line 21440: fi'

Quoted 1 line

This still needs investigation.

I'm taking the time to install a CentOS 6 on a VM. I'll test this asap.

From: Emmanuel Deloget <[email protected]...>

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including X509_STORE. We have to use the defined functions to do so.

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                     |  1 +
 src/openvpn/openssl_compat.h     | 15 +++++++++++++++
 src/openvpn/ssl_openssl.c        |  7 ++++---
 src/openvpn/ssl_verify_openssl.c |  6 ++++--
 4 files changed, 24 insertions(+), 5 deletions(-)

configure.ac +1 -0

index 5fe5d6046ceafa2b577296af772c347ac2ad8039..415128c9f8687a53e4a73419f3048d07f66b70cc 100644
@@ -902,6 +902,7 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 		[ \
 			SSL_CTX_get_default_passwd_cb \
 			SSL_CTX_get_default_passwd_cb_userdata \
+			X509_STORE_get0_objects \
 		],
 		,
 		[]

src/openvpn/openssl_compat.h +15 -0

index 59bad9ff24d10b358419d345181a0e2e52a0c662..016008bc1705a41ee0ee09fecfc0b16b282cede3 100644
@@ -42,6 +42,7 @@
 #endif
 
 #include <openssl/ssl.h>
+#include <openssl/x509.h>
 
 #if !defined(HAVE_SSL_CTX_GET_DEFAULT_PASSWD_CB_USERDATA)
 /**

@@ -71,4 +72,18 @@ SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx)
 }
 #endif
 
+#if !defined(HAVE_X509_STORE_GET0_OBJECTS)
+/**
+ * Fetch the X509 object stack from the X509 store
+ *
+ * @param store              X509 object store
+ * @return                   the X509 object stack
+ */
+static inline STACK_OF(X509_OBJECT) *
+X509_STORE_get0_objects(X509_STORE *store)
+{
+    return store ? store->objs : NULL;
+}
+#endif
+
 #endif /* OPENSSL_COMPAT_H_ */

src/openvpn/ssl_openssl.c +4 -3

index 39e92f8cdae52d54d0ad95a9362e4e0e1b2289f4..e57de43a748c89ff58ea00abade0b1c317013258 100644
@@ -900,13 +900,14 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
     /* Always start with a cleared CRL list, for that we
      * we need to manually find the CRL object from the stack
      * and remove it */
-    for (int i = 0; i < sk_X509_OBJECT_num(store->objs); i++)
+    STACK_OF(X509_OBJECT) *objs = X509_STORE_get0_objects(store);
+    for (int i = 0; i < sk_X509_OBJECT_num(objs); i++)
     {
-        X509_OBJECT *obj = sk_X509_OBJECT_value(store->objs, i);
+        X509_OBJECT *obj = sk_X509_OBJECT_value(objs, i);
         ASSERT(obj);
         if (obj->type == X509_LU_CRL)
         {
-            sk_X509_OBJECT_delete(store->objs, i);
+            sk_X509_OBJECT_delete(objs, i);
             X509_OBJECT_free_contents(obj);
             OPENSSL_free(obj);
         }

src/openvpn/ssl_verify_openssl.c +1 -0

index 274e2bbf96b6c943ce628eab143f8c76e1c47103..fabbb0c370b123f54ce4a1eaf5f9650b440f47f8 100644
@@ -43,6 +43,7 @@
 #include "ssl_openssl.h"
 #include "ssl_verify.h"
 #include "ssl_verify_backend.h"
+#include "openssl_compat.h"
 
 #include <openssl/x509v3.h>
 #include <openssl/err.h>

@@ -715,9 +716,10 @@ tls_verify_crl_missing(const struct tls_options *opt)
         crypto_msg(M_FATAL, "Cannot get certificate store");
     }

From: Emmanuel Deloget <[email protected]...>

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including X509_OBJECT. We have to use the defined functions to do so.

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                     |  2 ++
 src/openvpn/openssl_compat.h     | 31 +++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c        |  5 ++---
 src/openvpn/ssl_verify_openssl.c |  2 +-
 4 files changed, 36 insertions(+), 4 deletions(-)

configure.ac +2 -0

index 415128c9f8687a53e4a73419f3048d07f66b70cc..789ad08fbaa3b3fc4c95d2b7a22332c0a93aeab4 100644
@@ -903,6 +903,8 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			SSL_CTX_get_default_passwd_cb \
 			SSL_CTX_get_default_passwd_cb_userdata \
 			X509_STORE_get0_objects \
+			X509_OBJECT_free \
+			X509_OBJECT_get_type \
 		],
 		,
 		[]

src/openvpn/openssl_compat.h +31 -0

index 016008bc1705a41ee0ee09fecfc0b16b282cede3..458a6adbe2b3fcd5ea63dcea6596cc24315d463c 100644
@@ -86,4 +86,35 @@ X509_STORE_get0_objects(X509_STORE *store)
 }
 #endif
 
+#if !defined(HAVE_X509_OBJECT_FREE)
+/**
+ * Destroy a X509 object
+ *
+ * @param obj                X509 object
+ */
+static inline void
+X509_OBJECT_free(X509_OBJECT *obj)
+{
+    if (obj)
+    {
+        X509_OBJECT_free_contents(obj);
+        OPENSSL_free(obj);
+    }
+}
+#endif
+
+#if !defined(HAVE_X509_OBJECT_GET_TYPE)
+/**
+ * Get the type of an X509 object
+ *
+ * @param obj                X509 object
+ * @return                   The underlying object type
+ */
+static inline int
+X509_OBJECT_get_type(const X509_OBJECT *obj)
+{
+    return obj ? obj->type : X509_LU_FAIL;
+}
+#endif
+
 #endif /* OPENSSL_COMPAT_H_ */

src/openvpn/ssl_openssl.c +2 -3

index e57de43a748c89ff58ea00abade0b1c317013258..bf0f643f25439f71cbfe71bf5a7e8eb834b0f012 100644
@@ -905,11 +905,10 @@ backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
     {
         X509_OBJECT *obj = sk_X509_OBJECT_value(objs, i);
         ASSERT(obj);
-        if (obj->type == X509_LU_CRL)
+        if (X509_OBJECT_get_type(obj) == X509_LU_CRL)
         {
             sk_X509_OBJECT_delete(objs, i);
-            X509_OBJECT_free_contents(obj);
-            OPENSSL_free(obj);
+            X509_OBJECT_free(obj);
         }
     }
 

src/openvpn/ssl_verify_openssl.c +0 -0

index fabbb0c370b123f54ce4a1eaf5f9650b440f47f8..07975248035b48121d1383b47f40a56042bc7380 100644

From: Emmanuel Deloget <[email protected]...>

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including RSA_METHOD. We have to use the defined functions to do so.

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                 |   9 +++
 src/openvpn/openssl_compat.h | 186 +++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    |  22 ++---
 3 files changed, 206 insertions(+), 11 deletions(-)

configure.ac +9 -0

index 789ad08fbaa3b3fc4c95d2b7a22332c0a93aeab4..6f31609d0aeedd2c7841d271ecadd1aa6f3b11da 100644
@@ -905,6 +905,15 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			X509_STORE_get0_objects \
 			X509_OBJECT_free \
 			X509_OBJECT_get_type \
+			RSA_meth_new \
+			RSA_meth_free \
+			RSA_meth_set_pub_enc \
+			RSA_meth_set_pub_dec \
+			RSA_meth_set_priv_enc \
+			RSA_meth_set_priv_dec \
+			RSA_meth_set_init \
+			RSA_meth_set_finish \
+			RSA_meth_set0_app_data \
 		],
 		,
 		[]

src/openvpn/openssl_compat.h +23 -0

index 458a6adbe2b3fcd5ea63dcea6596cc24315d463c..b1748754f821f472cf9ed7083ade918336c9b075 100644
@@ -41,6 +41,8 @@
 #include "config-msvc.h"
 #endif
 
+#include "buffer.h"
+
 #include <openssl/ssl.h>
 #include <openssl/x509.h>
 

@@ -117,4 +119,188 @@ X509_OBJECT_get_type(const X509_OBJECT *obj)
 }
 #endif
 
+#if !defined(HAVE_RSA_METH_NEW)
+/**
+ * Allocate a new RSA method object
+ *
+ * @param name               The object name
+ * @param flags              Configuration flags
+ * @return                   A new RSA method object
+ */
+static inline RSA_METHOD *
+RSA_meth_new(const char *name, int flags)
+{
+    RSA_METHOD *rsa_meth = NULL;
+    ALLOC_OBJ_CLEAR(rsa_meth, RSA_METHOD);
+    rsa_meth->name = name;
+    rsa_meth->flags = flags;
+    return rsa_meth;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_FREE)
+/**

src/openvpn/openssl_compat.h +153 -0

+
+#if !defined(HAVE_RSA_METH_SET_PUB_ENC)
+/**
+ * Set the public encoding function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param pub_enc            the public encoding function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_pub_enc(RSA_METHOD *meth,
+                     int (*pub_enc) (int flen, const unsigned char *from,
+                                     unsigned char *to, RSA *rsa,
+                                     int padding))
+{
+    if (meth)
+    {
+        meth->rsa_pub_enc = pub_enc;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET_PUB_DEC)
+/**
+ * Set the public decoding function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param pub_dec            the public decoding function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_pub_dec(RSA_METHOD *meth,
+                     int (*pub_dec) (int flen, const unsigned char *from,
+                                     unsigned char *to, RSA *rsa,
+                                     int padding))
+{
+    if (meth)
+    {
+        meth->rsa_pub_dec = pub_dec;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET_PRIV_ENC)
+/**
+ * Set the private encoding function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param priv_enc           the private encoding function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_priv_enc(RSA_METHOD *meth,
+                      int (*priv_enc) (int flen, const unsigned char *from,
+                                       unsigned char *to, RSA *rsa,
+                                       int padding))
+{
+    if (meth)
+    {
+        meth->rsa_priv_enc = priv_enc;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET_PRIV_DEC)
+/**
+ * Set the private decoding function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param priv_dec           the private decoding function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_priv_dec(RSA_METHOD *meth,
+                      int (*priv_dec) (int flen, const unsigned char *from,
+                                       unsigned char *to, RSA *rsa,
+                                       int padding))
+{
+    if (meth)
+    {
+        meth->rsa_priv_dec = priv_dec;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET_INIT)
+/**
+ * Set the init function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param init               the init function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_init(RSA_METHOD *meth, int (*init) (RSA *rsa))
+{
+    if (meth)
+    {
+        meth->init = init;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET_FINISH)
+/**
+ * Set the finish function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param finish             the finish function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_finish(RSA_METHOD *meth, int (*finish) (RSA *rsa))
+{
+    if (meth)
+    {
+        meth->finish = finish;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET0_APP_DATA)
+/**
+ * Set the application data of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param app_data           Application data
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set0_app_data(RSA_METHOD *meth, void *app_data)
+{
+    if (meth)
+    {
+        meth->app_data = app_data;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
 #endif /* OPENSSL_COMPAT_H_ */

src/openvpn/ssl_openssl.c +0 -0

index bf0f643f25439f71cbfe71bf5a7e8eb834b0f012..f011e06702529ff34e91f6d0169d1adf8cc9d767 100644

src/openvpn/ssl_openssl.c +3 -10

@@ -1053,16 +1053,16 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
     ASSERT(NULL != cert);
 
     /* allocate custom RSA method object */
-    ALLOC_OBJ_CLEAR(rsa_meth, RSA_METHOD);
-    rsa_meth->name = "OpenVPN external private key RSA Method";
-    rsa_meth->rsa_pub_enc = rsa_pub_enc;
-    rsa_meth->rsa_pub_dec = rsa_pub_dec;
-    rsa_meth->rsa_priv_enc = rsa_priv_enc;
-    rsa_meth->rsa_priv_dec = rsa_priv_dec;
-    rsa_meth->init = NULL;
-    rsa_meth->finish = rsa_finish;
-    rsa_meth->flags = RSA_METHOD_FLAG_NO_CHECK;
-    rsa_meth->app_data = NULL;
+    rsa_meth = RSA_meth_new("OpenVPN external private key RSA Method",
+                            RSA_METHOD_FLAG_NO_CHECK);
+    check_malloc_return(rsa_meth);

Hi,

Quoted 81 lines · On 17-02-17 23:00, [email protected]... wrote:

From: Emmanuel Deloget <[email protected]...>

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including RSA_METHOD. We have to use the defined functions to do so.

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                 |   9 +++
 src/openvpn/openssl_compat.h | 186 +++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    |  22 ++---
 3 files changed, 206 insertions(+), 11 deletions(-)

configure.ac +9 -0

index 789ad08fbaa3b3fc4c95d2b7a22332c0a93aeab4..6f31609d0aeedd2c7841d271ecadd1aa6f3b11da 100644
@@ -905,6 +905,15 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
                      X509_STORE_get0_objects \
                      X509_OBJECT_free \
                      X509_OBJECT_get_type \
+                     RSA_meth_new \
+                     RSA_meth_free \
+                     RSA_meth_set_pub_enc \
+                     RSA_meth_set_pub_dec \
+                     RSA_meth_set_priv_enc \
+                     RSA_meth_set_priv_dec \
+                     RSA_meth_set_init \
+                     RSA_meth_set_finish \
+                     RSA_meth_set0_app_data \
              ],
              ,
              []

src/openvpn/openssl_compat.h +30 -0

index 458a6adbe2b3fcd5ea63dcea6596cc24315d463c..b1748754f821f472cf9ed7083ade918336c9b075 100644
@@ -41,6 +41,8 @@
 #include "config-msvc.h"
 #endif
 
+#include "buffer.h"
+
 #include <openssl/ssl.h>
 #include <openssl/x509.h>
 

@@ -117,4 +119,188 @@ X509_OBJECT_get_type(const X509_OBJECT *obj)
 }
 #endif
 
+#if !defined(HAVE_RSA_METH_NEW)
+/**
+ * Allocate a new RSA method object
+ *
+ * @param name               The object name
+ * @param flags              Configuration flags
+ * @return                   A new RSA method object
+ */
+static inline RSA_METHOD *
+RSA_meth_new(const char *name, int flags)
+{
+    RSA_METHOD *rsa_meth = NULL;
+    ALLOC_OBJ_CLEAR(rsa_meth, RSA_METHOD);
+    rsa_meth->name = name;
+    rsa_meth->flags = flags;
+    return rsa_meth;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_FREE)
+/**
+ * Free an existing RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ */
+static inline void
+RSA_meth_free(RSA_METHOD *meth)
+{

src/openvpn/openssl_compat.h X509_OBJECT_get_type(const X509_OBJECT *obj) · line 152

Quoted 3 lines

src/openvpn/openssl_compat.h +3 -0

+    free(meth);
+}
+#endif

I think it would be nicer to more closely mimic the 1.1 behaviour in RSA_meth_{new,free}(), and copy the name string in new() and free it again in free(). That could prevent a future use-after-free that would occur for pre-1.1.0, but not 1.1.0+:

  char *mystring = calloc(50, 1);
  RSA_METHOD *meth = RSA_meth_new(mystring, 0);
  free(mystring);

  meth.smoke();
       ^^ might cause problems

(Hint: use string_alloc(x, NULL).)

Quoted 159 lines

src/openvpn/openssl_compat.h +153 -0

+
+#if !defined(HAVE_RSA_METH_SET_PUB_ENC)
+/**
+ * Set the public encoding function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param pub_enc            the public encoding function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_pub_enc(RSA_METHOD *meth,
+                     int (*pub_enc) (int flen, const unsigned char *from,
+                                     unsigned char *to, RSA *rsa,
+                                     int padding))
+{
+    if (meth)
+    {
+        meth->rsa_pub_enc = pub_enc;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET_PUB_DEC)
+/**
+ * Set the public decoding function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param pub_dec            the public decoding function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_pub_dec(RSA_METHOD *meth,
+                     int (*pub_dec) (int flen, const unsigned char *from,
+                                     unsigned char *to, RSA *rsa,
+                                     int padding))
+{
+    if (meth)
+    {
+        meth->rsa_pub_dec = pub_dec;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET_PRIV_ENC)
+/**
+ * Set the private encoding function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param priv_enc           the private encoding function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_priv_enc(RSA_METHOD *meth,
+                      int (*priv_enc) (int flen, const unsigned char *from,
+                                       unsigned char *to, RSA *rsa,
+                                       int padding))
+{
+    if (meth)
+    {
+        meth->rsa_priv_enc = priv_enc;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET_PRIV_DEC)
+/**
+ * Set the private decoding function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param priv_dec           the private decoding function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_priv_dec(RSA_METHOD *meth,
+                      int (*priv_dec) (int flen, const unsigned char *from,
+                                       unsigned char *to, RSA *rsa,
+                                       int padding))
+{
+    if (meth)
+    {
+        meth->rsa_priv_dec = priv_dec;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET_INIT)
+/**
+ * Set the init function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param init               the init function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_init(RSA_METHOD *meth, int (*init) (RSA *rsa))
+{
+    if (meth)
+    {
+        meth->init = init;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET_FINISH)
+/**
+ * Set the finish function of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param finish             the finish function
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set_finish(RSA_METHOD *meth, int (*finish) (RSA *rsa))
+{
+    if (meth)
+    {
+        meth->finish = finish;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
+#if !defined(HAVE_RSA_METH_SET0_APP_DATA)
+/**
+ * Set the application data of an RSA_METHOD object
+ *
+ * @param meth               The RSA_METHOD object
+ * @param app_data           Application data
+ * @return                   1 on success, 0 on error
+ */
+static inline int
+RSA_meth_set0_app_data(RSA_METHOD *meth, void *app_data)
+{
+    if (meth)
+    {
+        meth->app_data = app_data;
+        return 1;
+    }
+    return 0;
+}
+#endif
+
 #endif /* OPENSSL_COMPAT_H_ */

src/openvpn/ssl_openssl.c +0 -0

index bf0f643f25439f71cbfe71bf5a7e8eb834b0f012..f011e06702529ff34e91f6d0169d1adf8cc9d767 100644

src/openvpn/ssl_openssl.c rsa_priv_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i · line 984

Quoted 8 lines

src/openvpn/ssl_openssl.c +1 -1

@@ -978,7 +978,7 @@ rsa_priv_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
 static int
 rsa_finish(RSA *rsa)
 {
-    free((void *)rsa->meth);
+    RSA_meth_free(rsa->meth);
     rsa->meth = NULL;
     return 1;
 }

This change still works, but he follow up change in this method in 07/15 causes problems:

    static int
    rsa_finish(RSA *rsa)
    {
   -    RSA_meth_free(rsa->meth);
   -    rsa->meth = NULL;
   +    RSA_METHOD *meth = (RSA_METHOD *)RSA_get_method(rsa);
   +    RSA_meth_free(meth);
   +    RSA_set_method(rsa, NULL);
        return 1;
    }

Casting away const on the object returned by RSA_get_method() is a smell, but it really fails because RSA_set_method(rsa, NULL) calls rsa->meth->finish(), which is implemented by this very function. That means that RSA_meth_free() will perform a double free on meth->name.

I briefly looked into a fix, but didn't immediately see a nice solution here. At least the RSA_set_method(rsa, RSA_get_default_method()) doesn't work either, because you'll end up with rsa_finish() and RSA_set_method() calling each other infinitely...

(Noting this here, because the fix for 07 might cause changes to this patch too.)

Quoted 25 lines

src/openvpn/ssl_openssl.c +10 -10

@@ -1053,16 +1053,16 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
     ASSERT(NULL != cert);
 
     /* allocate custom RSA method object */
-    ALLOC_OBJ_CLEAR(rsa_meth, RSA_METHOD);
-    rsa_meth->name = "OpenVPN external private key RSA Method";
-    rsa_meth->rsa_pub_enc = rsa_pub_enc;
-    rsa_meth->rsa_pub_dec = rsa_pub_dec;
-    rsa_meth->rsa_priv_enc = rsa_priv_enc;
-    rsa_meth->rsa_priv_dec = rsa_priv_dec;
-    rsa_meth->init = NULL;
-    rsa_meth->finish = rsa_finish;
-    rsa_meth->flags = RSA_METHOD_FLAG_NO_CHECK;
-    rsa_meth->app_data = NULL;
+    rsa_meth = RSA_meth_new("OpenVPN external private key RSA Method",
+                            RSA_METHOD_FLAG_NO_CHECK);
+    check_malloc_return(rsa_meth);
+    RSA_meth_set_pub_enc(rsa_meth, rsa_pub_enc);
+    RSA_meth_set_pub_dec(rsa_meth, rsa_pub_dec);
+    RSA_meth_set_priv_enc(rsa_meth, rsa_priv_enc);
+    RSA_meth_set_priv_dec(rsa_meth, rsa_priv_dec);
+    RSA_meth_set_init(rsa_meth, NULL);
+    RSA_meth_set_finish(rsa_meth, rsa_finish);
+    RSA_meth_set0_app_data(rsa_meth, NULL);
 

src/openvpn/ssl_openssl.c tls_ctx_use_external_private_key(struct tls_root_ctx *ctx, · line 1068

Quoted 3 lines

src/openvpn/ssl_openssl.c +0 -0

     /* allocate RSA object */
     rsa = RSA_new();

-Steffan

From: Emmanuel Deloget <[email protected]...>

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including X509. We have to use the defined functions to do so.

In x509_verify_ns_cert_type() in particular, this means that we cannot directly check for the extended flags to find whether the certificate should be used as a client or as a server certificate. We need to leverage the X509_check_purpose() API yet this API is far stricter than the currently implemented check. So far, I have not been able to find a situation where this stricter test fails (although I must admit that I haven't tested that very well).

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                     |  1 +
 src/openvpn/openssl_compat.h     | 15 +++++++++++++++
 src/openvpn/ssl_openssl.c        |  3 ++-
 src/openvpn/ssl_verify_openssl.c | 28 +++++++++++++++++++---------
 4 files changed, 37 insertions(+), 10 deletions(-)

configure.ac +1 -0

index 6f31609d0aeedd2c7841d271ecadd1aa6f3b11da..c41db3effbb26318be4f44009a5055e808b89b56 100644
@@ -902,6 +902,7 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 		[ \
 			SSL_CTX_get_default_passwd_cb \
 			SSL_CTX_get_default_passwd_cb_userdata \
+			X509_get0_pubkey \
 			X509_STORE_get0_objects \
 			X509_OBJECT_free \
 			X509_OBJECT_get_type \

src/openvpn/openssl_compat.h +15 -0

index b1748754f821f472cf9ed7083ade918336c9b075..6a89b91b05e0370a50ac5a1cae20ae659e6c7634 100644
@@ -74,6 +74,21 @@ SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx)
 }
 #endif
 
+#if !defined(HAVE_X509_GET0_PUBKEY)
+/**
+ * Get the public key from a X509 certificate
+ *
+ * @param x                  X509 certificate
+ * @return                   The certificate public key
+ */
+static inline EVP_PKEY *
+X509_get0_pubkey(const X509 *x)
+{
+    return (x && x->cert_info && x->cert_info->key) ?
+           x->cert_info->key->pkey : NULL;
+}
+#endif
+
 #if !defined(HAVE_X509_STORE_GET0_OBJECTS)
 /**
  * Fetch the X509 object stack from the X509 store

src/openvpn/ssl_openssl.c +2 -1

index f011e06702529ff34e91f6d0169d1adf8cc9d767..b683961d9e4e79b9ee04cfa7ecd1b377ade9651b 100644
@@ -1073,7 +1073,8 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
     }
 
     /* get the public key */
-    ASSERT(cert->cert_info->key->pkey); /* NULL before SSL_CTX_use_certificate() is called */
+    EVP_PKEY *pkey = X509_get0_pubkey(cert);
+    ASSERT(pkey); /* NULL before SSL_CTX_use_certificate() is called */
     pub_rsa = cert->cert_info->key->pkey->pkey.rsa;
 
     /* initialize RSA object */

src/openvpn/ssl_verify_openssl.c +13 -8

index 07975248035b48121d1383b47f40a56042bc7380..edc709b89eb05bca895639dde606b29f8e1f7024 100644
@@ -285,18 +285,20 @@ backend_x509_get_serial_hex(openvpn_x509_cert_t *cert, struct gc_arena *gc)
 struct buffer
 x509_get_sha1_fingerprint(X509 *cert, struct gc_arena *gc)
 {
-    struct buffer hash = alloc_buf_gc(sizeof(cert->sha1_hash), gc);
-    memcpy(BPTR(&hash), cert->sha1_hash, sizeof(cert->sha1_hash));
-    ASSERT(buf_inc_len(&hash, sizeof(cert->sha1_hash)));
+    const EVP_MD *sha1 = EVP_sha1();
+    struct buffer hash = alloc_buf_gc(EVP_MD_size(sha1), gc);
+    X509_digest(cert, EVP_sha1(), BPTR(&hash), NULL);
+    ASSERT(buf_inc_len(&hash, EVP_MD_size(sha1)));
     return hash;
 }
 
 struct buffer
 x509_get_sha256_fingerprint(X509 *cert, struct gc_arena *gc)
 {
-    struct buffer hash = alloc_buf_gc((EVP_sha256())->md_size, gc);
+    const EVP_MD *sha256 = EVP_sha256();
+    struct buffer hash = alloc_buf_gc(EVP_MD_size(sha256), gc);
     X509_digest(cert, EVP_sha256(), BPTR(&hash), NULL);
-    ASSERT(buf_inc_len(&hash, (EVP_sha256())->md_size));
+    ASSERT(buf_inc_len(&hash, EVP_MD_size(sha256)));
     return hash;
 }
 

@@ -573,13 +575,21 @@ x509_verify_ns_cert_type(const openvpn_x509_cert_t *peer_cert, const int usage)
     }
     if (usage == NS_CERT_CHECK_CLIENT)
     {
-        return ((peer_cert->ex_flags & EXFLAG_NSCERT)
-                && (peer_cert->ex_nscert & NS_SSL_CLIENT)) ? SUCCESS : FAILURE;
+        /*
+         * Unfortunately, X509_check_purpose() does some wierd thing that
+         * prevent it to take a const argument
+         */
+        return X509_check_purpose((X509 *)peer_cert, X509_PURPOSE_SSL_CLIENT, 0) ?
+	       SUCCESS : FAILURE;
     }
     if (usage == NS_CERT_CHECK_SERVER)
     {
-        return ((peer_cert->ex_flags & EXFLAG_NSCERT)

So, what I propose instead is:
* remove all the nsCertType code (except the option in add_option())
* update the help strings and man page to indicate that --ns-cert-type
is no longer supported and --remote-cert-tls should be used instead
* in add_option(), if the option is enabled in a config file, act as if
--remote-cert-tls was specified correspondingly, and print a clear warning that --ns-cert-type is no longer supported and stricter checks are enabled instead.

Quoted 8 lines · Steffan Karger

So, what I propose instead is:
* remove all the nsCertType code (except the option in add_option())
* update the help strings and man page to indicate that --ns-cert-type
is no longer supported and --remote-cert-tls should be used instead
* in add_option(), if the option is enabled in a config file, act as if
--remote-cert-tls was specified correspondingly, and print a clear warning that --ns-cert-type is no longer supported and stricter checks are enabled instead.

Mmmmh. Is there a way to get the old behaviour with OpenSSL 1.1?

We decided that we do want 1.1 compatibility in release/2.4, but what you propose might break people's working config when upgrading from 2.4.1 to 2.4.2 - bad enough if we make mistakes, but if there is an alternative to consciously changing cert validation behaviour in the middle of a release train, we should look again...

Hi,

Quoted 10 lines · Gert Doering

Quoted 8 lines · Steffan Karger

So, what I propose instead is:
* remove all the nsCertType code (except the option in add_option())
* update the help strings and man page to indicate that --ns-cert-type
is no longer supported and --remote-cert-tls should be used instead
* in add_option(), if the option is enabled in a config file, act as if
--remote-cert-tls was specified correspondingly, and print a clear warning that --ns-cert-type is no longer supported and stricter checks are enabled instead.

Quoted 7 lines

Mmmmh. Is there a way to get the old behaviour with OpenSSL 1.1?

We decided that we do want 1.1 compatibility in release/2.4, but what you propose might break people's working config when upgrading from 2.4.1 to 2.4.2 - bad enough if we make mistakes, but if there is an alternative to consciously changing cert validation behaviour in the middle of a release train, we should look again...

So I looked again, and there really seems to be no way to get the old behaviour with OpenSSL 1.1. However, the exact behaviour of X509_check_purpose() is not as strict as I initially thought. The current patch basically adds the following checks:
* if the cert has key usage set, it must be correct
* if the cert has extended key usage set, it must be correct
* if the cert has the CA flag set, it must be done correctly

These are fairly low-risk. I'd expect quite some issues if we would reject certs *without* (extended) key usage set, but if (e)ku is set, this will most likely be done correctly. Or in other words, we might reject weird certificates, but not proper certificates.

All in all, I think the original patch (after fixing const correctness) is fine.

As a last resort, we could consider keeping the old code inside #if OSSL_VER < 1.1.0 in release/2.4, but that might just create more confusion...

-Steffan

As a last resort, we could consider keeping the old code inside #if OSSL_VER < 1.1.0 in release/2.4, but that might just create more confusion...

Quoted 3 lines · Steffan Karger

As a last resort, we could consider keeping the old code inside #if OSSL_VER < 1.1.0 in release/2.4, but that might just create more confusion...

Just a very quick thought here ... I do dislike different behaviours depending on which OpenSSL version being used. But given that this feature is already deprecated and even removed in OpenSSL-1.1, I think that gives us some options.

I agree with Gert that breaking --ns-cert-type isn't good at all. However, consider when people upgrade from OpenSSL v1.0 to v1.1 - that is most commonly when there is major distro update. It is not something which happens "mid-term", as OpenSSL is quite commonly used by lots of base system packages these days.

Regardless of OpenSSL version, I agree to loudly deprecate --ns-cert-type, through documentation, --help and log files.

But I think we need to carry the existing behaviour for --ns-cert-type when built against OpenSSL v1.0. And we can solve through some compatibility wrapper when built against OpenSSL v1.1 - with even louder warnings in the logs that this may break apart.

​<snip>

--
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

Hi everyone,

I got some time to try to fix all that stuff.

First,

Quoted 5 lines · David Sommerseth

Quoted 3 lines · Steffan Karger

As a last resort, we could consider keeping the old code inside #if OSSL_VER < 1.1.0 in release/2.4, but that might just create more confusion...

​I think I found a solution -- see later in the email.​

I had a closer look to the corresponding code to see what are the differences between the old check and the new one -- and indeed, the discrepancies might only happen if the tested certificate is kind-of-weird. For exemple, a client certificate:

* has the client bit set
* has no key, or has a key but the key usage is neither for signature nor for key agreement
* is not used to auth a client

​If a user creates a Frankenstein certificate that look like this, it might be a good idea to warn him that such certificate is very likely to be refused in the near future (one can have a similar reasonning about server certificates).

Quoted 12 lines

Just a very quick thought here ... I do dislike different behaviours depending on which OpenSSL version being used. But given that this feature is already deprecated and even removed in OpenSSL-1.1, I think that gives us some options.

I agree with Gert that breaking --ns-cert-type isn't good at all. However, consider when people upgrade from OpenSSL v1.0 to v1.1 - that is most commonly when there is major distro update. It is not something which happens "mid-term", as OpenSSL is quite commonly used by lots of base system packages these days.

Quoted 8 lines

Regardless of OpenSSL version, I agree to loudly deprecate --ns-cert-type, through documentation, --help and log files.

But I think we need to carry the existing behaviour for --ns-cert-type when built against OpenSSL v1.0. And we can solve through some compatibility wrapper when built against OpenSSL v1.1 - with even louder warnings in the logs that this may break apart.

​Fortunately, I think I found a solution that would help when using OpenSSL 1.1. Idealy, this should be a call to X509_check_purpose(.., X509_PURPOSE_SSL_*_WEAK, ...)​ but I don't think the OpenSSL developers will accept such a change :)

It might be possible to use the X509_get_ext_d2i() call like this:

void *ns = X509_get_ext_d2i(x, NID_netscape_cert_type,

                            NULL, NULL);

​In this case, the ​obtained ns object contains the data we're interested in:

* if it exists, then EXFLAG_NSCERT is set
* if not null, ns is of type ASN1_BIT_STRING, and this type is not opaque (yep, sir).

​So the code would look like:

​ASN1_BIT_STRING *ns;
result_t result = FAILURE;
ns = X509_get_ext_d2i(x, NID_netscape_cert_type, NULL, NULL); if (ns)

{
    if (ns->length > 0)
    {
        int flags = ns->data[0];
        if (flags & NS_SSL_CLIENT)
        {
            result = SUCCESS;
        }
    }
    ASN1_BIT_STRING_free(ns);
}

​For the record, it's the code ​that is used within function x509v3_cache_extensions() which builds the X509 flags we are using so I'm failry confident it's the right thing to do (but it's a bit convoluted and I don't like it much).

​Good news: the same code should work with nearly all the previous versions of OpenSSL.

Quoted 9 lines

​<snip>

--
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

I'll post my new patches as soon as I get over every issues that have been talked on the ML (is that even a valid sentence?)

​Best regards,

-- Emmanuel Deloget​

I'm not sure why but it seems this mail (that I send yesterday) never found its way to the ML. So I re-send it.

Sorry for the inconvenience.

I'll post my new patches as soon as I get over every issues that have been talked on the ML (is that even a valid sentence?)

Hi Emmanuel,

Quoted 3 lines · Emmanuel Deloget

I'll post my new patches as soon as I get over every issues that have been talked on the ML (is that even a valid sentence?)

I'm wondering where this got stuck - are you waiting for us to move forward (like, missing review of parts of the patch set), or are we waiting for you, and you've been busy?

We didn't really follow up on this from our end since the CVEs and 2.4.2 got in the way - but I think now would be a good time to move ahead with this...

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including EVP_PKEY. We have to use the defined functions to do so.

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                 |  3 +++
 src/openvpn/openssl_compat.h | 42 ++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    |  6 +++---
 3 files changed, 48 insertions(+), 3 deletions(-)

configure.ac +3 -0

index 9d5e340b..a92e8142 100644
@@ -926,6 +926,9 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			X509_STORE_get0_objects \
 			X509_OBJECT_free \
 			X509_OBJECT_get_type \
+			EVP_PKEY_id \
+			EVP_PKEY_get0_RSA \
+			EVP_PKEY_get0_DSA \
 			RSA_meth_new \
 			RSA_meth_free \
 			RSA_meth_set_pub_enc \

src/openvpn/openssl_compat.h +42 -0

index 29a7588c..0d82cf25 100644
@@ -134,6 +134,48 @@ X509_OBJECT_get_type(const X509_OBJECT *obj)
 }
 #endif
 
+#if !defined(HAVE_EVP_PKEY_GET0_RSA)
+/**
+ * Get the RSA object of a public key
+ *
+ * @param pkey                Public key object
+ * @return                    The underlying RSA object
+ */
+static inline RSA *
+EVP_PKEY_get0_RSA(EVP_PKEY *pkey)
+{
+    return pkey ? pkey->pkey.rsa : NULL;
+}
+#endif
+
+#if !defined(HAVE_EVP_PKEY_ID)
+/**
+ * Get the PKEY type
+ *
+ * @param pkey                Public key object
+ * @return                    The key type
+ */
+static inline int
+EVP_PKEY_id(const EVP_PKEY *pkey)
+{
+    return pkey ? pkey->type : EVP_PKEY_NONE;
+}
+#endif
+
+#if !defined(HAVE_EVP_PKEY_GET0_DSA)
+/**
+ * Get the DSA object of a public key
+ *
+ * @param pkey                Public key object
+ * @return                    The underlying DSA object
+ */
+static inline DSA *
+EVP_PKEY_get0_DSA(EVP_PKEY *pkey)
+{
+    return pkey ? pkey->pkey.dsa : NULL;
+}
+#endif
+
 #if !defined(HAVE_RSA_METH_NEW)
 /**
  * Allocate a new RSA method object

src/openvpn/ssl_openssl.c +2 -2

index a082c3cd..1c73641c 100644
@@ -1072,7 +1072,7 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
     /* get the public key */
     EVP_PKEY *pkey = X509_get0_pubkey(cert);
     ASSERT(pkey); /* NULL before SSL_CTX_use_certificate() is called */
-    pub_rsa = cert->cert_info->key->pkey->pkey.rsa;
+    pub_rsa = EVP_PKEY_get0_RSA(pkey);
 
     /* initialize RSA object */
     rsa->n = BN_dup(pub_rsa->n);

@@ -1677,13 +1677,13 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)
         EVP_PKEY *pkey = X509_get_pubkey(cert);
         if (pkey != NULL)
         {
-            if (pkey->type == EVP_PKEY_RSA && pkey->pkey.rsa != NULL
+            if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including RSA. We have to use the defined functions to do so.

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                 |  3 ++
 src/openvpn/openssl_compat.h | 84 ++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    | 24 ++++++++-----
 3 files changed, 103 insertions(+), 8 deletions(-)

configure.ac +3 -0

index a92e8142..e4c053c8 100644
@@ -929,6 +929,9 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			EVP_PKEY_id \
 			EVP_PKEY_get0_RSA \
 			EVP_PKEY_get0_DSA \
+			RSA_set_flags \
+			RSA_get0_key \
+			RSA_set0_key \
 			RSA_meth_new \
 			RSA_meth_free \
 			RSA_meth_set_pub_enc \

src/openvpn/openssl_compat.h +84 -0

index 0d82cf25..29cd13a4 100644
@@ -176,6 +176,90 @@ EVP_PKEY_get0_DSA(EVP_PKEY *pkey)
 }
 #endif
 
+#if !defined(HAVE_RSA_SET_FLAGS)
+/**
+ * Set the RSA flags
+ *
+ * @param rsa                 The RSA object
+ * @param flags               New flags value
+ */
+static inline void
+RSA_set_flags(RSA *rsa, int flags)
+{
+    if (rsa)
+    {
+        rsa->flags = flags;
+    }
+}
+#endif
+
+#if !defined(HAVE_RSA_GET0_KEY)
+/**
+ * Get the RSA parameters
+ *
+ * @param rsa                 The RSA object
+ * @param n                   The @c n parameter
+ * @param e                   The @c e parameter
+ * @param d                   The @c d parameter
+ */
+static inline void
+RSA_get0_key(const RSA *rsa, const BIGNUM **n,
+             const BIGNUM **e, const BIGNUM **d)
+{
+    if (n != NULL)
+    {
+        *n = rsa ? rsa->n : NULL;
+    }
+    if (e != NULL)
+    {
+        *e = rsa ? rsa->e : NULL;
+    }
+    if (d != NULL)
+    {
+        *d = rsa ? rsa->d : NULL;
+    }
+}
+#endif
+
+#if !defined(HAVE_RSA_SET0_KEY)
+/**
+ * Set the RSA parameters
+ *
+ * @param rsa                 The RSA object
+ * @param n                   The @c n parameter
+ * @param e                   The @c e parameter
+ * @param d                   The @c d parameter
+ * @return                    1 on success, 0 on error
+ */
+static inline int
+RSA_set0_key(RSA *rsa, BIGNUM *n, BIGNUM *e, BIGNUM *d)
+{
+    if ((rsa->n == NULL && n == NULL)
+        || (rsa->e == NULL && e == NULL))
+    {
+        return 0;
+    }
+
+    if (n != NULL)
+    {
+        BN_free(rsa->n);
+        rsa->n = n;
+    }
+    if (e != NULL)
+    {
+        BN_free(rsa->e);
+        rsa->e = e;
+    }
+    if (d != NULL)
+    {
+        BN_free(rsa->d);
+        rsa->d = d;
+    }
+
+    return 1;
+}
+#endif
+
 #if !defined(HAVE_RSA_METH_NEW)
 /**
  * Allocate a new RSA method object

src/openvpn/ssl_openssl.c +0 -0

index 1c73641c..48479c0d 100644

src/openvpn/ssl_openssl.c +6 -4

     return 1;
 }
 

@@ -1075,8 +1075,11 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
     pub_rsa = EVP_PKEY_get0_RSA(pkey);
 
     /* initialize RSA object */
-    rsa->n = BN_dup(pub_rsa->n);
-    rsa->flags |= RSA_FLAG_EXT_PKEY;
+    const BIGNUM *n = NULL;
+    const BIGNUM *e = NULL;
+    RSA_get0_key(pub_rsa, &n, &e, NULL);
+    RSA_set0_key(rsa, BN_dup(n), BN_dup(e), NULL);
+    RSA_set_flags(rsa, RSA_flags(rsa) | RSA_FLAG_EXT_PKEY);
     if (!RSA_set_method(rsa, rsa_meth))
     {
         goto err;

@@ -1677,11 +1680,16 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)
         EVP_PKEY *pkey = X509_get_pubkey(cert);
         if (pkey != NULL)
         {
-            if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL
-                && pkey->pkey.rsa->n != NULL)
+            if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL)
             {

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including DSA. We have to use the defined functions to do so.

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                 |  1 +
 src/openvpn/openssl_compat.h | 28 ++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    | 13 +++++++++----
 3 files changed, 38 insertions(+), 4 deletions(-)

configure.ac +1 -0

index e4c053c8..d2dc1ffd 100644
@@ -932,6 +932,7 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			RSA_set_flags \
 			RSA_get0_key \
 			RSA_set0_key \
+			DSA_get0_pqg \
 			RSA_meth_new \
 			RSA_meth_free \
 			RSA_meth_set_pub_enc \

src/openvpn/openssl_compat.h +28 -0

index 29cd13a4..fdfc4a27 100644
@@ -260,6 +260,34 @@ RSA_set0_key(RSA *rsa, BIGNUM *n, BIGNUM *e, BIGNUM *d)
 }
 #endif
 
+#if !defined(HAVE_DSA_GET0_PQG)
+/**
+ * Get the DSA parameters
+ *
+ * @param dsa                 The DSA object
+ * @param p                   The @c p parameter
+ * @param q                   The @c q parameter
+ * @param g                   The @c g parameter
+ */
+static inline void
+DSA_get0_pqg(const DSA *dsa, const BIGNUM **p,
+             const BIGNUM **q, const BIGNUM **g)
+{
+    if (p != NULL)
+    {
+        *p = dsa ? dsa->p : NULL;
+    }
+    if (q != NULL)
+    {
+        *q = dsa ? dsa->q : NULL;
+    }
+    if (g != NULL)
+    {
+        *g = dsa ? dsa->g : NULL;
+    }
+}
+#endif
+
 #if !defined(HAVE_RSA_METH_NEW)
 /**
  * Allocate a new RSA method object

src/openvpn/ssl_openssl.c +1 -2

index 48479c0d..242ab397 100644
@@ -1691,11 +1691,16 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)
                                      BN_num_bits(n));
                 }
             }
-            else if (EVP_PKEY_id(pkey) == EVP_PKEY_DSA && EVP_PKEY_get0_DSA(pkey) != NULL
-                     && pkey->pkey.dsa->p != NULL)
+            else if (EVP_PKEY_id(pkey) == EVP_PKEY_DSA && EVP_PKEY_get0_DSA(pkey) != NULL)
             {

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including X509. We have to use the defined functions to do so.

In x509_verify_ns_cert_type() in particular, this means that we cannot directly check for the extended flags to find whether the certificate should be used as a client or as a server certificate. We need to leverage the X509_check_purpose() API yet this API is far stricter than the currently implemented check. So far, I have not been able to find a situation where this stricter test fails (although I must admit that I haven't tested that very well).

We double-check the certificate purpose using "direct access" to the internal of the certificate object (of course, this is not a real direct access, but we still fetch ASN1 strings within the X509 object and we check the internal value of these strings). This allow us to warn the user if there is a discrepancy between the X509_check_purpose() return value and our internal, less strict check.

Compatibility with OpenSSL 1.0 is kept by defining the corresponding functions when they are not found in the library.

Signed-off-by Emmanuel Deloget <[email protected]...>

---
 configure.ac                     |  1 +
 src/openvpn/openssl_compat.h     | 15 ++++++++++
 src/openvpn/ssl_openssl.c        |  3 +-
 src/openvpn/ssl_verify_openssl.c | 64 ++++++++++++++++++++++++++++++++++------
 4 files changed, 73 insertions(+), 10 deletions(-)

configure.ac +1 -0

index 7d3fce5b..9d5e340b 100644
@@ -922,6 +922,7 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 		[ \
 			SSL_CTX_get_default_passwd_cb \
 			SSL_CTX_get_default_passwd_cb_userdata \
+			X509_get0_pubkey \
 			X509_STORE_get0_objects \
 			X509_OBJECT_free \
 			X509_OBJECT_get_type \

src/openvpn/openssl_compat.h +15 -0

index 92f014d5..29a7588c 100644
@@ -74,6 +74,21 @@ SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx)
 }
 #endif
 
+#if !defined(HAVE_X509_GET0_PUBKEY)
+/**
+ * Get the public key from a X509 certificate
+ *
+ * @param x                  X509 certificate
+ * @return                   The certificate public key
+ */
+static inline EVP_PKEY *
+X509_get0_pubkey(const X509 *x)
+{
+    return (x && x->cert_info && x->cert_info->key) ?
+           x->cert_info->key->pkey : NULL;
+}
+#endif
+
 #if !defined(HAVE_X509_STORE_GET0_OBJECTS)
 /**
  * Fetch the X509 object stack from the X509 store

src/openvpn/ssl_openssl.c +2 -1

index 645ccf51..a082c3cd 100644
@@ -1070,7 +1070,8 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
     }
 
     /* get the public key */
-    ASSERT(cert->cert_info->key->pkey); /* NULL before SSL_CTX_use_certificate() is called */
+    EVP_PKEY *pkey = X509_get0_pubkey(cert);
+    ASSERT(pkey); /* NULL before SSL_CTX_use_certificate() is called */
     pub_rsa = cert->cert_info->key->pkey->pkey.rsa;
 
     /* initialize RSA object */

src/openvpn/ssl_verify_openssl.c +7 -5

index 9b1533bc..4785f314 100644
@@ -294,18 +294,20 @@ backend_x509_get_serial_hex(openvpn_x509_cert_t *cert, struct gc_arena *gc)
 struct buffer
 x509_get_sha1_fingerprint(X509 *cert, struct gc_arena *gc)
 {
-    struct buffer hash = alloc_buf_gc(sizeof(cert->sha1_hash), gc);
-    memcpy(BPTR(&hash), cert->sha1_hash, sizeof(cert->sha1_hash));
-    ASSERT(buf_inc_len(&hash, sizeof(cert->sha1_hash)));
+    const EVP_MD *sha1 = EVP_sha1();
+    struct buffer hash = alloc_buf_gc(EVP_MD_size(sha1), gc);
+    X509_digest(cert, EVP_sha1(), BPTR(&hash), NULL);
+    ASSERT(buf_inc_len(&hash, EVP_MD_size(sha1)));
     return hash;
 }
 
 struct buffer
 x509_get_sha256_fingerprint(X509 *cert, struct gc_arena *gc)
 {
-    struct buffer hash = alloc_buf_gc((EVP_sha256())->md_size, gc);
+    const EVP_MD *sha256 = EVP_sha256();
+    struct buffer hash = alloc_buf_gc(EVP_MD_size(sha256), gc);
     X509_digest(cert, EVP_sha256(), BPTR(&hash), NULL);
-    ASSERT(buf_inc_len(&hash, (EVP_sha256())->md_size));
+    ASSERT(buf_inc_len(&hash, EVP_MD_size(sha256)));
     return hash;
 }
 

@@ -578,13 +580,57 @@ x509_verify_ns_cert_type(const openvpn_x509_cert_t *peer_cert, const int usage)
     }

src/openvpn/ssl_verify_openssl.c +5 -0

+        /*
+         * old versions of OpenSSL allow us to make the less strict check we used to
+         * do. If this less strict check pass, warn user that this might not be the
+         * case when its distribution will update to OpenSSL 1.1
+         */

src/openvpn/ssl_verify_openssl.c +19 -2

+        }
+        return result;
     }
     if (usage == NS_CERT_CHECK_SERVER)
     {
-        return ((peer_cert->ex_flags & EXFLAG_NSCERT)
-                && (peer_cert->ex_nscert & NS_SSL_SERVER))  ? SUCCESS : FAILURE;
+        /*
+         * Unfortunately, X509_check_purpose() does some wierd thing that
+         * prevent it to take a const argument
+         */
+        result_t result = X509_check_purpose((X509 *)peer_cert, X509_PURPOSE_SSL_SERVER, 0) ?
+	       SUCCESS : FAILURE;
+
+        /*
+         * old versions of OpenSSL allow us to make the less strict check we used to
+         * do. If this less strict check pass, warn user that this might not be the
+         * case when its distribution will update to OpenSSL 1.1
+         */
+        if (result == FAILURE)
+        {
+            ASN1_BIT_STRING *ns;
+            ns = X509_get_ext_d2i((X509 *)peer_cert, NID_netscape_cert_type, NULL, NULL);
+            result = (ns && ns->length > 0 && (ns->data[0] & NS_SSL_SERVER)) ? SUCCESS : FAILURE;

Quoted 3 lines

I'm wondering where this got stuck - are you waiting for us to move forward (like, missing review of parts of the patch set), or are we waiting for you, and you've been busy?

Problem is that I'm working in a more-than-full-time manner on way-too-many-other subjects :)

Hi,

Quoted 6 lines · Emmanuel Deloget

Quoted 3 lines

I'm wondering where this got stuck - are you waiting for us to move forward (like, missing review of parts of the patch set), or are we waiting for you, and you've been busy?

Problem is that I'm working in a more-than-full-time manner on way-too-many-other subjects :)

Thanks for quickly sending us the current patch set. Now it's on us again.

(And yes, we fully understand the "too many projects, too many complaining customers, angry wife as well, and too little time" thing :) )

Over to Steffan now - I'm so happy that I do not have to understand OpenSSL code... ;-)

gert

Quoted 10 lines · Gert Doering

Quoted 6 lines · Emmanuel Deloget

Quoted 3 lines

I'm wondering where this got stuck - are you waiting for us to move forward (like, missing review of parts of the patch set), or are we waiting for you, and you've been busy?

Problem is that I'm working in a more-than-full-time manner on way-too-many-other subjects :)

Thanks for quickly sending us the current patch set. Now it's on us again.

If there is anything bad, please tell me - I should be able to do another round if needed :)

Quoted 2 lines

(And yes, we fully understand the "too many projects, too many complaining customers, angry wife as well, and too little time" thing :) )

Yeah. It seems that "somebody" limited the day length to a small value that fit on a 5 bit word. I'm wondering if this "person" was told about uint32_t...

Quoted 2 lines

Over to Steffan now - I'm so happy that I do not have to understand OpenSSL code... ;-)

Good thing is : there is nothing complex in the changes I made. I'm pretty sure you'll be able to understand both the changes and what the original code does :)