openvpn-devel Mailing List for OpenVPN

Hi, On 30/07/2019 07:15, michael-dev wrote: > Hi, > > any news on the VLAN feature? > sorry, still stuck in the queue. It's planned to be worked on as soon as we are done with our Ntlink code. And in theory this means August/September. Regards, > Regards, > M. Braun > > Am 11.08.2016 22:10, schrieb Mike Auty: >> Hi there, >> >> I was hoping to find out if there'd been any progress on getting the >> VLAN patches reviewed?  Thanks, >> >> Mike  5:) >> >> [1] https://sourceforge.net/p/openvpn/mailman/message/34988886/ >> [2] https://github.com/ikelos/openvpn >> >> ------------------------------------------------------------------------------ >> >> What NetFlow Analyzer can do for you? Monitors network bandwidth and >> traffic >> patterns at an interface-level. Reveals which users, apps, and >> protocols are >> consuming the most bandwidth. Provides multi-vendor support for NetFlow, >> J-Flow, sFlow and other flows. Make informed decisions using capacity >> planning reports. http://sdm.link/zohodev2dev >> _______________________________________________ >> Openvpn-devel mailing list >> Ope...@li... >> https://lists.sourceforge.net/lists/listinfo/openvpn-devel > > > _______________________________________________ > Openvpn-devel mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openvpn-devel > -- Antonio Quartulli 

Hi, any news on the VLAN feature? Regards, M. Braun Am 11.08.2016 22:10, schrieb Mike Auty: > Hi there, > > I was hoping to find out if there'd been any progress on getting the > VLAN patches reviewed? Thanks, > > Mike 5:) > > [1] https://sourceforge.net/p/openvpn/mailman/message/34988886/ > [2] https://github.com/ikelos/openvpn > > ------------------------------------------------------------------------------ > What NetFlow Analyzer can do for you? Monitors network bandwidth and > traffic > patterns at an interface-level. Reveals which users, apps, and > protocols are > consuming the most bandwidth. Provides multi-vendor support for > NetFlow, > J-Flow, sFlow and other flows. Make informed decisions using capacity > planning reports. http://sdm.link/zohodev2dev > _______________________________________________ > Openvpn-devel mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openvpn-devel 

Am 30.07.19 um 00:52 schrieb Jorge Barosa: > Hi, > > Is there a possibility to put an option where it disconnect auto after > example 15 minutes of innactivity? > We already have that: --inactive n [bytes] Arne 

Hi, Is there a possibility to put an option where it disconnect auto after example 15 minutes of innactivity? Best regards, Jorge Barosa Portugal

