| Message ID | 1516423974-22159-1-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | None | expand |
Hi, On 20-01-18 05:52, selva.nair@gmail.com wrote: > From: Selva Nair <selva.nair@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.nair@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: Acked-by: Steffan Karger <steffan.karger@fox-it.com> Thanks for fixing this long-standing shortcoming! -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi Steffan, Thank you for the review and the ack. On Sun, Jan 21, 2018 at 5:58 AM, Steffan Karger <steffan.karger@fox-it.com> wrote: > Hi, > > On 20-01-18 05:52, selva.nair@gmail.com wrote: >> From: Selva Nair <selva.nair@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.nair@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.nair@gmail.com> > > Acked-by: Steffan Karger <steffan.karger@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
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:
+ 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"?
commit 51d57d7dad6c6380df7b76bbec1897ea4f98474d (master)
commit 6c54745b8d417a534a6081588b1ecc7ff01fa9f7 (release/2.4)
Author: Selva Nair
Date: Fri Jan 19 23:52:54 2018 -0500
TLS v1.2 support for cryptoapicert -- RSA only
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <1516423974-22159-1-git-send-email-selva.nair@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16288.html
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
Hi, On Thu, Jan 25, 2018 at 6:18 AM, Gert Doering <gert@greenie.muc.de> wrote: > 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: > > + 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"? In short, there was a reasoning behind that "min" and not "max" but it turns out to be not really useful. For details (which is more of a note to self) read on: --tls-version-max 1.1 will make the same call as what we do here, so if this fails that would also. The reason for that hint to set min version instead of max was this: If the user has set tls-version-min to, say, 1.2 and openssl enforces max >= min, the call could fail and asking the user to set the min version to 1.1 (or lower) was thought of as a helpful hint. But it turns out openssl does not impose max >= min (though such a situation will eventually cause a handshake failure), and are unlikely to add such a check for legitimate reasons. In openssl 1.1.0, that call could fail only if min==max==1.1 and 1.1 is explicitly disabled -- hard to tell the user what to do in that case as it just should not happen in our current code base. 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. Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 25-01-18 16:14, Selva Nair wrote: > 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 :) -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 Thu, Jan 25, 2018 at 12:00 PM, Steffan Karger <steffan.karger@fox-it.com> wrote: > On 25-01-18 16:14, Selva Nair wrote: >> 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 :) No, I'll defer to you, I haven't written one. Was just looking into it and realized it may require a better get_min_proto_version compatibility function (current one returns 0). Thanks, Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
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