[Openvpn-devel,v2,1/2] Bring cryptoapi.c upto speed with openssl 1.1

From: Selva Nair <selva.nair@gmail.com>

- Replace direct access to internals of openssl structs
  by corresponding methods.

v2: Remove the call to EVP_PKEY_id() as its slated for removal
    from the compat layer (see also review by Stefan)

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
Freeing rsa_method in this code path requires more fixup than
just use of RSA_meth_free (not freed at in n some errors etc.),
so will be addressed in a separate patch.

 configure.ac                 |  1 +
 src/openvpn/cryptoapi.c      | 68 ++++++++++++++++++++++++++------------------
 src/openvpn/openssl_compat.h | 14 +++++++++
 3 files changed, 56 insertions(+), 27 deletions(-)

Hi,

On 14-01-18 20:04, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
> 
> - Replace direct access to internals of openssl structs
>   by corresponding methods.
> 
> v2: Remove the call to EVP_PKEY_id() as its slated for removal
>     from the compat layer (see also review by Stefan)
> 
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
> Freeing rsa_method in this code path requires more fixup than
> just use of RSA_meth_free (not freed at in n some errors etc.),
> so will be addressed in a separate patch.
> 
>  configure.ac                 |  1 +
>  src/openvpn/cryptoapi.c      | 68 ++++++++++++++++++++++++++------------------
>  src/openvpn/openssl_compat.h | 14 +++++++++
>  3 files changed, 56 insertions(+), 27 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index b4fd1b3..2c1937e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -944,6 +944,7 @@ if test "${with_crypto_library}" = "openssl"; then
>  			RSA_meth_set_init \
>  			RSA_meth_set_finish \
>  			RSA_meth_set0_app_data \
> +			RSA_meth_get0_app_data \
>  			EC_GROUP_order_bits
>  		]
>  	)
> diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
> index d90cc5d..4f2c636 100644
> --- a/src/openvpn/cryptoapi.c
> +++ b/src/openvpn/cryptoapi.c
> @@ -47,6 +47,7 @@
>  #include <assert.h>
>  
>  #include "buffer.h"
> +#include "openssl_compat.h"
>  
>  /* MinGW w32api 3.17 is still incomplete when it comes to CryptoAPI while
>   * MinGW32-w64 defines all macros used. This is a hack around that problem.
> @@ -213,20 +214,20 @@ rsa_pub_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, in
>  static int
>  rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, int padding)
>  {
> -    CAPI_DATA *cd = (CAPI_DATA *) rsa->meth->app_data;
> +    CAPI_DATA *cd = (CAPI_DATA *) RSA_meth_get0_app_data(RSA_get_method(rsa));
>      HCRYPTHASH hash;
>      DWORD hash_size, len, i;
>      unsigned char *buf;
>  
>      if (cd == NULL)
>      {
> -        RSAerr(RSA_F_RSA_EAY_PRIVATE_ENCRYPT, ERR_R_PASSED_NULL_PARAMETER);
> +        RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, ERR_R_PASSED_NULL_PARAMETER);
>          return 0;
>      }
>      if (padding != RSA_PKCS1_PADDING)
>      {
>          /* AFAICS, CryptSignHash() *always* uses PKCS1 padding. */
> -        RSAerr(RSA_F_RSA_EAY_PRIVATE_ENCRYPT, RSA_R_UNKNOWN_PADDING_TYPE);
> +        RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_UNKNOWN_PADDING_TYPE);
>          return 0;
>      }
>      /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would
> @@ -236,7 +237,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
>      /* For now, we only support NID_md5_sha1 */
>      if (flen != SSL_SIG_LENGTH)
>      {
> -        RSAerr(RSA_F_RSA_EAY_PRIVATE_ENCRYPT, RSA_R_INVALID_MESSAGE_LENGTH);
> +        RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_INVALID_MESSAGE_LENGTH);
>          return 0;
>      }
>      if (!CryptCreateHash(cd->crypt_prov, CALG_SSL3_SHAMD5, 0, 0, &hash))
> @@ -253,7 +254,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
>      }
>      if ((int) hash_size != flen)
>      {
> -        RSAerr(RSA_F_RSA_EAY_PRIVATE_ENCRYPT, RSA_R_INVALID_MESSAGE_LENGTH);
> +        RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_INVALID_MESSAGE_LENGTH);
>          CryptDestroyHash(hash);
>          return 0;
>      }
> @@ -268,7 +269,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
>      buf = malloc(len);
>      if (buf == NULL)
>      {
> -        RSAerr(RSA_F_RSA_EAY_PRIVATE_ENCRYPT, ERR_R_MALLOC_FAILURE);
> +        RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, ERR_R_MALLOC_FAILURE);
>          CryptDestroyHash(hash);
>          return 0;
>      }
> @@ -312,7 +313,8 @@ init(RSA *rsa)
>  static int
>  finish(RSA *rsa)
>  {
> -    CAPI_DATA *cd = (CAPI_DATA *) rsa->meth->app_data;
> +    const RSA_METHOD *rsa_meth = RSA_get_method(rsa);
> +    CAPI_DATA *cd = (CAPI_DATA *) RSA_meth_get0_app_data(rsa_meth);
>  
>      if (cd == NULL)
>      {
> @@ -326,9 +328,8 @@ finish(RSA *rsa)
>      {
>          CertFreeCertificateContext(cd->cert_context);
>      }
> -    free(rsa->meth->app_data);
> -    free((char *) rsa->meth);
> -    rsa->meth = NULL;
> +    free(cd);
> +    RSA_meth_free((RSA_METHOD*) rsa_meth);
>      return 1;
>  }
>  
> @@ -412,9 +413,9 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
>      X509 *cert = NULL;
>      RSA *rsa = NULL, *pub_rsa;
>      CAPI_DATA *cd = calloc(1, sizeof(*cd));
> -    RSA_METHOD *my_rsa_method = calloc(1, sizeof(*my_rsa_method));
> +    RSA_METHOD *my_rsa_method = NULL;
>  
> -    if (cd == NULL || my_rsa_method == NULL)
> +    if (cd == NULL)
>      {
>          SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_FILE, ERR_R_MALLOC_FAILURE);
>          goto err;
> @@ -469,15 +470,16 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
>      /* here we don't need to do CryptGetUserKey() or anything; all necessary key
>       * info is in cd->cert_context, and then, in cd->crypt_prov.  */
>  
> -    my_rsa_method->name = "Microsoft CryptoAPI RSA Method";
> -    my_rsa_method->rsa_pub_enc = rsa_pub_enc;
> -    my_rsa_method->rsa_pub_dec = rsa_pub_dec;
> -    my_rsa_method->rsa_priv_enc = rsa_priv_enc;
> -    my_rsa_method->rsa_priv_dec = rsa_priv_dec;
> -    /* my_rsa_method->init = init; */
> -    my_rsa_method->finish = finish;
> -    my_rsa_method->flags = RSA_METHOD_FLAG_NO_CHECK;
> -    my_rsa_method->app_data = (char *) cd;
> +    my_rsa_method = RSA_meth_new("Microsoft Cryptography API RSA Method",
> +                                  RSA_METHOD_FLAG_NO_CHECK);
> +    check_malloc_return(my_rsa_method);
> +    RSA_meth_set_pub_enc(my_rsa_method, rsa_pub_enc);
> +    RSA_meth_set_pub_dec(my_rsa_method, rsa_pub_dec);
> +    RSA_meth_set_priv_enc(my_rsa_method, rsa_priv_enc);
> +    RSA_meth_set_priv_dec(my_rsa_method, rsa_priv_dec);
> +    RSA_meth_set_init(my_rsa_method, NULL);
> +    RSA_meth_set_finish(my_rsa_method, finish);
> +    RSA_meth_set0_app_data(my_rsa_method, cd);
>  
>      rsa = RSA_new();
>      if (rsa == NULL)
> @@ -486,23 +488,35 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
>          goto err;
>      }
>  
> -    /* cert->cert_info->key->pkey is NULL until we call SSL_CTX_use_certificate(),
> +    /* Public key in cert is NULL until we call SSL_CTX_use_certificate(),
>       * so we do it here then...  */
>      if (!SSL_CTX_use_certificate(ssl_ctx, cert))
>      {
>          goto err;
>      }
>      /* the public key */
> -    pub_rsa = cert->cert_info->key->pkey->pkey.rsa;
> +    EVP_PKEY *pkey = X509_get0_pubkey(cert);
> +
>      /* SSL_CTX_use_certificate() increased the reference count in 'cert', so
>       * we decrease it here with X509_free(), or it will never be cleaned up. */
>      X509_free(cert);
>      cert = NULL;
>  
> -    /* I'm not sure about what we have to fill in in the RSA, trying out stuff... */
> -    /* rsa->n indicates the key size */
> -    rsa->n = BN_dup(pub_rsa->n);
> -    rsa->flags |= RSA_FLAG_EXT_PKEY;
> +    if (!(pub_rsa = EVP_PKEY_get0_RSA(pkey)))
> +    {
> +        msg(M_WARN, "cryptoapicert requires an RSA certificate");
> +        goto err;
> +    }
> +
> +    /* Our private key is external, so we fill in only n and e from the public key */
> +    const BIGNUM *n = NULL;
> +    const BIGNUM *e = NULL;
> +    RSA_get0_key(pub_rsa, &n, &e, NULL);
> +    if (!RSA_set0_key(rsa, BN_dup(n), BN_dup(e), NULL))
> +    {
> +        goto err;
> +    }
> +    RSA_set_flags(rsa, RSA_flags(rsa) | RSA_FLAG_EXT_PKEY);
>      if (!RSA_set_method(rsa, my_rsa_method))
>      {
>          goto err;
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index 70b19ae..bc7dbdd 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -624,6 +624,20 @@ RSA_meth_set0_app_data(RSA_METHOD *meth, void *app_data)
>  }
>  #endif
>  
> +#if !defined(HAVE_RSA_METH_GET0_APP_DATA)
> +/**
> + * Get the application data of an RSA_METHOD object
> + *
> + * @param meth               The RSA_METHOD object
> + * @return                   pointer to application data, may be NULL
> + */
> +static inline void *
> +RSA_meth_get0_app_data(const RSA_METHOD *meth)
> +{
> +    return meth ? meth->app_data : NULL;
> +}
> +#endif
> +
>  #if !defined(HAVE_EC_GROUP_ORDER_BITS) && !defined(OPENSSL_NO_EC)
>  /**
>   * Gets the number of bits of the order of an EC_GROUP
> 

Codewise no more comments.  Only "compile-tested" myself, but given the
strong resemblance to the management-external-key code I'm still
confident that this change is good.  So:

Acked-by: Steffan Karger <steffan@karger.me>

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

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

(Technically it might not look like a bugfix, but since we decided 
"2.4 gets support for OpenSSL 1.1" and cryptoapi was overlooked, this 
*makes* it a bugfix.  I have not even compile-tested it, trusting Selva
and Steffan here - and buildbot will tell me if they overlooked 
something :-) )

commit 862cbe538b6d53435f60065b0235639095c9ad0d (master)
commit 94de253e720154efd2202281042560af18f2bdcb (release/2.4)
Author: Selva Nair
Date:   Sun Jan 14 14:04:22 2018 -0500

     Bring cryptoapi.c upto speed with openssl 1.1

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Steffan Karger <steffan.karger@fox-it.com>
     Message-Id: <1515956662-30572-1-git-send-email-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/search?l=mid&q=1515956662-30572-1-git-send-email-selva.nair@gmail.com
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot