diff mbox series

[Openvpn-devel,v2,2/2] crypto: move OpenSSL specific FIPS check to its backend

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

Commit Message

Antonio Quartulli Feb. 3, 2022, 8:36 a.m. UTC 
  Our crypto API already provides a function performing a validity check
on the specified ciphername. The OpenSSL counterpart also checks for the
cipher being FIPS-enabled.

This API is cipher_valid(). Extend it so that it can provide a reason
whenever the cipher is not valid and use it in crypto.c.

This way we move any OpenSSL specific bit to its own
backend and directly use the new cipher_valid_reason() API in the
generic code.

This patch fixes compilations with mbedTLS when some OpenSSL is also
installed. The issue was introduced with:
544330fe ("crypto: Fix OPENSSL_FIPS enabled builds")

Cc: David Sommerseth <davids@openvpn.net>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---

Changes from v1:
* rebased
* don't return cipher, but true in cipher_valid_reason()

 src/openvpn/crypto.c         | 11 +++--------
 src/openvpn/crypto_backend.h | 21 ++++++++++++++++++++-
 src/openvpn/crypto_mbedtls.c | 13 +++++++++----
 src/openvpn/crypto_openssl.c |  6 +++++-
 4 files changed, 37 insertions(+), 14 deletions(-)

Comments

David Sommerseth Feb. 3, 2022, 11:32 p.m. UTC | #1 
On 03/02/2022 20:36, Antonio Quartulli wrote:
> Our crypto API already provides a function performing a validity check
> on the specified ciphername. The OpenSSL counterpart also checks for the
> cipher being FIPS-enabled.
> 
> This API is cipher_valid(). Extend it so that it can provide a reason
> whenever the cipher is not valid and use it in crypto.c.
> 
> This way we move any OpenSSL specific bit to its own
> backend and directly use the new cipher_valid_reason() API in the
> generic code.
> 
> This patch fixes compilations with mbedTLS when some OpenSSL is also
> installed. The issue was introduced with:
> 544330fe ("crypto: Fix OPENSSL_FIPS enabled builds")
> 
> Cc: David Sommerseth <davids@openvpn.net>
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
> 
> Changes from v1:
> * rebased
> * don't return cipher, but true in cipher_valid_reason()
> 
>   src/openvpn/crypto.c         | 11 +++--------
>   src/openvpn/crypto_backend.h | 21 ++++++++++++++++++++-
>   src/openvpn/crypto_mbedtls.c | 13 +++++++++----
>   src/openvpn/crypto_openssl.c |  6 +++++-
>   4 files changed, 37 insertions(+), 14 deletions(-)
> 

I've done test builds on RHEL-8 with both openssl-1.1.1k and
mbedtls-2.16.12-1 without any issues.  Just done some lightweight 
testing on top of reviewing code.  This looks good to me.

Acked-By: David Sommerseth <davids@openvpn.net>
Gert Doering Feb. 4, 2022, 12:15 a.m. UTC | #2 
Basic client tests work, patch looks reasonable (and has an ACK :-) ).

Did not go to the software museum to actually test in FIPS mode.

Your patch has been applied to the master branch.

commit 2914444e7cd514eb03e6cd7949e5219557710ae8
Author: Antonio Quartulli
Date:   Thu Feb 3 20:36:55 2022 +0100

     crypto: move OpenSSL specific FIPS check to its backend

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


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 7fc7f8e7..461cfb8c 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -34,7 +34,6 @@ 
 #include "error.h"
 #include "integer.h"
 #include "platform.h"
-#include "openssl_compat.h"
 
 #include "memdbg.h"
 
@@ -1698,16 +1697,12 @@  print_cipher(const char *ciphername)
     {
         printf(", TLS client/server mode only");
     }
-#ifdef OPENSSL_FIPS
-    evp_cipher_type *cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL);
 
-    if (FIPS_mode() && cipher
-        && !(EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_FIPS))
+    const char *reason;
+    if (!cipher_valid_reason(ciphername, &reason))
     {
-        printf(", disabled by FIPS mode");
+        printf(", %s", reason);
     }