From: Selva Nair <sel...@gm...> For PSS padding, CNG requires the digest to be signed and the digest algorithm in use, which are not accessible via the rsa_sign and rsa_priv_enc callbacks of OpenSSL. This patch uses the EVP_KEY interface to hook to evp_pkey_sign callback if OpenSSL version is > 1.1.0. Mapping of OpenSSL hash algorithm types to CNG is moved to a function for code-reuse. To test, both the server and client should be built with OpenSSL 1.1.1 and use TLS version >= 1.2 Tested on Windows 7 client against a Linux server. Signed-off-by: Selva Nair <sel...@gm...> --- v2: rebased to release/2.4 after siglen -> *siglen change Essentially, commits 0cab3475a and ce1c1bee in master combined and adapted for 2.4. Required on Windows when built with OpenSSL 1.1.1 and cryptoapicert is in use against a 1.1.1 peer. src/openvpn/cryptoapi.c | 377 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 329 insertions(+), 48 deletions(-) diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index 35a9ebc..7f2c3c0 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -39,6 +39,7 @@ #ifdef ENABLE_CRYPTOAPI #include <openssl/ssl.h> +#include <openssl/evp.h> #include <openssl/err.h> #include <windows.h> #include <wincrypt.h> @@ -101,6 +102,12 @@ static ERR_STRING_DATA CRYPTOAPI_str_functs[] = { { 0, NULL } }; +/* Global EVP_PKEY_METHOD used to override the sign operation */ +static EVP_PKEY_METHOD *pmethod; +static int (*default_pkey_sign_init) (EVP_PKEY_CTX *ctx); +static int (*default_pkey_sign) (EVP_PKEY_CTX *ctx, unsigned char *sig, + size_t *siglen, const unsigned char *tbs, size_t tbslen); + typedef struct _CAPI_DATA { const CERT_CONTEXT *cert_context; HCRYPTPROV_OR_NCRYPT_KEY_HANDLE crypt_prov; @@ -108,6 +115,80 @@ typedef struct _CAPI_DATA { BOOL free_crypt_prov; } CAPI_DATA; +/** + * Translate OpenSSL padding type to CNG padding type + * Returns 0 for unknown/unsupported padding. + */ +static DWORD +cng_padding_type(int padding) +{ + DWORD pad = 0; + + switch (padding) + { + case RSA_NO_PADDING: + pad = BCRYPT_PAD_NONE; + break; + + case RSA_PKCS1_PADDING: + pad = BCRYPT_PAD_PKCS1; + break; + + case RSA_PKCS1_PSS_PADDING: + pad = BCRYPT_PAD_PSS; + break; + + default: + msg(M_WARN|M_INFO, "cryptoapicert: unknown OpenSSL padding type %d.", + padding); + } + + return pad; +} + +/** + * Translate OpenSSL hash OID to CNG algorithm name. Returns + * "UNKNOWN" for unsupported algorithms and NULL for MD5+SHA1 + * mixed hash used in TLS 1.1 and earlier. + */ +static const wchar_t * +cng_hash_algo(int md_type) +{ + const wchar_t *alg = L"UNKNOWN"; + switch (md_type) + { + case NID_md5: + alg = BCRYPT_MD5_ALGORITHM; + break; + + case NID_sha1: + alg = BCRYPT_SHA1_ALGORITHM; + break; + + case NID_sha256: + alg = BCRYPT_SHA256_ALGORITHM; + break; + + case NID_sha384: + alg = BCRYPT_SHA384_ALGORITHM; + break; + + case NID_sha512: + alg = BCRYPT_SHA512_ALGORITHM; + break; + + case NID_md5_sha1: + case 0: + alg = NULL; + break; + + default: + msg(M_WARN|M_INFO, "cryptoapicert: Unknown hash type NID=0x%x", md_type); + break; + } + return alg; +} + static char * ms_error_text(DWORD ms_err) { @@ -217,25 +298,44 @@ rsa_pub_dec(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, in * 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. + * Only RSA is supported and padding should be BCRYPT_PAD_PKCS1 or + * BCRYPT_PAD_PSS. * If the hash_algo is not NULL, PKCS #1 DigestInfo header gets added - * to 'from', else it is signed as is. - * For now we support only RSA and the padding is assumed to be PKCS1 v1.5 + * to |from|, else it is signed as is. Use NULL for MD5 + SHA1 hash used + * in TLS 1.1 and earlier. + * In case of PSS padding, |saltlen| should specify the size of salt to use. + * If |to| is NULL returns the required buffer size. */ static int priv_enc_CNG(const CAPI_DATA *cd, const wchar_t *hash_algo, const unsigned char *from, - int flen, unsigned char *to, int tlen, int padding) + int flen, unsigned char *to, int tlen, DWORD padding, DWORD saltlen) { NCRYPT_KEY_HANDLE hkey = cd->crypt_prov; DWORD len = 0; ASSERT(cd->key_spec == CERT_NCRYPT_KEY_SPEC); - msg(D_LOW, "Signing hash using CNG: data size = %d", flen); - - BCRYPT_PKCS1_PADDING_INFO padinfo = {hash_algo}; DWORD status; - status = NCryptSignHash(hkey, padding? &padinfo : NULL, (BYTE*) from, flen, - to, tlen, &len, padding? BCRYPT_PAD_PKCS1 : 0); + msg(D_LOW, "Signing hash using CNG: data size = %d padding = %lu", flen, padding); + + if (padding == BCRYPT_PAD_PKCS1) + { + BCRYPT_PKCS1_PADDING_INFO padinfo = {hash_algo}; + status = NCryptSignHash(hkey, &padinfo, (BYTE *)from, flen, + to, tlen, &len, padding); + } + else if (padding == BCRYPT_PAD_PSS) + { + BCRYPT_PSS_PADDING_INFO padinfo = {hash_algo, saltlen}; + status = NCryptSignHash(hkey, &padinfo, (BYTE *)from, flen, + to, tlen, &len, padding); + } + else + { + RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, RSA_R_UNKNOWN_PADDING_TYPE); + return 0; + } + if (status != ERROR_SUCCESS) { SetLastError(status); @@ -261,16 +361,19 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i RSAerr(RSA_F_RSA_OSSL_PRIVATE_ENCRYPT, ERR_R_PASSED_NULL_PARAMETER); return 0; } + + if (cd->key_spec == CERT_NCRYPT_KEY_SPEC) + { + return priv_enc_CNG(cd, NULL, from, flen, to, RSA_size(rsa), + cng_padding_type(padding), 0); + } + if (padding != RSA_PKCS1_PADDING) { /* AFAICS, CryptSignHash() *always* uses PKCS1 padding. */ 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, NULL, 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 @@ -333,12 +436,13 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i return len; } -/* +/** * Sign the hash in |m| and return the signature in |sig|. * Returns 1 on success, 0 on error. * NCryptSignHash() is used to sign and it is instructed to add the * the PKCS #1 DigestInfo header to |m| unless the hash algorithm is * the MD5/SHA1 combination used in TLS 1.1 and earlier versions. + * OpenSSL exercises this callback only when padding is PKCS1 v1.5. */ static int rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len, @@ -355,44 +459,16 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len, return 0; } - switch (type) + alg = cng_hash_algo(type); + if (alg && wcscmp(alg, L"UNKNOWN") == 0) { - case NID_md5: - alg = BCRYPT_MD5_ALGORITHM; - break; - - case NID_sha1: - alg = BCRYPT_SHA1_ALGORITHM; - break; - - case NID_sha256: - alg = BCRYPT_SHA256_ALGORITHM; - break; - - case NID_sha384: - alg = BCRYPT_SHA384_ALGORITHM; - break; - - case NID_sha512: - alg = BCRYPT_SHA512_ALGORITHM; - break; - - case NID_md5_sha1: - if (m_len != SSL_SIG_LENGTH) - { - RSAerr(RSA_F_RSA_SIGN, RSA_R_INVALID_MESSAGE_LENGTH); - return 0; - } - /* No DigestInfo header is required -- set alg-name to NULL */ - alg = NULL; - break; - default: - msg(M_WARN, "cryptoapicert: Unknown hash type NID=0x%x", type); - RSAerr(RSA_F_RSA_SIGN, RSA_R_UNKNOWN_ALGORITHM_TYPE); - return 0; + RSAerr(RSA_F_RSA_SIGN, RSA_R_UNKNOWN_ALGORITHM_TYPE); + return 0; } - *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), padding); + *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), + cng_padding_type(padding), 0); + return (*siglen == 0) ? 0 : 1; } @@ -518,6 +594,176 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) return rv; } +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L) + +static const CAPI_DATA * +retrieve_capi_data(EVP_PKEY *pkey) +{ + const CAPI_DATA *cd = NULL; + + if (pkey && EVP_PKEY_id(pkey) == EVP_PKEY_RSA) + { + RSA *rsa = EVP_PKEY_get0_RSA(pkey); + if (rsa) + { + cd = (CAPI_DATA *)RSA_meth_get0_app_data(RSA_get_method(rsa)); + } + } + return cd; +} + +static int +pkey_rsa_sign_init(EVP_PKEY_CTX *ctx) +{ + EVP_PKEY *pkey = EVP_PKEY_CTX_get0_pkey(ctx); + + if (pkey && retrieve_capi_data(pkey)) + { + return 1; /* Return success */ + } + else if (default_pkey_sign_init) /* Not our key. Call the default method */ + { + return default_pkey_sign_init(ctx); + } + return 1; +} + +/** + * Implementation of EVP_PKEY_sign() using CNG: sign the digest in |tbs| + * and save the the signature in |sig| and its size in |*siglen|. + * If |sig| is NULL the required buffer size is returned in |*siglen|. + * Returns 1 on success, 0 or a negative integer on error. + */ +static int +pkey_rsa_sign(EVP_PKEY_CTX *ctx, unsigned char *sig, size_t *siglen, + const unsigned char *tbs, size_t tbslen) +{ + EVP_PKEY *pkey = NULL; + const CAPI_DATA *cd = NULL; + EVP_MD *md = NULL; + const wchar_t *alg = NULL; + + int padding; + int hashlen; + int saltlen; + + pkey = EVP_PKEY_CTX_get0_pkey(ctx); + if (pkey) + { + cd = retrieve_capi_data(pkey); + } + + /* + * We intercept all sign requests, not just the one's for our key. + * Check the key and call the saved OpenSSL method for unknown keys. + */ + if (!pkey || !cd) + { + if (default_pkey_sign) + { + return default_pkey_sign(ctx, sig, siglen, tbs, tbslen); + } + else /* This should not happen */ + { + msg(M_FATAL, "cryptopaicert: Unknown key and no default sign operation to fallback on"); + return -1; + } + } + + if (!EVP_PKEY_CTX_get_rsa_padding(ctx, &padding)) + { + padding = RSA_PKCS1_PADDING; /* Default padding for RSA */ + } + + if (EVP_PKEY_CTX_get_signature_md(ctx, &md)) + { + hashlen = EVP_MD_size(md); + alg = cng_hash_algo(EVP_MD_type(md)); + + /* + * alg == NULL indicates legacy MD5+SHA1 hash, else alg should be a valid + * digest algorithm. + */ + if (alg && wcscmp(alg, L"UNKNOWN") == 0) + { + RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_UNKNOWN_ALGORITHM_TYPE); + return -1; + } + } + else + { + msg(M_NONFATAL, "cryptoapicert: could not determine the signature digest algorithm"); + RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_UNKNOWN_ALGORITHM_TYPE); + return -1; + } + + if (tbslen != (size_t)hashlen) + { + RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_INVALID_DIGEST_LENGTH); + return -1; + } + + /* If padding is PSS, determine parameters to pass to CNG */ + if (padding == RSA_PKCS1_PSS_PADDING) + { + /* + * Ensure the digest type for signature and mask generation match. + * In CNG there is no option to specify separate hash functions for + * the two, but OpenSSL supports it. However, I have not seen the + * two being different in practice. Also the recommended practice is + * to use the same for both (rfc 8017 sec 8.1). + */ + EVP_MD *mgf1md; + if (!EVP_PKEY_CTX_get_rsa_mgf1_md(ctx, &mgf1md) + || EVP_MD_type(mgf1md) != EVP_MD_type(md)) + { + msg(M_NONFATAL, "cryptoapicert: Unknown MGF1 digest type or does" + " not match the signature digest type."); + RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_UNSUPPORTED_MASK_PARAMETER); + } + + if (!EVP_PKEY_CTX_get_rsa_pss_saltlen(ctx, &saltlen)) + { + msg(M_WARN|M_INFO, "cryptoapicert: unable to get the salt length from context." + " Using the default value."); + saltlen = -1; + } + + /* + * In OpenSSL saltlen = -1 indicates to use the size of the digest as + * size of the salt. A value of -2 or -3 indicates maximum salt length + * that will fit. See RSA_padding_add_PKCS1_PSS_mgf1() of OpenSSL. + */ + if (saltlen == -1) + { + saltlen = hashlen; + } + else if (saltlen < 0) + { + const RSA *rsa = EVP_PKEY_get0_RSA(pkey); + saltlen = RSA_size(rsa) - hashlen - 2; /* max salt length for RSASSA-PSS */ + if (RSA_bits(rsa) &0x7) /* number of bits in the key not a multiple of 8 */ + { + saltlen--; + } + } + + if (saltlen < 0) + { + RSAerr(RSA_F_PKEY_RSA_SIGN, RSA_R_DATA_TOO_LARGE_FOR_KEY_SIZE); + return -1; + } + msg(D_LOW, "cryptoapicert: PSS padding using saltlen = %d", saltlen); + } + + *siglen = priv_enc_CNG(cd, alg, tbs, (int)tbslen, sig, *siglen, + cng_padding_type(padding), (DWORD)saltlen); + + return (*siglen == 0) ? 0 : 1; +} + +#endif /* OPENSSL_VERSION >= 1.1.0 */ + int SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) { @@ -620,10 +866,45 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) /* For CNG, set the RSA_sign method which gets priority over priv_enc(). * This method is called with the raw hash without the digestinfo * header and works better when using NCryptSignHash() with some tokens. + * However, if PSS padding is in use, openssl does not call this + * function but adds the padding and then calls rsa_priv_enc() + * with padding set to NONE which is not supported by CNG. + * So, when posisble (OpenSSL 1.1.0 and up), we hook on to the sign + * operation in EVP_PKEY_METHOD struct. */ if (cd->key_spec == CERT_NCRYPT_KEY_SPEC) { +#if (OPENSSL_VERSION_NUMBER < 0x10100000L) RSA_meth_set_sign(my_rsa_method, rsa_sign_CNG); +#else + /* pmethod is global -- initialize only if NULL */ + if (!pmethod) + { + pmethod = EVP_PKEY_meth_new(EVP_PKEY_RSA, 0); + if (!pmethod) + { + SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_FILE, ERR_R_MALLOC_FAILURE); + goto err; + } + const EVP_PKEY_METHOD *default_pmethod = EVP_PKEY_meth_find(EVP_PKEY_RSA); + EVP_PKEY_meth_copy(pmethod, default_pmethod); + + /* We want to override only sign_init() and sign() */ + EVP_PKEY_meth_set_sign(pmethod, pkey_rsa_sign_init, pkey_rsa_sign); + EVP_PKEY_meth_add0(pmethod); + + /* Keep a copy of the default sign and sign_init methods */ + +#if (OPENSSL_VERSION_NUMBER < 0x1010009fL) /* < version 1.1.0i */ + /* The function signature is not const-correct in these versions */ + EVP_PKEY_meth_get_sign((EVP_PKEY_METHOD *)default_pmethod, &default_pkey_sign_init, + &default_pkey_sign); +#else + EVP_PKEY_meth_get_sign(default_pmethod, &default_pkey_sign_init, + &default_pkey_sign); +#endif + } +#endif /* (OPENSSL_VERSION_NUMBER < 0x10100000L) */ } rsa = RSA_new(); -- 2.1.4 

Thanks. Your patch has been applied to the master and release/2.4 branch. I still have no idea what it does, but it applies and Lev says the change is good :) commit abf7a2f226a262860f369e0a3c5d0f6124b7f110 (master) commit 94157cb368d850032d7c6f436499738ed8bd7834 (release/2.4) Author: Gisle Vanem Date: Wed Jul 3 15:45:34 2019 +0200 Wrong FILETYPE in .rc files Acked-by: Lev Stipakov <lst...@gm...> Message-Id: <aa4...@gm...> URL: https://www.mail-archive.com/ope...@li.../msg18644.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Your patch has been applied to the master branch. (The code is not in 2.4, so no need to backport). Slightly tested with a "make check" run. commit 2df27b2fa3932372075afd0db6b706a38e0a9dc3 Author: Lev Stipakov Date: Thu Jul 18 12:35:03 2019 +0300 crypto.c: fix Visual Studio build Signed-off-by: Lev Stipakov <le...@op...> Acked-by: Arne Schwabe <ar...@rf...> Message-Id: <156...@gm...> URL: https://www.mail-archive.com/ope...@li.../msg18676.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

And thanks again :-) - even less tested, just checking that this is indeed the only occurance of siglen/*siglen in 2.4 Your patch has been applied to the release/2.4 branch. commit eed67cf0e7f6cbe596495c2f83aeea7c15db0d6e Author: Selva Nair Date: Fri Jul 26 23:12:21 2019 -0400 Correct the return value of cryptoapi RSA signature callbacks Signed-off-by: Selva Nair <sel...@gm...> Acked-by: Steffan Karger <ste...@fo...> Message-Id: <156...@gm...> URL: https://www.mail-archive.com/ope...@li.../msg18708.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Thanks (and thanks for sending the patch for 2.4 right with it) I have not tested anything, but the patch looks very much "obviously correct". Your patch has been applied to the master branch. commit f4ac6b780db2e0c3b60d180bd6545efe30a52059 Author: Selva Nair Date: Fri Jul 26 16:39:17 2019 -0400 Correct the return value of cryptoapi RSA signature callbacks Signed-off-by: Selva Nair <sel...@gm...> Acked-by: Steffan Karger <ste...@fo...> Message-Id: <156...@gm...> URL: https://www.mail-archive.com/ope...@li.../msg18706.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

 On 27-07-19 05:12, sel...@gm... wrote: > From: Selva Nair <sel...@gm...> > > Fixes the wrong check on siglen instead of *siglen for > signing failures. > > Bug reported by: lilulo <li...@gm...> > > Signed-off-by: Selva Nair <sel...@gm...> > --- > src/openvpn/cryptoapi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c > index 720fce09..35a9ebc4 100644 > --- a/src/openvpn/cryptoapi.c > +++ b/src/openvpn/cryptoapi.c > @@ -393,7 +393,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len, > } > > *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), padding); > - return (siglen == 0) ? 0 : 1; > + return (*siglen == 0) ? 0 : 1; > } > > /* decrypt */ > Acked-by: Steffan Karger <St...@ka...> 

 On 26-07-19 22:39, sel...@gm... wrote: > From: Selva Nair <sel...@gm...> > > Fixes the wrong check on siglen instead of *siglen for > signing failures. > > Bug reported by: lilulo <li...@gm...> > > Signed-off-by: Selva Nair <sel...@gm...> > --- > > 2.4 will need a separate patch > > src/openvpn/cryptoapi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c > index 0c11712e..2f2eee77 100644 > --- a/src/openvpn/cryptoapi.c > +++ b/src/openvpn/cryptoapi.c > @@ -499,7 +499,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len, > *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), > cng_padding_type(padding), 0); > > - return (siglen == 0) ? 0 : 1; > + return (*siglen == 0) ? 0 : 1; > } > > /* decrypt */ > @@ -973,7 +973,7 @@ pkey_rsa_sign(EVP_PKEY_CTX *ctx, unsigned char *sig, size_t *siglen, > *siglen = priv_enc_CNG(cd, alg, tbs, (int)tbslen, sig, *siglen, > cng_padding_type(padding), (DWORD)saltlen); > > - return (siglen == 0) ? 0 : 1; > + return (*siglen == 0) ? 0 : 1; > } > > #endif /* OPENSSL_VERSION >= 1.1.0 */ > Acked-by: Steffan Karger <St...@ka...> 

From: Selva Nair <sel...@gm...> Fixes the wrong check on siglen instead of *siglen for signing failures. Bug reported by: lilulo <li...@gm...> Signed-off-by: Selva Nair <sel...@gm...> --- src/openvpn/cryptoapi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index 720fce09..35a9ebc4 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -393,7 +393,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len, } *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), padding); - return (siglen == 0) ? 0 : 1; + return (*siglen == 0) ? 0 : 1; } /* decrypt */ -- 2.20.1 

Hi, With openvpn 2.4.5, we are using deferred mode for user verification through plugin library. Noticed there are a lot of auth_control_files are left around and wondering if we need to do our own clean up of these files outside of openvpn. The scenario involves two factor authentication where end user has to approve through their smartphone app which may take some time. The left-over files mostly contains "0" which indicates a failure due to timeout. If the second factor authentication is done within a minute, then auth_control_file is deleted right away if the auth result is failure. The auth result is success, then the file is deleted when the connection is closed. Any suggestion is appreciated. Thanks. -chang 

From: Selva Nair <sel...@gm...> Fixes the wrong check on siglen instead of *siglen for signing failures. Bug reported by: lilulo <li...@gm...> Signed-off-by: Selva Nair <sel...@gm...> --- 2.4 will need a separate patch src/openvpn/cryptoapi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index 0c11712e..2f2eee77 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -499,7 +499,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len, *siglen = priv_enc_CNG(cd, alg, m, (int)m_len, sig, RSA_size(rsa), cng_padding_type(padding), 0); - return (siglen == 0) ? 0 : 1; + return (*siglen == 0) ? 0 : 1; } /* decrypt */ @@ -973,7 +973,7 @@ pkey_rsa_sign(EVP_PKEY_CTX *ctx, unsigned char *sig, size_t *siglen, *siglen = priv_enc_CNG(cd, alg, tbs, (int)tbslen, sig, *siglen, cng_padding_type(padding), (DWORD)saltlen); - return (siglen == 0) ? 0 : 1; + return (*siglen == 0) ? 0 : 1; } #endif /* OPENSSL_VERSION >= 1.1.0 */ -- 2.20.1 

