openvpn-devel

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

[Steffan K. <st...@ka...>]

On 05-03-17 10:53, Gert Doering wrote:
> Small side note: I assume that RSA_meth_new() can fail and return NULL
> in OpenSSL 1.1?  Because for 1.0, the "check_malloc_return(rsa_meth)" call 
> isn't necessary, as ALLOC_OBJ_CLEAR() would call ALLOC_OBJ() and that
> already checks...  (mentioning this here in case someone wonders and goes
> to the list archives).

For the archives: yes, RSA_meth_new() indeed returns NULL if it's
internal malloc() call fails.

-Steffan

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

[Emmanuel D. <lo...@fr...>]

Hi,

On Sun, Mar 5, 2017 at 11:29 AM, Steffan Karger <st...@ka...> wrote:
>
> On 05-03-17 10:53, Gert Doering wrote:
>> Small side note: I assume that RSA_meth_new() can fail and return NULL
>> in OpenSSL 1.1?  Because for 1.0, the "check_malloc_return(rsa_meth)" call
>> isn't necessary, as ALLOC_OBJ_CLEAR() would call ALLOC_OBJ() and that
>> already checks...  (mentioning this here in case someone wonders and goes
>> to the list archives).
>
> For the archives: yes, RSA_meth_new() indeed returns NULL if it's
> internal malloc() call fails.

Yes, indeed. And that's the reason why I have a check_malloc_return()
here. I'm perfectly conscious that for OpenSSL < 1.1 we're checking
the pointer twice but on the other hand I would have missed the check
with OpenSSL 1.1. A solution would have been to use a direct
malloc()/calloc() call instead of ALLOC_OBJ_CLEAR() in the
compatibility code, but that would have looked weird. Another solution
would have been to encapsulate RSA_meth_new() but I don't think that
would have been a good idea (yet, I might be wrong on that one). So I
did this choice -- I don't like it much either but I cannot think of a
better solution.

> -Steffan

Best regards,

-- Emmanuel Deloget

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

[Gert D. <ge...@gr...>]

Hi,

On Sun, Mar 05, 2017 at 01:08:09PM +0100, Emmanuel Deloget wrote:
> I don't like it much either but I cannot think of a better solution.

I haven't said that I don't *like* it :-) - I came across it when 
doing a "post commit sanity check" and assumed that there are good 
reasons.  Just wanted to clarify this, in case someone else looks
at the call chain and wonders.

All is well :-)

gert

-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

[Openvpn-devel] [PATCH v3 07/15] OpenSSL: don't use direct access to the internal of RSA

[Emmanuel D. <lo...@fr...>]

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 <lo...@fr...>
---
 configure.ac                 |  3 ++
 src/openvpn/openssl_compat.h | 84 ++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_openssl.c    | 24 ++++++++-----
 3 files changed, 103 insertions(+), 8 deletions(-)

diff --git a/configure.ac b/configure.ac
index 089aa89459803234f847579d393c2be6e17d958a..e9942e38f5675c8bcfcc30f5e5c30731a08abba2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -909,6 +909,9 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			EVP_PKEY_id \
 			EVP_PKEY_get0_RSA \
 			EVP_PKEY_get0_DSA \
+			RSA_set_flags \
+			RSA_get0_key \
+			RSA_set0_key \
 			RSA_meth_new \
 			RSA_meth_free \
 			RSA_meth_set_pub_enc \
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 9d99c72920bd9ab450809e6d3247db846d4bd564..0f7b0bb799d2c5e506b6372aa9cc023e8cbf3b29 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -176,6 +176,90 @@ EVP_PKEY_get0_DSA(EVP_PKEY *pkey)
 }
 #endif
 
