[Openvpn-devel,v3,03/21,OSSL,3.0] Implement DES ECB encrypt via EVP_CIPHER api

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

Commit Message

Arne Schwabe Oct. 19, 2021, 6:31 p.m.
Even though DES is super outdated and also NTLM is super outdated,
eliminating the warnings for OpenSSL 3.0 is still a step in the right
direction and using the correct APIs.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/crypto_openssl.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Comments

Maximilian Fillinger Oct. 20, 2021, 5:36 p.m. | #1
On 19/10/2021 20:31, Arne Schwabe wrote:
> +    if (!EVP_EncryptInit_ex(ctx, EVP_bf_ecb(), NULL, key, 0))

EVP_bf_ecb() is the Blowfish cipher, not DES.
Selva Nair Oct. 21, 2021, 5:09 a.m. | #2
Hi,

On Tue, Oct 19, 2021 at 2:32 PM Arne Schwabe <arne@rfc2549.org> wrote:

> Even though DES is super outdated and also NTLM is super outdated,
> eliminating the warnings for OpenSSL 3.0 is still a step in the right
> direction and using the correct APIs.
>
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/crypto_openssl.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index 1c800df7f..021698f12 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -879,10 +879,26 @@ cipher_des_encrypt_ecb(const unsigned char
> key[DES_KEY_LENGTH],
>                         unsigned char src[DES_KEY_LENGTH],
>                         unsigned char dst[DES_KEY_LENGTH])
>  {
> -    DES_key_schedule sched;
> +    EVP_CIPHER_CTX *ctx = EVP_CIPHER_CTX_new();
> +    if (!ctx)
> +    {
> +        crypto_msg(M_FATAL, "%s: EVP_CIPHER_CTX_new() failed", __func__);
> +    }
> +    if (!EVP_EncryptInit_ex(ctx, EVP_bf_ecb(), NULL, key, 0))
>

Apart from the wrong cipher type that Max pointed out, this call will fail
in OpenSSL 3.0 unless legacy is loaded, right? Causing a run-time error in
that case sounds good to me but a helpful error message like legacy
provider may be required or even a check whether legacy is loaded and error
out appropriately would be helpful.

PS: can't we just get rid of the use of DES altogether?

Selva
<div dir="ltr"><div>Hi,</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">Even though DES is super outdated and also NTLM is super outdated,<br>
eliminating the warnings for OpenSSL 3.0 is still a step in the right<br>
direction and using the correct APIs.<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 | 22 +++++++++++++++++++---<br>
 1 file changed, 19 insertions(+), 3 deletions(-)<br>
<br>
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c<br>
index 1c800df7f..021698f12 100644<br>
--- a/src/openvpn/crypto_openssl.c<br>
+++ b/src/openvpn/crypto_openssl.c<br>
@@ -879,10 +879,26 @@ cipher_des_encrypt_ecb(const unsigned char key[DES_KEY_LENGTH],<br>
                        unsigned char src[DES_KEY_LENGTH],<br>
                        unsigned char dst[DES_KEY_LENGTH])<br>
 {<br>
-    DES_key_schedule sched;<br>
+    EVP_CIPHER_CTX *ctx = EVP_CIPHER_CTX_new();<br>
+    if (!ctx)<br>
+    {<br>
+        crypto_msg(M_FATAL, &quot;%s: EVP_CIPHER_CTX_new() failed&quot;, __func__);<br>
+    }<br>
+    if (!EVP_EncryptInit_ex(ctx, EVP_bf_ecb(), NULL, key, 0))<br></blockquote><div><br></div><div>Apart from the wrong cipher type that Max pointed out, this call will fail in OpenSSL 3.0 unless legacy is loaded, right? Causing a run-time error in that case sounds good to me but a helpful error message like legacy provider may be required or even a check whether legacy is loaded and error out appropriately would be helpful.</div><div><br></div><div>PS: can&#39;t we just get rid of the use of DES altogether?</div><div><br></div><div>Selva</div></div></div>
Gert Doering Oct. 21, 2021, 6:11 a.m. | #3
Hi,

On Thu, Oct 21, 2021 at 01:09:14AM -0400, Selva Nair wrote:
> PS: can't we just get rid of the use of DES altogether?

Is there a newer NTLM auth variant that does not use DES?

Otherwise it's "keep NTLM auth => keep DES"...

gert
Arne Schwabe Oct. 21, 2021, 9:41 a.m. | #4
Am 20.10.21 um 19:36 schrieb Max Fillinger:
> On 19/10/2021 20:31, Arne Schwabe wrote:
>> +    if (!EVP_EncryptInit_ex(ctx, EVP_bf_ecb(), NULL, key, 0))
> 
> EVP_bf_ecb() is the Blowfish cipher, not DES.
> 

Oops. I probably need to write a unit test for this one too. Because
that sounds like I broke this.

Arne
Arne Schwabe Oct. 21, 2021, 9:42 a.m. | #5
> 
> 
> Apart from the wrong cipher type that Max pointed out, this call will
> fail in OpenSSL 3.0 unless legacy is loaded, right? Causing a run-time
> error in that case sounds good to me but a helpful error message like
> legacy provider may be required or even a check whether legacy is loaded
> and error out appropriately would be helpful.
> 
> PS: can't we just get rid of the use of DES altogether?
> 

As Gert pointed out that NTLM depends on it. We can trick a bit here
with DES-EDE and three times the same key but yes it should have a
better error message.

Arne
Selva Nair Oct. 21, 2021, 6:04 p.m. | #6
On Thu, Oct 21, 2021 at 5:42 AM Arne Schwabe <arne@rfc2549.org> wrote:

>
> >
> >
> > Apart from the wrong cipher type that Max pointed out, this call will
> > fail in OpenSSL 3.0 unless legacy is loaded, right? Causing a run-time
> > error in that case sounds good to me but a helpful error message like
> > legacy provider may be required or even a check whether legacy is loaded
> > and error out appropriately would be helpful.
> >
> > PS: can't we just get rid of the use of DES altogether?
> >
>
> As Gert pointed out that NTLM depends on it. We can trick a bit here
> with DES-EDE and three times the same key but yes it should have a
> better error message.
>

Oh, proxy with ntlm auth... That will have to live on for a while longer.

In that case we could load the legacy provider if http-proxy with auto,
auto-nct or ntlm is in options (in a separate patch), and can leave the
generic error message in this patch?

Selva
<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 21, 2021 at 5:42 AM Arne Schwabe &lt;<a href="mailto:arne@rfc2549.org">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"><br>
&gt; <br>
&gt; <br>
&gt; Apart from the wrong cipher type that Max pointed out, this call will<br>
&gt; fail in OpenSSL 3.0 unless legacy is loaded, right? Causing a run-time<br>
&gt; error in that case sounds good to me but a helpful error message like<br>
&gt; legacy provider may be required or even a check whether legacy is loaded<br>
&gt; and error out appropriately would be helpful.<br>
&gt; <br>
&gt; PS: can&#39;t we just get rid of the use of DES altogether?<br>
&gt; <br>
<br>
As Gert pointed out that NTLM depends on it. We can trick a bit here<br>
with DES-EDE and three times the same key but yes it should have a<br>
better error message.<br></blockquote><div><br></div><div>Oh, proxy with ntlm auth... That will have to live on for a while longer.</div><div><br></div><div>In that case we could load the legacy provider if http-proxy with auto, auto-nct or ntlm is in options (in a separate patch), and can leave the generic error message in this patch?</div><div><br></div><div>Selva</div></div></div>

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 1c800df7f..021698f12 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -879,10 +879,26 @@  cipher_des_encrypt_ecb(const unsigned char key[DES_KEY_LENGTH],
                        unsigned char src[DES_KEY_LENGTH],
                        unsigned char dst[DES_KEY_LENGTH])
 {
-    DES_key_schedule sched;
+    EVP_CIPHER_CTX *ctx = EVP_CIPHER_CTX_new();
+    if (!ctx)
+    {
+        crypto_msg(M_FATAL, "%s: EVP_CIPHER_CTX_new() failed", __func__);
+    }
+    if (!EVP_EncryptInit_ex(ctx, EVP_bf_ecb(), NULL, key, 0))
+    {
+        crypto_msg(M_FATAL, "%s: EVP_EncryptInit_ex() failed", __func__);
+    }
 
-    DES_set_key_unchecked((DES_cblock *)key, &sched);
-    DES_ecb_encrypt((DES_cblock *)src, (DES_cblock *)dst, &sched, DES_ENCRYPT);
+    int len;
+    if(!EVP_EncryptUpdate(ctx, dst, &len, src, DES_KEY_LENGTH))
+    {
+        crypto_msg(M_FATAL, "%s: EVP_EncryptUpdate() failed", __func__);
+    }
+
+    if (!EVP_EncryptFinal(ctx, dst + len, &len))
+    {
+        crypto_msg(M_FATAL, "%s: EVP_EncryptFinal() failed", __func__);
+    }
 }
 
 /*