diff mbox series

[Openvpn-devel,v3] crypto: Fix OPENSSL_FIPS enabled builds

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

Commit Message

David Sommerseth Jan. 19, 2022, 7:21 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.  On these platforms, the OPENSSL_FIPS macro
is always defined via /usr/include/openssl/opensslconf-*.h.

Without this fix, the following compilation error appears:

  ./src/openvpn/crypto.c: In function ‘print_cipher’:
  ./src/openvpn/crypto.c:1707:43: error: ‘cipher’ undeclared (first use in this function); did you mean ‘iphdr’?
       if (FIPS_mode() && !(EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_FIPS))
                                           ^~~~~~
The EVP_CIPHER_fetch() and EVP_CIPHER_free() methods are also provided
via the openssl_compat.h for older than OpenSSL 3.0.

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

Comments

Gert Doering Jan. 20, 2022, 8:11 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

We'll add build tests on Fedora / CentOS, as soon as September brings
the new buildbot infrastructure... so we get the "looks like FIPS but
isn't" stuff tested as well.  (cipher) changed as instructed on IRC.

Your patch has been applied to the master branch.

commit 544330fefedc87a74b4e17e105ad9151b8ad1dc9
Author: David Sommerseth
Date:   Wed Jan 19 19:21:26 2022 +0100

     crypto: Fix OPENSSL_FIPS enabled builds

     Signed-off-by: David Sommerseth <davids@openvpn.net>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20220119182126.56880-1-openvpn@sf.lists.topphemmelig.net>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23570.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering Jan. 21, 2022, 6:09 a.m. UTC | #2 
Hi,

On Wed, Jan 19, 2022 at 07:21:26PM +0100, David Sommerseth wrote:
> index 5626e2b6..eb0b1254 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"

This breaks compilation for mbedtls builds, depending on which version
of OpenSSL happens to be installed on the system (if any).

In this particular case, mbedtls build with a system openssl of 0.9.8,
it blows up with

In file included from crypto.c:37:
openssl_compat.h: In function 'SSL_CTX_get_min_proto_version':
openssl_compat.h:635: error: 'SSL_OP_NO_TLSv1_1' undeclared 
                (first use in this

(and more of this)

which is unsurprising - it's not supposed to pull in these headers
in the first place.


I wondered about this header, but did not wonder enough to verify
that it indeed must not be included for non-openssl-builds.

gert
Selva Nair Jan. 21, 2022, 8:09 a.m. UTC | #3 
Hi

On Fri, Jan 21, 2022 at 12:10 PM Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Wed, Jan 19, 2022 at 07:21:26PM +0100, David Sommerseth wrote:
> > index 5626e2b6..eb0b1254 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"
>
> This breaks compilation for mbedtls builds, depending on which version
> of OpenSSL happens to be installed on the system (if any).
>
> In this particular case, mbedtls build with a system openssl of 0.9.8,
> it blows up with
>
> In file included from crypto.c:37:
> openssl_compat.h: In function 'SSL_CTX_get_min_proto_version':
> openssl_compat.h:635: error: 'SSL_OP_NO_TLSv1_1' undeclared
>                 (first use in this
>
> (and more of this)
>
> which is unsurprising - it's not supposed to pull in these headers
> in the first place.
>

Looking back at it, the patch and the problem it's trying to solve are both
misplaced in crypto.c. That file should be ssl-lib agnostic and openssl
related bits should go to crypto_openssl.c...

I think we need to remove that OPENSSL_FIPS clause and think of providing
that extra info somewhere else if possible.

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jan 21, 2022 at 12:10 PM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On Wed, Jan 19, 2022 at 07:21:26PM +0100, David Sommerseth wrote:<br>
&gt; index 5626e2b6..eb0b1254 100644<br>
&gt; --- a/src/openvpn/crypto.c<br>
&gt; +++ b/src/openvpn/crypto.c<br>
&gt; @@ -34,6 +34,7 @@<br>
&gt;  #include &quot;error.h&quot;<br>
&gt;  #include &quot;integer.h&quot;<br>
&gt;  #include &quot;platform.h&quot;<br>
&gt; +#include &quot;openssl_compat.h&quot;<br>
&gt;  <br>
&gt;  #include &quot;memdbg.h&quot;<br>
<br>
This breaks compilation for mbedtls builds, depending on which version<br>
of OpenSSL happens to be installed on the system (if any).<br>
<br>
In this particular case, mbedtls build with a system openssl of 0.9.8,<br>
it blows up with<br>
<br>
In file included from crypto.c:37:<br>
openssl_compat.h: In function &#39;SSL_CTX_get_min_proto_version&#39;:<br>
openssl_compat.h:635: error: &#39;SSL_OP_NO_TLSv1_1&#39; undeclared <br>
                (first use in this<br>
<br>
(and more of this)<br>
<br>
which is unsurprising - it&#39;s not supposed to pull in these headers<br>
in the first place.<br></blockquote><div><br></div><div>Looking back at it, the patch and the problem it&#39;s trying to solve are both misplaced in crypto.c. That file should be ssl-lib agnostic and openssl related bits should go to crypto_openssl.c...</div><div><br></div><div>I think we need to remove that OPENSSL_FIPS clause and think of providing that extra info somewhere else if possible.<br></div><div><br></div><div>Selva</div></div></div>
diff mbox series

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 5626e2b6..eb0b1254 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,10 +1705,15 @@  print_cipher(const char *ciphername)
         printf(", TLS client/server mode only");
     }
 #ifdef OPENSSL_FIPS
-    if (FIPS_mode() && !(EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_FIPS))
+    evp_cipher_type *cipher = EVP_CIPHER_fetch(NULL, ciphername, NULL);
+
+    if (FIPS_mode()
+        && (NULL != cipher)
+        && !(EVP_CIPHER_flags(cipher) & EVP_CIPH_FLAG_FIPS))
     {
         printf(", disabled by FIPS mode");
     }
+    EVP_CIPHER_free(cipher);
 #endif
 
     printf(")\n");