Hi, > This might be something with your mailer at openvpn... Antonio complained > about a well-formed patch yesterday, now you complain while I can apply > the patch just fine... > I am sorry, I downloaded patch as mbox from patchwork ( https://patchwork.openvpn.net/patch/773/mbox/) and tried "git am" on Windows and WSL. Other patches are applied just fine. Looks like we might need to look into patchwork. This would be a separate patch, I'd say - the patch here is actually > changing numbers, while your suggestion would be more of a cleanup > thing, not actually changing the result. > Well so far we were consistent by using numbers, albeit wrong ones. After this patch we will have a mix of numbers and strings. But I agree, that can be fixed later. $ git am /tmp/mail.eml > warning: Patch sent with format=flowed; space at the end of lines might be > lost. > Applying: Wrong FILETYPE in .rc files > > So - since I can apply the patch just fine, is the change itself ACKed? > Yep, if you can apply it I am fine with that. Acked-By: Lev Stipakov <lst...@gm...> -- -Lev 

Hi, On Thu, Jul 25, 2019 at 10:44:48AM +0300, Lev Stipakov wrote: > 1. Please fix your patch, I cannot apply it: This might be something with your mailer at openvpn... Antonio complained about a well-formed patch yesterday, now you complain while I can apply the patch just fine... $ git am /tmp/mail.eml warning: Patch sent with format=flowed; space at the end of lines might be lost. Applying: Wrong FILETYPE in .rc files So - since I can apply the patch just fine, is the change itself ACKed? > 3. Since you patch .rc files, could you replace rest of numeric values with > strings, like > > FILEOS VOS_NT_WINDOWS32 instead of FILEOS 0x40004L This would be a separate patch, I'd say - the patch here is actually changing numbers, while your suggestion would be more of a cleanup thing, not actually changing the result. gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany ge...@gr... 

 On 24-07-19 17:29, Arne Schwabe wrote: > From: Rosen Penev <ro...@gm...> > > EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were > replaced with _reset. > > EVP_CIPHER_CTX_free in OpenSSL 1.1 replaces the cleanup/free combo of > earlier OpenSSL version. And OpenSSL 1.0.2 already calls cleanup as part > of _free. > > Therefore we can remove the _cleanup calls and use the OpenSSL 1.1. API > everywhere. > > Also removed initialisation with OpenSSL 1.1 as it is no longer > needed and causes compilation errors when disabling deprecated APIs. > > Same with SSL_CTX_set_ecdh_auto as it got removed. > > Patch V3: Use EVP_CIPHER_CTX_reset instead of init/cleanup > > Signed-off-by: Rosen Penev <ro...@gm...> > Signed-off-by: Arne Schwabe <ar...@rf...> > --- > configure.ac | 3 +++ > src/openvpn/crypto.c | 1 - > src/openvpn/crypto_backend.h | 9 +-------- > src/openvpn/crypto_mbedtls.c | 7 +------ > src/openvpn/crypto_openssl.c | 8 +------- > src/openvpn/openssl_compat.h | 12 ++++++++++++ > src/openvpn/ssl_openssl.c | 18 ++++++++++++------ > 7 files changed, 30 insertions(+), 28 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 59673e04..b8e2476f 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -918,10 +918,13 @@ if test "${with_crypto_library}" = "openssl"; then >	EVP_MD_CTX_new \ >	EVP_MD_CTX_free \ >	EVP_MD_CTX_reset \ > +	EVP_CIPHER_CTX_reset \ >	OpenSSL_version \ >	SSL_CTX_get_default_passwd_cb \ >	SSL_CTX_get_default_passwd_cb_userdata \ >	SSL_CTX_set_security_level \ > +	X509_get0_notBefore \ > +	X509_get0_notAfter \ >	X509_get0_pubkey \ >	X509_STORE_get0_objects \ >	X509_OBJECT_free \ > diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c > index 8a92a8c1..585bfbc6 100644 > --- a/src/openvpn/crypto.c > +++ b/src/openvpn/crypto.c > @@ -906,7 +906,6 @@ free_key_ctx(struct key_ctx *ctx) > { > if (ctx->cipher) > { > - cipher_ctx_cleanup(ctx->cipher); > cipher_ctx_free(ctx->cipher); > ctx->cipher = NULL; > } > diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h > index 7e9a4bd2..d119442f 100644 > --- a/src/openvpn/crypto_backend.h > +++ b/src/openvpn/crypto_backend.h > @@ -341,7 +341,7 @@ bool cipher_kt_mode_aead(const cipher_kt_t *cipher); > cipher_ctx_t *cipher_ctx_new(void); > > /** > - * Free a cipher context > + * Cleanup and free a cipher context > * > * @param ctx Cipher context. > */ > @@ -360,13 +360,6 @@ void cipher_ctx_free(cipher_ctx_t *ctx); > void cipher_ctx_init(cipher_ctx_t *ctx, const uint8_t *key, int key_len, > const cipher_kt_t *kt, int enc); > > -/** > - * Cleanup the specified context. > - * > - * @param ctx Cipher context to cleanup. > - */ > -void cipher_ctx_cleanup(cipher_ctx_t *ctx); > - > /** > * Returns the size of the IV used by the cipher, in bytes, or 0 if no IV is > * used. > diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c > index 2e931440..f924323d 100644 > --- a/src/openvpn/crypto_mbedtls.c > +++ b/src/openvpn/crypto_mbedtls.c > @@ -616,12 +616,6 @@ cipher_ctx_init(mbedtls_cipher_context_t *ctx, const uint8_t *key, int key_len, > ASSERT(ctx->key_bitlen <= key_len*8); > } > > -void > -cipher_ctx_cleanup(mbedtls_cipher_context_t *ctx) > -{ > - mbedtls_cipher_free(ctx); > -} > - > int > cipher_ctx_iv_length(const mbedtls_cipher_context_t *ctx) > { > @@ -861,6 +855,7 @@ md_ctx_new(void) > void > md_ctx_free(mbedtls_md_context_t *ctx) > { > + mbedtls_cipher_free(ctx); > free(ctx); > } > > diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c > index c049e52d..520e40ee 100644 > --- a/src/openvpn/crypto_openssl.c > +++ b/src/openvpn/crypto_openssl.c > @@ -772,7 +772,7 @@ cipher_ctx_init(EVP_CIPHER_CTX *ctx, const uint8_t *key, int key_len, > { > ASSERT(NULL != kt && NULL != ctx); > > - EVP_CIPHER_CTX_init(ctx); > + EVP_CIPHER_CTX_reset(ctx); > if (!EVP_CipherInit(ctx, kt, NULL, NULL, enc)) > { > crypto_msg(M_FATAL, "EVP cipher init #1"); > @@ -792,12 +792,6 @@ cipher_ctx_init(EVP_CIPHER_CTX *ctx, const uint8_t *key, int key_len, > ASSERT(EVP_CIPHER_CTX_key_length(ctx) <= key_len); > } > > -void > -cipher_ctx_cleanup(EVP_CIPHER_CTX *ctx) > -{ > - EVP_CIPHER_CTX_cleanup(ctx); > -} > - > int > cipher_ctx_iv_length(const EVP_CIPHER_CTX *ctx) > { > diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h > index a4072b9a..4ac8f24d 100644 > --- a/src/openvpn/openssl_compat.h > +++ b/src/openvpn/openssl_compat.h > @@ -89,6 +89,18 @@ EVP_MD_CTX_new(void) > } > #endif > > +#if !defined(HAVE_EVP_CIPHER_CTX_RESET) > +#define EVP_CIPHER_CTX_reset EVP_CIPHER_CTX_init > +#endif > + > +#if !defined(HAVE_X509_GET0_NOTBEFORE) > +#define X509_get0_notBefore X509_get_notBefore > +#endif > + > +#if !defined(HAVE_X509_GET0_NOTAFTER) > +#define X509_get0_notAfter X509_get_notAfter > +#endif > + > #if !defined(HAVE_HMAC_CTX_RESET) > /** > * Reset a HMAC context > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c > index 05ca4113..c029d0f2 100644 > --- a/src/openvpn/ssl_openssl.c > +++ b/src/openvpn/ssl_openssl.c > @@ -76,12 +76,13 @@ int mydata_index; /* GLOBAL */ > void > tls_init_lib(void) > { > +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) > SSL_library_init(); > -#ifndef ENABLE_SMALL > +# ifndef ENABLE_SMALL > SSL_load_error_strings(); > -#endif > +# endif > OpenSSL_add_all_algorithms(); > - > +#endif > mydata_index = SSL_get_ex_new_index(0, "struct session *", NULL, NULL, NULL); > ASSERT(mydata_index >= 0); > } > @@ -89,9 +90,11 @@ tls_init_lib(void) > void > tls_free_lib(void) > { > +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) > EVP_cleanup(); > -#ifndef ENABLE_SMALL > +# ifndef ENABLE_SMALL > ERR_free_strings(); > +# endif > #endif > } > > @@ -567,7 +570,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx) > goto cleanup; /* Nothing to check if there is no certificate */ > } > > - ret = X509_cmp_time(X509_get_notBefore(cert), NULL); > + ret = X509_cmp_time(X509_get0_notBefore(cert), NULL); > if (ret == 0) > { > msg(D_TLS_DEBUG_MED, "Failed to read certificate notBefore field."); > @@ -577,7 +580,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx) > msg(M_WARN, "WARNING: Your certificate is not yet valid!"); > } > > - ret = X509_cmp_time(X509_get_notAfter(cert), NULL); > + ret = X509_cmp_time(X509_get0_notAfter(cert), NULL); > if (ret == 0) > { > msg(D_TLS_DEBUG_MED, "Failed to read certificate notAfter field."); > @@ -660,10 +663,13 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name > else > { > #if OPENSSL_VERSION_NUMBER >= 0x10002000L > +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) > + > /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter > * loading */ > SSL_CTX_set_ecdh_auto(ctx->ctx, 1); > return; > +#endif > #else > /* For older OpenSSL we have to extract the curve from key on our own */ > EC_KEY *eckey = NULL; > Code looks good, verified that TLS connections still works with mbedtls and openssl builds. Acked-by: Steffan Karger <st...@ka...> -Steffan 

Hi Gisle, 1. Please fix your patch, I cannot apply it: c:\Users\lev\Projects\openvpn>git am Openvpn-devel-Wrong-FILETYPE-in-.rc-files.patch error: patch failed: src/openvpn/openvpn_win32_resources.rc:19 error: src/openvpn/openvpn_win32_resources.rc: patch does not apply error: patch failed: src/openvpnserv/openvpnserv_resources.rc:19 error: src/openvpnserv/openvpnserv_resources.rc: patch does not apply hint: Use 'git am --show-current-patch' to see the failed patch Applying: Wrong FILETYPE in .rc files Patch failed at 0001 Wrong FILETYPE in .rc files When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Use git-send-email for sending patches. 2. Add "Signed-off-by" line to commit message (use git commit -s). 3. Since you patch .rc files, could you replace rest of numeric values with strings, like FILEOS VOS_NT_WINDOWS32 instead of FILEOS 0x40004L -- -Lev 

On Wed, Jul 24, 2019 at 8:29 AM Arne Schwabe <ar...@rf...> wrote: > > From: Rosen Penev <ro...@gm...> > > EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were > replaced with _reset. > > EVP_CIPHER_CTX_free in OpenSSL 1.1 replaces the cleanup/free combo of > earlier OpenSSL version. And OpenSSL 1.0.2 already calls cleanup as part > of _free. > > Therefore we can remove the _cleanup calls and use the OpenSSL 1.1. API > everywhere. > > Also removed initialisation with OpenSSL 1.1 as it is no longer > needed and causes compilation errors when disabling deprecated APIs. > > Same with SSL_CTX_set_ecdh_auto as it got removed. > > Patch V3: Use EVP_CIPHER_CTX_reset instead of init/cleanup > > Signed-off-by: Rosen Penev <ro...@gm...> > Signed-off-by: Arne Schwabe <ar...@rf...> ACK > --- > configure.ac | 3 +++ > src/openvpn/crypto.c | 1 - > src/openvpn/crypto_backend.h | 9 +-------- > src/openvpn/crypto_mbedtls.c | 7 +------ > src/openvpn/crypto_openssl.c | 8 +------- > src/openvpn/openssl_compat.h | 12 ++++++++++++ > src/openvpn/ssl_openssl.c | 18 ++++++++++++------ > 7 files changed, 30 insertions(+), 28 deletions(-) > > diff --git a/configure.ac b/configure.ac > index 59673e04..b8e2476f 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -918,10 +918,13 @@ if test "${with_crypto_library}" = "openssl"; then > EVP_MD_CTX_new \ > EVP_MD_CTX_free \ > EVP_MD_CTX_reset \ > + EVP_CIPHER_CTX_reset \ > OpenSSL_version \ > SSL_CTX_get_default_passwd_cb \ > SSL_CTX_get_default_passwd_cb_userdata \ > SSL_CTX_set_security_level \ > + X509_get0_notBefore \ > + X509_get0_notAfter \ > X509_get0_pubkey \ > X509_STORE_get0_objects \ > X509_OBJECT_free \ > diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c > index 8a92a8c1..585bfbc6 100644 > --- a/src/openvpn/crypto.c > +++ b/src/openvpn/crypto.c > @@ -906,7 +906,6 @@ free_key_ctx(struct key_ctx *ctx) > { > if (ctx->cipher) > { > - cipher_ctx_cleanup(ctx->cipher); > cipher_ctx_free(ctx->cipher); > ctx->cipher = NULL; > } > diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h > index 7e9a4bd2..d119442f 100644 > --- a/src/openvpn/crypto_backend.h > +++ b/src/openvpn/crypto_backend.h > @@ -341,7 +341,7 @@ bool cipher_kt_mode_aead(const cipher_kt_t *cipher); > cipher_ctx_t *cipher_ctx_new(void); > > /** > - * Free a cipher context > + * Cleanup and free a cipher context > * > * @param ctx Cipher context. > */ > @@ -360,13 +360,6 @@ void cipher_ctx_free(cipher_ctx_t *ctx); > void cipher_ctx_init(cipher_ctx_t *ctx, const uint8_t *key, int key_len, > const cipher_kt_t *kt, int enc); > > -/** > - * Cleanup the specified context. > - * > - * @param ctx Cipher context to cleanup. > - */ > -void cipher_ctx_cleanup(cipher_ctx_t *ctx); > - > /** > * Returns the size of the IV used by the cipher, in bytes, or 0 if no IV is > * used. > diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c > index 2e931440..f924323d 100644 > --- a/src/openvpn/crypto_mbedtls.c > +++ b/src/openvpn/crypto_mbedtls.c > @@ -616,12 +616,6 @@ cipher_ctx_init(mbedtls_cipher_context_t *ctx, const uint8_t *key, int key_len, > ASSERT(ctx->key_bitlen <= key_len*8); > } > > -void > -cipher_ctx_cleanup(mbedtls_cipher_context_t *ctx) > -{ > - mbedtls_cipher_free(ctx); > -} > - > int > cipher_ctx_iv_length(const mbedtls_cipher_context_t *ctx) > { > @@ -861,6 +855,7 @@ md_ctx_new(void) > void > md_ctx_free(mbedtls_md_context_t *ctx) > { > + mbedtls_cipher_free(ctx); > free(ctx); > } > > diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c > index c049e52d..520e40ee 100644 > --- a/src/openvpn/crypto_openssl.c > +++ b/src/openvpn/crypto_openssl.c > @@ -772,7 +772,7 @@ cipher_ctx_init(EVP_CIPHER_CTX *ctx, const uint8_t *key, int key_len, > { > ASSERT(NULL != kt && NULL != ctx); > > - EVP_CIPHER_CTX_init(ctx); > + EVP_CIPHER_CTX_reset(ctx); > if (!EVP_CipherInit(ctx, kt, NULL, NULL, enc)) > { > crypto_msg(M_FATAL, "EVP cipher init #1"); > @@ -792,12 +792,6 @@ cipher_ctx_init(EVP_CIPHER_CTX *ctx, const uint8_t *key, int key_len, > ASSERT(EVP_CIPHER_CTX_key_length(ctx) <= key_len); > } > > -void > -cipher_ctx_cleanup(EVP_CIPHER_CTX *ctx) > -{ > - EVP_CIPHER_CTX_cleanup(ctx); > -} > - > int > cipher_ctx_iv_length(const EVP_CIPHER_CTX *ctx) > { > diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h > index a4072b9a..4ac8f24d 100644 > --- a/src/openvpn/openssl_compat.h > +++ b/src/openvpn/openssl_compat.h > @@ -89,6 +89,18 @@ EVP_MD_CTX_new(void) > } > #endif > > +#if !defined(HAVE_EVP_CIPHER_CTX_RESET) > +#define EVP_CIPHER_CTX_reset EVP_CIPHER_CTX_init > +#endif > + > +#if !defined(HAVE_X509_GET0_NOTBEFORE) > +#define X509_get0_notBefore X509_get_notBefore > +#endif > + > +#if !defined(HAVE_X509_GET0_NOTAFTER) > +#define X509_get0_notAfter X509_get_notAfter > +#endif > + > #if !defined(HAVE_HMAC_CTX_RESET) > /** > * Reset a HMAC context > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c > index 05ca4113..c029d0f2 100644 > --- a/src/openvpn/ssl_openssl.c > +++ b/src/openvpn/ssl_openssl.c > @@ -76,12 +76,13 @@ int mydata_index; /* GLOBAL */ > void > tls_init_lib(void) > { > +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) > SSL_library_init(); > -#ifndef ENABLE_SMALL > +# ifndef ENABLE_SMALL > SSL_load_error_strings(); > -#endif > +# endif > OpenSSL_add_all_algorithms(); > - > +#endif > mydata_index = SSL_get_ex_new_index(0, "struct session *", NULL, NULL, NULL); > ASSERT(mydata_index >= 0); > } > @@ -89,9 +90,11 @@ tls_init_lib(void) > void > tls_free_lib(void) > { > +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) > EVP_cleanup(); > -#ifndef ENABLE_SMALL > +# ifndef ENABLE_SMALL > ERR_free_strings(); > +# endif > #endif > } > > @@ -567,7 +570,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx) > goto cleanup; /* Nothing to check if there is no certificate */ > } > > - ret = X509_cmp_time(X509_get_notBefore(cert), NULL); > + ret = X509_cmp_time(X509_get0_notBefore(cert), NULL); > if (ret == 0) > { > msg(D_TLS_DEBUG_MED, "Failed to read certificate notBefore field."); > @@ -577,7 +580,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx) > msg(M_WARN, "WARNING: Your certificate is not yet valid!"); > } > > - ret = X509_cmp_time(X509_get_notAfter(cert), NULL); > + ret = X509_cmp_time(X509_get0_notAfter(cert), NULL); > if (ret == 0) > { > msg(D_TLS_DEBUG_MED, "Failed to read certificate notAfter field."); > @@ -660,10 +663,13 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name > else > { > #if OPENSSL_VERSION_NUMBER >= 0x10002000L > +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) > + > /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter > * loading */ > SSL_CTX_set_ecdh_auto(ctx->ctx, 1); > return; > +#endif > #else > /* For older OpenSSL we have to extract the curve from key on our own */ > EC_KEY *eckey = NULL; > -- > 2.22.0 > 

From: Rosen Penev <ro...@gm...> EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were replaced with _reset. EVP_CIPHER_CTX_free in OpenSSL 1.1 replaces the cleanup/free combo of earlier OpenSSL version. And OpenSSL 1.0.2 already calls cleanup as part of _free. Therefore we can remove the _cleanup calls and use the OpenSSL 1.1. API everywhere. Also removed initialisation with OpenSSL 1.1 as it is no longer needed and causes compilation errors when disabling deprecated APIs. Same with SSL_CTX_set_ecdh_auto as it got removed. Patch V3: Use EVP_CIPHER_CTX_reset instead of init/cleanup Signed-off-by: Rosen Penev <ro...@gm...> Signed-off-by: Arne Schwabe <ar...@rf...> --- configure.ac | 3 +++ src/openvpn/crypto.c | 1 - src/openvpn/crypto_backend.h | 9 +-------- src/openvpn/crypto_mbedtls.c | 7 +------ src/openvpn/crypto_openssl.c | 8 +------- src/openvpn/openssl_compat.h | 12 ++++++++++++ src/openvpn/ssl_openssl.c | 18 ++++++++++++------ 7 files changed, 30 insertions(+), 28 deletions(-) diff --git a/configure.ac b/configure.ac index 59673e04..b8e2476f 100644 --- a/configure.ac +++ b/configure.ac @@ -918,10 +918,13 @@ if test "${with_crypto_library}" = "openssl"; then	EVP_MD_CTX_new \	EVP_MD_CTX_free \	EVP_MD_CTX_reset \ +	EVP_CIPHER_CTX_reset \	OpenSSL_version \	SSL_CTX_get_default_passwd_cb \	SSL_CTX_get_default_passwd_cb_userdata \	SSL_CTX_set_security_level \ +	X509_get0_notBefore \ +	X509_get0_notAfter \	X509_get0_pubkey \	X509_STORE_get0_objects \	X509_OBJECT_free \ diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c index 8a92a8c1..585bfbc6 100644 --- a/src/openvpn/crypto.c +++ b/src/openvpn/crypto.c @@ -906,7 +906,6 @@ free_key_ctx(struct key_ctx *ctx) { if (ctx->cipher) { - cipher_ctx_cleanup(ctx->cipher); cipher_ctx_free(ctx->cipher); ctx->cipher = NULL; } diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h index 7e9a4bd2..d119442f 100644 --- a/src/openvpn/crypto_backend.h +++ b/src/openvpn/crypto_backend.h @@ -341,7 +341,7 @@ bool cipher_kt_mode_aead(const cipher_kt_t *cipher); cipher_ctx_t *cipher_ctx_new(void); /** - * Free a cipher context + * Cleanup and free a cipher context * * @param ctx Cipher context. */ @@ -360,13 +360,6 @@ void cipher_ctx_free(cipher_ctx_t *ctx); void cipher_ctx_init(cipher_ctx_t *ctx, const uint8_t *key, int key_len, const cipher_kt_t *kt, int enc); -/** - * Cleanup the specified context. - * - * @param ctx Cipher context to cleanup. - */ -void cipher_ctx_cleanup(cipher_ctx_t *ctx); - /** * Returns the size of the IV used by the cipher, in bytes, or 0 if no IV is * used. diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c index 2e931440..f924323d 100644 --- a/src/openvpn/crypto_mbedtls.c +++ b/src/openvpn/crypto_mbedtls.c @@ -616,12 +616,6 @@ cipher_ctx_init(mbedtls_cipher_context_t *ctx, const uint8_t *key, int key_len, ASSERT(ctx->key_bitlen <= key_len*8); } -void -cipher_ctx_cleanup(mbedtls_cipher_context_t *ctx) -{ - mbedtls_cipher_free(ctx); -} - int cipher_ctx_iv_length(const mbedtls_cipher_context_t *ctx) { @@ -861,6 +855,7 @@ md_ctx_new(void) void md_ctx_free(mbedtls_md_context_t *ctx) { + mbedtls_cipher_free(ctx); free(ctx); } diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c index c049e52d..520e40ee 100644 --- a/src/openvpn/crypto_openssl.c +++ b/src/openvpn/crypto_openssl.c @@ -772,7 +772,7 @@ cipher_ctx_init(EVP_CIPHER_CTX *ctx, const uint8_t *key, int key_len, { ASSERT(NULL != kt && NULL != ctx); - EVP_CIPHER_CTX_init(ctx); + EVP_CIPHER_CTX_reset(ctx); if (!EVP_CipherInit(ctx, kt, NULL, NULL, enc)) { crypto_msg(M_FATAL, "EVP cipher init #1"); @@ -792,12 +792,6 @@ cipher_ctx_init(EVP_CIPHER_CTX *ctx, const uint8_t *key, int key_len, ASSERT(EVP_CIPHER_CTX_key_length(ctx) <= key_len); } -void -cipher_ctx_cleanup(EVP_CIPHER_CTX *ctx) -{ - EVP_CIPHER_CTX_cleanup(ctx); -} - int cipher_ctx_iv_length(const EVP_CIPHER_CTX *ctx) { diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h index a4072b9a..4ac8f24d 100644 --- a/src/openvpn/openssl_compat.h +++ b/src/openvpn/openssl_compat.h @@ -89,6 +89,18 @@ EVP_MD_CTX_new(void) } #endif +#if !defined(HAVE_EVP_CIPHER_CTX_RESET) +#define EVP_CIPHER_CTX_reset EVP_CIPHER_CTX_init +#endif + +#if !defined(HAVE_X509_GET0_NOTBEFORE) +#define X509_get0_notBefore X509_get_notBefore +#endif + +#if !defined(HAVE_X509_GET0_NOTAFTER) +#define X509_get0_notAfter X509_get_notAfter +#endif + #if !defined(HAVE_HMAC_CTX_RESET) /** * Reset a HMAC context diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index 05ca4113..c029d0f2 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -76,12 +76,13 @@ int mydata_index; /* GLOBAL */ void tls_init_lib(void) { +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) SSL_library_init(); -#ifndef ENABLE_SMALL +# ifndef ENABLE_SMALL SSL_load_error_strings(); -#endif +# endif OpenSSL_add_all_algorithms(); - +#endif mydata_index = SSL_get_ex_new_index(0, "struct session *", NULL, NULL, NULL); ASSERT(mydata_index >= 0); } @@ -89,9 +90,11 @@ tls_init_lib(void) void tls_free_lib(void) { +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) EVP_cleanup(); -#ifndef ENABLE_SMALL +# ifndef ENABLE_SMALL ERR_free_strings(); +# endif #endif } @@ -567,7 +570,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx) goto cleanup; /* Nothing to check if there is no certificate */ } - ret = X509_cmp_time(X509_get_notBefore(cert), NULL); + ret = X509_cmp_time(X509_get0_notBefore(cert), NULL); if (ret == 0) { msg(D_TLS_DEBUG_MED, "Failed to read certificate notBefore field."); @@ -577,7 +580,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx) msg(M_WARN, "WARNING: Your certificate is not yet valid!"); } - ret = X509_cmp_time(X509_get_notAfter(cert), NULL); + ret = X509_cmp_time(X509_get0_notAfter(cert), NULL); if (ret == 0) { msg(D_TLS_DEBUG_MED, "Failed to read certificate notAfter field."); @@ -660,10 +663,13 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name else { #if OPENSSL_VERSION_NUMBER >= 0x10002000L +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) + /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter * loading */ SSL_CTX_set_ecdh_auto(ctx->ctx, 1); return; +#endif #else /* For older OpenSSL we have to extract the curve from key on our own */ EC_KEY *eckey = NULL; -- 2.22.0 

Hi, Looks good now and works as expected. On Tue, Jul 23, 2019 at 5:22 AM Lev Stipakov <lst...@gm...> wrote: > > From: Lev Stipakov <le...@op...> > > This patch enables interactive service to open tun device. > This is mostly needed by Wintun, which could be opened > only by privileged process. > > When interactive service is used, instead of calling > CreateFile() directly by openvpn process we pass tun device path > into service process. There we open device, duplicate handle > and pass it back to openvpn process. > > Signed-off-by: Lev Stipakov <le...@op...> > --- > v6: > - simplify and strengthen guid check with wcsspn() > - fix doxygen comment > > v5: > - further strengthen security by passing only device guid from client process > to service, validating guid and constructing device path on service side > > v4: > - strengthen security by adding basic validation to device path > - reorder fields in msg_open_tun_device_result for backward compatibility > > v3: > - ensure that device path passed by client is null-terminated > - support for multiple openvpn processes > - return proper error code when device handle is invalid > > v2: > - introduce send_msg_iservice_ex() instead of changing > signature of existing send_msg_iservice() > - use wchar_t strings in interactive service code > > include/openvpn-msg.h | 12 +++++++ > src/openvpn/tun.c | 83 ++++++++++++++++++++++++++++++++++--------- > src/openvpn/win32.c | 9 ++++- > src/openvpn/win32.h | 30 +++++++++++++--- > src/openvpnserv/interactive.c | 78 ++++++++++++++++++++++++++++++++++++++-- > 5 files changed, 188 insertions(+), 24 deletions(-) ... > diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c > index 623c3ff..1b4a5e2 100644 > --- a/src/openvpnserv/interactive.c > +++ b/src/openvpnserv/interactive.c > @@ -58,7 +58,6 @@ static settings_t settings; > static HANDLE rdns_semaphore = NULL; > #define RDNS_TIMEOUT 600 /* seconds to wait for the semaphore */ > > - > openvpn_service_t interactive_service = { > interactive, > TEXT(PACKAGE_NAME "ServiceInteractive"), > @@ -1198,8 +1197,62 @@ HandleEnableDHCPMessage(const enable_dhcp_message_t *dhcp) > return err; > } > > +static DWORD > +HandleOpenTunDeviceMessage(const open_tun_device_message_t *open_tun, HANDLE ovpn_proc, HANDLE *remote_handle) > +{ > + DWORD err = ERROR_SUCCESS; > + HANDLE local_handle; > + LPWSTR wguid = NULL; > + WCHAR device_path[256] = {0}; > + const WCHAR prefix[] = L"\\\\.\\Global\\"; > + const WCHAR tap_suffix[] = L".tap"; > + > + *remote_handle = INVALID_HANDLE_VALUE; > + > + wguid = utf8to16(open_tun->device_guid); > + if (!wguid) > + { > + err = ERROR_OUTOFMEMORY; > + goto out; > + } > + > + /* validate device guid */ > + const size_t guid_len = wcslen(wguid); > + if (guid_len != 38 || wcsspn(wguid, L"0123456789ABCDEFabcdef-{}") != guid_len) > + { > + err = ERROR_MESSAGE_DATA; > + MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Invalid device guild (%s)"), wguid); guild -> guid (as pointed out by tincanteksup) > + goto out; > + } > + whitespace in line above. These could be fixed at commit time. Acked-by: Selva Nair <sel...@gm...> Selva Selva 

Hi, On 24/07/2019 13:57, Gert Doering wrote: > Hi, > > On Wed, Jul 24, 2019 at 01:46:36PM +0200, Antonio Quartulli wrote: >> this patch has been mangled by your e-mail client. >> >> Could you please re-send it using git send-email? > > That seems to have been a false positive on your side... git applies > this patch just fine, so at least here it arrived without mangling > (= nothing wrong on the sender side). > ops, sorry then. It probably was my local parser (used to highlight diffs) that failed. Cheers, -- Antonio Quartulli 

Hi, On Wed, Jul 24, 2019 at 01:46:36PM +0200, Antonio Quartulli wrote: > this patch has been mangled by your e-mail client. > > Could you please re-send it using git send-email? That seems to have been a false positive on your side... git applies this patch just fine, so at least here it arrived without mangling (= nothing wrong on the sender side). gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany ge...@gr... 

Hi, this patch has been mangled by your e-mail client. Could you please re-send it using git send-email? Thanks a lot On 24/07/2019 13:41, Daniel Kaldor wrote: > There is a ~1s delay between establishing connection with remote > server and starting TLS handshake. > This change removes delay and improves connection time. > > --- > src/openvpn/forward.c | 61 +++++++++++++++++++++++++++------------------------ > 1 file changed, 32 insertions(+), 29 deletions(-) > > diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c > index 35df089..2deaf93 100644 > --- a/src/openvpn/forward.c > +++ b/src/openvpn/forward.c > @@ -433,28 +433,6 @@ check_connection_established_dowork(struct context *c) > { > if (CONNECTION_ESTABLISHED(c)) > { > -#if P2MP > - /* if --pull was specified, send a push request to server */ > - if (c->c2.tls_multi && c->options.pull) > - { > -#ifdef ENABLE_MANAGEMENT > - if (management) > - { > - management_set_state(management, > - OPENVPN_STATE_GET_CONFIG, > - NULL, > - NULL, > - NULL, > - NULL, > - NULL); > - } > -#endif > - /* fire up push request right away (already 1s delayed) */ > - event_timeout_init(&c->c2.push_request_interval, 0, now); > - reset_coarse_timers(c); > - } > - else > -#endif /* if P2MP */ > { > do_up(c, false, 0); > } > @@ -1943,17 +1921,34 @@ pre_select(struct context *c) > } > } > #endif > - > - /* check coarse timers? */ > - check_coarse_timers(c); > - if (c->sig->signal_received) > - { > - return; > - } > + > + bool pre_connection_state = CONNECTION_ESTABLISHED(c); > > /* Does TLS need service? */ > check_tls(c); > > + bool post_connection_state = CONNECTION_ESTABLISHED(c); > + > + if(!pre_connection_state && post_connection_state){ > + > + if (c->c2.tls_multi && c->options.pull) > + { > +#ifdef ENABLE_MANAGEMENT > + if (management) > + { > + management_set_state(management, > + OPENVPN_STATE_GET_CONFIG, > + NULL, > + NULL, > + NULL, > + NULL, > + NULL); > + } > +#endif > + check_push_request_dowork(c); > + } > + } > + > /* In certain cases, TLS errors will require a restart */ > check_tls_errors(c); > if (c->sig->signal_received) > @@ -1961,6 +1956,14 @@ pre_select(struct context *c) > return; > } > > + /* check coarse timers */ > + check_coarse_timers(c); > + if (c->sig->signal_received) > + { > + return; > + } > + > + > /* check for incoming configuration info on the control channel */ > check_incoming_control_channel(c); > > -- > 2.9.3 (Apple Git-75) > > > _______________________________________________ > Openvpn-devel mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openvpn-devel > -- Antonio Quartulli 

There is a ~1s delay between establishing connection with remote server and starting TLS handshake. This change removes delay and improves connection time. --- src/openvpn/forward.c | 61 +++++++++++++++++++++++++++------------------------ 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 35df089..2deaf93 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -433,28 +433,6 @@ check_connection_established_dowork(struct context *c) { if (CONNECTION_ESTABLISHED(c)) { -#if P2MP - /* if --pull was specified, send a push request to server */ - if (c->c2.tls_multi && c->options.pull) - { -#ifdef ENABLE_MANAGEMENT - if (management) - { - management_set_state(management, - OPENVPN_STATE_GET_CONFIG, - NULL, - NULL, - NULL, - NULL, - NULL); - } -#endif - /* fire up push request right away (already 1s delayed) */ - event_timeout_init(&c->c2.push_request_interval, 0, now); - reset_coarse_timers(c); - } - else -#endif /* if P2MP */ { do_up(c, false, 0); } @@ -1943,17 +1921,34 @@ pre_select(struct context *c) } } #endif - - /* check coarse timers? */ - check_coarse_timers(c); - if (c->sig->signal_received) - { - return; - } + + bool pre_connection_state = CONNECTION_ESTABLISHED(c); /* Does TLS need service? */ check_tls(c); + bool post_connection_state = CONNECTION_ESTABLISHED(c); + + if(!pre_connection_state && post_connection_state){ + + if (c->c2.tls_multi && c->options.pull) + { +#ifdef ENABLE_MANAGEMENT + if (management) + { + management_set_state(management, + OPENVPN_STATE_GET_CONFIG, + NULL, + NULL, + NULL, + NULL, + NULL); + } +#endif + check_push_request_dowork(c); + } + } + /* In certain cases, TLS errors will require a restart */ check_tls_errors(c); if (c->sig->signal_received) @@ -1961,6 +1956,14 @@ pre_select(struct context *c) return; } + /* check coarse timers */ + check_coarse_timers(c); + if (c->sig->signal_received) + { + return; + } + + /* check for incoming configuration info on the control channel */ check_incoming_control_channel(c); -- 2.9.3 (Apple Git-75)