[Openvpn-devel,v3,10/21,OSSL,3.0] Replace EVP_get_cipherbyname with EVP_CIPHER_fetch

Message ID 20211019183127.614175-11-arne@rfc2549.org
State Accepted
Headers show
Series
  • OpenSSL 3.0 improvements for OpenVPN
Related show

Commit Message

Arne Schwabe Oct. 19, 2021, 6:31 p.m.
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(-)

Comments

Maximilian Fillinger Oct. 25, 2021, 3:55 p.m. | #1
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".
Selva Nair Oct. 30, 2021, 7:28 p.m. | #2
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 &lt;<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>&gt; 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 &lt;<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>&gt;<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 &#39;cipher&#39; 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>
Arne Schwabe Oct. 31, 2021, 11 a.m. | #3
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
Gert Doering Nov. 1, 2021, 7:47 p.m. | #4
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
Arne Schwabe Nov. 1, 2021, 10:18 p.m. | #5
>>
>> 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

Patch

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_ */