Hi Steffan, Thank you for the review and the ack.
On Sun, Jan 21, 2018 at 5:58 AM, Steffan Karger <steffan.kar...@fox-it.com> wrote: > Hi, > > On 20-01-18 05:52, selva.n...@gmail.com wrote: >> From: Selva Nair <selva.n...@gmail.com> >> >> - 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 <selva.n...@gmail.com> >> --- >> >> 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(-) >> >> diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am >> index fcc22d6..1a2f42e 100644 >> --- a/src/openvpn/Makefile.am >> +++ b/src/openvpn/Makefile.am >> @@ -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 >> diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c >> index 4f2c636..f155123 100644 >> --- a/src/openvpn/cryptoapi.c >> +++ b/src/openvpn/cryptoapi.c >> @@ -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) >> { >> diff --git a/src/openvpn/options.c b/src/openvpn/options.c >> index b240e2e..220c2e5 100644 >> --- a/src/openvpn/options.c >> +++ b/src/openvpn/options.c >> @@ -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."); >> - 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: Yes, tested again using an old e-token which triggers this: Sun Jan 21 09:39:23 2018 us=632921 WARNING: cryptoapicert: private key is in a legacy store. Restricting TLS version to 1.1 (on Windows 10). and connects with Sun Jan 21 09:39:53 2018 us=955878 Control Channel: TLSv1.1, cipher TLSv1.0 ECDHE-RSA-AES256-SHA, 1024 bit RSA If the patch-author is allowed to say Tested-by: Selva Nair <selva.n...@gmail.com> > > Acked-by: Steffan Karger <steffan.kar...@fox-it.com> > Thanks, Selva ------------------------------------------------------------------------------ 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 Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
