Skip to contentSkip to conversation

OpenSSL: support for version 1.1

PATCH In review [email protected]... 4 participants 29 messages Jun 12, 2017 – Jun 19, 2017 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]...

#1 lore raw

v7 cover OpenSSL: support for version 1.1

This is version 7 of the OpenSSL 1.1 patch series. It has been rebased on top of the latest git revision.

It builds upon the previous version and adds the following:

* fix a memory leak when playing with internal ASN1 strings
* introduce RSA_bits() and DSA_bits()
* add a few comments to explain that, yes, we are allowed to do weird const-to-non-const cast because of OpenSSL API. Of course, it would be better if we could have avoided them.

This version fixes all the problems that Steffan spotted on the previous version (to the best of my knowledge).

Please tell me if there is anything wrong (or not good enough).

[PATCH 1/8] OpenSSL: don't use direct access to the internal of X509 [PATCH 2/8] OpenSSL: don't use direct access to the internal of EVP_PKEY [PATCH 3/8] OpenSSL: don't use direct access to the internal of RSA [PATCH 4/8] OpenSSL: don't use direct access to the internal of DSA [PATCH 5/8] OpenSSL: don't use direct access to the internal of EVP_MD_CTX [PATCH 6/8] OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX [PATCH 7/8] OpenSSL: don't use direct access to the internal of HMAC_CTX [PATCH 8/8] OpenSSL: force meth->name as non-const when we free() it

Diffstat is here:

 configure.ac                     |  19 +++
 src/openvpn/crypto.c             |   8 +-
 src/openvpn/crypto_backend.h     |  42 +++++
 src/openvpn/crypto_mbedtls.c     |  40 +++++
 src/openvpn/crypto_openssl.c     |  50 +++++-
 src/openvpn/httpdigest.c         |  78 ++++-----
 src/openvpn/misc.c               |  14 +-
 src/openvpn/ntlm.c               |  12 +-
 src/openvpn/openssl_compat.h     | 351 ++++++++++++++++++++++++++++++++++++++-
 src/openvpn/openvpn.h            |   2 +-
 src/openvpn/push.c               |  11 +-
 src/openvpn/ssl.c                |  38 +++--
 src/openvpn/ssl_openssl.c        |  35 ++--
 src/openvpn/ssl_verify_backend.h |   2 +-
 src/openvpn/ssl_verify_mbedtls.c |   2 +-
 src/openvpn/ssl_verify_openssl.c |  68 ++++++--
 16 files changed, 660 insertions(+), 112 deletions(-)

The series can be checked on github at
https://github.com/emmanuel-deloget/openvpn/commits/openssl-1.1-v7

Best regards,

-- Emmanuel Deloget

#2 lore raw

PATCH v7 1/8 OpenSSL: don't use direct access to the internal of X509

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).

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.

We use these changes to make peer_cert a non-const parameter to x509_verify_ns_cert_type(). The underlying library waits for a non-const pointer, and forcing it to be a const pointer does not make much sense (please note that this has an effect on the mbedtls part too).

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

---
 configure.ac                     |  1 +
 src/openvpn/openssl_compat.h     | 15 +++++++++
 src/openvpn/ssl_openssl.c        |  3 +-
 src/openvpn/ssl_verify_backend.h |  2 +-
 src/openvpn/ssl_verify_mbedtls.c |  2 +-
 src/openvpn/ssl_verify_openssl.c | 68 ++++++++++++++++++++++++++++++++++------
 6 files changed, 78 insertions(+), 13 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_backend.h +1 -1
index c4330ba3..7e72ed30 100644
@@ -211,7 +211,7 @@ void x509_setenv_track(const struct x509_track *xt, struct env_set *es,
  *                      the expected bit set. \c FAILURE if the certificate does
  *                      not have NS cert type verification or the wrong bit set.
  */
-result_t x509_verify_ns_cert_type(const openvpn_x509_cert_t *cert, const int usage);
+result_t x509_verify_ns_cert_type(openvpn_x509_cert_t *cert, const int usage);
 
 /*
  * Verify X.509 key usage extension field.
src/openvpn/ssl_verify_mbedtls.c +1 -1
index c32e4815..c36393a8 100644
@@ -411,7 +411,7 @@ x509_setenv(struct env_set *es, int cert_depth, mbedtls_x509_crt *cert)
 }
 
 result_t
-x509_verify_ns_cert_type(const mbedtls_x509_crt *cert, const int usage)
+x509_verify_ns_cert_type(mbedtls_x509_crt *cert, const int usage)
 {
     if (usage == NS_CERT_CHECK_NONE)
     {
src/openvpn/ssl_verify_openssl.c +58 -10

96 lines not on this page · raw message

Signature · 1 line
2.11.0
#3 lore raw

Re: [Openvpn-devel] [PATCH 1/8] OpenSSL: don't use direct access to the internal of X509

Hi,

Quoted 158 lines · On 12-06-17 15:43, [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 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.

We use these changes to make peer_cert a non-const parameter to x509_verify_ns_cert_type(). The underlying library waits for a non-const pointer, and forcing it to be a const pointer does not make much sense (please note that this has an effect on the mbedtls part too).

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

---
 configure.ac                     |  1 +
 src/openvpn/openssl_compat.h     | 15 +++++++++
 src/openvpn/ssl_openssl.c        |  3 +-
 src/openvpn/ssl_verify_backend.h |  2 +-
 src/openvpn/ssl_verify_mbedtls.c |  2 +-
 src/openvpn/ssl_verify_openssl.c | 68 ++++++++++++++++++++++++++++++++++------
 6 files changed, 78 insertions(+), 13 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_backend.h +1 -1
index c4330ba3..7e72ed30 100644
@@ -211,7 +211,7 @@ void x509_setenv_track(const struct x509_track *xt, struct env_set *es,
  *                      the expected bit set. \c FAILURE if the certificate does
  *                      not have NS cert type verification or the wrong bit set.
  */
-result_t x509_verify_ns_cert_type(const openvpn_x509_cert_t *cert, const int usage);
+result_t x509_verify_ns_cert_type(openvpn_x509_cert_t *cert, const int usage);
 
 /*
  * Verify X.509 key usage extension field.
src/openvpn/ssl_verify_mbedtls.c +1 -1
index c32e4815..c36393a8 100644
@@ -411,7 +411,7 @@ x509_setenv(struct env_set *es, int cert_depth, mbedtls_x509_crt *cert)
 }
 
 result_t
-x509_verify_ns_cert_type(const mbedtls_x509_crt *cert, const int usage)
+x509_verify_ns_cert_type(mbedtls_x509_crt *cert, const int usage)
 {
     if (usage == NS_CERT_CHECK_NONE)
     {
src/openvpn/ssl_verify_openssl.c +8 -6
index 9b1533bc..7d5fcfb1 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;
 }
 
@@ -570,7 +572,7 @@ x509_setenv(struct env_set *es, int cert_depth, openvpn_x509_cert_t *peer_cert)
 }
 
 result_t
-x509_verify_ns_cert_type(const openvpn_x509_cert_t *peer_cert, const int usage)
+x509_verify_ns_cert_type(openvpn_x509_cert_t *peer_cert, const int usage)
 {
     if (usage == NS_CERT_CHECK_NONE)
     {

src/openvpn/ssl_verify_openssl.c x509_verify_ns_cert_type(const openvpn_x509_cert_t *peer_cert, const int usage) · line 586

Quoted 9 lines
src/openvpn/ssl_verify_openssl.c +4 -2
@@ -578,13 +580,59 @@ 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
+         */

This (and the similar comment below) needs s/wierd/weird/, but I think that can be done on-the-fly when applying.

Quoted 45 lines
src/openvpn/ssl_verify_openssl.c +39 -2
+        result_t result = X509_check_purpose(peer_cert, X509_PURPOSE_SSL_CLIENT, 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(peer_cert, NID_netscape_cert_type, NULL, NULL);
+            result = (ns && ns->length > 0 && (ns->data[0] & NS_SSL_CLIENT)) ? SUCCESS : FAILURE;
+            if (result == SUCCESS)
+            {
+                msg(M_WARN, "X509: Certificate is a client certificate yet it's purpose "
+                    "cannot be verified (check may fail in the future)");
+            }
+            ASN1_BIT_STRING_free(ns);
+        }
+        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(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(peer_cert, NID_netscape_cert_type, NULL, NULL);
+            result = (ns && ns->length > 0 && (ns->data[0] & NS_SSL_SERVER)) ? SUCCESS : FAILURE;
+            if (result == SUCCESS)

src/openvpn/ssl_verify_openssl.c x509_verify_ns_cert_type(const openvpn_x509_cert_t *peer_cert, const int usage) · line 638

Quoted 10 lines
src/openvpn/ssl_verify_openssl.c +7 -0
+            {
+                msg(M_WARN, "X509: Certificate is a server certificate yet it's purpose "
+                    "cannot be verified (check may fail in the future)");
+            }
+            ASN1_BIT_STRING_free(ns);
+        }
+        return result;
     }
 
     return FAILURE;

Other than that, this now looks good and works as advertised. I didn't go into the trouble of creating a certificate that actually fails check_purpose, but verified the code does what it claims by making the code always take that branch.

So, ACK.

-Steffan

#4 lore raw

[Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of X509

In reply to [email protected]...

Your patch has been applied to the master and release/2.4 branch.

I have changed the two occurences of "wierd" in comments to "weird", as requested.

commit 17d1ab90c228b1efbe774357bd3265b2af006899 (master) commit e78934adade51f79c847dcf0fee26905ebb9044d (release/2.4)

Author: Emmanuel Deloget
Date:   Mon Jun 12 15:43:23 2017 +0200
     OpenSSL: don't use direct access to the internal of X509

     Signed-off-by: Emmanuel Deloget <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14792.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#5 lore raw

PATCH v7 2/8 OpenSSL: don't use direct access to the internal of EVP_PKEY

In reply to [email protected]...

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

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.

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

48 lines not on this page · raw message

src/openvpn/ssl_openssl.c +3 -3
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
                 && pkey->pkey.rsa->n != NULL)
             {
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
                                  BN_num_bits(pkey->pkey.rsa->n));
             }
-            else if (pkey->type == EVP_PKEY_DSA && pkey->pkey.dsa != NULL
+            else if (EVP_PKEY_id(pkey) == EVP_PKEY_DSA && EVP_PKEY_get0_DSA(pkey) != NULL
                      && pkey->pkey.dsa->p != NULL)
             {
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
Signature · 1 line
2.11.0
#6 lore raw

Re: [Openvpn-devel] [PATCH 2/8] OpenSSL: don't use direct access to the internal of EVP_PKEY

Quoted 103 lines · On 12-06-17 15:43, [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 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.

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

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const char *prefix) · line 1689

Quoted 10 lines
src/openvpn/ssl_openssl.c +1 -1
                 && pkey->pkey.rsa->n != NULL)
             {
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
                                  BN_num_bits(pkey->pkey.rsa->n));
             }
