| Message ID | 20220119113446.17691-1-openvpn@sf.lists.topphemmelig.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] crypto: Fix OPENSSL_FIPS enabled builds | expand |
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");
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
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 --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");