* [Openvpn-devel] [PATCH 1/8] OpenSSL: don't use direct access to the internal of X509
2017-06-12 13:43 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 logout
@ 2017-06-12 13:43 ` logout
2017-06-18 7:56 ` Steffan Karger
2017-06-18 10:38 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 2/8] OpenSSL: don't use direct access to the internal of EVP_PKEY logout
` (7 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: logout @ 2017-06-12 13:43 UTC (permalink / raw)
To: openvpn-devel
From: Emmanuel Deloget <logout@...260...>
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.
Signed-off-by: Emmanuel Deloget <logout@...260...>
---
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(-)
diff --git a/configure.ac b/configure.ac
index 7d3fce5b..9d5e340b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -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 \
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 92f014d5..29a7588c 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -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
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 645ccf51..a082c3cd 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -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 */
diff --git a/src/openvpn/ssl_verify_backend.h b/src/openvpn/ssl_verify_backend.h
index c4330ba3..7e72ed30 100644
--- a/src/openvpn/ssl_verify_backend.h
+++ b/src/openvpn/ssl_verify_backend.h
@@ -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.
diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c
index c32e4815..c36393a8 100644
--- a/src/openvpn/ssl_verify_mbedtls.c
+++ b/src/openvpn/ssl_verify_mbedtls.c
@@ -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)
{
diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 9b1533bc..7d5fcfb1 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -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)
{
@@ -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
+ */
+ 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)
+ {
+ 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;
--
2.11.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH 1/8] OpenSSL: don't use direct access to the internal of X509
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 1/8] OpenSSL: don't use direct access to the internal of X509 logout
@ 2017-06-18 7:56 ` Steffan Karger
2017-06-18 10:38 ` [Openvpn-devel] [PATCH applied] " Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Steffan Karger @ 2017-06-18 7:56 UTC (permalink / raw)
To: openvpn-devel
Hi,
On 12-06-17 15:43, logout@...260... wrote:
> From: Emmanuel Deloget <logout@...260...>
>
> 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.
>
> Signed-off-by: Emmanuel Deloget <logout@...260...>
> ---
> 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(-)
>
> diff --git a/configure.ac b/configure.ac
> index 7d3fce5b..9d5e340b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -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 \
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index 92f014d5..29a7588c 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -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
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 645ccf51..a082c3cd 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -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 */
> diff --git a/src/openvpn/ssl_verify_backend.h b/src/openvpn/ssl_verify_backend.h
> index c4330ba3..7e72ed30 100644
> --- a/src/openvpn/ssl_verify_backend.h
> +++ b/src/openvpn/ssl_verify_backend.h
> @@ -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.
> diff --git a/src/openvpn/ssl_verify_mbedtls.c b/src/openvpn/ssl_verify_mbedtls.c
> index c32e4815..c36393a8 100644
> --- a/src/openvpn/ssl_verify_mbedtls.c
> +++ b/src/openvpn/ssl_verify_mbedtls.c
> @@ -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)
> {
> diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
> index 9b1533bc..7d5fcfb1 100644
> --- a/src/openvpn/ssl_verify_openssl.c
> +++ b/src/openvpn/ssl_verify_openssl.c
> @@ -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)
> {
> @@ -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.
> + 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)
> + {
> + 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
^ permalink raw reply [flat|nested] 29+ messages in thread* [Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of X509
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 1/8] OpenSSL: don't use direct access to the internal of X509 logout
2017-06-18 7:56 ` Steffan Karger
@ 2017-06-18 10:38 ` Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Gert Doering @ 2017-06-18 10:38 UTC (permalink / raw)
To: Emmanuel Deloget <logout@; +Cc: openvpn-devel
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 <logout@...260...>
Acked-by: Steffan Karger <steffan.karger@...1435...>
Message-Id: <20170612134330.20971-2-logout@...260...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14792.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Openvpn-devel] [PATCH 2/8] OpenSSL: don't use direct access to the internal of EVP_PKEY
2017-06-12 13:43 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 logout
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 1/8] OpenSSL: don't use direct access to the internal of X509 logout
@ 2017-06-12 13:43 ` logout
2017-06-18 8:04 ` Steffan Karger
2017-06-18 10:38 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 3/8] OpenSSL: don't use direct access to the internal of RSA logout
` (6 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: logout @ 2017-06-12 13:43 UTC (permalink / raw)
To: openvpn-devel
From: Emmanuel Deloget <logout@...260...>
OpenSSL 1.1 does not allow us to directly access the internal of
any data type, including EVP_PKEY. We have to use the defined
functions to do so.
Compatibility with OpenSSL 1.0 is kept by defining the corresponding
functions when they are not found in the library.
Signed-off-by: Emmanuel Deloget <logout@...260...>
---
configure.ac | 3 +++
src/openvpn/openssl_compat.h | 42 ++++++++++++++++++++++++++++++++++++++++++
src/openvpn/ssl_openssl.c | 6 +++---
3 files changed, 48 insertions(+), 3 deletions(-)
diff --git a/configure.ac b/configure.ac
index 9d5e340b..a92e8142 100644
--- a/configure.ac
+++ b/configure.ac
@@ -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 \
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 29a7588c..0d82cf25 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -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
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index a082c3cd..1c73641c 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -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",
--
2.11.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH 2/8] OpenSSL: don't use direct access to the internal of EVP_PKEY
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 2/8] OpenSSL: don't use direct access to the internal of EVP_PKEY logout
@ 2017-06-18 8:04 ` Steffan Karger
2017-06-18 10:38 ` [Openvpn-devel] [PATCH applied] " Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Steffan Karger @ 2017-06-18 8:04 UTC (permalink / raw)
To: openvpn-devel
On 12-06-17 15:43, logout@...260... wrote:
> From: Emmanuel Deloget <logout@...260...>
>
> OpenSSL 1.1 does not allow us to directly access the internal of
> any data type, including EVP_PKEY. We have to use the defined
> functions to do so.
>
> Compatibility with OpenSSL 1.0 is kept by defining the corresponding
> functions when they are not found in the library.
>
> Signed-off-by: Emmanuel Deloget <logout@...260...>
> ---
> configure.ac | 3 +++
> src/openvpn/openssl_compat.h | 42 ++++++++++++++++++++++++++++++++++++++++++
> src/openvpn/ssl_openssl.c | 6 +++---
> 3 files changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 9d5e340b..a92e8142 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -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 \
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index 29a7588c..0d82cf25 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -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
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index a082c3cd..1c73641c 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -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",
>
Patch content is the same as in the ACK'ed version from v7 - so ACK to
this one too.
-Steffan
^ permalink raw reply [flat|nested] 29+ messages in thread* [Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of EVP_PKEY
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 2/8] OpenSSL: don't use direct access to the internal of EVP_PKEY logout
2017-06-18 8:04 ` Steffan Karger
@ 2017-06-18 10:38 ` Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Gert Doering @ 2017-06-18 10:38 UTC (permalink / raw)
To: Emmanuel Deloget <logout@; +Cc: openvpn-devel
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 <logout@...260...>
Acked-by: Steffan Karger <steffan.karger@...1435...>
Message-Id: <20170612134330.20971-3-logout@...260...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14795.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Openvpn-devel] [PATCH 3/8] OpenSSL: don't use direct access to the internal of RSA
2017-06-12 13:43 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 logout
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 1/8] OpenSSL: don't use direct access to the internal of X509 logout
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 2/8] OpenSSL: don't use direct access to the internal of EVP_PKEY logout
@ 2017-06-12 13:43 ` logout
2017-06-18 8:38 ` Steffan Karger
2017-06-18 10:38 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 4/8] OpenSSL: don't use direct access to the internal of DSA logout
` (5 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: logout @ 2017-06-12 13:43 UTC (permalink / raw)
To: openvpn-devel
From: Emmanuel Deloget <logout@...260...>
OpenSSL 1.1 does not allow us to directly access the internal of
any data type, including RSA. We have to use the defined
functions to do so.
Compatibility with OpenSSL 1.0 is kept by defining the corresponding
functions when they are not found in the library.
Signed-off-by: Emmanuel Deloget <logout@...260...>
---
configure.ac | 4 ++
src/openvpn/openssl_compat.h | 100 +++++++++++++++++++++++++++++++++++++++++++
src/openvpn/ssl_openssl.c | 24 +++++++----
3 files changed, 119 insertions(+), 9 deletions(-)
diff --git a/configure.ac b/configure.ac
index a92e8142..4c5f28ed 100644
--- a/configure.ac
+++ b/configure.ac
@@ -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 \
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 0d82cf25..44e3e167 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -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
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 1c73641c..2a42d3c6 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -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)
--
2.11.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH 3/8] OpenSSL: don't use direct access to the internal of RSA
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 3/8] OpenSSL: don't use direct access to the internal of RSA logout
@ 2017-06-18 8:38 ` Steffan Karger
2017-06-18 10:38 ` [Openvpn-devel] [PATCH applied] " Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Steffan Karger @ 2017-06-18 8:38 UTC (permalink / raw)
To: openvpn-devel
Hi,
On 12-06-17 15:43, logout@...260... wrote:
> From: Emmanuel Deloget <logout@...260...>
>
> OpenSSL 1.1 does not allow us to directly access the internal of
> any data type, including RSA. We have to use the defined
> functions to do so.
>
> Compatibility with OpenSSL 1.0 is kept by defining the corresponding
> functions when they are not found in the library.
>
> Signed-off-by: Emmanuel Deloget <logout@...260...>
> ---
> configure.ac | 4 ++
> src/openvpn/openssl_compat.h | 100 +++++++++++++++++++++++++++++++++++++++++++
> src/openvpn/ssl_openssl.c | 24 +++++++----
> 3 files changed, 119 insertions(+), 9 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index a92e8142..4c5f28ed 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -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 \
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index 0d82cf25..44e3e167 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -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
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 1c73641c..2a42d3c6 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -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)
>
Code looks good, and works as advertised. ACK.
-Steffan
^ permalink raw reply [flat|nested] 29+ messages in thread* [Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of RSA
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 3/8] OpenSSL: don't use direct access to the internal of RSA logout
2017-06-18 8:38 ` Steffan Karger
@ 2017-06-18 10:38 ` Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Gert Doering @ 2017-06-18 10:38 UTC (permalink / raw)
To: Emmanuel Deloget <logout@; +Cc: openvpn-devel
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 <logout@...260...>
Acked-by: Steffan Karger <steffan.karger@...1435...>
Message-Id: <20170612134330.20971-4-logout@...260...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14790.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Openvpn-devel] [PATCH 4/8] OpenSSL: don't use direct access to the internal of DSA
2017-06-12 13:43 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 logout
` (2 preceding siblings ...)
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 3/8] OpenSSL: don't use direct access to the internal of RSA logout
@ 2017-06-12 13:43 ` logout
2017-06-18 8:40 ` Steffan Karger
2017-06-18 10:38 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 5/8] OpenSSL: don't use direct access to the internal of EVP_MD_CTX logout
` (4 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: logout @ 2017-06-12 13:43 UTC (permalink / raw)
To: openvpn-devel
From: Emmanuel Deloget <logout@...260...>
OpenSSL 1.1 does not allow us to directly access the internal of
any data type, including DSA. We have to use the defined
functions to do so.
Compatibility with OpenSSL 1.0 is kept by defining the corresponding
functions when they are not found in the library.
Signed-off-by: Emmanuel Deloget <logout@...260...>
---
configure.ac | 2 ++
src/openvpn/openssl_compat.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
src/openvpn/ssl_openssl.c | 6 +++---
3 files changed, 49 insertions(+), 3 deletions(-)
diff --git a/configure.ac b/configure.ac
index 4c5f28ed..6eded4e6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -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 \
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 44e3e167..24efa0fd 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -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
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 2a42d3c6..5faeafcc 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -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);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH 4/8] OpenSSL: don't use direct access to the internal of DSA
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 4/8] OpenSSL: don't use direct access to the internal of DSA logout
@ 2017-06-18 8:40 ` Steffan Karger
2017-06-18 10:38 ` [Openvpn-devel] [PATCH applied] " Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Steffan Karger @ 2017-06-18 8:40 UTC (permalink / raw)
To: openvpn-devel
On 12-06-17 15:43, logout@...260... wrote:
> From: Emmanuel Deloget <logout@...260...>
>
> OpenSSL 1.1 does not allow us to directly access the internal of
> any data type, including DSA. We have to use the defined
> functions to do so.
>
> Compatibility with OpenSSL 1.0 is kept by defining the corresponding
> functions when they are not found in the library.
>
> Signed-off-by: Emmanuel Deloget <logout@...260...>
> ---
> configure.ac | 2 ++
> src/openvpn/openssl_compat.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
> src/openvpn/ssl_openssl.c | 6 +++---
> 3 files changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 4c5f28ed..6eded4e6 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -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 \
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index 44e3e167..24efa0fd 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -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
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 2a42d3c6..5faeafcc 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -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);
> }
>
Looks good too now, ACK.
-Steffan
^ permalink raw reply [flat|nested] 29+ messages in thread* [Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of DSA
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 4/8] OpenSSL: don't use direct access to the internal of DSA logout
2017-06-18 8:40 ` Steffan Karger
@ 2017-06-18 10:38 ` Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Gert Doering @ 2017-06-18 10:38 UTC (permalink / raw)
To: Emmanuel Deloget <logout@; +Cc: openvpn-devel
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 <logout@...260...>
Acked-by: Steffan Karger <steffan.karger@...1435...>
Message-Id: <20170612134330.20971-5-logout@...260...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14791.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Openvpn-devel] [PATCH 5/8] OpenSSL: don't use direct access to the internal of EVP_MD_CTX
2017-06-12 13:43 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 logout
` (3 preceding siblings ...)
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 4/8] OpenSSL: don't use direct access to the internal of DSA logout
@ 2017-06-12 13:43 ` logout
2017-06-18 10:20 ` Steffan Karger
2017-06-18 13:20 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 6/8] OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX logout
` (3 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: logout @ 2017-06-12 13:43 UTC (permalink / raw)
To: openvpn-devel
From: Emmanuel Deloget <logout@...260...>
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.
Signed-off-by: Emmanuel Deloget <logout@...260...>
---
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(-)
diff --git a/configure.ac b/configure.ac
index 6eded4e6..6ac4e595 100644
--- a/configure.ac
+++ b/configure.ac
@@ -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 \
diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index 9b113d7b..8f03e2ba 100644
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -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
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 942684ce..d6741523 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -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)
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 881a2d13..fd599f40 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -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
diff --git a/src/openvpn/httpdigest.c b/src/openvpn/httpdigest.c
index ae4a638f..2a66d9b8 100644
--- a/src/openvpn/httpdigest.c
+++ b/src/openvpn/httpdigest.c
@@ -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);
}
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 68d06876..5a997750 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -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
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 24efa0fd..b315bcb7 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -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>
+
+#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
diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index f8682d17..21899bb9 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -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;
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index bcef0ef4..908b650f 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -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;
--
2.11.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH 5/8] OpenSSL: don't use direct access to the internal of EVP_MD_CTX
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 5/8] OpenSSL: don't use direct access to the internal of EVP_MD_CTX logout
@ 2017-06-18 10:20 ` Steffan Karger
2017-06-18 13:20 ` [Openvpn-devel] [PATCH applied] " Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Steffan Karger @ 2017-06-18 10:20 UTC (permalink / raw)
To: openvpn-devel
Hi,
On 12-06-17 15:43, logout@...260... wrote:
> From: Emmanuel Deloget <logout@...260...>
>
> 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.
>
> Signed-off-by: Emmanuel Deloget <logout@...260...>
> ---
> 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(-)
>
> diff --git a/configure.ac b/configure.ac
> index 6eded4e6..6ac4e595 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -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 \
> diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
> index 9b113d7b..8f03e2ba 100644
> --- a/src/openvpn/crypto_backend.h
> +++ b/src/openvpn/crypto_backend.h
> @@ -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
> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
> index 942684ce..d6741523 100644
> --- a/src/openvpn/crypto_mbedtls.c
> +++ b/src/openvpn/crypto_mbedtls.c
> @@ -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)
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index 881a2d13..fd599f40 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -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
> diff --git a/src/openvpn/httpdigest.c b/src/openvpn/httpdigest.c
> index ae4a638f..2a66d9b8 100644
> --- a/src/openvpn/httpdigest.c
> +++ b/src/openvpn/httpdigest.c
> @@ -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);
> }
>
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 68d06876..5a997750 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -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
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.
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index 24efa0fd..b315bcb7 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -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?
> +#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
> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
> index f8682d17..21899bb9 100644
> --- a/src/openvpn/openvpn.h
> +++ b/src/openvpn/openvpn.h
> @@ -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;
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index bcef0ef4..908b650f 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -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;
>
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
^ permalink raw reply [flat|nested] 29+ messages in thread* [Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of EVP_MD_CTX
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 5/8] OpenSSL: don't use direct access to the internal of EVP_MD_CTX logout
2017-06-18 10:20 ` Steffan Karger
@ 2017-06-18 13:20 ` Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Gert Doering @ 2017-06-18 13:20 UTC (permalink / raw)
To: Emmanuel Deloget <logout@; +Cc: openvpn-devel
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 <logout@...260...>
Acked-by: Steffan Karger <steffan.karger@...1435...>
Message-Id: <20170612134330.20971-6-logout@...260...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14793.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Openvpn-devel] [PATCH 6/8] OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX
2017-06-12 13:43 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 logout
` (4 preceding siblings ...)
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 5/8] OpenSSL: don't use direct access to the internal of EVP_MD_CTX logout
@ 2017-06-12 13:43 ` logout
2017-06-18 11:16 ` Steffan Karger
2017-06-18 13:21 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 7/8] OpenSSL: don't use direct access to the internal of HMAC_CTX logout
` (2 subsequent siblings)
8 siblings, 2 replies; 29+ messages in thread
From: logout @ 2017-06-12 13:43 UTC (permalink / raw)
To: openvpn-devel
From: Emmanuel Deloget <logout@...260...>
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.
Signed-off-by: Emmanuel Deloget <logout@...260...>
---
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(-)
diff --git a/configure.ac b/configure.ac
index 6ac4e595..e895cf0a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -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 \
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 50e6a734..893879cf 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -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)
diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index 8f03e2ba..3a911a47 100644
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -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
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index d6741523..4d38aadc 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -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,
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index fd599f40..0644f1c3 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -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))
{
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index b315bcb7..3f36212a 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -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
--
2.11.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH 6/8] OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 6/8] OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX logout
@ 2017-06-18 11:16 ` Steffan Karger
2017-06-18 13:21 ` [Openvpn-devel] [PATCH applied] " Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Steffan Karger @ 2017-06-18 11:16 UTC (permalink / raw)
To: openvpn-devel
Hi,
On 12-06-17 15:43, logout@...260... wrote:
> From: Emmanuel Deloget <logout@...260...>
>
> 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.
>
> Signed-off-by: Emmanuel Deloget <logout@...260...>
> ---
> 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(-)
>
> diff --git a/configure.ac b/configure.ac
> index 6ac4e595..e895cf0a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -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 \
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 50e6a734..893879cf 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -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)
> diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
> index 8f03e2ba..3a911a47 100644
> --- a/src/openvpn/crypto_backend.h
> +++ b/src/openvpn/crypto_backend.h
> @@ -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
> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
> index d6741523..4d38aadc 100644
> --- a/src/openvpn/crypto_mbedtls.c
> +++ b/src/openvpn/crypto_mbedtls.c
> @@ -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,
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index fd599f40..0644f1c3 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -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))
> {
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index b315bcb7..3f36212a 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -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
>
ACK
Code looks good, and passes my mbed and openssl tests.
-Steffan
^ permalink raw reply [flat|nested] 29+ messages in thread* [Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 6/8] OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX logout
2017-06-18 11:16 ` Steffan Karger
@ 2017-06-18 13:21 ` Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Gert Doering @ 2017-06-18 13:21 UTC (permalink / raw)
To: Emmanuel Deloget <logout@; +Cc: openvpn-devel
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 <logout@...260...>
Acked-by: Steffan Karger <steffan.karger@...1435...>
Message-Id: <20170612134330.20971-7-logout@...260...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14796.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Openvpn-devel] [PATCH 7/8] OpenSSL: don't use direct access to the internal of HMAC_CTX
2017-06-12 13:43 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 logout
` (5 preceding siblings ...)
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 6/8] OpenSSL: don't use direct access to the internal of EVP_CIPHER_CTX logout
@ 2017-06-12 13:43 ` logout
2017-06-18 11:38 ` Steffan Karger
2017-06-18 13:21 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 8/8] OpenSSL: force meth->name as non-const when we free() it logout
2017-06-15 12:04 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 Jeremie Courreges-Anglas
8 siblings, 2 replies; 29+ messages in thread
From: logout @ 2017-06-12 13:43 UTC (permalink / raw)
To: openvpn-devel
From: Emmanuel Deloget <logout@...260...>
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.
Signed-off-by: Emmanuel Deloget <logout@...260...>
---
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(-)
diff --git a/configure.ac b/configure.ac
index e895cf0a..e5c5c7df 100644
--- a/configure.ac
+++ b/configure.ac
@@ -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 \
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 893879cf..a80c3f7f 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -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;
diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index 3a911a47..3dc75ab9 100644
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -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.
*
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 4d38aadc..f0698f61 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -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)
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 0644f1c3..b64f7f04 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -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
diff --git a/src/openvpn/ntlm.c b/src/openvpn/ntlm.c
index 93483379..07a5d25f 100644
--- a/src/openvpn/ntlm.c
+++ b/src/openvpn/ntlm.c
@@ -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
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 3f36212a..71ecfc06 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -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.
+ *
+ * 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
+
+#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
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 3d5e8241..55cc457d 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -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));
--
2.11.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH 7/8] OpenSSL: don't use direct access to the internal of HMAC_CTX
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 7/8] OpenSSL: don't use direct access to the internal of HMAC_CTX logout
@ 2017-06-18 11:38 ` Steffan Karger
2017-06-19 14:04 ` Emmanuel Deloget
2017-06-18 13:21 ` [Openvpn-devel] [PATCH applied] " Gert Doering
1 sibling, 1 reply; 29+ messages in thread
From: Steffan Karger @ 2017-06-18 11:38 UTC (permalink / raw)
To: openvpn-devel
Hi,
On 12-06-17 15:43, logout@...260... wrote:
> From: Emmanuel Deloget <logout@...260...>
>
> 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.
>
> Signed-off-by: Emmanuel Deloget <logout@...260...>
> ---
> 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(-)
>
> diff --git a/configure.ac b/configure.ac
> index e895cf0a..e5c5c7df 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -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 \
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 893879cf..a80c3f7f 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -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;
> diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
> index 3a911a47..3dc75ab9 100644
> --- a/src/openvpn/crypto_backend.h
> +++ b/src/openvpn/crypto_backend.h
> @@ -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.
> *
> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
> index 4d38aadc..f0698f61 100644
> --- a/src/openvpn/crypto_mbedtls.c
> +++ b/src/openvpn/crypto_mbedtls.c
> @@ -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)
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index 0644f1c3..b64f7f04 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -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
> diff --git a/src/openvpn/ntlm.c b/src/openvpn/ntlm.c
> index 93483379..07a5d25f 100644
> --- a/src/openvpn/ntlm.c
> +++ b/src/openvpn/ntlm.c
> @@ -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
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index 3f36212a..71ecfc06 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -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.
> + *
> + * 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?
> +
> +#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
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 3d5e8241..55cc457d 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -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));
>
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH 7/8] OpenSSL: don't use direct access to the internal of HMAC_CTX
2017-06-18 11:38 ` Steffan Karger
@ 2017-06-19 14:04 ` Emmanuel Deloget
0 siblings, 0 replies; 29+ messages in thread
From: Emmanuel Deloget @ 2017-06-19 14:04 UTC (permalink / raw)
To: Steffan Karger <steffan@; +Cc: openvpn-devel
[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]
Hello Steffan,
On Sun, Jun 18, 2017 at 1:38 PM, Steffan Karger <steffan@...1856...> wrote:
> Hi,
>
> On 12-06-17 15:43, logout@...260... wrote:
> > +#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)
> > +{
> > + 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.
> > <snip
> >
>
> 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
[-- Attachment #2: Type: text/html, Size: 3167 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of HMAC_CTX
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 7/8] OpenSSL: don't use direct access to the internal of HMAC_CTX logout
2017-06-18 11:38 ` Steffan Karger
@ 2017-06-18 13:21 ` Gert Doering
2017-06-19 14:06 ` Emmanuel Deloget
1 sibling, 1 reply; 29+ messages in thread
From: Gert Doering @ 2017-06-18 13:21 UTC (permalink / raw)
To: Emmanuel Deloget <logout@; +Cc: openvpn-devel
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 <logout@...260...>
Acked-by: Steffan Karger <steffan.karger@...1435...>
Message-Id: <20170612134330.20971-8-logout@...260...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14797.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH applied] Re: OpenSSL: don't use direct access to the internal of HMAC_CTX
2017-06-18 13:21 ` [Openvpn-devel] [PATCH applied] " Gert Doering
@ 2017-06-19 14:06 ` Emmanuel Deloget
0 siblings, 0 replies; 29+ messages in thread
From: Emmanuel Deloget @ 2017-06-19 14:06 UTC (permalink / raw)
To: Gert Doering <gert@; +Cc: openvpn-devel
[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]
Hi Gert,
On Sun, Jun 18, 2017 at 3:21 PM, Gert Doering <gert@...1296...> wrote:
> 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.
>
> (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.
>
> 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 <logout@...260...>
> Acked-by: Steffan Karger <steffan.karger@...1435...>
> Message-Id: <20170612134330.20971-8-logout@...260...>
> URL: https://www.mail-archive.com/openvpn-devel@...1199...
> sourceforge.net/msg14797.html
> Signed-off-by: Gert Doeri
>
> ng <gert@...1296...>
>
>
> --
> kind regards,
>
> Gert Doering
>
>
BR,
-- Emmanuel Deloget
[-- Attachment #2: Type: text/html, Size: 3154 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Openvpn-devel] [PATCH 8/8] OpenSSL: force meth->name as non-const when we free() it
2017-06-12 13:43 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 logout
` (6 preceding siblings ...)
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 7/8] OpenSSL: don't use direct access to the internal of HMAC_CTX logout
@ 2017-06-12 13:43 ` logout
2017-06-18 7:27 ` Steffan Karger
2017-06-18 11:43 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2017-06-15 12:04 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 Jeremie Courreges-Anglas
8 siblings, 2 replies; 29+ messages in thread
From: logout @ 2017-06-12 13:43 UTC (permalink / raw)
To: openvpn-devel
From: Emmanuel Deloget <logout@...260...>
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(-)
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 71ecfc06..69db6fb5 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -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);
}
}
--
2.11.0
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH 8/8] OpenSSL: force meth->name as non-const when we free() it
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 8/8] OpenSSL: force meth->name as non-const when we free() it logout
@ 2017-06-18 7:27 ` Steffan Karger
2017-06-18 11:43 ` [Openvpn-devel] [PATCH applied] " Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Steffan Karger @ 2017-06-18 7:27 UTC (permalink / raw)
To: openvpn-devel
On 12-06-17 15:43, logout@...260... wrote:
> From: Emmanuel Deloget <logout@...260...>
>
> 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(-)
>
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index 71ecfc06..69db6fb5 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -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);
> }
> }
>
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
^ permalink raw reply [flat|nested] 29+ messages in thread* [Openvpn-devel] [PATCH applied] Re: OpenSSL: force meth->name as non-const when we free() it
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 8/8] OpenSSL: force meth->name as non-const when we free() it logout
2017-06-18 7:27 ` Steffan Karger
@ 2017-06-18 11:43 ` Gert Doering
1 sibling, 0 replies; 29+ messages in thread
From: Gert Doering @ 2017-06-18 11:43 UTC (permalink / raw)
To: Emmanuel Deloget <logout@; +Cc: openvpn-devel
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 <steffan.karger@...1435...>
Message-Id: <20170612134330.20971-9-logout@...260...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14798.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1
2017-06-12 13:43 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 logout
` (7 preceding siblings ...)
2017-06-12 13:43 ` [Openvpn-devel] [PATCH 8/8] OpenSSL: force meth->name as non-const when we free() it logout
@ 2017-06-15 12:04 ` Jeremie Courreges-Anglas
2017-06-15 15:37 ` Emmanuel Deloget
8 siblings, 1 reply; 29+ messages in thread
From: Jeremie Courreges-Anglas @ 2017-06-15 12:04 UTC (permalink / raw)
To: logout@; +Cc: openvpn-devel
[-- Attachment #1: Type: text/plain, Size: 369 bytes --]
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.
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1
2017-06-15 12:04 ` [Openvpn-devel] [PATCH v7 0/8] OpenSSL: support for version 1.1 Jeremie Courreges-Anglas
@ 2017-06-15 15:37 ` Emmanuel Deloget
0 siblings, 0 replies; 29+ messages in thread
From: Emmanuel Deloget @ 2017-06-15 15:37 UTC (permalink / raw)
To: Emmanuel Deloget <logout@
[-- Attachment #1: Type: text/plain, Size: 801 bytes --]
Hi Jeremie,
On Thu, Jun 15, 2017 at 2:04 PM, Jeremie Courreges-Anglas <jca@...2272...>
wrote:
>
> 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
[-- Attachment #2: Type: text/html, Size: 1907 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread