| Message ID | 20211019183127.614175-11-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | OpenSSL 3.0 improvements for OpenVPN | expand |
On 19/10/2021 20:31, Arne Schwabe wrote: > In OpenSSL 3.0 EVP_get_cipherbyname return a non NULL algorithm > even if the algorithm is not avaialble with the currently available > provider. Luckily EVP_get_cipherbyname can be used here as drop > in replacement and returns only non NULL if the algorithm is actually > currently supported. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Max Fillinger <maximilian.fillinger@foxcrypto.com> Looks good to me! Some small errors in the commit message: "return a non Null algorithm": Should be "may return", I think. second "EVP_get_cipherbyname" should be "EVP_CIPHER_fetch".
This has an ACK, but will leak memory in OpenSSL 3.0 On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <arne@rfc2549.org> wrote: > In OpenSSL 3.0 EVP_get_cipherbyname return a non NULL algorithm > even if the algorithm is not avaialble with the currently available > provider. Luckily EVP_get_cipherbyname can be used here as drop > in replacement and returns only non NULL if the algorithm is actually > currently supported. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > src/openvpn/crypto_openssl.c | 6 +++--- > src/openvpn/openssl_compat.h | 17 +++++++++++++++++ > 2 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c > index 93c85a836..b10bd7cd5 100644 > --- a/src/openvpn/crypto_openssl.c > +++ b/src/openvpn/crypto_openssl.c > @@ -572,7 +572,7 @@ cipher_kt_get(const char *ciphername) > ASSERT(ciphername); > > ciphername = translate_cipher_name_from_openvpn(ciphername); > - cipher = EVP_get_cipherbyname(ciphername); > + cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL); > In OpenSSL 3.0, this 'cipher' must be freed. But the compat function is written using get_cipherbyname() which returns a const variable that should not be freed. Also, here we want to return a const cipher to the caller. One option is to continue using get_cipherbyname() but add a helper call for OpenSSL 3.0 to check algorithm availability. Say, EVP_CIPHER_available() that fetches, checks the result and frees --- to be used on top of the existing code. Selva <div dir="ltr"><div>This has an ACK, but will leak memory in OpenSSL 3.0</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">In OpenSSL 3.0 EVP_get_cipherbyname return a non NULL algorithm<br> even if the algorithm is not avaialble with the currently available<br> provider. Luckily EVP_get_cipherbyname can be used here as drop<br> in replacement and returns only non NULL if the algorithm is actually<br> currently supported.<br> <br> Signed-off-by: Arne Schwabe <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>><br> ---<br> src/openvpn/crypto_openssl.c | 6 +++---<br> src/openvpn/openssl_compat.h | 17 +++++++++++++++++<br> 2 files changed, 20 insertions(+), 3 deletions(-)<br> <br> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c<br> index 93c85a836..b10bd7cd5 100644<br> --- a/src/openvpn/crypto_openssl.c<br> +++ b/src/openvpn/crypto_openssl.c<br> @@ -572,7 +572,7 @@ cipher_kt_get(const char *ciphername)<br> ASSERT(ciphername);<br> <br> ciphername = translate_cipher_name_from_openvpn(ciphername);<br> - cipher = EVP_get_cipherbyname(ciphername);<br> + cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL);<br></blockquote><div><br></div><div>In OpenSSL 3.0, this 'cipher' must be freed. But the compat function is written using get_cipherbyname() which returns a const variable that should not be freed. Also, here we want to return a const cipher to the caller.</div><div><br></div><div>One option is to continue using get_cipherbyname() but add a helper call for OpenSSL 3.0 to check algorithm availability. Say, EVP_CIPHER_available() that fetches, checks the result and frees --- to be used on top of the existing code.</div><div> </div><div>Selva</div></div></div>
Am 30.10.21 um 21:28 schrieb Selva Nair: > This has an ACK, but will leak memory in OpenSSL 3.0 > > On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <arne@rfc2549.org > <mailto:arne@rfc2549.org>> wrote: > > In OpenSSL 3.0 EVP_get_cipherbyname return a non NULL algorithm > even if the algorithm is not avaialble with the currently available > provider. Luckily EVP_get_cipherbyname can be used here as drop > in replacement and returns only non NULL if the algorithm is actually > currently supported. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org <mailto:arne@rfc2549.org>> > --- > src/openvpn/crypto_openssl.c | 6 +++--- > src/openvpn/openssl_compat.h | 17 +++++++++++++++++ > 2 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c > index 93c85a836..b10bd7cd5 100644 > --- a/src/openvpn/crypto_openssl.c > +++ b/src/openvpn/crypto_openssl.c > @@ -572,7 +572,7 @@ cipher_kt_get(const char *ciphername) > ASSERT(ciphername); > > ciphername = translate_cipher_name_from_openvpn(ciphername); > - cipher = EVP_get_cipherbyname(ciphername); > + cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL); > > > In OpenSSL 3.0, this 'cipher' must be freed. But the compat function is > written using get_cipherbyname() which returns a const variable that > should not be freed. Also, here we want to return a const cipher to the > caller. > > One option is to continue using get_cipherbyname() but add a helper call > for OpenSSL 3.0 to check algorithm availability. Say, > EVP_CIPHER_available() that fetches, checks the result and frees --- > to be used on top of the existing code. That is an option but will break as soon as we have the first cipher that is no longer defined with EVP_ORIG_GLOBAL compatibility definition. I need to check how much work it is to teach OpenVPN to free the cipher and md. Arne
This patch took at bit of massaging to get in, due to the context
having SSL_CTX_new_ex() which disappeared during 08 v3->v4 -> replaced
by proper context (adding #if OPENSSL_VERSION_NUMBER < 0x30000000L).
I have read Selva's comment about mem leaking, and decided to still
merge it - this OSS 3 thing is work in progress, and I want to get
the later patches in that have ACKs, so the code can get more exposure
(we must not forget to return here and fix it, of course).
I have stared at the code a bit, and given it "make check" treatment -
this now explodes for 3.0.0 in the ncp_testdriver
Unsupported cipher in --data-ciphers: BF-CBC
[ ERROR ] --- Test failed with exception: Segmentation fault(11)
(but I assume that this will be fixed as soon as the legacy provider
stuff gets in). t_lpback.sh still fails, but now with a proper error
message ("Cipher SEED-OFB not supported") - I seem to remember a later
patch fixing the cipher listing part, so SEED-OFB won't even show up
as a candidate.
1.1.1 passes just fine.
Your patch has been applied to the master branch.
commit f40edaa5abe5255710315deacd8c82cdfef12647
Author: Arne Schwabe
Date: Tue Oct 19 20:31:16 2021 +0200
Replace EVP_get_cipherbyname with EVP_CIPHER_fetch
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-By: Max Fillinger <maximilian.fillinger@foxcrypto.com>
Message-Id: <20211019183127.614175-11-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23005.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
>> >> One option is to continue using get_cipherbyname() but add a helper call >> for OpenSSL 3.0 to check algorithm availability. Say, >> EVP_CIPHER_available() that fetches, checks the result and frees --- >> to be used on top of the existing code. > > That is an option but will break as soon as we have the first cipher > that is no longer defined with EVP_ORIG_GLOBAL compatibility definition. > I need to check how much work it is to teach OpenVPN to free the cipher > and md. I looked at this and I think the best option is to change API to use strings rather than EVP_CIPHER in the "public" API of the ssl library implementation. That will simplify the code rather than to complicate it. Arne
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c index 93c85a836..b10bd7cd5 100644 --- a/src/openvpn/crypto_openssl.c +++ b/src/openvpn/crypto_openssl.c @@ -572,7 +572,7 @@ cipher_kt_get(const char *ciphername) ASSERT(ciphername); ciphername = translate_cipher_name_from_openvpn(ciphername); - cipher = EVP_get_cipherbyname(ciphername); + cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL); if (NULL == cipher) { @@ -658,7 +658,7 @@ cipher_kt_block_size(const EVP_CIPHER *cipher) strcpy(mode_str, "-CBC"); - cbc_cipher = EVP_get_cipherbyname(translate_cipher_name_from_openvpn(name)); + cbc_cipher = EVP_CIPHER_fetch(NULL,translate_cipher_name_from_openvpn(name), NULL); if (cbc_cipher) { block_size = EVP_CIPHER_block_size(cbc_cipher); @@ -894,7 +894,7 @@ md_kt_get(const char *digest) { const EVP_MD *md = NULL; ASSERT(digest); - md = EVP_get_digestbyname(digest); + md = EVP_MD_fetch(NULL, digest, NULL); if (!md) { crypto_msg(M_FATAL, "Message hash algorithm '%s' not found", digest); diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h index dda47d76c..0893bfbb2 100644 --- a/src/openvpn/openssl_compat.h +++ b/src/openvpn/openssl_compat.h @@ -758,6 +758,23 @@ SSL_CTX_new_ex(void *libctx, const char *propq, const SSL_METHOD *method) (void) propq; return SSL_CTX_new(method); } +/* Mimics the functions but only when the default context without + * options is chosen */ +static inline const EVP_CIPHER * +EVP_CIPHER_fetch(void *ctx, const char *algorithm, const char *properties) +{ + ASSERT(!ctx); + ASSERT(!properties); + return EVP_get_cipherbyname(algorithm); +} + +static inline const EVP_MD* +EVP_MD_fetch(void *ctx, const char *algorithm, const char *properties) +{ + ASSERT(!ctx); + ASSERT(!properties); + return EVP_get_digestbyname(algorithm); +} #endif /* OPENSSL_VERSION_NUMBER < 0x30000000L */ #endif /* OPENSSL_COMPAT_H_ */
In OpenSSL 3.0 EVP_get_cipherbyname return a non NULL algorithm even if the algorithm is not avaialble with the currently available provider. Luckily EVP_get_cipherbyname can be used here as drop in replacement and returns only non NULL if the algorithm is actually currently supported. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/crypto_openssl.c | 6 +++--- src/openvpn/openssl_compat.h | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-)