+#if !defined(HAVE_RSA_SET_FLAGS)
+/**
+ * Set the RSA flags
+ *
+ * @param rsa                 The RSA object
+ * @param flags               New flags value
+ */
+static inline void
+RSA_set_flags(RSA *rsa, int flags)
+{
+    if (rsa)
+    {
+        rsa->flags = flags;
+    }
+}
+#endif
+
+#if !defined(HAVE_RSA_GET0_KEY)
+/**
+ * Get the RSA parameters
+ *
+ * @param rsa                 The RSA object
+ * @param n                   The @c n parameter
+ * @param e                   The @c e parameter
+ * @param d                   The @c d parameter
+ */
+static inline void
+RSA_get0_key(const RSA *rsa, const BIGNUM **n,
+             const BIGNUM **e, const BIGNUM **d)
+{
+    if (n != NULL)
+    {
+        *n = rsa ? rsa->n : NULL;
+    }
+    if (e != NULL)
+    {
+        *e = rsa ? rsa->e : NULL;
+    }
+    if (d != NULL)
+    {
+        *d = rsa ? rsa->d : NULL;
+    }
+}
+#endif
+
+#if !defined(HAVE_RSA_SET0_KEY)
+/**
+ * Set the RSA parameters
+ *
+ * @param rsa                 The RSA object
+ * @param n                   The @c n parameter
+ * @param e                   The @c e parameter
+ * @param d                   The @c d parameter
+ * @return                    1 on success, 0 on error
+ */
+static inline int
+RSA_set0_key(RSA *rsa, BIGNUM *n, BIGNUM *e, BIGNUM *d)
+{
+    if ((rsa->n == NULL && n == NULL)
+        || (rsa->e == NULL && e == NULL))
+    {
+        return 0;
+    }
+
+    if (n != NULL)
+    {
+        BN_free(rsa->n);
+        rsa->n = n;
+    }
+    if (e != NULL)
+    {
+        BN_free(rsa->e);
+        rsa->e = e;
+    }
+    if (d != NULL)
+    {
+        BN_free(rsa->d);
+        rsa->d = d;
+    }
+
+    return 1;
+}
+#endif
+
 #if !defined(HAVE_RSA_METH_NEW)
 /**
  * Allocate a new RSA method object
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index dbeb868ebe89f8c18a7b89afd797c3e42dda1503..25935b45dcac6dfc5c9d034fe14e7b29ec1cc1c3 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -978,8 +978,8 @@ rsa_priv_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
 static int
 rsa_finish(RSA *rsa)
 {
-    RSA_meth_free(rsa->meth);
-    rsa->meth = NULL;
+    const RSA_METHOD *meth = RSA_get_method(rsa);
+    RSA_meth_free((RSA_METHOD *)meth);
     return 1;
 }
 
@@ -1078,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;
@@ -1680,11 +1683,16 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix)
         EVP_PKEY *pkey = X509_get_pubkey(cert);
         if (pkey != NULL)
         {
-            if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL
-                && pkey->pkey.rsa->n != NULL)
+            if (EVP_PKEY_id(pkey) == EVP_PKEY_RSA && EVP_PKEY_get0_RSA(pkey) != NULL)
             {
-                openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
-                                 BN_num_bits(pkey->pkey.rsa->n));
+                RSA *rsa = EVP_PKEY_get0_RSA(pkey);
+                const BIGNUM *n = NULL;
+                RSA_get0_key(rsa, &n, NULL, NULL);
+                if (n != NULL)
+                {
+                    openvpn_snprintf(s2, sizeof(s2), ", %d bit RSA",
+                                     BN_num_bits(n));
+                }
             }
             else if (EVP_PKEY_id(pkey) == EVP_PKEY_DSA && EVP_PKEY_get0_DSA(pkey) != NULL
                      && pkey->pkey.dsa->p != NULL)
-- 
2.7.4

Re: [Openvpn-devel] [PATCH v3 00/15] Add support for OpenSSL 1.1.x

[Christian H. <li...@ew...>]

Emmanuel Deloget <lo...@fr...> on Thu, 2017/02/23 15:35:
> This is v3 of the remaining patches for the "Add support for OpenSSL 
> 1.1.x" series. This series is partial: only the modified patches are 
> sent to the ML -- the other have not changed. The stats are a bit off 
> so I don't include them in this mail.
> 
> They have been generated after a rebase from the master tree. Individual
> commits can be viewed at
> 
>   https://github.com/emmanuel-deloget/openvpn/commits/openssl-1.1-v3
> 
> (This time, the branch name is correct :))
> 
> Changes v2 --> v3: 
> 
> * RSA_METHOD (04/15): rsa_meth->name is now a dup of the name parameter; 
>   it's freed in RSA_meth_free(). 
> 
> * RSA (07/15): calling RSA_set_method() in rsa_finish() is both a Bad 
>   Idea and not required so it has been removed.
>   
> Changes v1 --> v2:
> 
> * EVP_PKEY (06/15): add missing function EVP_PKEY_id() for 0.9.8.
> 
> * replace patch 15/15 with a new patch to use EVP_CipherInit_ex() 
>   instead of EVP_CipherInit() when a full init is not needed.
> 
> 
> Emmanuel Deloget (15):
>   [commited] OpenSSL: don't use direct access to the internal of SSL_CTX
>   [commited] OpenSSL: don't use direct access to the internal of X509_STORE
>   [commited] OpenSSL: don't use direct access to the internal of X509_OBJECT
>   OpenSSL: don't use direct access to the internal of RSA_METHOD
>   OpenSSL: don't use direct access to the internal of X509
>   OpenSSL: don't use direct access to the internal of EVP_PKEY
>   OpenSSL: don't use direct access to the internal of RSA
>   OpenSSL: don't use direct access to the internal of DSA
>   [commited] OpenSSL: don't use direct access to the internal of
> X509_STORE_CTX OpenSSL: don't use direct access to the internal of
> EVP_MD_CTX OpenSSL: don't use direct access to the internal of
> EVP_CIPHER_CTX OpenSSL: don't use direct access to the internal of HMAC_CTX
>   OpenSSL: SSLeay symbols are no longer available in OpenSSL 1.1
>   OpenSSL: constify getbio() parameters
>   OpenSSL: use EVP_CipherInit_ex() instead of EVP_CipherInit()

Built v3 against openssl 1.0.2k without issues, tests succeed and two
instanced successfully established vpn connection (with server version 2.3.12
and 2.4.0).

Built against openssl 1.1.0e without issues, tests succeed. Did not test with
real world connectivity, though.
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}

Re: [Openvpn-devel] [PATCH v3 00/15] Add support for OpenSSL 1.1.x

[Christian H. <li...@ew...>]

Christian Hesse <li...@ew...> on Thu, 2017/02/23 21:57:
> Built v3 against openssl 1.0.2k without issues, tests succeed and two
> instanced successfully established vpn connection (with server version
> 2.3.12 and 2.4.0).

Just tested a server instance with ancient client (version 2.1.4). Works as
well.

I will try to restart another server instance later.
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}

Re: [Openvpn-devel] [PATCH v3 00/15] Add support for OpenSSL 1.1.x

[Christian H. <li...@ew...>]

Christian Hesse <li...@ew...> on Fri, 2017/02/24 13:13:
> Christian Hesse <li...@ew...> on Thu, 2017/02/23 21:57:
> > Built v3 against openssl 1.0.2k without issues, tests succeed and two
> > instanced successfully established vpn connection (with server version
> > 2.3.12 and 2.4.0).  
> 
> Just tested a server instance with ancient client (version 2.1.4). Works as
> well.
> 
> I will try to restart another server instance later.

Just restarted the server. Here is the current client connect statistic:

      1  2.3.8
      4  2.3.12
      6  2.3.13
      7  2.3.11
     18  2.3.14
     70  2.4.0

So looks good for now. ;)
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}

Re: [Openvpn-devel] [RFC PATCH v1 09/15] OpenSSL: don't use direct access to the internal of X509_STORE_CTX

[Steffan K. <st...@ka...>]

Hi,

On 17-02-17 23:00, lo...@fr... wrote:
> From: Emmanuel Deloget <lo...@fr...>
> 
> OpenSSL 1.1 does not allow us to directly access the internal of
> any data type, including X509_STORE_CTX. We have to use the defined
> functions to do so.
> 
> Fortunately, these functions have existed since the dawn of time so
> we don't have any compatibility issue here.
> 
> Signed-off-by: Emmanuel Deloget <lo...@fr...>
> ---
>  src/openvpn/ssl_verify_openssl.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
> index edc709b89eb05bca895639dde606b29f8e1f7024..5bdd1e3609c4a2693e16c0835a9e5c39babd5ff8 100644
> --- a/src/openvpn/ssl_verify_openssl.c
> +++ b/src/openvpn/ssl_verify_openssl.c
> @@ -62,14 +62,15 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
>      session = (struct tls_session *) SSL_get_ex_data(ssl, mydata_index);
>      ASSERT(session);
>  
> -    struct buffer cert_hash = x509_get_sha256_fingerprint(ctx->current_cert, &gc);
> -    cert_hash_remember(session, ctx->error_depth, &cert_hash);
> +    X509 *current_cert = X509_STORE_CTX_get_current_cert(ctx);
> +    struct buffer cert_hash = x509_get_sha256_fingerprint(current_cert, &gc);
> +    cert_hash_remember(session, X509_STORE_CTX_get_error_depth(ctx), &cert_hash);
>  
>      /* did peer present cert which was signed by our root cert? */
>      if (!preverify_ok)
>      {
>          /* get the X509 name */
> -        char *subject = x509_get_subject(ctx->current_cert, &gc);
> +        char *subject = x509_get_subject(current_cert, &gc);
>  
>          if (!subject)
>          {
> @@ -77,11 +78,11 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
>          }
>  
>          /* Log and ignore missing CRL errors */
> -        if (ctx->error == X509_V_ERR_UNABLE_TO_GET_CRL)
> +        if (X509_STORE_CTX_get_error(ctx) == X509_V_ERR_UNABLE_TO_GET_CRL)
>          {
>              msg(D_TLS_DEBUG_LOW, "VERIFY WARNING: depth=%d, %s: %s",
> -                ctx->error_depth,
> -                X509_verify_cert_error_string(ctx->error),
> +                X509_STORE_CTX_get_error_depth(ctx),
> +                X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
>                  subject);
>              ret = 1;
>              goto cleanup;
> @@ -89,8 +90,8 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
>  
>          /* Remote site specified a certificate, but it's not correct */
>          msg(D_TLS_ERRORS, "VERIFY ERROR: depth=%d, error=%s: %s",
> -            ctx->error_depth,
> -            X509_verify_cert_error_string(ctx->error),
> +            X509_STORE_CTX_get_error_depth(ctx),
> +            X509_verify_cert_error_string(X509_STORE_CTX_get_error(ctx)),
>              subject);
>  
>          ERR_clear_error();
> @@ -99,7 +100,7 @@ verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
>          goto cleanup;
>      }
>  
> -    if (SUCCESS != verify_cert(session, ctx->current_cert, ctx->error_depth))
> +    if (SUCCESS != verify_cert(session, current_cert, X509_STORE_CTX_get_error_depth(ctx)))
>      {
>          goto cleanup;
>      }
> 

ACK.  Changes look good and tested against OpenSSL 0.9.8, 1.0.0, 1.0.1
and 1.0.2.

-Steffan

Re: [Openvpn-devel] [RFC PATCH v1 09/15] OpenSSL: don't use direct access to the internal of X509_STORE_CTX

[Christian H. <li...@ew...>]

Steffan Karger <st...@ka...> on Tue, 2017/02/21 22:30:
> ACK.  Changes look good and tested against OpenSSL 0.9.8, 1.0.0, 1.0.1
> and 1.0.2.

You answered to a patch in the middle of a series. Does this ACK apply to the
complete series or just this patch?
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}

