Make cryptoapicert work with TLS 1.2

From: Selva Nair <[email protected]...>

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

Signed-off-by Selva Nair <[email protected]...>

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

configure.ac +1 -0

index b4fd1b3..2c1937e 100644
@@ -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
 		]
 	)

src/openvpn/cryptoapi.c +26 -23

index d90cc5d..7052e4e 100644
@@ -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;
 

src/openvpn/cryptoapi.c +10 -0

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

src/openvpn/openssl_compat.h +7 -0

index 70b19ae..bc7dbdd 100644
@@ -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
+ */

Hi,

Quoted 15 lines · On 08-01-18 03:21, [email protected]... wrote:

From: Selva Nair <[email protected]...>

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

Signed-off-by Selva Nair <[email protected]...>

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

src/openvpn/cryptoapi.c SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) · line 510

Quoted 8 lines

src/openvpn/cryptoapi.c +6 -2

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

Quoted 16 lines · Steffan Karger

Quoted 15 lines · On 08-01-18 03:21, [email protected]... wrote:

From: Selva Nair <[email protected]...>

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

Signed-off-by Selva Nair <[email protected]...>

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

..

Quoted 14 lines

src/openvpn/cryptoapi.c SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) · line 510

Quoted 8 lines

src/openvpn/cryptoapi.c +6 -2

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

Quoted 1 line

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

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.

From: Selva Nair <[email protected]...>

- 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 <[email protected]...>

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

configure.ac +1 -0

index b4fd1b3..2c1937e 100644
@@ -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
 		]
 	)

src/openvpn/cryptoapi.c +41 -27

index d90cc5d..4f2c636 100644
@@ -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;

src/openvpn/openssl_compat.h +7 -0

index 70b19ae..bc7dbdd 100644
@@ -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
+ */

From: Selva Nair <[email protected]...>

- If an NCRYPT handle for the private key can be obtained, use NCryptSignHash from the Cryptography NG API to sign the hash.

This should work for all keys in the Windows certifiate stores but may fail for keys in a legacy token, for example. In such cases, we disable TLS v1.2 and fall back to the current behaviour. A warning is logged unless TLS version is already restricted to <= 1.1

Signed-off-by Selva Nair <[email protected]...>

---
Tested on Windows 10 with openssl 1.0.1r and 1.1.0g with certificate/key in Windows user store

 src/openvpn/Makefile.am |  2 +-
 src/openvpn/cryptoapi.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++---
 src/openvpn/options.c   | 18 -----------
 3 files changed, 83 insertions(+), 23 deletions(-)

src/openvpn/Makefile.am +1 -1

index fcc22d6..1a2f42e 100644
@@ -132,5 +132,5 @@ openvpn_LDADD = \
 	$(OPTIONAL_DL_LIBS)
 if WIN32
 openvpn_SOURCES += openvpn_win32_resources.rc block_dns.c block_dns.h
-openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm -lfwpuclnt -lrpcrt4
+openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm -lfwpuclnt -lrpcrt4 -lncrypt
 endif

src/openvpn/cryptoapi.c +9 -1

index 7052e4e..cd3f023 100644
@@ -42,6 +42,7 @@
 #include <openssl/err.h>
 #include <windows.h>
 #include <wincrypt.h>
+#include <ncrypt.h>
 #include <stdio.h>
 #include <ctype.h>
 #include <assert.h>

@@ -68,6 +69,14 @@
 #define CERT_STORE_OPEN_EXISTING_FLAG 0x00004000
 #endif
 
 /* Size of an SSL signature: MD5+SHA1 */
 #define SSL_SIG_LENGTH  36
 

@@ -83,6 +92,7 @@
 #define CRYPTOAPI_F_CRYPT_SIGN_HASH                         106
 #define CRYPTOAPI_F_LOAD_LIBRARY                            107
 #define CRYPTOAPI_F_GET_PROC_ADDRESS                        108
+#define CRYPTOAPI_F_NCRYPT_SIGN_HASH                        109
 
 static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
     { ERR_PACK(ERR_LIB_CRYPTOAPI, 0, 0),                                    "microsoft cryptoapi"},

@@ -95,12 +105,13 @@ static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
     { ERR_PACK(0, CRYPTOAPI_F_CRYPT_SIGN_HASH, 0),                          "CryptSignHash" },
     { ERR_PACK(0, CRYPTOAPI_F_LOAD_LIBRARY, 0),                             "LoadLibrary" },
     { ERR_PACK(0, CRYPTOAPI_F_GET_PROC_ADDRESS, 0),                         "GetProcAddress" },
+    { ERR_PACK(0, CRYPTOAPI_F_NCRYPT_SIGN_HASH, 0),                         "NCryptSignHash" },
     { 0, NULL }
 };
 
 typedef struct _CAPI_DATA {
     const CERT_CONTEXT *cert_context;
-    HCRYPTPROV crypt_prov;
+    HCRYPTPROV_OR_NCRYPT_KEY_HANDLE crypt_prov;
     DWORD key_spec;
     BOOL free_crypt_prov;
 } CAPI_DATA;

@@ -210,6 +221,41 @@ rsa_pub_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, in
     return 0;
 }
 
+/**
+ * Sign the hash in 'from' using NCryptSignHash(). This requires an NCRYPT
+ * key handle in cd->crypt_prov. On return the signature is in 'to'. Returns
+ * the length of the signature or 0 on error.
+ * For now we support only RSA and the padding is assumed to be PKCS1 v1.5

src/openvpn/cryptoapi.c +32 -1

+    /* The hash OID is already in 'from'.  So set the hash algorithm
+     * in the padding info struct to NULL.
+     */
+    BCRYPT_PKCS1_PADDING_INFO padinfo = {NULL};
+    DWORD status;
+
+    status = NCryptSignHash(hkey, padding? &padinfo : NULL, (BYTE*) from, flen,
+                            to, tlen, &len, padding? BCRYPT_PAD_PKCS1 : 0);
+    if (status != ERROR_SUCCESS)
+    {
+        SetLastError(status);
+        CRYPTOAPIerr(CRYPTOAPI_F_NCRYPT_SIGN_HASH);
+        len = 0;
+    }
+
+    /* Unlike CAPI, CNG signature is in big endian order. No reversing needed. */
+    return len;
+}
+
 /* sign arbitrary data */
 static int
 rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, int padding)

@@ -230,6 +276,11 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
         RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_UNKNOWN_PADDING_TYPE);
         return 0;
     }
+    if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+    {
+        return priv_enc_CNG(cd, from, flen, to, RSA_size(rsa), padding);
+    }
+
     /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would
      * be way to straightforward for M$, I guess... So we have to do it this
      * tricky way instead, by creating a "Hash", and load the already-made hash

@@ -322,7 +373,14 @@ finish(RSA *rsa)
     }
     if (cd->crypt_prov && cd->free_crypt_prov)
     {
-        CryptReleaseContext(cd->crypt_prov, 0);
+        if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+        {
+            NCryptFreeObject(cd->crypt_prov);
+        }
+        else
+        {
+            CryptReleaseContext(cd->crypt_prov, 0);
+        }
     }
     if (cd->cert_context)
     {

src/openvpn/cryptoapi.c +14 -1

+            msg(M_WARN, "WARNING: cryptapicert: only legacy private key handle is available."
+                        "Disabling TLS v1.2");
+            SSL_CTX_set_options(ssl_ctx, SSL_OP_NO_TLSv1_2);
+        }
+    }
+
     my_rsa_method = RSA_meth_new("Microsoft Cryptography API RSA Method",
                                   RSA_METHOD_FLAG_NO_CHECK);
     check_malloc_return(my_rsa_method);

