diff mbox series

[Openvpn-devel] crypto: Fix OPENSSL_FIPS enabled builds

Message ID 20220119113446.17691-1-openvpn@sf.lists.topphemmelig.net
State Superseded
Headers
Series [Openvpn-devel] crypto: Fix OPENSSL_FIPS enabled builds |

Commit Message

David Sommerseth Jan. 19, 2022, 12:34 a.m. UTC 
  From: David Sommerseth <davids@openvpn.net>

On Fedora and RHEL/CentOS, the standard OpenSSL library has the FIPS
module enabled by default.  This revealed some incompatible code with
the added DCO support.

Signed-off-by: David Sommerseth <davids@openvpn.net>
---
 src/openvpn/crypto.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Antonio Quartulli Jan. 19, 2022, 2:44 a.m. UTC | #1 
Hi David,

On 19/01/2022 12:34, David Sommerseth wrote:
> From: David Sommerseth <davids@openvpn.net>
> 
> On Fedora and RHEL/CentOS, the standard OpenSSL library has the FIPS
> module enabled by default.  This revealed some incompatible code with
> the added DCO support.
> 
> Signed-off-by: David Sommerseth <davids@openvpn.net>
> ---
>   src/openvpn/crypto.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 5626e2b6..0415f59d 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -34,6 +34,7 @@
>   #include "error.h"
>   #include "integer.h"
>   #include "platform.h"
> +#include "openssl_compat.h"
>   
>   #include "memdbg.h"
>   
> @@ -1704,6 +1705,8 @@ print_cipher(const char *ciphername)
>           printf(", TLS client/server mode only");
>       }
>   #ifdef OPENSSL_FIPS
> +    evp_cipher_type *cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL);
> +

from the commit message it is clear that there are incompatibilities, 
but I am unable to guess what the real problem is.

Maybe you could extend your commit message or add a comment here?

How is declaring a non-used variable going to help?
Oh, now I see that cipher is used but was never declared/initialized. 
Still worth a word in the commit message? ;-)

On top of that, anything returned by EVP_get_cipherbyname() must be 
free'd later, or it will result in a memory leak.


On top of that ²: should we add some FIPS build to our GH Actions (for 
another patch of course)?


Regards,

>       if (FIPS_mode() && !(EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_FIPS))
>       {
>           printf(", disabled by FIPS mode");
Arne Schwabe Jan. 19, 2022, 2:52 a.m. UTC | #2 
Am 19.01.22 um 12:34 schrieb David Sommerseth:
> From: David Sommerseth <davids@openvpn.net>
> 
> On Fedora and RHEL/CentOS, the standard OpenSSL library has the FIPS
> module enabled by default.  This revealed some incompatible code with
> the added DCO support.
> 
> Signed-off-by: David Sommerseth <davids@openvpn.net>
> ---
>   src/openvpn/crypto.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 5626e2b6..0415f59d 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -34,6 +34,7 @@
>   #include "error.h"
>   #include "integer.h"
>   #include "platform.h"
> +#include "openssl_compat.h"
>   
>   #include "memdbg.h"
>   
> @@ -1704,6 +1705,8 @@ print_cipher(const char *ciphername)
>           printf(", TLS client/server mode only");
>       }
>   #ifdef OPENSSL_FIPS
> +    evp_cipher_type *cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL);
> +

EVP_CIPHER_fetch without EVP_CIPHER_free

Arne
David Sommerseth Jan. 19, 2022, 3:58 a.m. UTC | #3 
On 19/01/2022 14:44, Antonio Quartulli wrote:
> Hi David,
> 
> On 19/01/2022 12:34, David Sommerseth wrote:
>> From: David Sommerseth <davids@openvpn.net>
>>
>> On Fedora and RHEL/CentOS, the standard OpenSSL library has the FIPS
>> module enabled by default.  This revealed some incompatible code with
>> the added DCO support.
>>
>> Signed-off-by: David Sommerseth <davids@openvpn.net>
>> ---
>>   src/openvpn/crypto.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
>> index 5626e2b6..0415f59d 100644
>> --- a/src/openvpn/crypto.c
>> +++ b/src/openvpn/crypto.c
>> @@ -34,6 +34,7 @@
>>   #include "error.h"
>>   #include "integer.h"
>>   #include "platform.h"
>> +#include "openssl_compat.h"
>>   #include "memdbg.h"
>> @@ -1704,6 +1705,8 @@ print_cipher(const char *ciphername)
>>           printf(", TLS client/server mode only");
>>       }
>>   #ifdef OPENSSL_FIPS
>> +    evp_cipher_type *cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL);
>> +
> 
> from the commit message it is clear that there are incompatibilities, 
> but I am unable to guess what the real problem is.
> 
> Maybe you could extend your commit message or add a comment here?

Will do!

> How is declaring a non-used variable going to help > Oh, now I see that cipher is used but was never declared/initialized.
> Still worth a word in the commit message? ;-)

Yes!

> On top of that, anything returned by EVP_get_cipherbyname() must be 
> free'd later, or it will result in a memory leak.

Will fix!

> On top of that ²: should we add some FIPS build to our GH Actions (for 
> another patch of course)?

This exploded on stock Fedora 35 inside a mock build chroot, no FIPS 
enabling required.  The devil in the detail is that the OPENSSL_FIPS 
macro is defined unconditionally in 
/usr/include/openssl/opensslconf-x86_64.h.  So our OPENSSL_FIPS blocks 
are always built on Fedora and related downstream distros.
diff mbox series

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 5626e2b6..0415f59d 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -34,6 +34,7 @@ 
 #include "error.h"
 #include "integer.h"
 #include "platform.h"
+#include "openssl_compat.h"
 
 #include "memdbg.h"
 
@@ -1704,6 +1705,8 @@  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() && !(EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_FIPS))
     {
         printf(", disabled by FIPS mode");