[Openvpn-devel] Completely remove DES checks

Message ID 20211107090138.3150187-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel] Completely remove DES checks
Related show

Commit Message

Arne Schwabe Nov. 7, 2021, 9:01 a.m.
We already removed the check in d67658fee for OpenSSL 3.0. This removes the
checks entirely for all crypto libraries.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/crypto.c         | 15 --------
 src/openvpn/crypto_backend.h | 28 ---------------
 src/openvpn/crypto_mbedtls.c | 56 ------------------------------
 src/openvpn/crypto_openssl.c | 66 ------------------------------------
 4 files changed, 165 deletions(-)

Comments

Max Fillinger Nov. 7, 2021, 10:47 a.m. | #1
On 07/11/2021 10:01, Arne Schwabe wrote:
> We already removed the check in d67658fee for OpenSSL 3.0. This removes the
> checks entirely for all crypto libraries.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Acked-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>

Looks good to me!

Compiled and ran --test-crypto for DES/DES3, with mbedtls and OpenSSL 3.
Matthias Andree Nov. 7, 2021, 11:57 a.m. | #2
Am 07.11.21 um 10:01 schrieb Arne Schwabe:
> We already removed the check in d67658fee for OpenSSL 3.0. This removes the
> checks entirely for all crypto libraries.
>
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>   src/openvpn/crypto.c         | 15 --------
>   src/openvpn/crypto_backend.h | 28 ---------------
>   src/openvpn/crypto_mbedtls.c | 56 ------------------------------
>   src/openvpn/crypto_openssl.c | 66 ------------------------------------
>   4 files changed, 165 deletions(-)
> -    /* DES is deprecated and the method to even check the keys is deprecated
> -     * in OpenSSL 3.0. Instead of checking for the 16 weak/semi-weak keys
> -     * we just accept them in OpenSSL 3.0 since the risk of randomly getting
> -     * these is pretty low (and "all DES keys are weak" anyway) */
> -    return true;

Should not we nuke DES altogether in that case? Or am I misunderstanding
the patch?
Arne Schwabe Nov. 7, 2021, 12:13 p.m. | #3
Am 07.11.21 um 12:57 schrieb Matthias Andree:
> Am 07.11.21 um 10:01 schrieb Arne Schwabe:
>> We already removed the check in d67658fee for OpenSSL 3.0. This 
>> removes the
>> checks entirely for all crypto libraries.
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>> ---
>>   src/openvpn/crypto.c         | 15 --------
>>   src/openvpn/crypto_backend.h | 28 ---------------
>>   src/openvpn/crypto_mbedtls.c | 56 ------------------------------
>>   src/openvpn/crypto_openssl.c | 66 ------------------------------------
>>   4 files changed, 165 deletions(-)
>> -    /* DES is deprecated and the method to even check the keys is 
>> deprecated
>> -     * in OpenSSL 3.0. Instead of checking for the 16 weak/semi-weak 
>> keys
>> -     * we just accept them in OpenSSL 3.0 since the risk of randomly 
>> getting
>> -     * these is pretty low (and "all DES keys are weak" anyway) */
>> -    return true;
> 
> Should not we nuke DES altogether in that case? Or am I misunderstanding
> the patch?


The patch removes checking for weak keys and making DES just like any 
other CBC cipher and not doing extra checks for this. It basically 
removes the special treatment of DES.

Arne
Matthias Andree Nov. 7, 2021, 12:24 p.m. | #4
Am 07.11.21 um 13:13 schrieb Arne Schwabe:
> Am 07.11.21 um 12:57 schrieb Matthias Andree:
>> Am 07.11.21 um 10:01 schrieb Arne Schwabe:
>>> We already removed the check in d67658fee for OpenSSL 3.0. This
>>> removes the
>>> checks entirely for all crypto libraries.
>>>
>>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>>> ---
>>>   src/openvpn/crypto.c         | 15 --------
>>>   src/openvpn/crypto_backend.h | 28 ---------------
>>>   src/openvpn/crypto_mbedtls.c | 56 ------------------------------
>>>   src/openvpn/crypto_openssl.c | 66
>>> ------------------------------------
>>>   4 files changed, 165 deletions(-)
>>> -    /* DES is deprecated and the method to even check the keys is
>>> deprecated
>>> -     * in OpenSSL 3.0. Instead of checking for the 16
>>> weak/semi-weak keys
>>> -     * we just accept them in OpenSSL 3.0 since the risk of
>>> randomly getting
>>> -     * these is pretty low (and "all DES keys are weak" anyway) */
>>> -    return true;
>>
>> Should not we nuke DES altogether in that case? Or am I misunderstanding
>> the patch?
>
>
> The patch removes checking for weak keys and making DES just like any
> other CBC cipher and not doing extra checks for this. It basically
> removes the special treatment of DES.