Re: [Openvpn-devel] [RFC PATCH v1 09/15] OpenSSL: don't use direct access to the internal of X509_STORE_CTX

[Steffan K. <st...@ka...>]

On 22 February 2017 at 15:47, Christian Hesse <li...@ew...> wrote:
> Steffan Karger <st...@ka...> on Tue, 2017/02/21 22:30:
>> ACK.  Changes look good and tested against OpenSSL 0.9.8, 1.0.0, 1.0.1
>> and 1.0.2.
>
> You answered to a patch in the middle of a series. Does this ACK apply to the
> complete series or just this patch?

Just this one.  Not much brains left last night, so I only reviewed
this rather simple and independent patch out of the series :)

-Steffan

Re: [Openvpn-devel] [RFC PATCH v1 04/15] OpenSSL: don't use direct access to the internal of RSA_METHOD

[Steffan K. <st...@ka...>]

Hi,

On 17-02-17 23:00, lo...@fr... wrote:
> From: Emmanuel Deloget <lo...@fr...>
> 
> OpenSSL 1.1 does not allow us to directly access the internal of
> any data type, including RSA_METHOD. We have to use the defined
> functions to do so.
> 
> Compatibility with OpenSSL 1.0 is kept by defining the corresponding
> functions when they are not found in the library.
> 
> Signed-off-by: Emmanuel Deloget <lo...@fr...>
> ---
>  configure.ac                 |   9 +++
>  src/openvpn/openssl_compat.h | 186 +++++++++++++++++++++++++++++++++++++++++++
>  src/openvpn/ssl_openssl.c    |  22 ++---
>  3 files changed, 206 insertions(+), 11 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 789ad08fbaa3b3fc4c95d2b7a22332c0a93aeab4..6f31609d0aeedd2c7841d271ecadd1aa6f3b11da 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -905,6 +905,15 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
>  			X509_STORE_get0_objects \
>  			X509_OBJECT_free \
>  			X509_OBJECT_get_type \
> +			RSA_meth_new \
> +			RSA_meth_free \
> +			RSA_meth_set_pub_enc \
> +			RSA_meth_set_pub_dec \
> +			RSA_meth_set_priv_enc \
> +			RSA_meth_set_priv_dec \
> +			RSA_meth_set_init \
> +			RSA_meth_set_finish \
> +			RSA_meth_set0_app_data \
>  		],
>  		,
>  		[]
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index 458a6adbe2b3fcd5ea63dcea6596cc24315d463c..b1748754f821f472cf9ed7083ade918336c9b075 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -41,6 +41,8 @@
>  #include "config-msvc.h"
>  #endif
>  
> +#include "buffer.h"
> +
>  #include <openssl/ssl.h>
>  #include <openssl/x509.h>
>  
> @@ -117,4 +119,188 @@ X509_OBJECT_get_type(const X509_OBJECT *obj)
>  }
>  #endif
>  
> +#if !defined(HAVE_RSA_METH_NEW)
> +/**
> + * Allocate a new RSA method object
> + *
> + * @param name               The object name
> + * @param flags              Configuration flags
> + * @return                   A new RSA method object
> + */
> +static inline RSA_METHOD *
> +RSA_meth_new(const char *name, int flags)
> +{
> +    RSA_METHOD *rsa_meth = NULL;
> +    ALLOC_OBJ_CLEAR(rsa_meth, RSA_METHOD);
> +    rsa_meth->name = name;
> +    rsa_meth->flags = flags;
> +    return rsa_meth;
> +}
> +#endif
> +
> +#if !defined(HAVE_RSA_METH_FREE)
> +/**
> + * Free an existing RSA_METHOD object
> + *
> + * @param meth               The RSA_METHOD object
> + */
> +static inline void
> +RSA_meth_free(RSA_METHOD *meth)
> +{
> +    free(meth);
> +}
> +#endif

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

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

  meth.smoke();
       ^^ might cause problems

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

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

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

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

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

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

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

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

-Steffan

Re: [Openvpn-devel] [RFC PATCH v1 04/15] OpenSSL: don't use direct access to the internal of RSA_METHOD

[Emmanuel D. <lo...@fr...>]

Hi Steffan,

