diff mbox series

[Openvpn-devel,v4,OSSL,3.0] Implement DES ECB encrypt via EVP_CIPHER api

Message ID 20211029111109.2003101-1-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel,v4,OSSL,3.0] Implement DES ECB encrypt via EVP_CIPHER api | expand

Commit Message

Arne Schwabe Oct. 29, 2021, 12:11 a.m. UTC 
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. We cheat a bit by using 3DES instead
of DES to avoid needing legacy provider for DES encryption for now.

Patch v4: add unit test, use 3DES to avoid legacy provider for now

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/crypto_openssl.c           | 40 ++++++++++++++++++++++++--
 tests/unit_tests/openvpn/test_crypto.c | 27 ++++++++++++++++-
 2 files changed, 63 insertions(+), 4 deletions(-)

Comments

Maximilian Fillinger Nov. 2, 2021, 5:12 a.m. UTC | #1 
On 29/10/2021 13:11, Arne Schwabe 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. We cheat a bit by using 3DES instead
> of DES to avoid needing legacy provider for DES encryption for now.
> 
> Patch v4: add unit test, use 3DES to avoid legacy provider for now
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

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

Looks good to me, and the unit tests succeed with OpenSSL 1.1.1 and 3.

Small nitpick that can be fixed at compile time:

> +    if (!EVP_EncryptInit_ex(ctx, EVP_des_ede3_ecb(), NULL, key3, 0))

The last argument for this function is "const unsigned char *iv", so 
this should be NULL instead of 0. (Passing NULL here is correct because 
ECB mode doesn't need an initialization vector.)
Gert Doering Nov. 5, 2021, 3:56 a.m. UTC | #2 
Cliend side tested on OpenSSL 1.1.1 and 3.0.0 - thanks for the in-person
explanation about DES and 3DES-with-same-key :-) and for adding a unit
test.

[ RUN      ] test_des_encrypt
[       OK ] test_des_encrypt

Your patch has been applied to the master branch.

commit c426a3e77ee5377ca1c6254feabb04e41aa6dfed
Author: Arne Schwabe
Date:   Fri Oct 29 13:11:08 2021 +0200

     Implement DES ECB encrypt via EVP_CIPHER api

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


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 6b18551ea..999805e88 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -867,10 +867,44 @@  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;
+    /* We are using 3DES here with three times the same key to cheat
+     * and emulate DES as 3DES is better supported than DES */
+    EVP_CIPHER_CTX *ctx = EVP_CIPHER_CTX_new();
+    if (!ctx)
+    {
+        crypto_msg(M_FATAL, "%s: EVP_CIPHER_CTX_new() failed", __func__);
+    }
+
+    unsigned char key3[DES_KEY_LENGTH*3];
+    for (int i = 0;i < 3;i++)
+    {
+        memcpy(key3 + (i * DES_KEY_LENGTH), key, DES_KEY_LENGTH);
+    }
 
-    DES_set_key_unchecked((DES_cblock *)key, &sched);
-    DES_ecb_encrypt((DES_cblock *)src, (DES_cblock *)dst, &sched, DES_ENCRYPT);
+    if (!EVP_EncryptInit_ex(ctx, EVP_des_ede3_ecb(), NULL, key3, 0))
+    {
+        crypto_msg(M_FATAL, "%s: EVP_EncryptInit_ex() failed", __func__);
+    }
+
+    int len;
+
+    /* The EVP_EncryptFinal method will write to the dst+len pointer even
+     * though there is nothing to encrypt anymore, provide space for that to
+     * not overflow the stack */
+    unsigned char dst2[DES_KEY_LENGTH * 2];
+    if(!EVP_EncryptUpdate(ctx, dst2, &len, src, DES_KEY_LENGTH))
+    {
+        crypto_msg(M_FATAL, "%s: EVP_EncryptUpdate() failed", __func__);
+    }
+
+    if (!EVP_EncryptFinal(ctx, dst2 + len, &len))
+    {
+        crypto_msg(M_FATAL, "%s: EVP_EncryptFinal() failed", __func__);
+    }
+
+    memcpy(dst, dst2, DES_KEY_LENGTH);
+
+    EVP_CIPHER_CTX_free(ctx);
 }
 
 /*
diff --git a/tests/unit_tests/openvpn/test_crypto.c b/tests/unit_tests/openvpn/test_crypto.c
index 66f53a020..6d8d40896 100644
--- a/tests/unit_tests/openvpn/test_crypto.c
+++ b/tests/unit_tests/openvpn/test_crypto.c
@@ -212,6 +212,30 @@  crypto_test_hmac(void **state)
     hmac_ctx_free(hmac);
 }
 
+void
+test_des_encrypt(void **state)
+{
+    /* We have a small des encrypt method that is only for NTLMv1. This unit
+     * test ensures that it is not accidentally broken */
+
+    const unsigned char des_key[DES_KEY_LENGTH] = {0x42, 0x23};
+
+    const char *src = "MoinWelt";
+
+    /* cipher_des_encrypt_ecb wants a non const */
+    unsigned char *src2 = (unsigned char *) strdup(src);
+
+    unsigned char dst[DES_KEY_LENGTH];
+    cipher_des_encrypt_ecb(des_key, src2, dst);
+
+    const unsigned char dst_good[DES_KEY_LENGTH] = {0xd3, 0x8f, 0x61, 0xf7, 0xbe, 0x27, 0xb6, 0xa2};
+
+    assert_memory_equal(dst, dst_good, DES_KEY_LENGTH);
+
+    free(src2);
+}
+
+
 int
 main(void)
 {
@@ -219,7 +243,8 @@  main(void)
         cmocka_unit_test(crypto_pem_encode_decode_loopback),
         cmocka_unit_test(crypto_translate_cipher_names),
         cmocka_unit_test(crypto_test_tls_prf),
-        cmocka_unit_test(crypto_test_hmac)
+        cmocka_unit_test(crypto_test_hmac),
+        cmocka_unit_test(test_des_encrypt)
     };
 
 #if defined(ENABLE_CRYPTO_OPENSSL)