-    EVP_CIPHER_free(cipher);
-#endif
 
     printf(")\n");
 }
diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index 7beaf9c3..abf1b876 100644
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -187,6 +187,21 @@  void cipher_des_encrypt_ecb(const unsigned char key[DES_KEY_LENGTH],
  */
 #define MAX_CIPHER_KEY_LENGTH 64
 
+/**
+ * Returns if the cipher is valid, based on the given cipher name and provides a
+ * reason if invalid.
+ *
+ * @param ciphername    Name of the cipher to check for validity (e.g.
+ *                      \c AES-128-CBC). Will be translated to the library name
+ *                      from the openvpn config name if needed.
+ * @param reason        Pointer where a static string indicating the reason
+ *                      for rejecting the cipher should be stored. It is set to
+ *                      NULL if the cipher is valid.
+ *
+ * @return              if the cipher is valid
+ */
+bool cipher_valid_reason(const char *ciphername, const char **reason);
+
 /**
  * Returns if the cipher is valid, based on the given cipher name.
  *
@@ -196,7 +211,11 @@  void cipher_des_encrypt_ecb(const unsigned char key[DES_KEY_LENGTH],
  *
  * @return              if the cipher is valid
  */
-bool cipher_valid(const char *ciphername);
+static inline bool cipher_valid(const char *ciphername)
+{
+    const char *reason;
+    return cipher_valid_reason(ciphername, &reason);
+}
 
 /**
  * Checks if the cipher is defined and is not the null (none) cipher
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 01bfa020..a771777e 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -403,14 +403,17 @@  cipher_get(const char* ciphername)
 }
 
 bool
-cipher_valid(const char *ciphername)
+cipher_valid_reason(const char *ciphername, const char **reason)
 {
+    ASSERT(reason);
+
     const mbedtls_cipher_info_t *cipher = cipher_get(ciphername);
 
     if (NULL == cipher)
     {
         msg(D_LOW, "Cipher algorithm '%s' not found", ciphername);
-        return NULL;
+        *reason = "disabled because unknown";
+        return false;
     }
 
     if (cipher->key_bitlen/8 > MAX_CIPHER_KEY_LENGTH)
@@ -418,10 +421,12 @@  cipher_valid(const char *ciphername)
         msg(D_LOW, "Cipher algorithm '%s' uses a default key size (%d bytes) "
             "which is larger than " PACKAGE_NAME "'s current maximum key size "
             "(%d bytes)", ciphername, cipher->key_bitlen/8, MAX_CIPHER_KEY_LENGTH);
-        return NULL;
+        *reason = "disabled due to key size too large";
+        return false;
     }
 
-    return cipher != NULL;
+    *reason = NULL;
+    return true;
 }
 
 const char *
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 6f3fbacd..8bc41792 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -572,13 +572,14 @@  cipher_get(const char *ciphername)
 }
 
 bool
-cipher_valid(const char *ciphername)
+cipher_valid_reason(const char *ciphername, const char **reason)
 {
     bool ret = false;
     evp_cipher_type *cipher = cipher_get(ciphername);
     if (!cipher)
     {
         crypto_msg(D_LOW, "Cipher algorithm '%s' not found", ciphername);
+        *reason = "disabled because unknown";
         goto out;
     }
 
@@ -590,6 +591,7 @@  cipher_valid(const char *ciphername)
     {
         msg(D_LOW, "Cipher algorithm '%s' is known by OpenSSL library but "
                     "currently disabled by running in FIPS mode.", ciphername);
+        *reason = "disabled by FIPS mode";
         goto out;
     }
 #endif
@@ -599,10 +601,12 @@  cipher_valid(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);
+        *reason = "disabled due to key size too large";
         goto out;
     }
 
     ret = true;
+    *reason = NULL;
 out:
     EVP_CIPHER_free(cipher);
     return ret;