On Wed, Feb 22, 2017 at 11:13 PM, Steffan Karger <st...@ka...> wrote:
> Hi,
>
> On 17-02-17 23:00, lo...@fr... wrote:
>> From: Emmanuel Deloget <lo...@fr...>
>>
>> OpenSSL 1.1 does not allow us to directly access the internal of
>> any data type, including RSA_METHOD. We have to use the defined
>> functions to do so.
>>
>> Compatibility with OpenSSL 1.0 is kept by defining the corresponding
>> functions when they are not found in the library.
>>
>> Signed-off-by: Emmanuel Deloget <lo...@fr...>
>> ---
>>  configure.ac                 |   9 +++
>>  src/openvpn/openssl_compat.h | 186 +++++++++++++++++++++++++++++++++++++++++++
>>  src/openvpn/ssl_openssl.c    |  22 ++---
>>  3 files changed, 206 insertions(+), 11 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 789ad08fbaa3b3fc4c95d2b7a22332c0a93aeab4..6f31609d0aeedd2c7841d271ecadd1aa6f3b11da 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -905,6 +905,15 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
>>                       X509_STORE_get0_objects \
>>                       X509_OBJECT_free \
>>                       X509_OBJECT_get_type \
>> +                     RSA_meth_new \
>> +                     RSA_meth_free \
>> +                     RSA_meth_set_pub_enc \
>> +                     RSA_meth_set_pub_dec \
>> +                     RSA_meth_set_priv_enc \
>> +                     RSA_meth_set_priv_dec \
>> +                     RSA_meth_set_init \
>> +                     RSA_meth_set_finish \
>> +                     RSA_meth_set0_app_data \
>>               ],
>>               ,
>>               []
>> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
>> index 458a6adbe2b3fcd5ea63dcea6596cc24315d463c..b1748754f821f472cf9ed7083ade918336c9b075 100644
>> --- a/src/openvpn/openssl_compat.h
>> +++ b/src/openvpn/openssl_compat.h
>> @@ -41,6 +41,8 @@
>>  #include "config-msvc.h"
>>  #endif
>>
>> +#include "buffer.h"
>> +
>>  #include <openssl/ssl.h>
>>  #include <openssl/x509.h>
>>
>> @@ -117,4 +119,188 @@ X509_OBJECT_get_type(const X509_OBJECT *obj)
>>  }
>>  #endif
>>
>> +#if !defined(HAVE_RSA_METH_NEW)
>> +/**
>> + * Allocate a new RSA method object
>> + *
>> + * @param name               The object name
>> + * @param flags              Configuration flags
>> + * @return                   A new RSA method object
>> + */
>> +static inline RSA_METHOD *
>> +RSA_meth_new(const char *name, int flags)
>> +{
>> +    RSA_METHOD *rsa_meth = NULL;
>> +    ALLOC_OBJ_CLEAR(rsa_meth, RSA_METHOD);
>> +    rsa_meth->name = name;
>> +    rsa_meth->flags = flags;
>> +    return rsa_meth;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_FREE)
>> +/**
>> + * Free an existing RSA_METHOD object
>> + *
>> + * @param meth               The RSA_METHOD object
>> + */
>> +static inline void
>> +RSA_meth_free(RSA_METHOD *meth)
>> +{
>> +    free(meth);
>> +}
>> +#endif
>
> I think it would be nicer to more closely mimic the 1.1 behaviour in
> RSA_meth_{new,free}(), and copy the name string in new() and free it
> again in free().  That could prevent a future use-after-free that would
> occur for pre-1.1.0, but not 1.1.0+:

I failed to see that when I implemented my solution. I'll give a look
as soon as possible.

>   char *mystring = calloc(50, 1);
>   RSA_METHOD *meth = RSA_meth_new(mystring, 0);
>   free(mystring);
>
>   meth.smoke();
>        ^^ might cause problems
>
> (Hint: use string_alloc(x, NULL).)
>
>> +
>> +#if !defined(HAVE_RSA_METH_SET_PUB_ENC)
>> +/**
>> + * Set the public encoding function of an RSA_METHOD object
>> + *
>> + * @param meth               The RSA_METHOD object
>> + * @param pub_enc            the public encoding function
>> + * @return                   1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_pub_enc(RSA_METHOD *meth,
>> +                     int (*pub_enc) (int flen, const unsigned char *from,
>> +                                     unsigned char *to, RSA *rsa,
>> +                                     int padding))
>> +{
>> +    if (meth)
>> +    {
>> +        meth->rsa_pub_enc = pub_enc;
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET_PUB_DEC)
>> +/**
>> + * Set the public decoding function of an RSA_METHOD object
>> + *
>> + * @param meth               The RSA_METHOD object
>> + * @param pub_dec            the public decoding function
>> + * @return                   1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_pub_dec(RSA_METHOD *meth,
>> +                     int (*pub_dec) (int flen, const unsigned char *from,
>> +                                     unsigned char *to, RSA *rsa,
>> +                                     int padding))
>> +{
>> +    if (meth)
>> +    {
>> +        meth->rsa_pub_dec = pub_dec;
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET_PRIV_ENC)
>> +/**
>> + * Set the private encoding function of an RSA_METHOD object
>> + *
>> + * @param meth               The RSA_METHOD object
>> + * @param priv_enc           the private encoding function
>> + * @return                   1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_priv_enc(RSA_METHOD *meth,
>> +                      int (*priv_enc) (int flen, const unsigned char *from,
>> +                                       unsigned char *to, RSA *rsa,
>> +                                       int padding))
>> +{
>> +    if (meth)
>> +    {
>> +        meth->rsa_priv_enc = priv_enc;
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET_PRIV_DEC)
>> +/**
>> + * Set the private decoding function of an RSA_METHOD object
>> + *
>> + * @param meth               The RSA_METHOD object
>> + * @param priv_dec           the private decoding function
>> + * @return                   1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_priv_dec(RSA_METHOD *meth,
>> +                      int (*priv_dec) (int flen, const unsigned char *from,
>> +                                       unsigned char *to, RSA *rsa,
>> +                                       int padding))
>> +{
>> +    if (meth)
>> +    {
>> +        meth->rsa_priv_dec = priv_dec;
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET_INIT)
>> +/**
>> + * Set the init function of an RSA_METHOD object
>> + *
>> + * @param meth               The RSA_METHOD object
>> + * @param init               the init function
>> + * @return                   1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_init(RSA_METHOD *meth, int (*init) (RSA *rsa))
>> +{
>> +    if (meth)
>> +    {
>> +        meth->init = init;
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET_FINISH)
>> +/**
>> + * Set the finish function of an RSA_METHOD object
>> + *
>> + * @param meth               The RSA_METHOD object
>> + * @param finish             the finish function
>> + * @return                   1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set_finish(RSA_METHOD *meth, int (*finish) (RSA *rsa))
>> +{
>> +    if (meth)
>> +    {
>> +        meth->finish = finish;
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +#endif
>> +
>> +#if !defined(HAVE_RSA_METH_SET0_APP_DATA)
>> +/**
>> + * Set the application data of an RSA_METHOD object
>> + *
>> + * @param meth               The RSA_METHOD object
>> + * @param app_data           Application data
>> + * @return                   1 on success, 0 on error
>> + */
>> +static inline int
>> +RSA_meth_set0_app_data(RSA_METHOD *meth, void *app_data)
>> +{
>> +    if (meth)
>> +    {
>> +        meth->app_data = app_data;
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +#endif
>> +
>>  #endif /* OPENSSL_COMPAT_H_ */
>> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
>> index bf0f643f25439f71cbfe71bf5a7e8eb834b0f012..f011e06702529ff34e91f6d0169d1adf8cc9d767 100644
>> --- a/src/openvpn/ssl_openssl.c
>> +++ b/src/openvpn/ssl_openssl.c
>> @@ -978,7 +978,7 @@ rsa_priv_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
>>  static int
>>  rsa_finish(RSA *rsa)
>>  {
>> -    free((void *)rsa->meth);
>> +    RSA_meth_free(rsa->meth);
>>      rsa->meth = NULL;
>>      return 1;
>>  }
>
> This change still works, but he follow up change in this method in 07/15
> causes problems:
>
>     static int
>     rsa_finish(RSA *rsa)
>     {
>    -    RSA_meth_free(rsa->meth);
>    -    rsa->meth = NULL;
>    +    RSA_METHOD *meth = (RSA_METHOD *)RSA_get_method(rsa);
>    +    RSA_meth_free(meth);
>    +    RSA_set_method(rsa, NULL);
>         return 1;
>     }
>
> Casting away const on the object returned by RSA_get_method() is a
> smell, but it really fails because RSA_set_method(rsa, NULL) calls
> rsa->meth->finish(), which is implemented by this very function.  That
> means that RSA_meth_free() will perform a double free on meth->name.
>
> I briefly looked into a fix, but didn't immediately see a nice solution
> here.  At least the RSA_set_method(rsa, RSA_get_default_method())
> doesn't work either, because you'll end up with rsa_finish() and
> RSA_set_method() calling each other infinitely...

Ouch. I failed to see that as well. Sorry :)

I'll try to find a better solution to this issue as well.

> (Noting this here, because the fix for 07 might cause changes to this
> patch too.)
>
>> @@ -1053,16 +1053,16 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
>>      ASSERT(NULL != cert);
>>
>>      /* allocate custom RSA method object */
>> -    ALLOC_OBJ_CLEAR(rsa_meth, RSA_METHOD);
>> -    rsa_meth->name = "OpenVPN external private key RSA Method";
>> -    rsa_meth->rsa_pub_enc = rsa_pub_enc;
>> -    rsa_meth->rsa_pub_dec = rsa_pub_dec;
>> -    rsa_meth->rsa_priv_enc = rsa_priv_enc;
>> -    rsa_meth->rsa_priv_dec = rsa_priv_dec;
>> -    rsa_meth->init = NULL;
>> -    rsa_meth->finish = rsa_finish;
>> -    rsa_meth->flags = RSA_METHOD_FLAG_NO_CHECK;
>> -    rsa_meth->app_data = NULL;
>> +    rsa_meth = RSA_meth_new("OpenVPN external private key RSA Method",
>> +                            RSA_METHOD_FLAG_NO_CHECK);
>> +    check_malloc_return(rsa_meth);
>> +    RSA_meth_set_pub_enc(rsa_meth, rsa_pub_enc);
>> +    RSA_meth_set_pub_dec(rsa_meth, rsa_pub_dec);
>> +    RSA_meth_set_priv_enc(rsa_meth, rsa_priv_enc);
>> +    RSA_meth_set_priv_dec(rsa_meth, rsa_priv_dec);
>> +    RSA_meth_set_init(rsa_meth, NULL);
>> +    RSA_meth_set_finish(rsa_meth, rsa_finish);
>> +    RSA_meth_set0_app_data(rsa_meth, NULL);
>>
>>      /* allocate RSA object */
>>      rsa = RSA_new();
>>
>
> -Steffan
>

Best regards,

-- Emmanuel Deloget

Re: [Openvpn-devel] [RFC PATCH v1 01/15] OpenSSL: don't use direct access to the internal of SSL_CTX

[Gert D. <ge...@gr...>]

Good morning,

On Fri, Feb 17, 2017 at 11:00:40PM +0100, lo...@fr... wrote:
> From: Emmanuel Deloget <lo...@fr...>
> 
> OpenSSL 1.1 does not allow us to directly access the internal of
> any data type, including SSL_CTX. We have to use the defined functions
> to do so.
> 
> Compatibility with OpenSSL 1.0 is kept by defining the corresponding
> functions when they are not found in the library.
> 
> Signed-off-by: Emmanuel Deloget <lo...@fr...>
> ---
>  configure.ac                 |  9 ++++++
>  src/openvpn/openssl_compat.h | 74 ++++++++++++++++++++++++++++++++++++++++++++

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

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

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

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



The first one is easily fixed, but I do not know how to tackle the 
second one - no access to a CentOS6/RHEL6 box, and not enough autoconf
clue to see this right away.  Patch *looks* good...  most likely just a 
stray "\" where none should be, or so...

Please :-)