After this, do we have any DES functionality left in OpenVPN? If so, we
should remove it.
Arne Schwabe Nov. 7, 2021, 12:29 p.m. | #5
>>
>> The patch removes checking for weak keys and making DES just like any
>> other CBC cipher and not doing extra checks for this. It basically
>> removes the special treatment of DES.
> 
> 
> After this, do we have any DES functionality left in OpenVPN? If so, we
> should remove it.
> 

After this patch, no special handling for DES anymore. YOu can still use 
DES but it is handled like any other cipher, e.g. BF-CBC, AES-CBC

Arne
Max Fillinger Nov. 7, 2021, 12:34 p.m. | #6
On 07/11/2021 13:29, Arne Schwabe wrote:
> 
>>>
>>> The patch removes checking for weak keys and making DES just like any
>>> other CBC cipher and not doing extra checks for this. It basically
>>> removes the special treatment of DES.
>>
>>
>> After this, do we have any DES functionality left in OpenVPN? If so, we
>> should remove it.
>>
> 
> After this patch, no special handling for DES anymore. YOu can still use 
> DES but it is handled like any other cipher, e.g. BF-CBC, AES-CBC
> 
> Arne

I think the point is that if we stop checking weak keys, we should rip 
out DES support completely. (I'd be in favor, but I'm not deep enough 
into it to know what the fallout would be.)

My view is, if someone's doing DES, they're not caring about security, 
so the small risk of weak keys is acceptable. Basically, "all DES keys 
are weak keys."
Gert Doering Nov. 7, 2021, 7:25 p.m. | #7
Patch looks good, explanation makes sense, logical continuation of 
the process started with the "removal for 3.0.0".  All DES keys are
weak :-)

Lightly tested with OpenSSL 1.1.1 and mbedTLS builds (no actual *use*
of DES, though, besides "make check").

Your patch has been applied to the master branch.

commit 1325cf1198f78ccd8ab74394bb2e9b13f410ef20
Author: Arne Schwabe
Date:   Sun Nov 7 10:01:38 2021 +0100

     Completely remove DES checks

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
     Message-Id: <20211107090138.3150187-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23115.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 1d242ac5a..e267e7a06 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -986,21 +986,6 @@  check_key(struct key *key, const struct key_type *kt)
         {
             return false;
         }
-
-        /*
-         * Check for weak or semi-weak DES keys.
-         */
-        {
-            const int ndc = key_des_num_cblocks(kt->cipher);
-            if (ndc)
-            {
-                return key_des_check(key->cipher, kt->cipher_length, ndc);
-            }
-            else
-            {
-                return true;
-            }
-        }
     }
     return true;
 }
diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index 8bf6012a9..40984c559 100644
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -156,34 +156,6 @@  bool crypto_pem_decode(const char *name, struct buffer *dst,
  */
 int rand_bytes(uint8_t *output, int len);
 
-/*
- *
- * Key functions, allow manipulation of keys.
- *
- */
-
-
-/**
- * Return number of DES cblocks (1 cblock = length of a single-DES key) for the
- * current key type or 0 if not a DES cipher.
- *
- * @param kt            Type of key
- *
- * @return              Number of DES cblocks that the key consists of, or 0.
- */
-int key_des_num_cblocks(const cipher_kt_t *kt);
-
-/*
- * Check the given DES key. Checks the given key's length, weakness and parity.
- *
- * @param key           Key to check
- * @param key_len       Length of the key, in bytes
- * @param ndc           Number of DES cblocks that the key is made up of.
- *
- * @return              \c true if the key is valid, \c false otherwise.
- */
-bool key_des_check(uint8_t *key, int key_len, int ndc);
-
 /**
  * Encrypt the given block, using DES ECB mode
  *
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index a31ff5561..781da1ca9 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -386,62 +386,6 @@  rand_bytes(uint8_t *output, int len)
     return 1;
 }
 
-/*
- *
- * Key functions, allow manipulation of keys.
- *
- */
-
-
-int
-key_des_num_cblocks(const mbedtls_cipher_info_t *kt)
-{
-    int ret = 0;
-    if (kt->type == MBEDTLS_CIPHER_DES_CBC)
-    {
-        ret = 1;
-    }
-    if (kt->type == MBEDTLS_CIPHER_DES_EDE_CBC)
-    {
-        ret = 2;
-    }
-    if (kt->type == MBEDTLS_CIPHER_DES_EDE3_CBC)
-    {
-        ret = 3;
-    }
-
-    dmsg(D_CRYPTO_DEBUG, "CRYPTO INFO: n_DES_cblocks=%d", ret);
-    return ret;
-}
-
-bool
-key_des_check(uint8_t *key, int key_len, int ndc)
-{
-    int i;
-    struct buffer b;
-
-    buf_set_read(&b, key, key_len);
-
-    for (i = 0; i < ndc; ++i)
-    {
-        unsigned char *key = buf_read_alloc(&b, MBEDTLS_DES_KEY_SIZE);
-        if (!key)
-        {
-            msg(D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: insufficient key material");
-            goto err;
-        }
-        if (0 != mbedtls_des_key_check_weak(key))
-        {
-            msg(D_CRYPT_ERRORS, "CRYPTO INFO: check_key_DES: weak key detected");
-            goto err;
-        }
-    }
-    return true;
-
-err:
-    return false;
-}
-
 /*
  *
  * Generic cipher key type functions
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index bbfe15143..116c99c8e 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -552,72 +552,6 @@  rand_bytes(uint8_t *output, int len)
     return 1;
 }
 
-/*
- *
- * Key functions, allow manipulation of keys.
- *
- */
-
-
-int
-key_des_num_cblocks(const EVP_CIPHER *kt)
-{
-    int ret = 0;
-    const char *name = OBJ_nid2sn(EVP_CIPHER_nid(kt));
-    if (name)
-    {
-        if (!strncmp(name, "DES-", 4))
-        {
-            ret = EVP_CIPHER_key_length(kt) / sizeof(DES_cblock);
-        }
-        else if (!strncmp(name, "DESX-", 5))
-        {
-            ret = 1;
-        }
-    }
-    dmsg(D_CRYPTO_DEBUG, "CRYPTO INFO: n_DES_cblocks=%d", ret);
-    return ret;
-}
-
-bool
-key_des_check(uint8_t *key, int key_len, int ndc)
-{
-#if OPENSSL_VERSION_NUMBER < 0x30000000L
-    int i;
-    struct buffer b;
-
-    buf_set_read(&b, key, key_len);
-
-    for (i = 0; i < ndc; ++i)
-    {
-        DES_cblock *dc = (DES_cblock *) buf_read_alloc(&b, sizeof(DES_cblock));
-        if (!dc)
-        {
-            crypto_msg(D_CRYPT_ERRORS,
-                       "CRYPTO INFO: check_key_DES: insufficient key material");
-            goto err;
-        }
-        if (DES_is_weak_key(dc))
-        {
-            crypto_msg(D_CRYPT_ERRORS,
-                       "CRYPTO INFO: check_key_DES: weak key detected");
-            goto err;
-        }
-    }
-    return true;
-
-err:
-    ERR_clear_error();
-    return false;
-#else
-    /* DES is deprecated and the method to even check the keys is deprecated
-     * in OpenSSL 3.0. Instead of checking for the 16 weak/semi-weak keys
-     * we just accept them in OpenSSL 3.0 since the risk of randomly getting
-     * these is pretty low (and "all DES keys are weak" anyway) */
-    return true;
-#endif
-}
-
 /*
  *
  * Generic cipher key type functions