[Openvpn-devel,v2,1/2] crypto: move validation logic from cipher_get to cipher_valid

Message ID 20220203193655.28791-1-a@unstable.cc
State Accepted
Headers show
Series
  • [Openvpn-devel,v2,1/2] crypto: move validation logic from cipher_get to cipher_valid
Related show

Commit Message

Antonio Quartulli Feb. 3, 2022, 7:36 p.m.
With cipher validation performed in cipher_get(), a cipher is never
returned in any case if some check fails.

This prevents OpenVPN from operating on all ciphers provided by the SSL
library, like printing them to the user.

Move the validation logic to cipher_valid() so that checks are performed
only when OpenVPN really want to know if a cipher is usable or not.

Fixes: ce2954a0 ("Remove cipher_kt_t and change type to const char* in API")
Cc: Arne Schwabe <arne@rfc2549.org>
Cc: David Sommerseth <davids@openvpn.net>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---

Changes from v1:
* properly release cipher in case of error in cipher_valid();


 src/openvpn/crypto_openssl.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

Comments

David Sommerseth Feb. 4, 2022, 10:27 a.m. | #1
On 03/02/2022 20:36, Antonio Quartulli wrote:
> With cipher validation performed in cipher_get(), a cipher is never

> returned in any case if some check fails.

> 

> This prevents OpenVPN from operating on all ciphers provided by the SSL

> library, like printing them to the user.

> 

> Move the validation logic to cipher_valid() so that checks are performed

> only when OpenVPN really want to know if a cipher is usable or not.

> 

> Fixes: ce2954a0 ("Remove cipher_kt_t and change type to const char* in API")

> Cc: Arne Schwabe <arne@rfc2549.org>

> Cc: David Sommerseth <davids@openvpn.net>

> Signed-off-by: Antonio Quartulli <a@unstable.cc>

> ---

> 

> Changes from v1:

> * properly release cipher in case of error in cipher_valid();

> 

> 

>   src/openvpn/crypto_openssl.c | 34 +++++++++++++++-------------------

>   1 file changed, 15 insertions(+), 19 deletions(-)



I've done compile testing and some lightweight testing as well as code 
review.  These changes looks reasonable.

Acked-By: David Sommerseth <davids@openvpn.net>


-- 
kind regards,

David Sommerseth
OpenVPN Inc
Gert Doering Feb. 4, 2022, 11:15 a.m. | #2
Basic client test works, quick glance looks reasonable.

Your patch has been applied to the master branch.

commit 2d822550ad990fbd498523fb1ab62ca19b3bb93c
Author: Antonio Quartulli
Date:   Thu Feb 3 20:36:54 2022 +0100

     crypto: move validation logic from cipher_get to cipher_valid

     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Acked-by: David Sommerseth <davids@openvpn.net>
     Message-Id: <20220203193655.28791-1-a@unstable.cc>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23713.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index a725306c..6f3fbacd 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -565,16 +565,21 @@  rand_bytes(uint8_t *output, int len)
 static evp_cipher_type *
 cipher_get(const char *ciphername)
 {
-    evp_cipher_type *cipher = NULL;
-
     ASSERT(ciphername);
 
     ciphername = translate_cipher_name_from_openvpn(ciphername);
-    cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL);
+    return EVP_CIPHER_fetch(NULL, ciphername, NULL);
+}
 
-    if (NULL == cipher)
+bool
+cipher_valid(const char *ciphername)
+{
+    bool ret = false;
+    evp_cipher_type *cipher = cipher_get(ciphername);
+    if (!cipher)
     {
-        return NULL;
+        crypto_msg(D_LOW, "Cipher algorithm '%s' not found", ciphername);
+        goto out;
     }
 
 #ifdef OPENSSL_FIPS
@@ -585,7 +590,7 @@  cipher_get(const char *ciphername)
     {
         msg(D_LOW, "Cipher algorithm '%s' is known by OpenSSL library but "
                     "currently disabled by running in FIPS mode.", ciphername);
-        return NULL;
+        goto out;
     }
 #endif
     if (EVP_CIPHER_key_length(cipher) > MAX_CIPHER_KEY_LENGTH)
@@ -594,22 +599,13 @@  cipher_get(const char *ciphername)
             "which is larger than " PACKAGE_NAME "'s current maximum key size "
             "(%d bytes)", ciphername, EVP_CIPHER_key_length(cipher),
             MAX_CIPHER_KEY_LENGTH);
-        return NULL;
+        goto out;
     }
 
-    return cipher;
-}
-
-bool cipher_valid(const char *ciphername)
-{
-    evp_cipher_type *cipher = cipher_get(ciphername);
-    bool valid = (cipher != NULL);
-    if (!valid)
-    {
-        crypto_msg(D_LOW, "Cipher algorithm '%s' not found", ciphername);
-    }
+    ret = true;
+out:
     EVP_CIPHER_free(cipher);
-    return valid;
+    return ret;
 }
 
 bool cipher_var_key_size(const char *ciphername)