-            else if (pkey->type == EVP_PKEY_DSA && pkey->pkey.dsa != NULL
+            else if (EVP_PKEY_id(pkey) == EVP_PKEY_DSA && EVP_PKEY_get0_DSA(pkey) != NULL
                      && pkey->pkey.dsa->p != NULL)
             {
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",

Patch content is the same as in the ACK'ed version from v7 - so ACK to this one too.

-Steffan

#7 lore raw

[Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of EVP_PKEY

In reply to [email protected]...

Your patch has been applied to the master and release/2.4 branch.

commit b8ca5bc3593e539d0735a74b55ed41a792e55033 (master) commit cc7c9122dc1d9e87d3f9b953b7d3b3db9166c540 (release/2.4)

Author: Emmanuel Deloget
Date:   Mon Jun 12 15:43:24 2017 +0200
     OpenSSL: don't use direct access to the internal of EVP_PKEY

     Signed-off-by: Emmanuel Deloget <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14795.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#8 lore raw

PATCH v7 3/8 OpenSSL: don't use direct access to the internal of RSA

In reply to [email protected]...

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

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.

---
 configure.ac                 |   4 ++
 src/openvpn/openssl_compat.h | 100 +++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    |  24 +++++++----
 3 files changed, 119 insertions(+), 9 deletions(-)
configure.ac +4 -0
index a92e8142..4c5f28ed 100644
@@ -929,6 +929,10 @@ 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_bits \
+			RSA_get0_key \
+			RSA_set0_key \
 			RSA_meth_new \
 			RSA_meth_free \
 			RSA_meth_set_pub_enc \
src/openvpn/openssl_compat.h +100 -0

106 lines not on this page · raw message

src/openvpn/ssl_openssl.c +15 -9
index 1c73641c..2a42d3c6 100644
@@ -973,10 +973,13 @@ rsa_priv_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
 
 /* called at RSA_free */
 static int
-rsa_finish(RSA *rsa)
+openvpn_extkey_rsa_finish(RSA *rsa)
 {
-    RSA_meth_free(rsa->meth);
-    rsa->meth = NULL;
+    /* meth was allocated in tls_ctx_use_external_private_key() ; since
+     * this function is called when the parent RSA object is destroyed,
+     * it is no longer used after this point so kill it. */
+    const RSA_METHOD *meth = RSA_get_method(rsa);
+    RSA_meth_free((RSA_METHOD *)meth);
     return 1;
 }
 
@@ -1058,7 +1061,7 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
     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_set_finish(rsa_meth, openvpn_extkey_rsa_finish);
     RSA_meth_set0_app_data(rsa_meth, NULL);
 
     /* allocate RSA object */
@@ -1075,8 +1078,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 +1683,11 @@ 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)
             {
+                RSA *rsa = EVP_PKEY_get0_RSA(pkey);
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
-                                 BN_num_bits(pkey->pkey.rsa->n));
+                                 RSA_bits(rsa));
             }
             else if (EVP_PKEY_id(pkey) == EVP_PKEY_DSA && EVP_PKEY_get0_DSA(pkey) != NULL
                      && pkey->pkey.dsa->p != NULL)
Signature · 1 line
2.11.0
#9 lore raw

Re: [Openvpn-devel] [PATCH 3/8] OpenSSL: don't use direct access to the internal of RSA

Hi,

Quoted 192 lines · On 12-06-17 15:43, [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. 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.

---
 configure.ac                 |   4 ++
 src/openvpn/openssl_compat.h | 100 +++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    |  24 +++++++----
 3 files changed, 119 insertions(+), 9 deletions(-)
configure.ac +4 -0
index a92e8142..4c5f28ed 100644
@@ -929,6 +929,10 @@ 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_bits \
+			RSA_get0_key \
+			RSA_set0_key \
 			RSA_meth_new \
 			RSA_meth_free \
 			RSA_meth_set_pub_enc \
src/openvpn/openssl_compat.h +100 -0
index 0d82cf25..44e3e167 100644
@@ -176,6 +176,106 @@ 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_BITS)
+/**
+ * Number of significant RSA bits
+ *
+ * @param rsa                The RSA object ; shall not be NULL
+ * @return                   The number of RSA bits or 0 on error
+ */
+static inline int
+RSA_bits(const RSA *rsa)
+{
+    const BIGNUM *n = NULL;
+    RSA_get0_key(rsa, &n, NULL, NULL);
+    return n ? BN_num_bits(n) : 0;
+}
+#endif
+
 #if !defined(HAVE_RSA_METH_NEW)
 /**
  * Allocate a new RSA method object
src/openvpn/ssl_openssl.c +12 -7
index 1c73641c..2a42d3c6 100644
@@ -973,10 +973,13 @@ rsa_priv_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
 
 /* called at RSA_free */
 static int
-rsa_finish(RSA *rsa)
+openvpn_extkey_rsa_finish(RSA *rsa)
 {
-    RSA_meth_free(rsa->meth);
-    rsa->meth = NULL;
+    /* meth was allocated in tls_ctx_use_external_private_key() ; since
+     * this function is called when the parent RSA object is destroyed,
+     * it is no longer used after this point so kill it. */
+    const RSA_METHOD *meth = RSA_get_method(rsa);
+    RSA_meth_free((RSA_METHOD *)meth);
     return 1;
 }
 
@@ -1058,7 +1061,7 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
     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_set_finish(rsa_meth, openvpn_extkey_rsa_finish);
     RSA_meth_set0_app_data(rsa_meth, NULL);
 
     /* allocate RSA object */
@@ -1075,8 +1078,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 +1683,11 @@ 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

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const char *prefix) · line 1693

Quoted 10 lines
src/openvpn/ssl_openssl.c +3 -2
-                && pkey->pkey.rsa->n != NULL)
+            if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL)
             {
+                RSA *rsa = EVP_PKEY_get0_RSA(pkey);
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
-                                 BN_num_bits(pkey->pkey.rsa->n));
+                                 RSA_bits(rsa));
             }
             else if (EVP_PKEY_id(pkey) == EVP_PKEY_DSA && EVP_PKEY_get0_DSA(pkey) != NULL
                      && pkey->pkey.dsa->p != NULL)

Code looks good, and works as advertised. ACK.

-Steffan

#10 lore raw

[Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of RSA

In reply to [email protected]...

Your patch has been applied to the master and release/2.4 branch.

commit f7780af6f1aaffcbbfb8b4dde0f2af052f84b28a (master) commit dd1ae0e48d0ce5c715ad6595b21c3cce1f4c8c6b (master)

Author: Emmanuel Deloget
Date:   Mon Jun 12 15:43:25 2017 +0200
     OpenSSL: don't use direct access to the internal of RSA

     Signed-off-by: Emmanuel Deloget <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14790.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#11 lore raw

PATCH v7 4/8 OpenSSL: don't use direct access to the internal of DSA

In reply to [email protected]...

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

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.

---
 configure.ac                 |  2 ++
 src/openvpn/openssl_compat.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    |  6 +++---
 3 files changed, 49 insertions(+), 3 deletions(-)
configure.ac +2 -0
index 4c5f28ed..6eded4e6 100644
@@ -933,6 +933,8 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			RSA_bits \
 			RSA_get0_key \
 			RSA_set0_key \
+			DSA_get0_pqg \
+			DSA_bits \
 			RSA_meth_new \
 			RSA_meth_free \
 			RSA_meth_set_pub_enc \
src/openvpn/openssl_compat.h +44 -0

50 lines not on this page · raw message

src/openvpn/ssl_openssl.c +3 -3
index 2a42d3c6..5faeafcc 100644
@@ -1689,11 +1689,11 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
                                  RSA_bits(rsa));
             }
-            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)
             {
+                DSA *dsa = EVP_PKEY_get0_DSA(pkey);
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
-                                 BN_num_bits(pkey->pkey.dsa->p));
+                                 DSA_bits(dsa));
             }
             EVP_PKEY_free(pkey);
         }
Signature · 1 line
2.11.0
#12 lore raw

Re: [Openvpn-devel] [PATCH 4/8] OpenSSL: don't use direct access to the internal of DSA

Quoted 94 lines · On 12-06-17 15:43, [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 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.

---
 configure.ac                 |  2 ++
 src/openvpn/openssl_compat.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    |  6 +++---
 3 files changed, 49 insertions(+), 3 deletions(-)
configure.ac +2 -0
index 4c5f28ed..6eded4e6 100644
@@ -933,6 +933,8 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			RSA_bits \
 			RSA_get0_key \
 			RSA_set0_key \
+			DSA_get0_pqg \
+			DSA_bits \
 			RSA_meth_new \
 			RSA_meth_free \
 			RSA_meth_set_pub_enc \
src/openvpn/openssl_compat.h +44 -0
index 44e3e167..24efa0fd 100644
@@ -276,6 +276,50 @@ RSA_bits(const RSA *rsa)
 }
 #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_DSA_BITS)