gert
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

Re: [Openvpn-devel] [RFC PATCH v1 01/15] OpenSSL: don't use direct access to the internal of SSL_CTX

[Gert D. <ge...@gr...>]

Hi,

On Thu, Feb 23, 2017 at 09:03:47AM +0100, Gert Doering wrote:
> This patch brings two problems outside the "OpenSSL functionality" 
> part.
> 
>  - openssl_compat.h is not included in the built tarballs, so mingw builds
>    fail (and "builds for anyone building from tarballs" would break) ->
>    findable by running "make distcheck"

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

>  - configure.ac does something to CentOS 6 / RHEL 6 which makes configure
>    explode:
> 
> ...
> checking for linux/if_tun.h... yes
> checking tap-windows.h usability... no
> checking tap-windows.h presence... no
> checking for tap-windows.h... no
> checking whether TUNSETPERSIST is declared... yes
> checking for setcon in -lselinux... yes
> checking for pam_start in -lpam... yes
> checking for PKCS11_HELPER... no
> ./configure: line 21440: syntax error near unexpected token `fi'
> ./configure: line 21440: `fi'

This still needs investigation.

gert
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

[Openvpn-devel] [PATCH] OpenSSL: 1.1 fallout - fix configure on old autoconf

[Steffan K. <ste...@fo...>]

Older versions of autoconf generate an empty "else fi" block for empty
fields in an AC_CHECK_FUNCS() macro.  This breaks on e.g. RHEL6.

Signed-off-by: Steffan Karger <ste...@fo...>
---
 configure.ac | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 546a7d6..79ef52d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -912,9 +912,7 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			X509_STORE_get0_objects \
 			X509_OBJECT_free \
 			X509_OBJECT_get_type \
-		],
-		,
-		[]
+		]
 	)
 
 	CFLAGS="${saved_CFLAGS}"
-- 
2.7.4

[Openvpn-devel] [PATCH applied] Re: OpenSSL: 1.1 fallout - fix configure on old autoconf

[Gert D. <ge...@gr...>]

ACK, thanks.

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

commit 07372a0fdeb3638204d197d0614f776a0eb73ab9 (master)
commit b97a5cc044dc6db3f0e1f9f06a6f5da522f0a33a (release/2.4)
Author: Steffan Karger
Date:   Thu Feb 23 11:35:38 2017 +0100

     OpenSSL: 1.1 fallout - fix configure on old autoconf

     Signed-off-by: Steffan Karger <ste...@fo...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <148...@fo...>
     URL: http://www.mail-archive.com/search?l=mid&q=1...@fo...
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [RFC PATCH v1 01/15] OpenSSL: don't use direct access to the internal of SSL_CTX

[Emmanuel D. <lo...@fr...>]

Hello,

On Thu, Feb 23, 2017 at 10:23 AM, Gert Doering <ge...@gr...> wrote:
> Hi,
>
> On Thu, Feb 23, 2017 at 09:03:47AM +0100, Gert Doering wrote:
>> This patch brings two problems outside the "OpenSSL functionality"
>> part.
>>
>>  - openssl_compat.h is not included in the built tarballs, so mingw builds
>>    fail (and "builds for anyone building from tarballs" would break) ->
>>    findable by running "make distcheck"
>
> This has been fixed & pushed (so we can build windows snapshots again).
>
>>  - configure.ac does something to CentOS 6 / RHEL 6 which makes configure
>>    explode:
>>
>> ...
>> checking for linux/if_tun.h... yes
>> checking tap-windows.h usability... no
>> checking tap-windows.h presence... no
>> checking for tap-windows.h... no
>> checking whether TUNSETPERSIST is declared... yes
>> checking for setcon in -lselinux... yes
>> checking for pam_start in -lpam... yes
>> checking for PKCS11_HELPER... no
>> ./configure: line 21440: syntax error near unexpected token `fi'
>> ./configure: line 21440: `fi'
>
> This still needs investigation.

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

Best regards,

-- Emmanuel Deloget

Re: [Openvpn-devel] [RFC PATCH v1 01/15] OpenSSL: don't use direct access to the internal of SSL_CTX

[Steffan K. <ste...@fo...>]

On 23-02-17 10:31, Emmanuel Deloget wrote:
>>>  - configure.ac does something to CentOS 6 / RHEL 6 which makes configure
>>>    explode:
>>>
>>> ...
>>> checking for linux/if_tun.h... yes
>>> checking tap-windows.h usability... no
>>> checking tap-windows.h presence... no
>>> checking for tap-windows.h... no
>>> checking whether TUNSETPERSIST is declared... yes
>>> checking for setcon in -lselinux... yes
>>> checking for pam_start in -lpam... yes
>>> checking for PKCS11_HELPER... no
>>> ./configure: line 21440: syntax error near unexpected token `fi'
>>> ./configure: line 21440: `fi'
>>
>> This still needs investigation.
> 
> I'm taking the time to install a CentOS 6 on a VM. I'll test this asap.

I have a fix ready, but am busy with testing it on various platforms.

-Steffan

Re: [Openvpn-devel] [RFC PATCH v1 05/15] OpenSSL: don't use direct access to the internal of X509

[Steffan K. <st...@ka...>]

Hi,

On 17-02-17 23:00, lo...@fr... wrote:
> From: Emmanuel Deloget <lo...@fr...>
> 
> 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).

The nsCertType x509 extension is very old, and replaced by keyUsage and
extendedKeyUsage (supported by OpenVPN via --remote-cert-tls or
--remote-cert-ku and --remote-cert-eku).

Changing this to the stricter API without warning will probably break
some people's setups with certs that do have nsCertType set, but not
keyUsage and/or extendedKeyUsage, while leaving them guessing why.

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

> 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 <lo...@fr...>
> ---
>  configure.ac                     |  1 +
>  src/openvpn/openssl_compat.h     | 15 +++++++++++++++
>  src/openvpn/ssl_openssl.c        |  3 ++-
>  src/openvpn/ssl_verify_openssl.c | 28 +++++++++++++++++++---------
>  4 files changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 6f31609d0aeedd2c7841d271ecadd1aa6f3b11da..c41db3effbb26318be4f44009a5055e808b89b56 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -902,6 +902,7 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
>  		[ \
>  			SSL_CTX_get_default_passwd_cb \
>  			SSL_CTX_get_default_passwd_cb_userdata \
> +			X509_get0_pubkey \
>  			X509_STORE_get0_objects \
>  			X509_OBJECT_free \
>  			X509_OBJECT_get_type \
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index b1748754f821f472cf9ed7083ade918336c9b075..6a89b91b05e0370a50ac5a1cae20ae659e6c7634 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 f011e06702529ff34e91f6d0169d1adf8cc9d767..b683961d9e4e79b9ee04cfa7ecd1b377ade9651b 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -1073,7 +1073,8 @@ tls_ctx_use_external_private_key(struct tls_root_ctx *ctx,
>      }
>  
>      /* get the public key */
> -    ASSERT(cert->cert_info->key->pkey); /* NULL before SSL_CTX_use_certificate() is called */
> +    EVP_PKEY *pkey = X509_get0_pubkey(cert);
> +    ASSERT(pkey); /* NULL before SSL_CTX_use_certificate() is called */
>      pub_rsa = cert->cert_info->key->pkey->pkey.rsa;
>  
>      /* initialize RSA object */
> diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
> index 07975248035b48121d1383b47f40a56042bc7380..edc709b89eb05bca895639dde606b29f8e1f7024 100644
> --- a/src/openvpn/ssl_verify_openssl.c
> +++ b/src/openvpn/ssl_verify_openssl.c
> @@ -285,18 +285,20 @@ backend_x509_get_serial_hex(openvpn_x509_cert_t *cert, struct gc_arena *gc)
>  struct buffer
>  x509_get_sha1_fingerprint(X509 *cert, struct gc_arena *gc)
>  {
> -    struct buffer hash = alloc_buf_gc(sizeof(cert->sha1_hash), gc);
> -    memcpy(BPTR(&hash), cert->sha1_hash, sizeof(cert->sha1_hash));
> -    ASSERT(buf_inc_len(&hash, sizeof(cert->sha1_hash)));
> +    const EVP_MD *sha1 = EVP_sha1();
> +    struct buffer hash = alloc_buf_gc(EVP_MD_size(sha1), gc);
> +    X509_digest(cert, EVP_sha1(), BPTR(&hash), NULL);
> +    ASSERT(buf_inc_len(&hash, EVP_MD_size(sha1)));
>      return hash;
>  }
>  
>  struct buffer
>  x509_get_sha256_fingerprint(X509 *cert, struct gc_arena *gc)
>  {
> -    struct buffer hash = alloc_buf_gc((EVP_sha256())->md_size, gc);
> +    const EVP_MD *sha256 = EVP_sha256();
> +    struct buffer hash = alloc_buf_gc(EVP_MD_size(sha256), gc);
>      X509_digest(cert, EVP_sha256(), BPTR(&hash), NULL);
> -    ASSERT(buf_inc_len(&hash, (EVP_sha256())->md_size));
> +    ASSERT(buf_inc_len(&hash, EVP_MD_size(sha256)));
>      return hash;
>  }
>  
> @@ -573,13 +575,21 @@ x509_verify_ns_cert_type(const openvpn_x509_cert_t *peer_cert, const int usage)
>      }
>      if (usage == NS_CERT_CHECK_CLIENT)
>      {
> -        return ((peer_cert->ex_flags & EXFLAG_NSCERT)
> -                && (peer_cert->ex_nscert & NS_SSL_CLIENT)) ? SUCCESS : FAILURE;
> +        /*
> +         * Unfortunately, X509_check_purpose() does some wierd thing that
> +         * prevent it to take a const argument
> +         */
> +        return X509_check_purpose((X509 *)peer_cert, X509_PURPOSE_SSL_CLIENT, 0) ?
> +	       SUCCESS : FAILURE;
>      }
>      if (usage == NS_CERT_CHECK_SERVER)
>      {
> -        return ((peer_cert->ex_flags & EXFLAG_NSCERT)
> -                && (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
> +         */
> +        return X509_check_purpose((X509 *)peer_cert, X509_PURPOSE_SSL_SERVER, 0) ?
> +	       SUCCESS : FAILURE;
>      }
>  
>      return FAILURE;
> 

If we are to use X509_check_purpose() (see my comments above), we should
not cast away const here, but rather remove the const qualified from the
peer_cert argument.  Casting away const is bad, in particular if a
comment on the function states that it really can't take a const
argument because it changes the object.

-Steffan

Re: [Openvpn-devel] [RFC PATCH v1 05/15] OpenSSL: don't use direct access to the internal of X509

[Gert D. <ge...@gr...>]

Hi,

On Thu, Mar 02, 2017 at 09:36:32PM +0100, Steffan Karger wrote:
> So, what I propose instead is:
>  * remove all the nsCertType code (except the option in add_option())
>  * update the help strings and man page to indicate that --ns-cert-type
> is no longer supported and --remote-cert-tls should be used instead
>  * in add_option(), if the option is enabled in a config file, act as if
> --remote-cert-tls was specified correspondingly, and print a clear
> warning that --ns-cert-type is no longer supported and stricter checks
> are enabled instead.

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

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

gert
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

Re: [Openvpn-devel] [RFC PATCH v1 05/15] OpenSSL: don't use direct access to the internal of X509

[Steffan K. <st...@ka...>]

Hi,

On 02-03-17 22:26, Gert Doering wrote:
> On Thu, Mar 02, 2017 at 09:36:32PM +0100, Steffan Karger wrote:
>> So, what I propose instead is:
>>  * remove all the nsCertType code (except the option in add_option())
>>  * update the help strings and man page to indicate that --ns-cert-type
>> is no longer supported and --remote-cert-tls should be used instead
>>  * in add_option(), if the option is enabled in a config file, act as if
>> --remote-cert-tls was specified correspondingly, and print a clear
>> warning that --ns-cert-type is no longer supported and stricter checks
>> are enabled instead.
> 
> Mmmmh.  Is there a way to get the old behaviour with OpenSSL 1.1?
> 
> We decided that we do want 1.1 compatibility in release/2.4, but what
> you propose might break people's working config when upgrading from 2.4.1
> to 2.4.2 - bad enough if we make mistakes, but if there is an alternative
> to consciously changing cert validation behaviour in the middle of a
> release train, we should look again...

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

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

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

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

-Steffan

Re: [Openvpn-devel] [RFC PATCH v1 05/15] OpenSSL: don't use direct access to the internal of X509

[Emmanuel D. <lo...@fr...>]

Hello,

On Sat, Mar 4, 2017 at 4:13 PM, Steffan Karger <st...@ka...> wrote:
> Hi,
>
> On 02-03-17 22:26, Gert Doering wrote:
>> On Thu, Mar 02, 2017 at 09:36:32PM +0100, Steffan Karger wrote:
>>> So, what I propose instead is:
>>>  * remove all the nsCertType code (except the option in add_option())
>>>  * update the help strings and man page to indicate that --ns-cert-type
>>> is no longer supported and --remote-cert-tls should be used instead
>>>  * in add_option(), if the option is enabled in a config file, act as if
>>> --remote-cert-tls was specified correspondingly, and print a clear
>>> warning that --ns-cert-type is no longer supported and stricter checks
>>> are enabled instead.
>>
>> Mmmmh.  Is there a way to get the old behaviour with OpenSSL 1.1?
>>
>> We decided that we do want 1.1 compatibility in release/2.4, but what
>> you propose might break people's working config when upgrading from 2.4.1
>> to 2.4.2 - bad enough if we make mistakes, but if there is an alternative
>> to consciously changing cert validation behaviour in the middle of a
>> release train, we should look again...
>
> So I looked again, and there really seems to be no way to get the old
> behaviour with OpenSSL 1.1.  However, the exact behaviour of
> X509_check_purpose() is not as strict as I initially thought.  The
> current patch basically adds the following checks:
>  * if the cert has key usage set, it must be correct
>  * if the cert has extended key usage set, it must be correct
>  * if the cert has the CA flag set, it must be done correctly

When I coded this part, I searched for a long time before resorting to
using this function. It's true that the library does more check, yet I
didn't find any way to not do them, as the necessary members are not
available through getters.

However, my understanding is that OpenSSL does a full, correct check,
and that certificates that does not pass this check are probably not
correct - or at least, they hide some weird issues that could make
them a bit dangerous to use. Of course, I might be wrong. The
certificates I tested were valid w.r.t. this new code and I'm now sure
how I can generate certificates that do not pass.

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

Yet, rejecting certificates might pose problems in the organisations
that use them, and they should be warned by (at least) and
intermediate release that their certificates going to be rejected. So,
after a bit of thinking I would do :

1/ check the certificate with X209_check_purpose()
2/ on failure, for OpenSSL < 1.1.0, warn the user that the
certificates are probably not fully correct, and do the old check. If
it pass, continue

> All in all, I think the original patch (after fixing const correctness)
> is fine.
>
> As a last resort, we could consider keeping the old code inside #if
> OSSL_VER < 1.1.0 in release/2.4, but that might just create more
> confusion...

Unfortunately, I am overbooked right now and I'm not sure I'll be able
to do this fast (say, in less than 2 weeks). I'd be grateful of
someone else does it.

> -Steffan

Best regards,

-- Emmanuel Deloget

Re: [Openvpn-devel] [RFC PATCH v1 05/15] OpenSSL: don't use direct access to the internal of X509

[David S. <op...@sf...>]

On 04/03/17 16:13, Steffan Karger wrote:
> As a last resort, we could consider keeping the old code inside #if
> OSSL_VER < 1.1.0 in release/2.4, but that might just create more
> confusion...

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

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

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

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

The reason I say this, is that Fedora users are quite unhappy that
--tls-remote disappeared in v2.4.0 *despite* a more recent
NetworkManager-OpenVPN plug-in actually making it easy to switch to
--verify-x509-name.  But IIRC, this wasn't backported to all older and
still valid Fedora releases, so that caused additional pain for the
users.   And don't get me started with the attempt of temporarily build
against mbed TLS instead of OpenSSL in Fedora Rawhide (which will become
Fedora 26), as that broke lots of configurations with CA certificates
using RSA 1024 bit keys.  All this results in very little OpenVPN 2.4
adaptation among many Fedora users.

I can barely imagine what happens if we break --ns-cert-type on top of
that ... Just that the behaviour might change for a minority of users in
when Fedora 26 hits the streets, with OpenSSL v1.1, might hit us bad
again.  But at least with OpenSSL v1.1, many users knows that things
will be somewhat different too.  And we can actually point at OpenSSL
and explain why it broke, which is not something we could do with
the --tls-remote option.


Just my way too many cents ..... :)


-- 
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

Re: [Openvpn-devel] [RFC PATCH v1 05/15] OpenSSL: don't use direct access to the internal of X509

[Emmanuel D. <lo...@fr...>]

Hi everyone,

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

First,

On Sat, Mar 4, 2017 at 11:38 PM, David Sommerseth <
op...@sf...> wrote:

> On 04/03/17 16:13, Steffan Karger wrote:
> > As a last resort, we could consider keeping the old code inside #if
> > OSSL_VER < 1.1.0 in release/2.4, but that might just create more
> > confusion...
>


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

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

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

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



>
> Just a very quick thought here ... I do dislike different behaviours
> depending on which OpenSSL version being used.  But given that this
> feature is already deprecated and even removed in OpenSSL-1.1, I think
> that gives us some options.
>
> I agree with Gert that breaking --ns-cert-type isn't good at all.
> However, consider when people upgrade from OpenSSL v1.0 to v1.1 - that
> is most commonly when there is major distro update.  It is not something
> which happens "mid-term", as OpenSSL is quite commonly used by lots of
> base system packages these days.
>
> Regardless of OpenSSL version, I agree to loudly deprecate
> --ns-cert-type, through documentation, --help and log files.
>
> But I think we need to carry the existing behaviour for --ns-cert-type
> when built against OpenSSL v1.0.  And we can solve through some
> compatibility wrapper when built against OpenSSL v1.1 - with even louder
> warnings in the logs that this may break apart.
>


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

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

void *ns = X509_get_ext_d2i(x, NID_netscape_cert_type,
                            NULL, NULL);

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

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

​So the code would look like:

​ASN1_BIT_STRING *ns;
result_t result = FAILURE;
ns = X509_get_ext_d2i(x, NID_netscape_cert_type, NULL, NULL);
if (ns)
{
    if (ns->length > 0)
    {
        int flags = ns->data[0];
        if (flags & NS_SSL_CLIENT)
        {
            result = SUCCESS;
        }
    }
    ASN1_BIT_STRING_free(ns);
}


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

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



> ​<snip>
>
>
> --
> kind regards,
>
> David Sommerseth
> OpenVPN Technologies, Inc
>


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

​Best regards,

-- Emmanuel Deloget​

Re: [Openvpn-devel] [RFC PATCH v1 05/15] OpenSSL: don't use direct access to the internal of X509

[Emmanuel D. <lo...@fr...>]

Hi,

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

Sorry for the inconvenience.

BR,

-- Emmanuel Deloget

On Mon, Mar 27, 2017 at 5:49 PM, Emmanuel Deloget <lo...@fr...> wrote:

> Hi everyone,
>
> I got some time to try to fix all that stuff.
>
> First,
>
> On Sat, Mar 4, 2017 at 11:38 PM, David Sommerseth <op...@sf....
> topphemmelig.net> wrote:
>
>> On 04/03/17 16:13, Steffan Karger wrote:
>> > As a last resort, we could consider keeping the old code inside #if
>> > OSSL_VER < 1.1.0 in release/2.4, but that might just create more
>> > confusion...
>>
>
>
> ​I think I found a solution -- see later in the email.​
>
> I had a closer look to the corresponding code to see what are the
> differences between the old check and the new one -- and indeed,
> the discrepancies might only happen if the tested certificate is
> kind-of-weird. For exemple, a client certificate:
>
> * has the client bit set
> * has no key, or has a key but the key usage is neither for
>   signature nor for key agreement
> * is not used to auth a client
>
> ​If a user creates a Frankenstein certificate that look like this,
> it might be a good idea to warn him that such certificate is very
> likely to be refused in the near future (one can have a similar
> reasonning about server certificates).
>
>
>
>>
>> Just a very quick thought here ... I do dislike different behaviours
>> depending on which OpenSSL version being used.  But given that this
>> feature is already deprecated and even removed in OpenSSL-1.1, I think
>> that gives us some options.
>>
>> I agree with Gert that breaking --ns-cert-type isn't good at all.
>> However, consider when people upgrade from OpenSSL v1.0 to v1.1 - that
>> is most commonly when there is major distro update.  It is not something
>> which happens "mid-term", as OpenSSL is quite commonly used by lots of
>> base system packages these days.
>>
>> Regardless of OpenSSL version, I agree to loudly deprecate
>> --ns-cert-type, through documentation, --help and log files.
>>
>> But I think we need to carry the existing behaviour for --ns-cert-type
>> when built against OpenSSL v1.0.  And we can solve through some
>> compatibility wrapper when built against OpenSSL v1.1 - with even louder
>> warnings in the logs that this may break apart.
>>
>
>
> ​Fortunately, I think I found a solution that would help
> when using OpenSSL 1.1. Idealy, this should be a call
> to X509_check_purpose(.., X509_PURPOSE_SSL_*_WEAK, ...)​
> but I don't think the OpenSSL developers will accept such
> a change :)
>
> It might be possible to use the X509_get_ext_d2i() call
> like this:
>
> void *ns = X509_get_ext_d2i(x, NID_netscape_cert_type,
>                             NULL, NULL);
>
> ​In this case, the ​obtained ns object contains the data
> we're interested in:
>
> * if it exists, then EXFLAG_NSCERT is set
> * if not null, ns is of type ASN1_BIT_STRING, and this
>   type is not opaque (yep, sir).
>
> ​So the code would look like:
>
> ​ASN1_BIT_STRING *ns;
> result_t result = FAILURE;
> ns = X509_get_ext_d2i(x, NID_netscape_cert_type, NULL, NULL);
> if (ns)
> {
>     if (ns->length > 0)
>     {
>         int flags = ns->data[0];
>         if (flags & NS_SSL_CLIENT)
>         {
>             result = SUCCESS;
>         }
>     }
>     ASN1_BIT_STRING_free(ns);
> }
>
>
> ​For the record, it's the code ​that is used within function
> x509v3_cache_extensions() which builds the X509 flags we
> are using so I'm failry confident it's the right thing to
> do (but it's a bit convoluted and I don't like it much).
>
> ​Good news: the same code should work with nearly all the
> previous versions of OpenSSL.
>
>
>
>> ​<snip>
>>
>>
>> --
>> kind regards,
>>
>> David Sommerseth
>> OpenVPN Technologies, Inc
>>
>
>
> I'll post my new patches as soon as I get over every issues
> that have been talked on the ML (is that even a valid
> sentence?)
>
> ​Best regards,
>
> -- Emmanuel Deloget​
>