| Message ID | 20211019183127.614175-4-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OpenSSL 3.0 improvements for OpenVPN | expand |
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.
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 <<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">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 <<a href="mailto:arne@rfc2549.org" target="_blank">arne@rfc2549.org</a>><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, "%s: EVP_CIPHER_CTX_new() failed", __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't we just get rid of the use of DES altogether?</div><div><br></div><div>Selva</div></div></div>
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
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
> > > 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
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 <<a href="mailto:arne@rfc2549.org">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"><br> > <br> > <br> > Apart from the wrong cipher type that Max pointed out, this call will<br> > fail in OpenSSL 3.0 unless legacy is loaded, right? Causing a run-time<br> > error in that case sounds good to me but a helpful error message like<br> > legacy provider may be required or even a check whether legacy is loaded<br> > and error out appropriately would be helpful.<br> > <br> > PS: can't we just get rid of the use of DES altogether?<br> > <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>
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__); + } } /*
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(-)