+/**
+ * Number of significant DSA bits
+ *
+ * @param rsa                The DSA object ; shall not be NULL
+ * @return                   The number of DSA bits or 0 on error
+ */
+static inline int
+DSA_bits(const DSA *dsa)
+{
+    const BIGNUM *p = NULL;
+    DSA_get0_pqg(dsa, &p, NULL, NULL);
+    return p ? BN_num_bits(p) : 0;
+}
+#endif
+
 #if !defined(HAVE_RSA_METH_NEW)
 /**
  * Allocate a new RSA method object
src/openvpn/ssl_openssl.c +0 -1
index 2a42d3c6..5faeafcc 100644
@@ -1689,11 +1689,11 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
                                  RSA_bits(rsa));
             }
-            else if (EVP_PKEY_id(pkey) == EVP_PKEY_DSA && EVP_PKEY_get0_DSA(pkey) != NULL

src/openvpn/ssl_openssl.c print_details(struct key_state_ssl *ks_ssl, const char *prefix) · line 1699

Quoted 10 lines
src/openvpn/ssl_openssl.c +3 -2
-                     && pkey->pkey.dsa->p != NULL)
+            else if (EVP_PKEY_id(pkey) == EVP_PKEY_DSA && EVP_PKEY_get0_DSA(pkey) != NULL)
             {
+                DSA *dsa = EVP_PKEY_get0_DSA(pkey);
                 openvpn_snprintf(s2, sizeof(s2), ", %d bit DSA",
-                                 BN_num_bits(pkey->pkey.dsa->p));
+                                 DSA_bits(dsa));
             }
             EVP_PKEY_free(pkey);
         }

Looks good too now, ACK.

-Steffan

#13 lore raw

[Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of DSA

In reply to [email protected]...

Your patch has been applied to the master and release/2.4 branch.

commit c07c0358b553c519ed9d80e2e0a9ba48ca8850e4 (master) commit 6f5aa04456f29d7084e004c980e219a21a954fa9 (master)

Author: Emmanuel Deloget
Date:   Mon Jun 12 15:43:26 2017 +0200
     OpenSSL: don't use direct access to the internal of DSA

     Signed-off-by: Emmanuel Deloget <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14791.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#14 lore raw

PATCH v7 5/8 OpenSSL: don't use direct access to the internal of EVP_MD_CTX

In reply to [email protected]...

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

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including EVP_MD_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.

---
 configure.ac                 |  3 ++
 src/openvpn/crypto_backend.h | 14 ++++++++
 src/openvpn/crypto_mbedtls.c | 12 +++++++
 src/openvpn/crypto_openssl.c | 18 ++++++++--
 src/openvpn/httpdigest.c     | 78 +++++++++++++++++++++++---------------------
 src/openvpn/misc.c           | 14 ++++----
 src/openvpn/openssl_compat.h | 50 ++++++++++++++++++++++++++++
 src/openvpn/openvpn.h        |  2 +-
 src/openvpn/push.c           | 11 ++++---
 9 files changed, 150 insertions(+), 52 deletions(-)
configure.ac +3 -0
index 6eded4e6..6ac4e595 100644
@@ -920,6 +920,9 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 
 	AC_CHECK_FUNCS(
 		[ \
+			EVP_MD_CTX_new \
+			EVP_MD_CTX_free \
+			EVP_MD_CTX_reset \
 			SSL_CTX_get_default_passwd_cb \
 			SSL_CTX_get_default_passwd_cb_userdata \
 			X509_get0_pubkey \
src/openvpn/crypto_backend.h +14 -0
index 9b113d7b..8f03e2ba 100644
@@ -508,6 +508,20 @@ int md_kt_size(const md_kt_t *kt);
 int md_full(const md_kt_t *kt, const uint8_t *src, int src_len, uint8_t *dst);
 
 /*
+ * Allocate a new message digest context
+ *
+ * @return              a new zeroed MD context
+ */
+md_ctx_t *md_ctx_new(void);
+
+/*
+ * Free an existing, non-null message digest context
+ *
+ * @param ctx           Message digest context
+ */
+void md_ctx_free(md_ctx_t *ctx);
+
+/*
  * Initialises the given message digest context.
  *
  * @param ctx           Message digest context
src/openvpn/crypto_mbedtls.c +12 -0
index 942684ce..d6741523 100644
@@ -766,6 +766,18 @@ md_full(const md_kt_t *kt, const uint8_t *src, int src_len, uint8_t *dst)
     return 0 == mbedtls_md(kt, src, src_len, dst);
 }
 
+mbedtls_md_context_t *
+md_ctx_new(void)
+{
+    mbedtls_md_context_t *ctx;
+    ALLOC_OBJ_CLEAR(ctx, mbedtls_md_context_t);
+    return ctx;
+}
+
+void md_ctx_free(mbedtls_md_context_t *ctx)
+{
+    free(ctx);
+}
 
 void
 md_ctx_init(mbedtls_md_context_t *ctx, const mbedtls_md_info_t *kt)
src/openvpn/crypto_openssl.c +15 -3
index 881a2d13..fd599f40 100644
@@ -42,6 +42,7 @@
 #include "integer.h"
 #include "crypto.h"
 #include "crypto_backend.h"
+#include "openssl_compat.h"
 
 #include <openssl/des.h>
 #include <openssl/err.h>
@@ -844,13 +845,24 @@ md_full(const EVP_MD *kt, const uint8_t *src, int src_len, uint8_t *dst)
     return EVP_Digest(src, src_len, dst, &in_md_len, kt, NULL);
 }
 
+EVP_MD_CTX *
+md_ctx_new(void)
+{
+    EVP_MD_CTX *ctx = EVP_MD_CTX_new();
+    check_malloc_return(ctx);
+    return ctx;
+}
+
+void md_ctx_free(EVP_MD_CTX *ctx)
+{
+    EVP_MD_CTX_free(ctx);
+}
+
 void
 md_ctx_init(EVP_MD_CTX *ctx, const EVP_MD *kt)
 {
     ASSERT(NULL != ctx && NULL != kt);
 
-    CLEAR(*ctx);
-
     EVP_MD_CTX_init(ctx);
     EVP_DigestInit(ctx, kt);
 }
@@ -858,7 +870,7 @@ md_ctx_init(EVP_MD_CTX *ctx, const EVP_MD *kt)
 void
 md_ctx_cleanup(EVP_MD_CTX *ctx)
 {
-    EVP_MD_CTX_cleanup(ctx);
+    EVP_MD_CTX_reset(ctx);
 }
 
 int
src/openvpn/httpdigest.c +40 -38

107 lines not on this page · raw message

src/openvpn/misc.c +8 -6
index 68d06876..5a997750 100644
@@ -1388,7 +1388,7 @@ get_user_pass_auto_userid(struct user_pass *up, const char *tag)
     static const uint8_t hashprefix[] = "AUTO_USERID_DIGEST";
 
     const md_kt_t *md5_kt = md_kt_get("MD5");
-    md_ctx_t ctx;
+    md_ctx_t *ctx;
 
     CLEAR(*up);
     buf_set_write(&buf, (uint8_t *)up->username, USER_PASS_LEN);
@@ -1396,11 +1396,13 @@ get_user_pass_auto_userid(struct user_pass *up, const char *tag)
     if (get_default_gateway_mac_addr(macaddr))
     {
         dmsg(D_AUTO_USERID, "GUPAU: macaddr=%s", format_hex_ex(macaddr, sizeof(macaddr), 0, 1, ":", &gc));
-        md_ctx_init(&ctx, md5_kt);
-        md_ctx_update(&ctx, hashprefix, sizeof(hashprefix) - 1);
-        md_ctx_update(&ctx, macaddr, sizeof(macaddr));
-        md_ctx_final(&ctx, digest);
-        md_ctx_cleanup(&ctx)
+        ctx = md_ctx_new();
+        md_ctx_init(ctx, md5_kt);
+        md_ctx_update(ctx, hashprefix, sizeof(hashprefix) - 1);
+        md_ctx_update(ctx, macaddr, sizeof(macaddr));
+        md_ctx_final(ctx, digest);
+        md_ctx_cleanup(ctx);
+        md_ctx_free(ctx);
         buf_printf(&buf, "%s", format_hex_ex(digest, sizeof(digest), 0, 256, " ", &gc));
     }
     else
src/openvpn/openssl_compat.h +50 -0

56 lines not on this page · raw message

src/openvpn/openvpn.h +1 -1
index f8682d17..21899bb9 100644
@@ -473,7 +473,7 @@ struct context_2
 
     /* hash of pulled options, so we can compare when options change */
     bool pulled_options_digest_init_done;
-    md_ctx_t pulled_options_state;
+    md_ctx_t *pulled_options_state;
     struct sha256_digest pulled_options_digest;
 
     struct event_timeout scheduled_exit;
src/openvpn/push.c +7 -4
index bcef0ef4..908b650f 100644
@@ -724,7 +724,8 @@ process_incoming_push_msg(struct context *c,
             struct buffer buf_orig = buf;
             if (!c->c2.pulled_options_digest_init_done)
             {
-                md_ctx_init(&c->c2.pulled_options_state, md_kt_get("SHA256"));
+                c->c2.pulled_options_state = md_ctx_new();
+                md_ctx_init(c->c2.pulled_options_state, md_kt_get("SHA256"));
                 c->c2.pulled_options_digest_init_done = true;
             }
             if (!c->c2.did_pre_pull_restore)
@@ -738,14 +739,16 @@ process_incoming_push_msg(struct context *c,
                                    option_types_found,
                                    c->c2.es))
             {
-                push_update_digest(&c->c2.pulled_options_state, &buf_orig,
+                push_update_digest(c->c2.pulled_options_state, &buf_orig,
                                    &c->options);
                 switch (c->options.push_continuation)
                 {
                     case 0:
                     case 1:
-                        md_ctx_final(&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
-                        md_ctx_cleanup(&c->c2.pulled_options_state);
+                        md_ctx_final(c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
+                        md_ctx_cleanup(c->c2.pulled_options_state);
+                        md_ctx_free(c->c2.pulled_options_state);
+                        c->c2.pulled_options_state = NULL;
                         c->c2.pulled_options_digest_init_done = false;
                         ret = PUSH_MSG_REPLY;
                         break;
Signature · 1 line
2.11.0
#15 lore raw

Re: [Openvpn-devel] [PATCH 5/8] OpenSSL: don't use direct access to the internal of EVP_MD_CTX

Hi,

Quoted 267 lines · On 12-06-17 15:43, [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 EVP_MD_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.

---
 configure.ac                 |  3 ++
 src/openvpn/crypto_backend.h | 14 ++++++++
 src/openvpn/crypto_mbedtls.c | 12 +++++++
 src/openvpn/crypto_openssl.c | 18 ++++++++--
 src/openvpn/httpdigest.c     | 78 +++++++++++++++++++++++---------------------
 src/openvpn/misc.c           | 14 ++++----
 src/openvpn/openssl_compat.h | 50 ++++++++++++++++++++++++++++
 src/openvpn/openvpn.h        |  2 +-
 src/openvpn/push.c           | 11 ++++---
 9 files changed, 150 insertions(+), 52 deletions(-)
configure.ac +3 -0
index 6eded4e6..6ac4e595 100644
@@ -920,6 +920,9 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 
 	AC_CHECK_FUNCS(
 		[ \
+			EVP_MD_CTX_new \
+			EVP_MD_CTX_free \
+			EVP_MD_CTX_reset \
 			SSL_CTX_get_default_passwd_cb \
 			SSL_CTX_get_default_passwd_cb_userdata \
 			X509_get0_pubkey \
src/openvpn/crypto_backend.h +14 -0
index 9b113d7b..8f03e2ba 100644
@@ -508,6 +508,20 @@ int md_kt_size(const md_kt_t *kt);
 int md_full(const md_kt_t *kt, const uint8_t *src, int src_len, uint8_t *dst);
 
 /*
+ * Allocate a new message digest context
+ *
+ * @return              a new zeroed MD context
+ */
+md_ctx_t *md_ctx_new(void);
+
+/*
+ * Free an existing, non-null message digest context
+ *
+ * @param ctx           Message digest context
+ */
+void md_ctx_free(md_ctx_t *ctx);
+
+/*
  * Initialises the given message digest context.
  *
  * @param ctx           Message digest context
src/openvpn/crypto_mbedtls.c +12 -0
index 942684ce..d6741523 100644
@@ -766,6 +766,18 @@ md_full(const md_kt_t *kt, const uint8_t *src, int src_len, uint8_t *dst)
     return 0 == mbedtls_md(kt, src, src_len, dst);
 }
 
+mbedtls_md_context_t *
+md_ctx_new(void)
+{
+    mbedtls_md_context_t *ctx;
+    ALLOC_OBJ_CLEAR(ctx, mbedtls_md_context_t);
+    return ctx;
+}
+
+void md_ctx_free(mbedtls_md_context_t *ctx)
+{
+    free(ctx);
+}
 
 void
 md_ctx_init(mbedtls_md_context_t *ctx, const mbedtls_md_info_t *kt)
src/openvpn/crypto_openssl.c +15 -3
index 881a2d13..fd599f40 100644
@@ -42,6 +42,7 @@
 #include "integer.h"
 #include "crypto.h"
 #include "crypto_backend.h"
+#include "openssl_compat.h"
 
 #include <openssl/des.h>
 #include <openssl/err.h>
@@ -844,13 +845,24 @@ md_full(const EVP_MD *kt, const uint8_t *src, int src_len, uint8_t *dst)
     return EVP_Digest(src, src_len, dst, &in_md_len, kt, NULL);
 }
 
+EVP_MD_CTX *
+md_ctx_new(void)
+{
+    EVP_MD_CTX *ctx = EVP_MD_CTX_new();
+    check_malloc_return(ctx);
+    return ctx;
+}
+
+void md_ctx_free(EVP_MD_CTX *ctx)
+{
+    EVP_MD_CTX_free(ctx);
+}
+
 void
 md_ctx_init(EVP_MD_CTX *ctx, const EVP_MD *kt)
 {
     ASSERT(NULL != ctx && NULL != kt);
 
-    CLEAR(*ctx);
-
     EVP_MD_CTX_init(ctx);
     EVP_DigestInit(ctx, kt);
 }
@@ -858,7 +870,7 @@ md_ctx_init(EVP_MD_CTX *ctx, const EVP_MD *kt)
 void
 md_ctx_cleanup(EVP_MD_CTX *ctx)
 {
-    EVP_MD_CTX_cleanup(ctx);
+    EVP_MD_CTX_reset(ctx);
 }
 
 int
src/openvpn/httpdigest.c +40 -38
index ae4a638f..2a66d9b8 100644
@@ -81,27 +81,28 @@ DigestCalcHA1(
     )
 {
     HASH HA1;
-    md_ctx_t md5_ctx;
+    md_ctx_t *md5_ctx = md_ctx_new();
     const md_kt_t *md5_kt = md_kt_get("MD5");
 
-    md_ctx_init(&md5_ctx, md5_kt);
-    md_ctx_update(&md5_ctx, (const uint8_t *) pszUserName, strlen(pszUserName));
-    md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
-    md_ctx_update(&md5_ctx, (const uint8_t *) pszRealm, strlen(pszRealm));
-    md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
-    md_ctx_update(&md5_ctx, (const uint8_t *) pszPassword, strlen(pszPassword));
-    md_ctx_final(&md5_ctx, HA1);
+    md_ctx_init(md5_ctx, md5_kt);
+    md_ctx_update(md5_ctx, (const uint8_t *) pszUserName, strlen(pszUserName));
+    md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
+    md_ctx_update(md5_ctx, (const uint8_t *) pszRealm, strlen(pszRealm));
+    md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
+    md_ctx_update(md5_ctx, (const uint8_t *) pszPassword, strlen(pszPassword));
+    md_ctx_final(md5_ctx, HA1);
     if (pszAlg && strcasecmp(pszAlg, "md5-sess") == 0)
     {
-        md_ctx_init(&md5_ctx, md5_kt);
-        md_ctx_update(&md5_ctx, HA1, HASHLEN);
-        md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
-        md_ctx_update(&md5_ctx, (const uint8_t *) pszNonce, strlen(pszNonce));
-        md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
-        md_ctx_update(&md5_ctx, (const uint8_t *) pszCNonce, strlen(pszCNonce));
-        md_ctx_final(&md5_ctx, HA1);
+        md_ctx_init(md5_ctx, md5_kt);
+        md_ctx_update(md5_ctx, HA1, HASHLEN);
+        md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
+        md_ctx_update(md5_ctx, (const uint8_t *) pszNonce, strlen(pszNonce));
+        md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
+        md_ctx_update(md5_ctx, (const uint8_t *) pszCNonce, strlen(pszCNonce));
+        md_ctx_final(md5_ctx, HA1);
     }
-    md_ctx_cleanup(&md5_ctx);
+    md_ctx_cleanup(md5_ctx);
+    md_ctx_free(md5_ctx);
     CvtHex(HA1, SessionKey);
 }
 
@@ -123,40 +124,41 @@ DigestCalcResponse(
     HASH RespHash;
     HASHHEX HA2Hex;
 
-    md_ctx_t md5_ctx;
+    md_ctx_t *md5_ctx = md_ctx_new();
     const md_kt_t *md5_kt = md_kt_get("MD5");
 
     /* calculate H(A2) */
-    md_ctx_init(&md5_ctx, md5_kt);
-    md_ctx_update(&md5_ctx, (const uint8_t *) pszMethod, strlen(pszMethod));
-    md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
-    md_ctx_update(&md5_ctx, (const uint8_t *) pszDigestUri, strlen(pszDigestUri));
+    md_ctx_init(md5_ctx, md5_kt);
+    md_ctx_update(md5_ctx, (const uint8_t *) pszMethod, strlen(pszMethod));
+    md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
+    md_ctx_update(md5_ctx, (const uint8_t *) pszDigestUri, strlen(pszDigestUri));
     if (strcasecmp(pszQop, "auth-int") == 0)
     {
-        md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
-        md_ctx_update(&md5_ctx, HEntity, HASHHEXLEN);
+        md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
+        md_ctx_update(md5_ctx, HEntity, HASHHEXLEN);
     }
-    md_ctx_final(&md5_ctx, HA2);
+    md_ctx_final(md5_ctx, HA2);
     CvtHex(HA2, HA2Hex);
 
     /* calculate response */
-    md_ctx_init(&md5_ctx, md5_kt);
-    md_ctx_update(&md5_ctx, HA1, HASHHEXLEN);
-    md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
-    md_ctx_update(&md5_ctx, (const uint8_t *) pszNonce, strlen(pszNonce));
-    md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
+    md_ctx_init(md5_ctx, md5_kt);
+    md_ctx_update(md5_ctx, HA1, HASHHEXLEN);
+    md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
+    md_ctx_update(md5_ctx, (const uint8_t *) pszNonce, strlen(pszNonce));
+    md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
     if (*pszQop)
     {
-        md_ctx_update(&md5_ctx, (const uint8_t *) pszNonceCount, strlen(pszNonceCount));
-        md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
-        md_ctx_update(&md5_ctx, (const uint8_t *) pszCNonce, strlen(pszCNonce));
-        md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
-        md_ctx_update(&md5_ctx, (const uint8_t *) pszQop, strlen(pszQop));
-        md_ctx_update(&md5_ctx, (const uint8_t *) ":", 1);
+        md_ctx_update(md5_ctx, (const uint8_t *) pszNonceCount, strlen(pszNonceCount));
+        md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
+        md_ctx_update(md5_ctx, (const uint8_t *) pszCNonce, strlen(pszCNonce));
+        md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
+        md_ctx_update(md5_ctx, (const uint8_t *) pszQop, strlen(pszQop));
+        md_ctx_update(md5_ctx, (const uint8_t *) ":", 1);
     }
-    md_ctx_update(&md5_ctx, HA2Hex, HASHHEXLEN);
-    md_ctx_final(&md5_ctx, RespHash);
-    md_ctx_cleanup(&md5_ctx);
+    md_ctx_update(md5_ctx, HA2Hex, HASHHEXLEN);
+    md_ctx_final(md5_ctx, RespHash);
+    md_ctx_cleanup(md5_ctx);
+    md_ctx_free(md5_ctx);
     CvtHex(RespHash, Response);
 }
 
src/openvpn/misc.c +1 -6
index 68d06876..5a997750 100644
@@ -1388,7 +1388,7 @@ get_user_pass_auto_userid(struct user_pass *up, const char *tag)
     static const uint8_t hashprefix[] = "AUTO_USERID_DIGEST";
 
     const md_kt_t *md5_kt = md_kt_get("MD5");
-    md_ctx_t ctx;
+    md_ctx_t *ctx;
 
     CLEAR(*up);
     buf_set_write(&buf, (uint8_t *)up->username, USER_PASS_LEN);
@@ -1396,11 +1396,13 @@ get_user_pass_auto_userid(struct user_pass *up, const char *tag)
     if (get_default_gateway_mac_addr(macaddr))
     {
         dmsg(D_AUTO_USERID, "GUPAU: macaddr=%s", format_hex_ex(macaddr, sizeof(macaddr), 0, 1, ":", &gc));
-        md_ctx_init(&ctx, md5_kt);
-        md_ctx_update(&ctx, hashprefix, sizeof(hashprefix) - 1);
-        md_ctx_update(&ctx, macaddr, sizeof(macaddr));
-        md_ctx_final(&ctx, digest);
-        md_ctx_cleanup(&ctx)

src/openvpn/misc.c get_user_pass_auto_userid(struct user_pass *up, const char *tag) · line 1408

Quoted 10 lines
src/openvpn/misc.c +7 -0
+        ctx = md_ctx_new();
+        md_ctx_init(ctx, md5_kt);
+        md_ctx_update(ctx, hashprefix, sizeof(hashprefix) - 1);
+        md_ctx_update(ctx, macaddr, sizeof(macaddr));
+        md_ctx_final(ctx, digest);
+        md_ctx_cleanup(ctx);
+        md_ctx_free(ctx);
         buf_printf(&buf, "%s", format_hex_ex(digest, sizeof(digest), 0, 256, " ", &gc));
     }
     else

Hm, this code block doesn't even compile anymore since 2011 (commit 7fb0e07e removed get_default_gateway_mac_addr()). I think this means we should just remove everything inside #if AUTO_USERID. But I'll pick that up in a later patch - for this patch the changes look good.

Quoted 5 lines
src/openvpn/openssl_compat.h +0 -0
index 24efa0fd..b315bcb7 100644

src/openvpn/openssl_compat.h line 55

Quoted 10 lines
src/openvpn/openssl_compat.h +7 -0
@@ -46,6 +46,56 @@
 #include <openssl/ssl.h>
 #include <openssl/x509.h>
 
+#include <openssl/des.h>
+#include <openssl/err.h>
+#include <openssl/evp.h>
+#include <openssl/objects.h>
+#include <openssl/rand.h>
+#include <openssl/ssl.h>
+

This looks like a lot of includes, given the functions below. Were these missing from the previous commits?

Quoted 84 lines
src/openvpn/openssl_compat.h +43 -0
+#if !defined(HAVE_EVP_MD_CTX_RESET)
+/**
+ * Reset a message digest context
+ *
+ * @param ctx                 The message digest context
+ * @return                    1 on success, 0 on error
+ */
+static inline int
+EVP_MD_CTX_reset(EVP_MD_CTX *ctx)
+{
+    EVP_MD_CTX_cleanup(ctx);
+    return 1;
+}
+#endif
+
+#if !defined(HAVE_EVP_MD_CTX_FREE)
+/**
+ * Free an existing message digest context
+ *
+ * @param ctx                 The message digest context
+ */
+static inline void
+EVP_MD_CTX_free(EVP_MD_CTX *ctx)
+{
+    free(ctx);
+}
+#endif
+
+#if !defined(HAVE_EVP_MD_CTX_NEW)
+/**
+ * Allocate a new message digest object
+ *
+ * @return                    A zero'ed message digest object
+ */
+static inline EVP_MD_CTX *
+EVP_MD_CTX_new(void)
+{
+    EVP_MD_CTX *ctx = NULL;
+    ALLOC_OBJ_CLEAR(ctx, EVP_MD_CTX);
+    return ctx;
+}
+#endif
+
 #if !defined(HAVE_SSL_CTX_GET_DEFAULT_PASSWD_CB_USERDATA)
 /**
  * Fetch the default password callback user data from the SSL context
src/openvpn/openvpn.h +1 -1
index f8682d17..21899bb9 100644
@@ -473,7 +473,7 @@ struct context_2
 
     /* hash of pulled options, so we can compare when options change */
     bool pulled_options_digest_init_done;
-    md_ctx_t pulled_options_state;
+    md_ctx_t *pulled_options_state;
     struct sha256_digest pulled_options_digest;
 
     struct event_timeout scheduled_exit;
src/openvpn/push.c +3 -2
index bcef0ef4..908b650f 100644
@@ -724,7 +724,8 @@ process_incoming_push_msg(struct context *c,
             struct buffer buf_orig = buf;
             if (!c->c2.pulled_options_digest_init_done)
             {
-                md_ctx_init(&c->c2.pulled_options_state, md_kt_get("SHA256"));
+                c->c2.pulled_options_state = md_ctx_new();
+                md_ctx_init(c->c2.pulled_options_state, md_kt_get("SHA256"));
                 c->c2.pulled_options_digest_init_done = true;
             }
             if (!c->c2.did_pre_pull_restore)
@@ -738,14 +739,16 @@ process_incoming_push_msg(struct context *c,
                                    option_types_found,
                                    c->c2.es))
             {
-                push_update_digest(&c->c2.pulled_options_state, &buf_orig,
+                push_update_digest(c->c2.pulled_options_state, &buf_orig,
                                    &c->options);
                 switch (c->options.push_continuation)
                 {
                     case 0:

src/openvpn/push.c process_incoming_push_msg(struct context *c, · line 754

Quoted 10 lines
src/openvpn/push.c +4 -2
                     case 1:
-                        md_ctx_final(&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
-                        md_ctx_cleanup(&c->c2.pulled_options_state);
+                        md_ctx_final(c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);
+                        md_ctx_cleanup(c->c2.pulled_options_state);
+                        md_ctx_free(c->c2.pulled_options_state);
+                        c->c2.pulled_options_state = NULL;
                         c->c2.pulled_options_digest_init_done = false;
                         ret = PUSH_MSG_REPLY;
                         break;

Apart from the includes, the patch looks good and passes my tests.

I didn't test the auto_userid because it's broken anyway, and did not test the http_proxy code because I don't have a test setup. But the changes look simple and sane enough to still ACK this patch.

So, if the includes are fixed, ACK to the patch. I'll leave it up to Gert whether a v9 is needed for that.

-Steffan

#16 lore raw

[Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of EVP_MD_CTX

In reply to [email protected]...

Your patch has been applied to the master and release/2.4 branch.

I have removed the extra #include lines again, as suggested by Steffan, and they do not seem to be missing - if they are part of a larger cleanup (like "move all the <openssl/foo.h> includes into a central place") please re-send as an additional patch.

commit c481ef002803f360743c72727ae3ca971ce59a5d (master) commit 87a392369d812d8c2ac81174bb2d6fad17544408 (release/2.4)

Author: Emmanuel Deloget
Date:   Mon Jun 12 15:43:27 2017 +0200
     OpenSSL: don't use direct access to the internal of EVP_MD_CTX

     Signed-off-by: Emmanuel Deloget <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14793.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#17 lore raw

PATCH v7 6/8 OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX

In reply to [email protected]...

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

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including EVP_CIPHER_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.

---
 configure.ac                 |  2 ++
 src/openvpn/crypto.c         |  4 ++--
 src/openvpn/crypto_backend.h | 14 ++++++++++++++
 src/openvpn/crypto_mbedtls.c | 13 +++++++++++++
 src/openvpn/crypto_openssl.c | 15 +++++++++++++--
 src/openvpn/openssl_compat.h | 28 ++++++++++++++++++++++++++++
 6 files changed, 72 insertions(+), 4 deletions(-)
configure.ac +2 -0
index 6ac4e595..e895cf0a 100644
@@ -920,6 +920,8 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 
 	AC_CHECK_FUNCS(
 		[ \
+			EVP_CIPHER_CTX_new \
+			EVP_CIPHER_CTX_free \
 			EVP_MD_CTX_new \
 			EVP_MD_CTX_free \
 			EVP_MD_CTX_reset \
src/openvpn/crypto.c +2 -2
index 50e6a734..893879cf 100644
@@ -830,7 +830,7 @@ init_key_ctx(struct key_ctx *ctx, struct key *key,
     if (kt->cipher && kt->cipher_length > 0)
     {
 
-        ALLOC_OBJ(ctx->cipher, cipher_ctx_t);
+        ctx->cipher = cipher_ctx_new();
         cipher_ctx_init(ctx->cipher, key->cipher, kt->cipher_length,
                         kt->cipher, enc);
 
@@ -879,7 +879,7 @@ free_key_ctx(struct key_ctx *ctx)
     if (ctx->cipher)
     {
         cipher_ctx_cleanup(ctx->cipher);
-        free(ctx->cipher);
+        cipher_ctx_free(ctx->cipher);
         ctx->cipher = NULL;
     }
     if (ctx->hmac)
src/openvpn/crypto_backend.h +14 -0
index 8f03e2ba..3a911a47 100644
@@ -301,6 +301,20 @@ bool cipher_kt_mode_aead(const cipher_kt_t *cipher);
  */
 
 /**
+ * Allocate a new cipher context
+ *
+ * @return              a new cipher context
+ */
+cipher_ctx_t *cipher_ctx_new(void);
+
+/**
+ * Free a cipher context
+ *
+ * @param ctx           Cipher context.
+ */
+void cipher_ctx_free(cipher_ctx_t *ctx);
+
+/**
  * Initialise a cipher context, based on the given key and key type.
  *
  * @param ctx           Cipher context. May not be NULL
src/openvpn/crypto_mbedtls.c +13 -0
index d6741523..4d38aadc 100644
@@ -509,6 +509,19 @@ cipher_kt_mode_aead(const cipher_kt_t *cipher)
  *
  */
 
+mbedtls_cipher_context_t *
+cipher_ctx_new(void)
+{
+    mbedtls_cipher_context_t *ctx;
+    ALLOC_OBJ(ctx, mbedtls_cipher_context_t);
+    return ctx;
+}
+
+void
+cipher_ctx_free(mbedtls_cipher_context_t *ctx)
+{
+    free(ctx);
+}
 
 void
 cipher_ctx_init(mbedtls_cipher_context_t *ctx, uint8_t *key, int key_len,
src/openvpn/crypto_openssl.c +13 -2
index fd599f40..0644f1c3 100644
@@ -651,6 +651,19 @@ cipher_kt_mode_aead(const cipher_kt_t *cipher)
  *
  */
 
+cipher_ctx_t *
+cipher_ctx_new(void)
+{
+    EVP_CIPHER_CTX *ctx = EVP_CIPHER_CTX_new();
+    check_malloc_return(ctx);
+    return ctx;
+}
+
+void
+cipher_ctx_free(EVP_CIPHER_CTX *ctx)
+{
+    EVP_CIPHER_CTX_free(ctx);
+}
 
 void
 cipher_ctx_init(EVP_CIPHER_CTX *ctx, uint8_t *key, int key_len,
@@ -658,8 +671,6 @@ cipher_ctx_init(EVP_CIPHER_CTX *ctx, uint8_t *key, int key_len,
 {
     ASSERT(NULL != kt && NULL != ctx);
 
-    CLEAR(*ctx);
-
     EVP_CIPHER_CTX_init(ctx);
     if (!EVP_CipherInit(ctx, kt, NULL, NULL, enc))
     {
src/openvpn/openssl_compat.h +28 -0
index b315bcb7..3f36212a 100644
@@ -96,6 +96,34 @@ EVP_MD_CTX_new(void)
 }
 #endif
 
+#if !defined(HAVE_EVP_CIPHER_CTX_FREE)
+/**
+ * Free an existing cipher context
+ *
+ * @param ctx                 The cipher context
+ */
+static inline void
+EVP_CIPHER_CTX_free(EVP_CIPHER_CTX *c)
+{
+	free(c);
+}
+#endif
+
+#if !defined(HAVE_EVP_CIPHER_CTX_NEW)
+/**
+ * Allocate a new cipher context object
+ *
+ * @return                    A zero'ed cipher context object
+ */
+static inline EVP_CIPHER_CTX *
+EVP_CIPHER_CTX_new(void)
+{
+    EVP_CIPHER_CTX *ctx = NULL;
+    ALLOC_OBJ_CLEAR(ctx, EVP_CIPHER_CTX);
+    return ctx;
+}
+#endif
+
 #if !defined(HAVE_SSL_CTX_GET_DEFAULT_PASSWD_CB_USERDATA)
 /**
  * Fetch the default password callback user data from the SSL context
Signature · 1 line
2.11.0
#18 lore raw

Re: [Openvpn-devel] [PATCH 6/8] OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX

Hi,

Quoted 166 lines · On 12-06-17 15:43, [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 EVP_CIPHER_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.

---
 configure.ac                 |  2 ++
 src/openvpn/crypto.c         |  4 ++--
 src/openvpn/crypto_backend.h | 14 ++++++++++++++
 src/openvpn/crypto_mbedtls.c | 13 +++++++++++++
 src/openvpn/crypto_openssl.c | 15 +++++++++++++--
 src/openvpn/openssl_compat.h | 28 ++++++++++++++++++++++++++++
 6 files changed, 72 insertions(+), 4 deletions(-)
configure.ac +2 -0
index 6ac4e595..e895cf0a 100644
@@ -920,6 +920,8 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 
 	AC_CHECK_FUNCS(
 		[ \
+			EVP_CIPHER_CTX_new \
+			EVP_CIPHER_CTX_free \
 			EVP_MD_CTX_new \
 			EVP_MD_CTX_free \
 			EVP_MD_CTX_reset \
src/openvpn/crypto.c +2 -2
index 50e6a734..893879cf 100644
@@ -830,7 +830,7 @@ init_key_ctx(struct key_ctx *ctx, struct key *key,
     if (kt->cipher && kt->cipher_length > 0)
     {
 
-        ALLOC_OBJ(ctx->cipher, cipher_ctx_t);
+        ctx->cipher = cipher_ctx_new();
         cipher_ctx_init(ctx->cipher, key->cipher, kt->cipher_length,
                         kt->cipher, enc);
 
@@ -879,7 +879,7 @@ free_key_ctx(struct key_ctx *ctx)
     if (ctx->cipher)
     {
         cipher_ctx_cleanup(ctx->cipher);
-        free(ctx->cipher);
+        cipher_ctx_free(ctx->cipher);
         ctx->cipher = NULL;
     }
     if (ctx->hmac)
src/openvpn/crypto_backend.h +14 -0
index 8f03e2ba..3a911a47 100644
@@ -301,6 +301,20 @@ bool cipher_kt_mode_aead(const cipher_kt_t *cipher);
  */
 
 /**
+ * Allocate a new cipher context
+ *
+ * @return              a new cipher context
+ */
+cipher_ctx_t *cipher_ctx_new(void);
+
+/**
+ * Free a cipher context
+ *
+ * @param ctx           Cipher context.
+ */
+void cipher_ctx_free(cipher_ctx_t *ctx);
+
+/**
  * Initialise a cipher context, based on the given key and key type.
  *
  * @param ctx           Cipher context. May not be NULL
src/openvpn/crypto_mbedtls.c +13 -0
index d6741523..4d38aadc 100644
@@ -509,6 +509,19 @@ cipher_kt_mode_aead(const cipher_kt_t *cipher)
  *
  */
 
+mbedtls_cipher_context_t *
+cipher_ctx_new(void)
+{
+    mbedtls_cipher_context_t *ctx;
+    ALLOC_OBJ(ctx, mbedtls_cipher_context_t);
+    return ctx;
+}
+
+void
+cipher_ctx_free(mbedtls_cipher_context_t *ctx)
+{
+    free(ctx);
+}
 
 void
 cipher_ctx_init(mbedtls_cipher_context_t *ctx, uint8_t *key, int key_len,
src/openvpn/crypto_openssl.c +13 -2
index fd599f40..0644f1c3 100644
@@ -651,6 +651,19 @@ cipher_kt_mode_aead(const cipher_kt_t *cipher)
  *
  */
 
+cipher_ctx_t *
+cipher_ctx_new(void)
+{
+    EVP_CIPHER_CTX *ctx = EVP_CIPHER_CTX_new();
+    check_malloc_return(ctx);
+    return ctx;
+}
+
+void
+cipher_ctx_free(EVP_CIPHER_CTX *ctx)
+{
+    EVP_CIPHER_CTX_free(ctx);
+}
 
 void
 cipher_ctx_init(EVP_CIPHER_CTX *ctx, uint8_t *key, int key_len,
@@ -658,8 +671,6 @@ cipher_ctx_init(EVP_CIPHER_CTX *ctx, uint8_t *key, int key_len,
 {
     ASSERT(NULL != kt && NULL != ctx);
 
-    CLEAR(*ctx);
-
     EVP_CIPHER_CTX_init(ctx);
     if (!EVP_CipherInit(ctx, kt, NULL, NULL, enc))
     {
src/openvpn/openssl_compat.h +21 -0
index b315bcb7..3f36212a 100644
@@ -96,6 +96,34 @@ EVP_MD_CTX_new(void)
 }
 #endif
 
+#if !defined(HAVE_EVP_CIPHER_CTX_FREE)
+/**
+ * Free an existing cipher context
+ *
+ * @param ctx                 The cipher context
+ */
+static inline void
+EVP_CIPHER_CTX_free(EVP_CIPHER_CTX *c)
+{
+	free(c);
+}
+#endif
+
+#if !defined(HAVE_EVP_CIPHER_CTX_NEW)
+/**
+ * Allocate a new cipher context object
+ *
+ * @return                    A zero'ed cipher context object
+ */
+static inline EVP_CIPHER_CTX *
+EVP_CIPHER_CTX_new(void)

src/openvpn/openssl_compat.h EVP_MD_CTX_new(void) · line 129

Quoted 10 lines
src/openvpn/openssl_compat.h +7 -0
+{
+    EVP_CIPHER_CTX *ctx = NULL;
+    ALLOC_OBJ_CLEAR(ctx, EVP_CIPHER_CTX);
+    return ctx;
+}
+#endif
+
 #if !defined(HAVE_SSL_CTX_GET_DEFAULT_PASSWD_CB_USERDATA)
 /**
  * Fetch the default password callback user data from the SSL context

ACK

Code looks good, and passes my mbed and openssl tests.

-Steffan

#19 lore raw

[Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX

In reply to [email protected]...

Your patch has been applied to the master and release/2.4 branch.

commit 6cbd48a3ead23f004f25943d067fa668efdc580e (master) commit a5ac1ecf35f35cecbbb89111f914fe95c5560df2 (release/2.4)

Author: Emmanuel Deloget
Date:   Mon Jun 12 15:43:28 2017 +0200
     OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX

     Signed-off-by: Emmanuel Deloget <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14796.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#20 lore raw

PATCH v7 7/8 OpenSSL: don't use direct access to the internal of HMAC_CTX

In reply to [email protected]...

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

OpenSSL 1.1 does not allow us to directly access the internal of any data type, including HMAC_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.

---
 configure.ac                 |  4 +++
 src/openvpn/crypto.c         |  4 +--
 src/openvpn/crypto_backend.h | 14 ++++++++++
 src/openvpn/crypto_mbedtls.c | 15 ++++++++++
 src/openvpn/crypto_openssl.c | 17 ++++++++++--
 src/openvpn/ntlm.c           | 12 ++++----
 src/openvpn/openssl_compat.h | 65 ++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl.c            | 38 ++++++++++++++------------
 8 files changed, 140 insertions(+), 29 deletions(-)
configure.ac +4 -0
index e895cf0a..e5c5c7df 100644
@@ -922,6 +922,10 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 		[ \
 			EVP_CIPHER_CTX_new \
 			EVP_CIPHER_CTX_free \
+			HMAC_CTX_new \
+			HMAC_CTX_free \
+			HMAC_CTX_reset \
+			HMAC_CTX_init \
 			EVP_MD_CTX_new \
 			EVP_MD_CTX_free \
 			EVP_MD_CTX_reset \
src/openvpn/crypto.c +2 -2
index 893879cf..a80c3f7f 100644
@@ -854,7 +854,7 @@ init_key_ctx(struct key_ctx *ctx, struct key *key,
     }
     if (kt->digest && kt->hmac_length > 0)
     {
-        ALLOC_OBJ(ctx->hmac, hmac_ctx_t);
+        ctx->hmac = hmac_ctx_new();
         hmac_ctx_init(ctx->hmac, key->hmac, kt->hmac_length, kt->digest);
 
         msg(D_HANDSHAKE,
@@ -885,7 +885,7 @@ free_key_ctx(struct key_ctx *ctx)
     if (ctx->hmac)
     {
         hmac_ctx_cleanup(ctx->hmac);
-        free(ctx->hmac);
+        hmac_ctx_free(ctx->hmac);
         ctx->hmac = NULL;
     }
     ctx->implicit_iv_len = 0;
src/openvpn/crypto_backend.h +14 -0
index 3a911a47..3dc75ab9 100644
@@ -584,6 +584,20 @@ void md_ctx_final(md_ctx_t *ctx, uint8_t *dst);
  */
 
 /*
+ * Create a new HMAC context
+ *
+ * @return              A new HMAC context
+ */
+hmac_ctx_t *hmac_ctx_new(void);
+
+/*
+ * Free an existing HMAC context
+ *
+ * @param  ctx           HMAC context to free
+ */
+void hmac_ctx_free(hmac_ctx_t *ctx);
+
+/*
  * Initialises the given HMAC context, using the given digest
  * and key.
  *
src/openvpn/crypto_mbedtls.c +15 -0
index 4d38aadc..f0698f61 100644
@@ -841,6 +841,21 @@ md_ctx_final(mbedtls_md_context_t *ctx, uint8_t *dst)
 /*
  * TODO: re-enable dmsg for crypto debug
  */
+
+mbedtls_md_context_t *
+hmac_ctx_new(void)
+{
+    mbedtls_md_context_t *ctx;
+    ALLOC_OBJ(ctx, mbedtls_md_context_t);
+    return ctx;
+}
+
+void
+hmac_ctx_free(mbedtls_md_context_t *ctx)
+{
+    free(ctx);
+}
+
 void
 hmac_ctx_init(mbedtls_md_context_t *ctx, const uint8_t *key, int key_len,
               const mbedtls_md_info_t *kt)
src/openvpn/crypto_openssl.c +14 -3
index 0644f1c3..b64f7f04 100644
@@ -911,6 +911,19 @@ md_ctx_final(EVP_MD_CTX *ctx, uint8_t *dst)
  *
  */
 
+HMAC_CTX *
+hmac_ctx_new(void)
+{
+    HMAC_CTX *ctx = HMAC_CTX_new();
+    check_malloc_return(ctx);
+    return ctx;
+}
+
+void
+hmac_ctx_free(HMAC_CTX *ctx)
+{
+    HMAC_CTX_free(ctx);
+}
 
 void
 hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int key_len,
@@ -918,8 +931,6 @@ hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int key_len,
 {
     ASSERT(NULL != kt && NULL != ctx);
 
-    CLEAR(*ctx);
-
     HMAC_CTX_init(ctx);
     HMAC_Init_ex(ctx, key, key_len, kt, NULL);
 
@@ -930,7 +941,7 @@ hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int key_len,
 void
 hmac_ctx_cleanup(HMAC_CTX *ctx)
 {
-    HMAC_CTX_cleanup(ctx);
+    HMAC_CTX_reset(ctx);
 }
 
 int
src/openvpn/ntlm.c +6 -6
index 93483379..07a5d25f 100644
@@ -86,13 +86,13 @@ static void
 gen_hmac_md5(const char *data, int data_len, const char *key, int key_len,char *result)
 {
     const md_kt_t *md5_kt = md_kt_get("MD5");
-    hmac_ctx_t hmac_ctx;
-    CLEAR(hmac_ctx);
+    hmac_ctx_t *hmac_ctx = hmac_ctx_new();
 
-    hmac_ctx_init(&hmac_ctx, key, key_len, md5_kt);
-    hmac_ctx_update(&hmac_ctx, (const unsigned char *)data, data_len);
-    hmac_ctx_final(&hmac_ctx, (unsigned char *)result);
-    hmac_ctx_cleanup(&hmac_ctx);
+    hmac_ctx_init(hmac_ctx, key, key_len, md5_kt);
+    hmac_ctx_update(hmac_ctx, (const unsigned char *)data, data_len);
+    hmac_ctx_final(hmac_ctx, (unsigned char *)result);
+    hmac_ctx_cleanup(hmac_ctx);
+    hmac_ctx_free(hmac_ctx);
 }
 
 static void
src/openvpn/openssl_compat.h +65 -0

71 lines not on this page · raw message

src/openvpn/ssl.c +20 -18
index 3d5e8241..55cc457d 100644
@@ -1607,8 +1607,8 @@ tls1_P_hash(const md_kt_t *md_kt,
 {
     struct gc_arena gc = gc_new();
     int chunk;
-    hmac_ctx_t ctx;
-    hmac_ctx_t ctx_tmp;
+    hmac_ctx_t *ctx;
+    hmac_ctx_t *ctx_tmp;
     uint8_t A1[MAX_HMAC_KEY_LENGTH];
     unsigned int A1_len;
 
@@ -1617,8 +1617,8 @@ tls1_P_hash(const md_kt_t *md_kt,
     const uint8_t *out_orig = out;
 #endif
 
-    CLEAR(ctx);
-    CLEAR(ctx_tmp);
+    ctx = hmac_ctx_new();
+    ctx_tmp = hmac_ctx_new();
 
     dmsg(D_SHOW_KEY_SOURCE, "tls1_P_hash sec: %s", format_hex(sec, sec_len, 0, &gc));
     dmsg(D_SHOW_KEY_SOURCE, "tls1_P_hash seed: %s", format_hex(seed, seed_len, 0, &gc));
@@ -1626,36 +1626,38 @@ tls1_P_hash(const md_kt_t *md_kt,
     chunk = md_kt_size(md_kt);
     A1_len = md_kt_size(md_kt);
 
-    hmac_ctx_init(&ctx, sec, sec_len, md_kt);
-    hmac_ctx_init(&ctx_tmp, sec, sec_len, md_kt);
+    hmac_ctx_init(ctx, sec, sec_len, md_kt);
+    hmac_ctx_init(ctx_tmp, sec, sec_len, md_kt);
 
-    hmac_ctx_update(&ctx,seed,seed_len);
-    hmac_ctx_final(&ctx, A1);
+    hmac_ctx_update(ctx,seed,seed_len);
+    hmac_ctx_final(ctx, A1);
 
     for (;; )
     {
-        hmac_ctx_reset(&ctx);
-        hmac_ctx_reset(&ctx_tmp);
-        hmac_ctx_update(&ctx,A1,A1_len);
-        hmac_ctx_update(&ctx_tmp,A1,A1_len);
-        hmac_ctx_update(&ctx,seed,seed_len);
+        hmac_ctx_reset(ctx);
+        hmac_ctx_reset(ctx_tmp);
+        hmac_ctx_update(ctx,A1,A1_len);
+        hmac_ctx_update(ctx_tmp,A1,A1_len);
+        hmac_ctx_update(ctx,seed,seed_len);
 
         if (olen > chunk)
         {
-            hmac_ctx_final(&ctx, out);
+            hmac_ctx_final(ctx, out);
             out += chunk;
             olen -= chunk;
-            hmac_ctx_final(&ctx_tmp, A1); /* calc the next A1 value */
+            hmac_ctx_final(ctx_tmp, A1); /* calc the next A1 value */
         }
         else    /* last one */
         {
-            hmac_ctx_final(&ctx, A1);
+            hmac_ctx_final(ctx, A1);
             memcpy(out,A1,olen);
             break;
         }
     }
-    hmac_ctx_cleanup(&ctx);
-    hmac_ctx_cleanup(&ctx_tmp);
+    hmac_ctx_cleanup(ctx);
+    hmac_ctx_free(ctx);
+    hmac_ctx_cleanup(ctx_tmp);
+    hmac_ctx_free(ctx_tmp);
     secure_memzero(A1, sizeof(A1));
 
     dmsg(D_SHOW_KEY_SOURCE, "tls1_P_hash out: %s", format_hex(out_orig, olen_orig, 0, &gc));
Signature · 1 line
2.11.0
#21 lore raw

Re: [Openvpn-devel] [PATCH 7/8] OpenSSL: don't use direct access to the internal of HMAC_CTX

Hi,

Quoted 209 lines · On 12-06-17 15:43, [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 HMAC_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.

---
 configure.ac                 |  4 +++
 src/openvpn/crypto.c         |  4 +--
 src/openvpn/crypto_backend.h | 14 ++++++++++
 src/openvpn/crypto_mbedtls.c | 15 ++++++++++
 src/openvpn/crypto_openssl.c | 17 ++++++++++--
 src/openvpn/ntlm.c           | 12 ++++----
 src/openvpn/openssl_compat.h | 65 ++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl.c            | 38 ++++++++++++++------------
 8 files changed, 140 insertions(+), 29 deletions(-)
configure.ac +4 -0
index e895cf0a..e5c5c7df 100644
@@ -922,6 +922,10 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 		[ \
 			EVP_CIPHER_CTX_new \
 			EVP_CIPHER_CTX_free \
+			HMAC_CTX_new \
+			HMAC_CTX_free \
+			HMAC_CTX_reset \
+			HMAC_CTX_init \
 			EVP_MD_CTX_new \
 			EVP_MD_CTX_free \
 			EVP_MD_CTX_reset \
src/openvpn/crypto.c +2 -2
index 893879cf..a80c3f7f 100644
@@ -854,7 +854,7 @@ init_key_ctx(struct key_ctx *ctx, struct key *key,
     }
     if (kt->digest && kt->hmac_length > 0)
     {
-        ALLOC_OBJ(ctx->hmac, hmac_ctx_t);
+        ctx->hmac = hmac_ctx_new();
         hmac_ctx_init(ctx->hmac, key->hmac, kt->hmac_length, kt->digest);
 
         msg(D_HANDSHAKE,
@@ -885,7 +885,7 @@ free_key_ctx(struct key_ctx *ctx)
     if (ctx->hmac)
     {
         hmac_ctx_cleanup(ctx->hmac);
-        free(ctx->hmac);
+        hmac_ctx_free(ctx->hmac);
         ctx->hmac = NULL;
     }
     ctx->implicit_iv_len = 0;
src/openvpn/crypto_backend.h +14 -0
index 3a911a47..3dc75ab9 100644
@@ -584,6 +584,20 @@ void md_ctx_final(md_ctx_t *ctx, uint8_t *dst);
  */
 
 /*
+ * Create a new HMAC context
+ *
+ * @return              A new HMAC context
+ */
+hmac_ctx_t *hmac_ctx_new(void);
+
+/*
+ * Free an existing HMAC context
+ *
+ * @param  ctx           HMAC context to free
+ */
+void hmac_ctx_free(hmac_ctx_t *ctx);
+
+/*
  * Initialises the given HMAC context, using the given digest
  * and key.
  *
src/openvpn/crypto_mbedtls.c +15 -0
index 4d38aadc..f0698f61 100644
@@ -841,6 +841,21 @@ md_ctx_final(mbedtls_md_context_t *ctx, uint8_t *dst)
 /*
  * TODO: re-enable dmsg for crypto debug
  */
+
+mbedtls_md_context_t *
+hmac_ctx_new(void)
+{
+    mbedtls_md_context_t *ctx;
+    ALLOC_OBJ(ctx, mbedtls_md_context_t);
+    return ctx;
+}
+
+void
+hmac_ctx_free(mbedtls_md_context_t *ctx)
+{
+    free(ctx);
+}
+
 void
 hmac_ctx_init(mbedtls_md_context_t *ctx, const uint8_t *key, int key_len,
               const mbedtls_md_info_t *kt)
src/openvpn/crypto_openssl.c +14 -3
index 0644f1c3..b64f7f04 100644
@@ -911,6 +911,19 @@ md_ctx_final(EVP_MD_CTX *ctx, uint8_t *dst)
  *
  */
 
+HMAC_CTX *
+hmac_ctx_new(void)
+{
+    HMAC_CTX *ctx = HMAC_CTX_new();
+    check_malloc_return(ctx);
+    return ctx;
+}
+
+void
+hmac_ctx_free(HMAC_CTX *ctx)
+{
+    HMAC_CTX_free(ctx);
+}
 
 void
 hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int key_len,
@@ -918,8 +931,6 @@ hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int key_len,
 {
     ASSERT(NULL != kt && NULL != ctx);
 
-    CLEAR(*ctx);
-
     HMAC_CTX_init(ctx);
     HMAC_Init_ex(ctx, key, key_len, kt, NULL);
 
@@ -930,7 +941,7 @@ hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int key_len,
 void
 hmac_ctx_cleanup(HMAC_CTX *ctx)
 {
-    HMAC_CTX_cleanup(ctx);
+    HMAC_CTX_reset(ctx);
 }
 
 int
src/openvpn/ntlm.c +6 -6
index 93483379..07a5d25f 100644
@@ -86,13 +86,13 @@ static void
 gen_hmac_md5(const char *data, int data_len, const char *key, int key_len,char *result)
 {
     const md_kt_t *md5_kt = md_kt_get("MD5");
-    hmac_ctx_t hmac_ctx;
-    CLEAR(hmac_ctx);
+    hmac_ctx_t *hmac_ctx = hmac_ctx_new();
 
-    hmac_ctx_init(&hmac_ctx, key, key_len, md5_kt);
-    hmac_ctx_update(&hmac_ctx, (const unsigned char *)data, data_len);
-    hmac_ctx_final(&hmac_ctx, (unsigned char *)result);
-    hmac_ctx_cleanup(&hmac_ctx);
+    hmac_ctx_init(hmac_ctx, key, key_len, md5_kt);
+    hmac_ctx_update(hmac_ctx, (const unsigned char *)data, data_len);
+    hmac_ctx_final(hmac_ctx, (unsigned char *)result);
+    hmac_ctx_cleanup(hmac_ctx);
+    hmac_ctx_free(hmac_ctx);
 }
 
 static void
src/openvpn/openssl_compat.h +26 -0
index 3f36212a..71ecfc06 100644
@@ -124,6 +124,71 @@ EVP_CIPHER_CTX_new(void)
 }
 #endif
 
+#if !defined(HAVE_HMAC_CTX_RESET)
+/**
+ * Reset a HMAC context
+ *
+ * @param ctx                 The HMAC context
+ * @return                    1 on success, 0 on error
+ */
+static inline int
+HMAC_CTX_reset(HMAC_CTX *ctx)
+{
+    HMAC_CTX_cleanup(ctx);
+    return 1;
+}
+#endif
+
+#if !defined(HAVE_HMAC_CTX_INIT)
+/**
+ * Init a HMAC context
+ *
+ * @param ctx                 The HMAC context
+ *
+ * Contrary to many functions in this file, HMAC_CTX_init() is not
+ * an OpenSSL 1.1 function: it comes from previous versions and was
+ * removed in v1.1. As a consequence, there is no distincting in
+ * v1.1 between a cleanup, and init and a reset. Yet, previous OpenSSL
+ * version need this distinction.

src/openvpn/openssl_compat.h EVP_CIPHER_CTX_new(void) · line 162

Quoted 10 lines
src/openvpn/openssl_compat.h +10 -0
+ *
+ * In order to respect previous OpenSSL versions, we implement init
+ * as reset for OpenSSL 1.1+.
+ */
+static inline void
+HMAC_CTX_init(HMAC_CTX *ctx)
+{
+    HMAC_CTX_reset(ctx);
+}
+#endif

Hm, shouldn't we do this the other way around then? Implement a HMAC_CTX_reset() here that calls HMAC_CTX_init(), and use _reset() in our hmac_ctx_init() function?

Quoted 102 lines
src/openvpn/openssl_compat.h +29 -0
+
+#if !defined(HAVE_HMAC_CTX_FREE)
+/**
+ * Free an existing HMAC context
+ *
+ * @param ctx                 The HMAC context
+ */
+static inline void
+HMAC_CTX_free(HMAC_CTX *c)
+{
+	free(c);
+}
+#endif
+
+#if !defined(HAVE_HMAC_CTX_NEW)
+/**
+ * Allocate a new HMAC context object
+ *
+ * @return                    A zero'ed HMAC context object
+ */
+static inline HMAC_CTX *
+HMAC_CTX_new(void)
+{
+    HMAC_CTX *ctx = NULL;
+    ALLOC_OBJ_CLEAR(ctx, HMAC_CTX);
+    return ctx;
+}
+#endif
+
 #if !defined(HAVE_SSL_CTX_GET_DEFAULT_PASSWD_CB_USERDATA)
 /**
  * Fetch the default password callback user data from the SSL context
src/openvpn/ssl.c +16 -16
index 3d5e8241..55cc457d 100644
@@ -1607,8 +1607,8 @@ tls1_P_hash(const md_kt_t *md_kt,
 {
     struct gc_arena gc = gc_new();
     int chunk;
-    hmac_ctx_t ctx;
-    hmac_ctx_t ctx_tmp;
+    hmac_ctx_t *ctx;
+    hmac_ctx_t *ctx_tmp;
     uint8_t A1[MAX_HMAC_KEY_LENGTH];
     unsigned int A1_len;
 
@@ -1617,8 +1617,8 @@ tls1_P_hash(const md_kt_t *md_kt,
     const uint8_t *out_orig = out;
 #endif
 
-    CLEAR(ctx);
-    CLEAR(ctx_tmp);
+    ctx = hmac_ctx_new();
+    ctx_tmp = hmac_ctx_new();
 
     dmsg(D_SHOW_KEY_SOURCE, "tls1_P_hash sec: %s", format_hex(sec, sec_len, 0, &gc));
     dmsg(D_SHOW_KEY_SOURCE, "tls1_P_hash seed: %s", format_hex(seed, seed_len, 0, &gc));
@@ -1626,36 +1626,38 @@ tls1_P_hash(const md_kt_t *md_kt,
     chunk = md_kt_size(md_kt);
     A1_len = md_kt_size(md_kt);
 
-    hmac_ctx_init(&ctx, sec, sec_len, md_kt);
-    hmac_ctx_init(&ctx_tmp, sec, sec_len, md_kt);
+    hmac_ctx_init(ctx, sec, sec_len, md_kt);
+    hmac_ctx_init(ctx_tmp, sec, sec_len, md_kt);
 
-    hmac_ctx_update(&ctx,seed,seed_len);
-    hmac_ctx_final(&ctx, A1);
+    hmac_ctx_update(ctx,seed,seed_len);
+    hmac_ctx_final(ctx, A1);
 
     for (;; )
     {
-        hmac_ctx_reset(&ctx);
-        hmac_ctx_reset(&ctx_tmp);
-        hmac_ctx_update(&ctx,A1,A1_len);
-        hmac_ctx_update(&ctx_tmp,A1,A1_len);
-        hmac_ctx_update(&ctx,seed,seed_len);
+        hmac_ctx_reset(ctx);
+        hmac_ctx_reset(ctx_tmp);
+        hmac_ctx_update(ctx,A1,A1_len);
+        hmac_ctx_update(ctx_tmp,A1,A1_len);
+        hmac_ctx_update(ctx,seed,seed_len);
 
         if (olen > chunk)
         {
-            hmac_ctx_final(&ctx, out);
+            hmac_ctx_final(ctx, out);
             out += chunk;
             olen -= chunk;
-            hmac_ctx_final(&ctx_tmp, A1); /* calc the next A1 value */
+            hmac_ctx_final(ctx_tmp, A1); /* calc the next A1 value */
         }
         else    /* last one */
         {
-            hmac_ctx_final(&ctx, A1);
+            hmac_ctx_final(ctx, A1);
             memcpy(out,A1,olen);
             break;
         }

src/openvpn/ssl.c tls1_P_hash(const md_kt_t *md_kt, · line 1663

Quoted 10 lines
src/openvpn/ssl.c +4 -2
     }
-    hmac_ctx_cleanup(&ctx);
-    hmac_ctx_cleanup(&ctx_tmp);
+    hmac_ctx_cleanup(ctx);
+    hmac_ctx_free(ctx);
+    hmac_ctx_cleanup(ctx_tmp);
+    hmac_ctx_free(ctx_tmp);
     secure_memzero(A1, sizeof(A1));
 
     dmsg(D_SHOW_KEY_SOURCE, "tls1_P_hash out: %s", format_hex(out_orig, olen_orig, 0, &gc));

Otherwise this looks good, and passes my tests. I would prefer to change the _init()/reset() thing before applying (if you agree that this is better), but if this is the only thing left to get 1.1 support into our next release that shouldn't block applying the patch.

So, basically, ACK :)

-Steffan

#22 lore raw

Re: [Openvpn-devel] [PATCH 7/8] OpenSSL: don't use direct access to the internal of HMAC_CTX

Hello Steffan,

Quoted 20 lines · Steffan Karger

Hi,

Quoted 17 lines · On 12-06-17 15:43, [email protected]... wrote:
src/openvpn/openssl_compat.h +17 -0
+#if !defined(HAVE_HMAC_CTX_INIT)
+/**
+ * Init a HMAC context
+ *
+ * @param ctx                 The HMAC context
+ *
+ * Contrary to many functions in this file, HMAC_CTX_init() is not
+ * an OpenSSL 1.1 function: it comes from previous versions and was
+ * removed in v1.1. As a consequence, there is no distincting in
+ * v1.1 between a cleanup, and init and a reset. Yet, previous OpenSSL
+ * version need this distinction.
+ *
+ * In order to respect previous OpenSSL versions, we implement init
+ * as reset for OpenSSL 1.1+.
+ */
+static inline void
+HMAC_CTX_init(HMAC_CTX *ctx)
Quoted 10 lines

src/openvpn/openssl_compat.h EVP_CIPHER_CTX_new(void) · line 162

Quoted 4 lines
src/openvpn/openssl_compat.h +4 -0
+{
+    HMAC_CTX_reset(ctx);
+}
+#endif

Hm, shouldn't we do this the other way around then? Implement a HMAC_CTX_reset() here that calls HMAC_CTX_init(), and use _reset() in our hmac_ctx_init() function?

I think I did it that way to enhance code reading but it's true that it introduces a pre-1.1 artefact into an interface which is based upon OpenSSL 1.1.

I'm sending a patch to fix that.

Quoted 12 lines

​> <snip

Quoted 1 line

Otherwise this looks good, and passes my tests. I would prefer to change the _init()/reset() thing before applying (if you agree that this is better), but if this is the only thing left to get 1.1 support into our next release that shouldn't block applying the patch.

So, basically, ACK :)

-Steffan

​BR,

-- Emmanuel Deloget​

#23 lore raw

[Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of HMAC_CTX

In reply to [email protected]...

Your patch has been applied to the master and release/2.4 branch.

I have not changed anything wrt _init()/reset() to not delay getting this in-tree before the upcoming v2.4.3 release - but if you agree to Steffan's comment on that, please send a followup patch which can then go in later.

(Thanks for all your work!)

commit aba98e9050eb54d72d921e70bcd422cb892b9c6c (master) commit 2bf4aee4b043151bd2abe7101421fd74763f1230 (release/2.4)

Author: Emmanuel Deloget
Date:   Mon Jun 12 15:43:29 2017 +0200
     OpenSSL: don't use direct access to the internal of HMAC_CTX

     Signed-off-by: Emmanuel Deloget <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14797.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#24 lore raw

Re: [Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of HMAC_CTX

Hi Gert,

Quoted 7 lines · Gert Doering

Your patch has been applied to the master and release/2.4 branch.

I have not changed anything wrt _init()/reset() to not delay getting this in-tree before the upcoming v2.4.3 release - but if you agree to Steffan's comment on that, please send a followup patch which can then go in later.

​I'm going to do that ASAP. ​

Quoted 3 lines

(Thanks for all your work!)

​It's me who want to thank you all: you did a marvellous job at pointing my mistakes :) ​

​So thanks to you, Steffan, David, and all the other who helped me. ​

Quoted 18 lines

commit aba98e9050eb54d72d921e70bcd422cb892b9c6c (master) commit 2bf4aee4b043151bd2abe7101421fd74763f1230 (release/2.4)

Author: Emmanuel Deloget
Date:   Mon Jun 12 15:43:29 2017 +0200
     OpenSSL: don't use direct access to the internal of HMAC_CTX

     Signed-off-by: Emmanuel Deloget <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]...

sourceforge.net/msg14797.html
Signed-off-by: Gert Doeri
​​
ng <[email protected]...>

Quoted 6 lines

--
kind regards,

Gert Doering

​BR,

-- Emmanuel Deloget​

#25 lore raw

PATCH v7 8/8 OpenSSL: force meth->name as non-const when we free() it

In reply to [email protected]...

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

We are in control of meth->name (we string_alloc() it in RSA_meth_new()) so we know that we can free() it when it's no longer needed. Yet we have to force the value to be non-const to avoid a compiler warning -- due to the fact that OpenSSL defines the value as a const char*, regardless of its origin.

---
 src/openvpn/openssl_compat.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
src/openvpn/openssl_compat.h +6 -1
index 71ecfc06..69db6fb5 100644
@@ -493,7 +493,12 @@ RSA_meth_free(RSA_METHOD *meth)
 {
     if (meth)
     {
-        free(meth->name);
+        /* OpenSSL defines meth->name to be a const pointer, yet we
+         * feed it with an allocated string (from RSA_meth_new()).
+         * Thus we are allowed to free it here. In order to avoid a
+         * ‘free’ discards ‘const’ warning, we force the pointer to
+         * be a non-const value. */
+        free((char *)meth->name);
         free(meth);
     }
 }
Signature · 1 line
2.11.0
#26 lore raw

Re: [Openvpn-devel] [PATCH 8/8] OpenSSL: force meth->name as non-const when we free() it

Quoted 20 lines · On 12-06-17 15:43, [email protected]... wrote:

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

We are in control of meth->name (we string_alloc() it in RSA_meth_new()) so we know that we can free() it when it's no longer needed. Yet we have to force the value to be non-const to avoid a compiler warning -- due to the fact that OpenSSL defines the value as a const char*, regardless of its origin.

---
 src/openvpn/openssl_compat.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
src/openvpn/openssl_compat.h +0 -0
index 71ecfc06..69db6fb5 100644
@@ -493,7 +493,12 @@ RSA_meth_free(RSA_METHOD *meth)
 {
     if (meth)
     {

src/openvpn/openssl_compat.h RSA_meth_free(RSA_METHOD *meth) · line 504

Quoted 10 lines
src/openvpn/openssl_compat.h +6 -1
-        free(meth->name);
+        /* OpenSSL defines meth->name to be a const pointer, yet we
+         * feed it with an allocated string (from RSA_meth_new()).
+         * Thus we are allowed to free it here. In order to avoid a
+         * ‘free’ discards ‘const’ warning, we force the pointer to
+         * be a non-const value. */
+        free((char *)meth->name);
         free(meth);
     }
 }

Silences the warning and explains why the code does what it does, which I agree is the correct way to work around OpenSSL API funkyness.

ACK

-Steffan

#27 lore raw

[Openvpn-devel] [PATCH applied] Re: OpenSSL: force meth->name as non-const when we free() it

In reply to [email protected]...

Your patch has been applied to the master and release/2.4 branch.

I have changed the UTF8 characters in the quoted warning message to standard ISO characters, though (copy-pasting the warning that clang throws on FreeBSD). And fixed the dangling comment close.

commit 3fd07c31fe8878dc75e760d151d291379c0f8743
commit 57eaf994379e054203ce42338b4495588ae28c44

Author: Emmanuel Deloget
Date:   Mon Jun 12 15:43:30 2017 +0200
     OpenSSL: force meth->name as non-const when we free() it

     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14798.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#28 lore raw
In reply to [email protected]...

Hi,

fwiw, this builds fine against LibreSSL, which seems to provide some but not all of the functions for which you wrote fallback implementations. LibreSSL as shipped in OpenBSD-current, ie the development version.

make check passes, openvpn seems to behave correctly in client mode.

Signature · 1 line
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
#29 lore raw

Hi Jeremie,

Quoted 9 lines · Jeremie Courreges-Anglas

Hi,

fwiw, this builds fine against LibreSSL, which seems to provide some but not all of the functions for which you wrote fallback implementations. LibreSSL as shipped in OpenBSD-current, ie the development version.

make check passes, openvpn seems to behave correctly in client mode.

​Thanks ! (and it's good to know :)​)

Frankly, testing against LibreSSL didn't even cross my mind :) I guess I (wrongly) assumed that LibreSSL and OpenSSL were too different by now -- it's good to see than some of the API is still the same (for any definition of good ; I guess the folk at LibreSSL find this a bit discouraging...).

Best regards,

-- Emmanuel Deloget