@@ -551,7 +622,14 @@ err:
         {
             if (cd->free_crypt_prov && cd->crypt_prov)
             {
-                CryptReleaseContext(cd->crypt_prov, 0);
+                if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+                {
+                    NCryptFreeObject(cd->crypt_prov);
+                }
+                else
+                {
+                    CryptReleaseContext(cd->crypt_prov, 0);
+                }
             }
             if (cd->cert_context)
             {

src/openvpn/options.c +0 -11

index 7be5f38..e4fbade 100644
@@ -3018,24 +3018,6 @@ options_postprocess_mutate(struct options *o)
     }
 #endif
 
-#ifdef ENABLE_CRYPTOAPI
-    if (o->cryptoapi_cert)
-    {
-        const int tls_version_max =
-            (o->ssl_flags >> SSLF_TLS_VERSION_MAX_SHIFT)
-            &SSLF_TLS_VERSION_MAX_MASK;
-
-        if (tls_version_max == TLS_VER_UNSPEC || tls_version_max > TLS_VER_1_1)
-        {
-            msg(M_WARN, "Warning: cryptapicert used, setting maximum TLS "
-                "version to 1.1.");

Hi,

Some preliminary comments:

Quoted 12 lines · On 08-01-18 03:21, [email protected]... wrote:

From: Selva Nair <[email protected]...>

- If an NCRYPT handle for the private key can be obtained, use NCryptSignHash from the Cryptography NG API to sign the hash.

This should work for all keys in the Windows certifiate stores but may fail for keys in a legacy token, for example. In such cases, we disable TLS v1.2 and fall back to the current behaviour. A warning is logged unless TLS version is already restricted to <= 1.1

src/openvpn/cryptoapi.c rsa_pub_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, in · line 238

Quoted 9 lines

src/openvpn/cryptoapi.c +9 -0

+static int
+priv_enc_CNG(const CAPI_DATA *cd, const unsigned char *from, int flen,
+              unsigned char *to, int tlen, int padding)
+{
+    NCRYPT_KEY_HANDLE hkey = cd->crypt_prov;
+    DWORD len;
+    ASSERT(cd->key_spec == CERT_NCRYPT_KEY_SPEC);
+
+    msg(M_INFO, "Signing hash using CNG: data size = %d", flen);

M_INFO is probably a bit verbose here.

Quoted 22 lines

/* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would
* be way to straightforward for M$, I guess... So we have to do it this
* tricky way instead, by creating a "Hash", and load the already-made hash

src/openvpn/cryptoapi.c +8 -1

@@ -322,7 +373,14 @@ finish(RSA *rsa)
     }
     if (cd->crypt_prov && cd->free_crypt_prov)
     {
-        CryptReleaseContext(cd->crypt_prov, 0);
+        if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+        {
+            NCryptFreeObject(cd->crypt_prov);
+        }
+        else
+        {
+            CryptReleaseContext(cd->crypt_prov, 0);
+        }
     }
     if (cd->cert_context)
     {

@@ -458,7 +516,9 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
     }
 

src/openvpn/cryptoapi.c SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) · line 522

Quoted 6 lines

src/openvpn/cryptoapi.c +3 -1

     /* set up stuff to use the private key */
-    if (!CryptAcquireCertificatePrivateKey(cd->cert_context, CRYPT_ACQUIRE_COMPARE_KEY_FLAG,
+    /* We prefer to get an NCRYPT key handle so that TLS1.2 can be supported */
+    DWORD flags = CRYPT_ACQUIRE_COMPARE_KEY_FLAG|CRYPT_ACQUIRE_PREFER_NCRYPT_KEY_FLAG;
+    if (!CryptAcquireCertificatePrivateKey(cd->cert_context, flags,
                                            NULL, &cd->crypt_prov, &cd->key_spec, &cd->free_crypt_prov))

Some rewrapping of the arguments to no longer exceed 80 chars would be nice if you touch this anyway.

Quoted 7 lines

src/openvpn/cryptoapi.c +0 -0

     {
         /* if we don't have a smart card reader here, and we try to access a

@@ -470,6 +530,17 @@ 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.  */
 

src/openvpn/cryptoapi.c SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) · line 537

Quoted 5 lines

src/openvpn/cryptoapi.c +5 -0

+    /* If we do not have an NCRYPT key handle disable TLS v1.2 */
+    if (cd->key_spec != CERT_NCRYPT_KEY_SPEC)
+    {
+        if (!(SSL_CTX_get_options(ssl_ctx) & SSL_OP_NO_TLSv1_2))
+        {

This no longer works for OpenSSL 1.1, we'll need the patch from
https://patchwork.openvpn.net/patch/160/ (awaiting review), and then use the SSL_CTX_{get,set}_max_proto_version functions instead.

From: Selva Nair <[email protected]...>

- If an NCRYPT handle for the private key can be obtained, use NCryptSignHash from the Cryptography NG API to sign the hash.

This should work for all keys in the Windows certifiate stores but may fail for keys in a legacy token, for example. In such cases, we disable TLS v1.2 and fall back to the current behaviour. A warning is logged unless TLS version is already restricted to <= 1.1

Signed-off-by Selva Nair <[email protected]...>

---

Depends on patches:
patch 200: https://patchwork.openvpn.net/patch/200/
patch 201: https://patchwork.openvpn.net/patch/201/

v2: Based on Stefan's review:
- Replace SSL_CTX_get(set)_option with SSL_CTX_get(set)_max_proto_version
- Wrap some lines to 80 chars
- replace M_INFO by D_LOW in a low level debug message

Also removed some defines added in v1 that are actually present in mingw versions that we target

 src/openvpn/Makefile.am |  2 +-
 src/openvpn/cryptoapi.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++---
 src/openvpn/options.c   | 18 -----------
 3 files changed, 81 insertions(+), 24 deletions(-)

src/openvpn/Makefile.am +1 -1

index fcc22d6..1a2f42e 100644
@@ -132,5 +132,5 @@ openvpn_LDADD = \
 	$(OPTIONAL_DL_LIBS)
 if WIN32
 openvpn_SOURCES += openvpn_win32_resources.rc block_dns.c block_dns.h
-openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm -lfwpuclnt -lrpcrt4
+openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm -lfwpuclnt -lrpcrt4 -lncrypt
 endif

src/openvpn/cryptoapi.c +80 -5

index 4f2c636..f155123 100644
@@ -42,6 +42,7 @@
 #include <openssl/err.h>
 #include <windows.h>
 #include <wincrypt.h>
+#include <ncrypt.h>
 #include <stdio.h>
 #include <ctype.h>
 #include <assert.h>

@@ -83,6 +84,7 @@
 #define CRYPTOAPI_F_CRYPT_SIGN_HASH                         106
 #define CRYPTOAPI_F_LOAD_LIBRARY                            107
 #define CRYPTOAPI_F_GET_PROC_ADDRESS                        108
+#define CRYPTOAPI_F_NCRYPT_SIGN_HASH                        109
 
 static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
     { ERR_PACK(ERR_LIB_CRYPTOAPI, 0, 0),                                    "microsoft cryptoapi"},

@@ -95,12 +97,13 @@ static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
     { ERR_PACK(0, CRYPTOAPI_F_CRYPT_SIGN_HASH, 0),                          "CryptSignHash" },
     { ERR_PACK(0, CRYPTOAPI_F_LOAD_LIBRARY, 0),                             "LoadLibrary" },
     { ERR_PACK(0, CRYPTOAPI_F_GET_PROC_ADDRESS, 0),                         "GetProcAddress" },
+    { ERR_PACK(0, CRYPTOAPI_F_NCRYPT_SIGN_HASH, 0),                         "NCryptSignHash" },
     { 0, NULL }
 };
 
 typedef struct _CAPI_DATA {
     const CERT_CONTEXT *cert_context;
-    HCRYPTPROV crypt_prov;
+    HCRYPTPROV_OR_NCRYPT_KEY_HANDLE crypt_prov;
     DWORD key_spec;
     BOOL free_crypt_prov;
 } CAPI_DATA;

@@ -210,6 +213,41 @@ rsa_pub_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, in
     return 0;
 }
 
+/**
+ * Sign the hash in 'from' using NCryptSignHash(). This requires an NCRYPT
+ * key handle in cd->crypt_prov. On return the signature is in 'to'. Returns
+ * the length of the signature or 0 on error.
+ * For now we support only RSA and the padding is assumed to be PKCS1 v1.5
+ */
+static int
+priv_enc_CNG(const CAPI_DATA *cd, const unsigned char *from, int flen,
+              unsigned char *to, int tlen, int padding)
+{
+    NCRYPT_KEY_HANDLE hkey = cd->crypt_prov;
+    DWORD len;
+    ASSERT(cd->key_spec == CERT_NCRYPT_KEY_SPEC);
+
+    msg(D_LOW, "Signing hash using CNG: data size = %d", flen);
+
+    /* The hash OID is already in 'from'.  So set the hash algorithm
+     * in the padding info struct to NULL.
+     */
+    BCRYPT_PKCS1_PADDING_INFO padinfo = {NULL};
+    DWORD status;
+
+    status = NCryptSignHash(hkey, padding? &padinfo : NULL, (BYTE*) from, flen,
+                            to, tlen, &len, padding? BCRYPT_PAD_PKCS1 : 0);
+    if (status != ERROR_SUCCESS)
+    {
+        SetLastError(status);
+        CRYPTOAPIerr(CRYPTOAPI_F_NCRYPT_SIGN_HASH);
+        len = 0;
+    }
+
+    /* Unlike CAPI, CNG signature is in big endian order. No reversing needed. */
+    return len;
+}
+
 /* sign arbitrary data */
 static int
 rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, int padding)

@@ -230,6 +268,11 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
         RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_UNKNOWN_PADDING_TYPE);
         return 0;
     }
+    if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+    {
+        return priv_enc_CNG(cd, from, flen, to, RSA_size(rsa), padding);
+    }
+
     /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would
      * be way to straightforward for M$, I guess... So we have to do it this
      * tricky way instead, by creating a "Hash", and load the already-made hash

@@ -322,7 +365,14 @@ finish(RSA *rsa)
     }
     if (cd->crypt_prov && cd->free_crypt_prov)
     {
-        CryptReleaseContext(cd->crypt_prov, 0);
+        if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+        {
+            NCryptFreeObject(cd->crypt_prov);
+        }
+        else
+        {
+            CryptReleaseContext(cd->crypt_prov, 0);
+        }
     }
     if (cd->cert_context)
     {

@@ -458,8 +508,11 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
     }
 
     /* set up stuff to use the private key */
-    if (!CryptAcquireCertificatePrivateKey(cd->cert_context, CRYPT_ACQUIRE_COMPARE_KEY_FLAG,
-                                           NULL, &cd->crypt_prov, &cd->key_spec, &cd->free_crypt_prov))
+    /* We prefer to get an NCRYPT key handle so that TLS1.2 can be supported */
+    DWORD flags = CRYPT_ACQUIRE_COMPARE_KEY_FLAG
+                  | CRYPT_ACQUIRE_PREFER_NCRYPT_KEY_FLAG;
+    if (!CryptAcquireCertificatePrivateKey(cd->cert_context, flags, NULL,
+                    &cd->crypt_prov, &cd->key_spec, &cd->free_crypt_prov))
     {
         /* if we don't have a smart card reader here, and we try to access a
          * smart card certificate, we get:

@@ -470,6 +523,21 @@ 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.  */
 
+    /* if we do not have an NCRYPT key handle restrict TLS to v1.1 or lower */
+    int max_version = SSL_CTX_get_max_proto_version(ssl_ctx);
+    if ((!max_version || max_version > TLS1_1_VERSION)
+        && cd->key_spec != CERT_NCRYPT_KEY_SPEC)
+    {
+        msg(M_WARN,"WARNING: cryptoapicert: private key is in a legacy store."
+            " Restricting TLS version to 1.1");
+        if (!SSL_CTX_set_max_proto_version(ssl_ctx, TLS1_1_VERSION))
+        {
+            msg(M_NONFATAL,"ERROR: cryptoapicert: unable to set max TLS version"
+                " to 1.1. Try config option --tls-version-min 1.1");
+            goto err;
+        }
+    }
+
     my_rsa_method = RSA_meth_new("Microsoft Cryptography API RSA Method",
                                   RSA_METHOD_FLAG_NO_CHECK);
     check_malloc_return(my_rsa_method);

@@ -550,7 +618,14 @@ err:
         {
             if (cd->free_crypt_prov && cd->crypt_prov)
             {
-                CryptReleaseContext(cd->crypt_prov, 0);
+                if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+                {
+                    NCryptFreeObject(cd->crypt_prov);
+                }
+                else
+                {
+                    CryptReleaseContext(cd->crypt_prov, 0);
+                }
             }
             if (cd->cert_context)
             {

src/openvpn/options.c +0 -11

index b240e2e..220c2e5 100644
@@ -3018,24 +3018,6 @@ options_postprocess_mutate(struct options *o)
     }
 #endif
 
-#ifdef ENABLE_CRYPTOAPI
-    if (o->cryptoapi_cert)
-    {
-        const int tls_version_max =
-            (o->ssl_flags >> SSLF_TLS_VERSION_MAX_SHIFT)
-            &SSLF_TLS_VERSION_MAX_MASK;
-
-        if (tls_version_max == TLS_VER_UNSPEC || tls_version_max > TLS_VER_1_1)
-        {
-            msg(M_WARN, "Warning: cryptapicert used, setting maximum TLS "
-                "version to 1.1.");

Hi,

Quoted 219 lines · On 20-01-18 05:52, [email protected]... wrote:

From: Selva Nair <[email protected]...>

- If an NCRYPT handle for the private key can be obtained, use NCryptSignHash from the Cryptography NG API to sign the hash.

This should work for all keys in the Windows certifiate stores but may fail for keys in a legacy token, for example. In such cases, we disable TLS v1.2 and fall back to the current behaviour. A warning is logged unless TLS version is already restricted to <= 1.1

Signed-off-by Selva Nair <[email protected]...>

---

Depends on patches:
patch 200: https://patchwork.openvpn.net/patch/200/
patch 201: https://patchwork.openvpn.net/patch/201/

v2: Based on Stefan's review:
- Replace SSL_CTX_get(set)_option with SSL_CTX_get(set)_max_proto_version
- Wrap some lines to 80 chars
- replace M_INFO by D_LOW in a low level debug message

Also removed some defines added in v1 that are actually present in mingw versions that we target

 src/openvpn/Makefile.am |  2 +-
 src/openvpn/cryptoapi.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++---
 src/openvpn/options.c   | 18 -----------
 3 files changed, 81 insertions(+), 24 deletions(-)

src/openvpn/Makefile.am +1 -1

index fcc22d6..1a2f42e 100644
@@ -132,5 +132,5 @@ openvpn_LDADD = \
      $(OPTIONAL_DL_LIBS)
 if WIN32
 openvpn_SOURCES += openvpn_win32_resources.rc block_dns.c block_dns.h
-openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm -lfwpuclnt -lrpcrt4
+openvpn_LDADD += -lgdi32 -lws2_32 -lwininet -lcrypt32 -liphlpapi -lwinmm -lfwpuclnt -lrpcrt4 -lncrypt
 endif

src/openvpn/cryptoapi.c +80 -5

index 4f2c636..f155123 100644
@@ -42,6 +42,7 @@
 #include <openssl/err.h>
 #include <windows.h>
 #include <wincrypt.h>
+#include <ncrypt.h>
 #include <stdio.h>
 #include <ctype.h>
 #include <assert.h>

@@ -83,6 +84,7 @@
 #define CRYPTOAPI_F_CRYPT_SIGN_HASH                         106
 #define CRYPTOAPI_F_LOAD_LIBRARY                            107
 #define CRYPTOAPI_F_GET_PROC_ADDRESS                        108
+#define CRYPTOAPI_F_NCRYPT_SIGN_HASH                        109
 
 static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
     { ERR_PACK(ERR_LIB_CRYPTOAPI, 0, 0),                                    "microsoft cryptoapi"},

@@ -95,12 +97,13 @@ static ERR_STRING_DATA CRYPTOAPI_str_functs[] = {
     { ERR_PACK(0, CRYPTOAPI_F_CRYPT_SIGN_HASH, 0),                          "CryptSignHash" },
     { ERR_PACK(0, CRYPTOAPI_F_LOAD_LIBRARY, 0),                             "LoadLibrary" },
     { ERR_PACK(0, CRYPTOAPI_F_GET_PROC_ADDRESS, 0),                         "GetProcAddress" },
+    { ERR_PACK(0, CRYPTOAPI_F_NCRYPT_SIGN_HASH, 0),                         "NCryptSignHash" },
     { 0, NULL }
 };
 
 typedef struct _CAPI_DATA {
     const CERT_CONTEXT *cert_context;
-    HCRYPTPROV crypt_prov;
+    HCRYPTPROV_OR_NCRYPT_KEY_HANDLE crypt_prov;
     DWORD key_spec;
     BOOL free_crypt_prov;
 } CAPI_DATA;

@@ -210,6 +213,41 @@ rsa_pub_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, in
     return 0;
 }
 
+/**
+ * Sign the hash in 'from' using NCryptSignHash(). This requires an NCRYPT
+ * key handle in cd->crypt_prov. On return the signature is in 'to'. Returns
+ * the length of the signature or 0 on error.
+ * For now we support only RSA and the padding is assumed to be PKCS1 v1.5
+ */
+static int
+priv_enc_CNG(const CAPI_DATA *cd, const unsigned char *from, int flen,
+              unsigned char *to, int tlen, int padding)
+{
+    NCRYPT_KEY_HANDLE hkey = cd->crypt_prov;
+    DWORD len;
+    ASSERT(cd->key_spec == CERT_NCRYPT_KEY_SPEC);
+
+    msg(D_LOW, "Signing hash using CNG: data size = %d", flen);
+
+    /* The hash OID is already in 'from'.  So set the hash algorithm
+     * in the padding info struct to NULL.
+     */
+    BCRYPT_PKCS1_PADDING_INFO padinfo = {NULL};
+    DWORD status;
+
+    status = NCryptSignHash(hkey, padding? &padinfo : NULL, (BYTE*) from, flen,
+                            to, tlen, &len, padding? BCRYPT_PAD_PKCS1 : 0);
+    if (status != ERROR_SUCCESS)
+    {
+        SetLastError(status);
+        CRYPTOAPIerr(CRYPTOAPI_F_NCRYPT_SIGN_HASH);
+        len = 0;
+    }
+
+    /* Unlike CAPI, CNG signature is in big endian order. No reversing needed. */
+    return len;
+}
+
 /* sign arbitrary data */
 static int
 rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, int padding)

@@ -230,6 +268,11 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i
         RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_UNKNOWN_PADDING_TYPE);
         return 0;
     }
+    if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+    {
+        return priv_enc_CNG(cd, from, flen, to, RSA_size(rsa), padding);
+    }
+
     /* Unfortunately, there is no "CryptSign()" function in CryptoAPI, that would
      * be way to straightforward for M$, I guess... So we have to do it this
      * tricky way instead, by creating a "Hash", and load the already-made hash

@@ -322,7 +365,14 @@ finish(RSA *rsa)
     }
     if (cd->crypt_prov && cd->free_crypt_prov)
     {
-        CryptReleaseContext(cd->crypt_prov, 0);
+        if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+        {
+            NCryptFreeObject(cd->crypt_prov);
+        }
+        else
+        {
+            CryptReleaseContext(cd->crypt_prov, 0);
+        }
     }
     if (cd->cert_context)
     {

@@ -458,8 +508,11 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
     }
 
     /* set up stuff to use the private key */
-    if (!CryptAcquireCertificatePrivateKey(cd->cert_context, CRYPT_ACQUIRE_COMPARE_KEY_FLAG,
-                                           NULL, &cd->crypt_prov, &cd->key_spec, &cd->free_crypt_prov))
+    /* We prefer to get an NCRYPT key handle so that TLS1.2 can be supported */
+    DWORD flags = CRYPT_ACQUIRE_COMPARE_KEY_FLAG
+                  | CRYPT_ACQUIRE_PREFER_NCRYPT_KEY_FLAG;
+    if (!CryptAcquireCertificatePrivateKey(cd->cert_context, flags, NULL,
+                    &cd->crypt_prov, &cd->key_spec, &cd->free_crypt_prov))
     {
         /* if we don't have a smart card reader here, and we try to access a
          * smart card certificate, we get:

@@ -470,6 +523,21 @@ 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.  */
 
+    /* if we do not have an NCRYPT key handle restrict TLS to v1.1 or lower */
+    int max_version = SSL_CTX_get_max_proto_version(ssl_ctx);
+    if ((!max_version || max_version > TLS1_1_VERSION)
+        && cd->key_spec != CERT_NCRYPT_KEY_SPEC)
+    {
+        msg(M_WARN,"WARNING: cryptoapicert: private key is in a legacy store."
+            " Restricting TLS version to 1.1");
+        if (!SSL_CTX_set_max_proto_version(ssl_ctx, TLS1_1_VERSION))
+        {
+            msg(M_NONFATAL,"ERROR: cryptoapicert: unable to set max TLS version"
+                " to 1.1. Try config option --tls-version-min 1.1");
+            goto err;
+        }
+    }
+
     my_rsa_method = RSA_meth_new("Microsoft Cryptography API RSA Method",
                                   RSA_METHOD_FLAG_NO_CHECK);
     check_malloc_return(my_rsa_method);

@@ -550,7 +618,14 @@ err:
         {
             if (cd->free_crypt_prov && cd->crypt_prov)
             {
-                CryptReleaseContext(cd->crypt_prov, 0);
+                if (cd->key_spec == CERT_NCRYPT_KEY_SPEC)
+                {
+                    NCryptFreeObject(cd->crypt_prov);
+                }
+                else
+                {
+                    CryptReleaseContext(cd->crypt_prov, 0);
+                }
             }
             if (cd->cert_context)
             {

src/openvpn/options.c +0 -11

index b240e2e..220c2e5 100644
@@ -3018,24 +3018,6 @@ options_postprocess_mutate(struct options *o)
     }
 #endif
 
-#ifdef ENABLE_CRYPTOAPI
-    if (o->cryptoapi_cert)
-    {
-        const int tls_version_max =
-            (o->ssl_flags >> SSLF_TLS_VERSION_MAX_SHIFT)
-            &SSLF_TLS_VERSION_MAX_MASK;
-
-        if (tls_version_max == TLS_VER_UNSPEC || tls_version_max > TLS_VER_1_1)
-        {
-            msg(M_WARN, "Warning: cryptapicert used, setting maximum TLS "
-                "version to 1.1.");

src/openvpn/options.c options_postprocess_mutate(struct options *o) · line 3023

Quoted 10 lines

src/openvpn/options.c +0 -7

-            o->ssl_flags &= ~(SSLF_TLS_VERSION_MAX_MASK
-                              <<SSLF_TLS_VERSION_MAX_SHIFT);
-            o->ssl_flags |= (TLS_VER_1_1 << SSLF_TLS_VERSION_MAX_SHIFT);
-        }
-    }
-#endif /* ENABLE_CRYPTOAPI */
-
 #if P2MP
     /*
      * Save certain parms before modifying options via --pull

Code looks good, and now nicely connects using TLS 1.2 with a certificate from the windows certificate store, nice!

I don't know how to trigger the 'legacy store' code path. Have you tested that yourself? If you can confirm that you've tested it:

Acked-by Steffan Karger <[email protected]...>

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

I have tested on unix (where it obviously did not make a difference), compile-tested on my old ubuntu 14.04 build environment (fails with CERT_NCRYPT_KEY_SPEC not being defined, because mingw *there* needs "_WIN32_WINNT >= 0x0601") and successfully built on a brand new ubuntu 16.04 build environment, which adds a new "wincrypt.h" file with new #if WINAPI_FAMILY_PARTITION fun... :-)

[short summary: throw away your 14.04 build systems, we have decided to
break them, and that's what they are: broken!]

There is one thing I'm not sure I understand in the code, which might warrant a typo-fix patch:

src/openvpn/cryptoapi.c +8 -0

+        msg(M_WARN,"WARNING: cryptoapicert: private key is in a legacy store."
+            " Restricting TLS version to 1.1");
+        if (!SSL_CTX_set_max_proto_version(ssl_ctx, TLS1_1_VERSION))
+        {
+            msg(M_NONFATAL,"ERROR: cryptoapicert: unable to set max TLS version"
+                " to 1.1. Try config option --tls-version-min 1.1");
+            goto err;
+        }

should that be "--tls-version-*max* 1.1"?

A more useful thing to do is to catch the case of user setting min = 1.2 (as in trac 977) and we changing max to 1.1 when key is in a legacy key store. That would cause a handshake failure with a cryptic error. Better to catch early with a helpful warning. Will consider a patch for that and trim down the above log message at that time.

Quoted 5 lines · Selva Nair

A more useful thing to do is to catch the case of user setting min = 1.2 (as in trac 977) and we changing max to 1.1 when key is in a legacy key store. That would cause a handshake failure with a cryptic error. Better to catch early with a helpful warning. Will consider a patch for that and trim down the above log message at that time.

Ah, I actually have a patch ready for that in my local master branch, but still needed to send it out. So if you haven't started yet, I can send that out to the list. Otherwise I'll happily discard mine :)

From: Selva Nair <[email protected]...>

Hi,

I am not sure how receptive the crypto maintaineres are to the idea of adding more code into cryptoapi.c, but here goes:

I've been wanting to add TLS 1.2 support for certs in the Windows cert store using management external key. But that's a lot more work than extending cryptoapicert support. And, rather surprsingly, it turns out that the CNG API for signing is easy to use (well after some groping in the dark..) and doesn't take much to implement.

So these patches..

The first patch is not really related and to make the existing code "openssl-1.1 ready" (missed by past patches as no one probably builds Windows binary with 1.1..).

The second patch is not dependent on this, but close-by code paths are touched by both.

Selva

Selva Nair (2):
Bring cryptoapi.c upto speed with openssl 1.1
TLS v1.2 support for cryptoapicert -- RSA only

 configure.ac                 |   1 +
 src/openvpn/Makefile.am      |   2 +-
 src/openvpn/cryptoapi.c      | 155 ++++++++++++++++++++++++++++++
++++---------
 src/openvpn/openssl_compat.h |  14 ++++
 src/openvpn/options.c        |  18 -----
 5 files changed, 140 insertions(+), 50 deletions(-)

--
2.1.4

------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

2018-01-08 7:21 GMT+05:00 <[email protected]...>:

Quoted 15 lines

From: Selva Nair <[email protected]...>

Hi,

I am not sure how receptive the crypto maintaineres are to the idea of adding more code into cryptoapi.c, but here goes:

I've been wanting to add TLS 1.2 support for certs in the Windows cert store using management external key. But that's a lot more work than extending cryptoapicert support. And, rather surprsingly, it turns out that the CNG API for signing is easy to use (well after some groping in the dark..) and doesn't take much to implement.

Quoted 5 lines

So these patches..

The first patch is not really related and to make the existing code "openssl-1.1 ready" (missed by past patches as no one probably builds Windows binary with 1.1..).

there was an agreement on one of the recent community meetings to gracefully deprecate both libressl and openssl-1.0.X in favour of openssl-1.1.X

so, we should learn how to build windows binary with 1.1.X :)

I am not sure how receptive the crypto maintaineres are to the idea of adding more code into cryptoapi.c, but here goes:

I've been wanting to add TLS 1.2 support for certs in the Windows cert store using management external key. But that's a lot more work than extending cryptoapicert support. And, rather surprsingly, it turns out that the CNG API for signing is easy to use (well after some groping in the dark..) and doesn't take much to implement.

So these patches..

The first patch is not really related and to make the existing code "openssl-1.1 ready" (missed by past patches as no one probably builds Windows binary with 1.1..).

The second patch is not dependent on this, but close-by code paths are touched by both.