[Openvpn-devel,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.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
Tested on Windows 10 with openssl 1.0.1r and 1.1.0g

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

Hi,

On 08-01-18 03:21, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
> 
> - Replace direct access to internals of openssl structs
>   by corresponding methods.
> 
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
> Tested on Windows 10 with openssl 1.0.1r and 1.1.0g
> 
>  configure.ac                 |  1 +
>  src/openvpn/cryptoapi.c      | 69 +++++++++++++++++++++++++++-----------------
>  src/openvpn/openssl_compat.h | 14 +++++++++
>  3 files changed, 57 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..7052e4e 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,36 @@ 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 (EVP_PKEY_id(pkey) != EVP_PKEY_RSA)
> +    {
> +        msg(M_WARN, "cryptoapicert requires an RSA certificate");
> +        goto err;
> +    }
> +    pub_rsa = EVP_PKEY_get0_RSA(pkey);

This conflicts with the patch set from Emmanuel, where he removes
EVP_PKEY_id().  Canbe easily resolved by changing the if to

   if (!(pub_rsa = EVP_PKEY_get0_RSA))

... but only once the NULL-check patch for openssl_compat.h is applied.

> +
> +    /* 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
> 

Otherwise this looks good (does exactly the same we've done before for
management-external-key), and compiles fine for both OpenSSL 1.0.1 and
1.1.0.

I haven't tested the resulting binary on Windows myself, because I ran
out of time for today.  So testing volunteers are welcome :)

-Steffan

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

Hi,

On Sun, Jan 14, 2018 at 6:28 AM, Steffan Karger <steffan@karger.me> wrote:
> Hi,
>
> On 08-01-18 03:21, selva.nair@gmail.com wrote:
>> From: Selva Nair <selva.nair@gmail.com>
>>
>> - Replace direct access to internals of openssl structs
>>   by corresponding methods.
>>
>> Signed-off-by: Selva Nair <selva.nair@gmail.com>
>> ---
>> Tested on Windows 10 with openssl 1.0.1r and 1.1.0g
>>
>>  configure.ac                 |  1 +
>>  src/openvpn/cryptoapi.c      | 69 +++++++++++++++++++++++++++-----------------
>>  src/openvpn/openssl_compat.h | 14 +++++++++
>>  3 files changed, 57 insertions(+), 27 deletions(-)
>>
>>
..

>> -    rsa->n = BN_dup(pub_rsa->n);
>> -    rsa->flags |= RSA_FLAG_EXT_PKEY;
>> +    if (EVP_PKEY_id(pkey) != EVP_PKEY_RSA)
>> +    {
>> +        msg(M_WARN, "cryptoapicert requires an RSA certificate");
>> +        goto err;
>> +    }
>> +    pub_rsa = EVP_PKEY_get0_RSA(pkey);
>
> This conflicts with the patch set from Emmanuel, where he removes
> EVP_PKEY_id().  Canbe easily resolved by changing the if to
>
>    if (!(pub_rsa = EVP_PKEY_get0_RSA))
>
> ... but only once the NULL-check patch for openssl_compat.h is applied.

I'll send a v2 assuming that patch will get merged eventually.

I would have preferred to have  EVP_PKEY_id() retained in the compat layer
as its much more convenient to use it when multiple key types are to be
differentiated.. (read as: I want to support EC certs here).

Thanks for the review.

Selva

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

Hi,

On 14-01-18 17:31, Selva Nair wrote:
> On Sun, Jan 14, 2018 at 6:28 AM, Steffan Karger <steffan@karger.me> wrote:
>> On 08-01-18 03:21, selva.nair@gmail.com wrote:
>>> From: Selva Nair <selva.nair@gmail.com>
>>>
>>> - Replace direct access to internals of openssl structs
>>>   by corresponding methods.
>>>
>>> Signed-off-by: Selva Nair <selva.nair@gmail.com>
>>> ---
>>> Tested on Windows 10 with openssl 1.0.1r and 1.1.0g
>>>
>>>  configure.ac                 |  1 +
>>>  src/openvpn/cryptoapi.c      | 69 +++++++++++++++++++++++++++-----------------
>>>  src/openvpn/openssl_compat.h | 14 +++++++++
>>>  3 files changed, 57 insertions(+), 27 deletions(-)
>>>
>>>
> ..
> 
>>> -    rsa->n = BN_dup(pub_rsa->n);
>>> -    rsa->flags |= RSA_FLAG_EXT_PKEY;
>>> +    if (EVP_PKEY_id(pkey) != EVP_PKEY_RSA)
>>> +    {
>>> +        msg(M_WARN, "cryptoapicert requires an RSA certificate");
>>> +        goto err;
>>> +    }
>>> +    pub_rsa = EVP_PKEY_get0_RSA(pkey);
>>
>> This conflicts with the patch set from Emmanuel, where he removes
>> EVP_PKEY_id().  Canbe easily resolved by changing the if to
>>
>>    if (!(pub_rsa = EVP_PKEY_get0_RSA))
>>
>> ... but only once the NULL-check patch for openssl_compat.h is applied.
> 
> I'll send a v2 assuming that patch will get merged eventually.
> 
> I would have preferred to have  EVP_PKEY_id() retained in the compat layer
> as its much more convenient to use it when multiple key types are to be
> differentiated.. (read as: I want to support EC certs here).

That should still be possible just fine, see e.g. how Emmanuel does that
in "OpenSSL: remove some EVP_PKEY type checks".

-Steffan

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

Moin,

On Sun, Jan 14, 2018 at 12:28:40PM +0100, Steffan Karger wrote:
> This conflicts with the patch set from Emmanuel, where he removes
> EVP_PKEY_id().  Canbe easily resolved by changing the if to
> 
>    if (!(pub_rsa = EVP_PKEY_get0_RSA))
> 
> ... but only once the NULL-check patch for openssl_compat.h is applied.

"go for it" :-)

gert