diff mbox series

[Openvpn-devel] Do not assume that SSL_CTX_get/set_min/max_proto_version are macros

Message ID 1520185442-22901-1-git-send-email-selva.nair@gmail.com
State Superseded
Headers
Series [Openvpn-devel] Do not assume that SSL_CTX_get/set_min/max_proto_version are macros |

Commit Message

Selva Nair March 4, 2018, 6:44 a.m. UTC 
  From: Selva Nair <selva.nair@gmail.com>

Openssl docs do not explicitly state these to be macros although they
are currently defined as such. Use AC_CHECK_DECLS to test for these so that
both function and macro forms could be detected.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
Though not meant as a fixup for libressl, as a side effect
this also makes 2.4.5 build with newer libressl versions.
(built on freebsd 11 using libressl 2.6.4 while testing patch 238)
Notes: (i) libressl defines only the set functions and neither
are macros. So get functions will get used from the compat layer.

 configure.ac                 | 12 ++++++++++++
 src/openvpn/openssl_compat.h |  8 ++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

Comments

haixiao.yan.cn--- via Openvpn-devel March 4, 2018, 7:05 a.m. UTC | #1 
Great, Thank You!



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jeremie Courreges-Anglas March 4, 2018, 7:48 a.m. UTC | #2 
On Sun, Mar 04 2018, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
>
> Openssl docs do not explicitly state these to be macros although they
> are currently defined as such.

Actually they are documented as macros by OpenSSL since day 1, see
NOTES.

> Use AC_CHECK_DECLS to test for these so that
> both function and macro forms could be detected.

Looks like the right way to handle such a situation.
Your diff looks good, and works for me against LibreSSL HEAD on
OpenBSD-current:

checking whether SSL_CTX_get_min_proto_version is declared... no
checking whether SSL_CTX_get_max_proto_version is declared... no
checking whether SSL_CTX_set_min_proto_version is declared... yes
checking whether SSL_CTX_set_max_proto_version is declared... yes

PASS: t_lpback.sh
The following test will take about two minutes.
If the addresses are in use, this test will retry up to two times.
PASS: t_cltsrv.sh
====================
All 2 tests passed
(1 test was not run)
====================

> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
> Though not meant as a fixup for libressl, as a side effect
> this also makes 2.4.5 build with newer libressl versions.
> (built on freebsd 11 using libressl 2.6.4 while testing patch 238)
> Notes: (i) libressl defines only the set functions and neither
> are macros. So get functions will get used from the compat layer.

More notes, possibly relevant:
- LibreSSL implement those as functions to provide better type checking.
  IIUC this is inspired by the same choice done in BoringSSL.
- yesterday I added macros for compatibility in LibreSSL HEAD, see
  https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libssl/ssl.h
  This should land in LibreSSL 2.7.0.
- adding the getters part is planned

>  configure.ac                 | 12 ++++++++++++
>  src/openvpn/openssl_compat.h |  8 ++++----
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 626b4dd..2a8e87f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -948,6 +948,18 @@ if test "${with_crypto_library}" = "openssl"; then
>  			EC_GROUP_order_bits
>  		]
>  	)
> +	AC_CHECK_DECLS(
> +		[
> +			SSL_CTX_get_min_proto_version,
> +			SSL_CTX_get_max_proto_version,
> +			SSL_CTX_set_min_proto_version,
> +			SSL_CTX_set_max_proto_version,
> +		],
> +		,
> +		,
> +		[[#include <openssl/ssl.h>]]
> +
> +	)
>  
>  	CFLAGS="${saved_CFLAGS}"
>  	LIBS="${saved_LIBS}"
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index d375fab..340d452 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -661,7 +661,7 @@ EC_GROUP_order_bits(const EC_GROUP *group)
>  #define RSA_F_RSA_OSSL_PRIVATE_ENCRYPT       RSA_F_RSA_EAY_PRIVATE_ENCRYPT
>  #endif
>  
> -#ifndef SSL_CTX_get_min_proto_version
> +#if !HAVE_DECL_SSL_CTX_GET_MIN_PROTO_VERSION
>  /** Return the min SSL protocol version currently enabled in the context.
>   *  If no valid version >= TLS1.0 is found, return 0. */
>  static inline int
> @@ -684,7 +684,7 @@ SSL_CTX_get_min_proto_version(SSL_CTX *ctx)
>  }
>  #endif /* SSL_CTX_get_min_proto_version */
>  
> -#ifndef SSL_CTX_get_max_proto_version
> +#if !HAVE_DECL_SSL_CTX_GET_MAX_PROTO_VERSION
>  /** Return the max SSL protocol version currently enabled in the context.
>   *  If no valid version >= TLS1.0 is found, return 0. */
>  static inline int
> @@ -707,7 +707,7 @@ SSL_CTX_get_max_proto_version(SSL_CTX *ctx)
>  }
>  #endif /* SSL_CTX_get_max_proto_version */
>  
> -#ifndef SSL_CTX_set_min_proto_version
> +#if !HAVE_DECL_SSL_CTX_SET_MIN_PROTO_VERSION
>  /** Mimics SSL_CTX_set_min_proto_version for OpenSSL < 1.1 */
>  static inline int
>  SSL_CTX_set_min_proto_version(SSL_CTX *ctx, long tls_ver_min)
> @@ -736,7 +736,7 @@ SSL_CTX_set_min_proto_version(SSL_CTX *ctx, long tls_ver_min)
>  }
>  #endif /* SSL_CTX_set_min_proto_version */
>  
> -#ifndef SSL_CTX_set_max_proto_version
> +#if !HAVE_DECL_SSL_CTX_SET_MAX_PROTO_VERSION
>  /** Mimics SSL_CTX_set_max_proto_version for OpenSSL < 1.1 */
>  static inline int
>  SSL_CTX_set_max_proto_version(SSL_CTX *ctx, long tls_ver_max)
Selva Nair March 4, 2018, 8:08 a.m. UTC | #3 
Hi,

On Sun, Mar 4, 2018 at 1:48 PM, Jeremie Courreges-Anglas <jca@wxcvbn.org> wrote:
> On Sun, Mar 04 2018, selva.nair@gmail.com wrote:
>> From: Selva Nair <selva.nair@gmail.com>
>>
>> Openssl docs do not explicitly state these to be macros although they
>> are currently defined as such.
>
> Actually they are documented as macros by OpenSSL since day 1, see
> NOTES.

You are right, I missed that in the docs. In that case my patch is not
needed especially so if libressl will provide those macros.

I'm still concerned about set and get functions coming from different
sources and may be we should fix that by requiring that if set is
defined we need get too. But that will once again break libressl
compatibility.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jeremie Courreges-Anglas March 4, 2018, 12:13 p.m. UTC | #4 
On Sun, Mar 04 2018, Selva Nair <selva.nair@gmail.com> wrote:
> Hi,
>
> On Sun, Mar 4, 2018 at 1:48 PM, Jeremie Courreges-Anglas <jca@wxcvbn.org> wrote:
>> On Sun, Mar 04 2018, selva.nair@gmail.com wrote:
>>> From: Selva Nair <selva.nair@gmail.com>
>>>
>>> Openssl docs do not explicitly state these to be macros although they
>>> are currently defined as such.
>>
>> Actually they are documented as macros by OpenSSL since day 1, see
>> NOTES.
>
> You are right, I missed that in the docs. In that case my patch is not
> needed especially so if libressl will provide those macros.

It all depends if you want to support LibreSSL 2.6.x installations, as
I'm not sure I'll be able to backport this in the 2.6 branch (which
is supposed to receive security/reliability fixes only).

> I'm still concerned about set and get functions coming from different
> sources

Indeed.  A diff is floating to also add the getters to LibreSSL,
hopefully this will make it in the upcoming 2.7.x release.

> and may be we should fix that by requiring that if set is
> defined we need get too. But that will once again break libressl
> compatibility.

From a mail I sent recently:

--8<--
[...]. OpenSSL itself only provided said setters (since 2015)[2].  The
 getters were added to OpenSSL later (Sep 2017)[3].

[2] https://github.com/openssl/openssl/commit/7946ab33cecce60afcc00afc8fc18f31f9e66bff
[3] https://github.com/openssl/openssl/commit/3edabd3ccb7aac89af5a63cfb2378e33a8be05d7
-->8--

IIUC there are OpenSSL 1.1.0 releases out there that provide only the
setters, and that would also be affected by the requirement you propose.

Github suggests that besides the master branch, the following tags have
the setters[2]:

    OpenSSL_1_1_1-pre2 OpenSSL_1_1_1-pre1 OpenSSL_1_1_0 OpenSSL_1_1_0g
    OpenSSL_1_1_0f OpenSSL_1_1_0e OpenSSL_1_1_0d OpenSSL_1_1_0c
    OpenSSL_1_1_0b OpenSSL_1_1_0a OpenSSL_1_1_0-pre6 OpenSSL_1_1_0-pre5
    OpenSSL_1_1_0-pre4 OpenSSL_1_1_0-pre3 OpenSSL_1_1_0-pre2

while support for getters[3] is only in:

    OpenSSL_1_1_1-pre2 OpenSSL_1_1_1-pre1
Steffan Karger March 4, 2018, 12:22 p.m. UTC | #5 
On 05-03-18 00:13, Jeremie Courreges-Anglas wrote:
> On Sun, Mar 04 2018, Selva Nair <selva.nair@gmail.com> wrote:
> --8<--
> [...]. OpenSSL itself only provided said setters (since 2015)[2].  The
>  getters were added to OpenSSL later (Sep 2017)[3].
> 
> [2] https://github.com/openssl/openssl/commit/7946ab33cecce60afcc00afc8fc18f31f9e66bff
> [3] https://github.com/openssl/openssl/commit/3edabd3ccb7aac89af5a63cfb2378e33a8be05d7
> -->8--
> 
> IIUC there are OpenSSL 1.1.0 releases out there that provide only the
> setters, and that would also be affected by the requirement you propose.
> 
> Github suggests that besides the master branch, the following tags have
> the setters[2]:
> 
>     OpenSSL_1_1_1-pre2 OpenSSL_1_1_1-pre1 OpenSSL_1_1_0 OpenSSL_1_1_0g
>     OpenSSL_1_1_0f OpenSSL_1_1_0e OpenSSL_1_1_0d OpenSSL_1_1_0c
>     OpenSSL_1_1_0b OpenSSL_1_1_0a OpenSSL_1_1_0-pre6 OpenSSL_1_1_0-pre5
>     OpenSSL_1_1_0-pre4 OpenSSL_1_1_0-pre3 OpenSSL_1_1_0-pre2
> 
> while support for getters[3] is only in:
> 
>     OpenSSL_1_1_1-pre2 OpenSSL_1_1_1-pre1

That commit was cherry-picked to the OpenSSL_1_1_0-stable branch, and is
available int 1.1.0g+:
https://github.com/openssl/openssl/commit/af51a74ade8bbab5ed49a3560dcb70d89896dc29

But yeah, that's still something we might need to think about.

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Selva Nair March 4, 2018, 1:18 p.m. UTC | #6 
Hi,

On Sun, Mar 4, 2018 at 6:22 PM, Steffan Karger <steffan@karger.me> wrote:
>
> On 05-03-18 00:13, Jeremie Courreges-Anglas wrote:
>> On Sun, Mar 04 2018, Selva Nair <selva.nair@gmail.com> wrote:
>> --8<--
>> [...]. OpenSSL itself only provided said setters (since 2015)[2].  The
>>  getters were added to OpenSSL later (Sep 2017)[3].
>>
>> [2] https://github.com/openssl/openssl/commit/7946ab33cecce60afcc00afc8fc18f31f9e66bff
>> [3] https://github.com/openssl/openssl/commit/3edabd3ccb7aac89af5a63cfb2378e33a8be05d7
>> -->8--
>>
>> IIUC there are OpenSSL 1.1.0 releases out there that provide only the
>> setters, and that would also be affected by the requirement you propose.
>>
>> Github suggests that besides the master branch, the following tags have
>> the setters[2]:
>>
>>     OpenSSL_1_1_1-pre2 OpenSSL_1_1_1-pre1 OpenSSL_1_1_0 OpenSSL_1_1_0g
>>     OpenSSL_1_1_0f OpenSSL_1_1_0e OpenSSL_1_1_0d OpenSSL_1_1_0c
>>     OpenSSL_1_1_0b OpenSSL_1_1_0a OpenSSL_1_1_0-pre6 OpenSSL_1_1_0-pre5
>>     OpenSSL_1_1_0-pre4 OpenSSL_1_1_0-pre3 OpenSSL_1_1_0-pre2
>>
>> while support for getters[3] is only in:
>>
>>     OpenSSL_1_1_1-pre2 OpenSSL_1_1_1-pre1
>
> That commit was cherry-picked to the OpenSSL_1_1_0-stable branch, and is
> available int 1.1.0g+:
> https://github.com/openssl/openssl/commit/af51a74ade8bbab5ed49a3560dcb70d89896dc29
>
> But yeah, that's still something we might need to think about.

Yes this is troubling. I had tested Windows build using 1.1.0g, but
our release is built with 1.1.0f. So, for example, --tls-version-min
1.2 will not get read back as 1.2. Most likely it'll only lead to less
than ideal UX in some corner cases (e.g. the error check min <= max in
cryptoapi.c will not work as expected).

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering Oct. 6, 2018, 8:38 p.m. UTC | #7 
Hi,

On Sun, Mar 04, 2018 at 12:44:02PM -0500, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
> 
> Openssl docs do not explicitly state these to be macros although they
> are currently defined as such. Use AC_CHECK_DECLS to test for these so that
> both function and macro forms could be detected.
> 
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
> Though not meant as a fixup for libressl, as a side effect
> this also makes 2.4.5 build with newer libressl versions.
> (built on freebsd 11 using libressl 2.6.4 while testing patch 238)
> Notes: (i) libressl defines only the set functions and neither
> are macros. So get functions will get used from the compat layer.

So, going through open patches in patchwork I came to this one.

Reading through the thread, I'm not sure what the final outcome on
the patch and the problem was?  LibreSSL seems to be fixed (thanks,
Jeremie :-) ) and if we build with OpenSSL 1.1.0g+, all is good?

Phrased differently: does anything need to be done with this patch or
should I click on "superceded" in patchwork now?

gert
Selva Nair Oct. 10, 2018, 8:08 a.m. UTC | #8 
Hi,

On Sun, Oct 7, 2018 at 3:38 AM Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Sun, Mar 04, 2018 at 12:44:02PM -0500, selva.nair@gmail.com wrote:
> > From: Selva Nair <selva.nair@gmail.com>
> >
> > Openssl docs do not explicitly state these to be macros although they
> > are currently defined as such. Use AC_CHECK_DECLS to test for these so
> that
> > both function and macro forms could be detected.
> >
> > Signed-off-by: Selva Nair <selva.nair@gmail.com>
> > ---
> > Though not meant as a fixup for libressl, as a side effect
> > this also makes 2.4.5 build with newer libressl versions.
> > (built on freebsd 11 using libressl 2.6.4 while testing patch 238)
> > Notes: (i) libressl defines only the set functions and neither
> > are macros. So get functions will get used from the compat layer.
>
> So, going through open patches in patchwork I came to this one.
>
> Reading through the thread, I'm not sure what the final outcome on
> the patch and the problem was?  LibreSSL seems to be fixed (thanks,
> Jeremie :-) ) and if we build with OpenSSL 1.1.0g+, all is good?
>
> Phrased differently: does anything need to be done with this patch or
> should I click on "superceded" in patchwork now?
>

Agreed, if LibreSSL has got those macros defined now, we should just drop
this patch.

I'm marking these as superseded.

Selva
<div dir="ltr">Hi,<br><div><br><div class="gmail_quote"><div dir="ltr">On Sun, Oct 7, 2018 at 3:38 AM Gert <span class="" style="" id=":363.1" tabindex="-1">Doering</span> &lt;<span class="" style="" id=":363.2" tabindex="-1">gert</span>@<span class="" style="" id=":363.3" tabindex="-1">greenie</span>.<span class="" style="" id=":363.4" tabindex="-1">muc</span>.<span class="" style="" id=":363.5" tabindex="-1">de</span>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
On Sun, Mar 04, 2018 at 12:44:02PM -0500, <a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a> wrote:<br>
&gt; From: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br>
&gt; <br>
&gt; Openssl docs do not explicitly state these to be macros although they<br>
&gt; are currently defined as such. Use AC_CHECK_DECLS to test for these so that<br>
&gt; both function and macro forms could be detected.<br>
&gt; <br>
&gt; Signed-off-by: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br>
&gt; ---<br>
&gt; Though not meant as a fixup for libressl, as a side effect<br>
&gt; this also makes 2.4.5 build with newer libressl versions.<br>
&gt; (built on freebsd 11 using libressl 2.6.4 while testing patch 238)<br>
&gt; Notes: (i) libressl defines only the set functions and neither<br>
&gt; are macros. So get functions will get used from the compat layer.<br>
<br>
So, going through open patches in patchwork I came to this one.<br>
<br>
Reading through the thread, I&#39;m not sure what the final outcome on<br>
the patch and the problem was?  LibreSSL seems to be fixed (thanks,<br>
Jeremie :-) ) and if we build with OpenSSL 1.1.0g+, all is good?<br>
<br>
Phrased differently: does anything need to be done with this patch or<br>
should I click on &quot;superceded&quot; in patchwork now?<br></blockquote><div><br></div><div>Agreed, if <span class="" style="" id=":363.6" tabindex="-1">LibreSSL</span> has got those macros defined now, we should just drop<br>this patch.<br></div><div> <br></div><div>I&#39;m marking these as superseded.<br><br></div><div><span class="" style="" id=":363.7" tabindex="-1">Selva</span><br></div></div></div></div>
diff mbox series

Patch

diff --git a/configure.ac b/configure.ac
index 626b4dd..2a8e87f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -948,6 +948,18 @@  if test "${with_crypto_library}" = "openssl"; then
 			EC_GROUP_order_bits
 		]
 	)
+	AC_CHECK_DECLS(
+		[
+			SSL_CTX_get_min_proto_version,
+			SSL_CTX_get_max_proto_version,
+			SSL_CTX_set_min_proto_version,
+			SSL_CTX_set_max_proto_version,
+		],
+		,
+		,
+		[[#include <openssl/ssl.h>]]
+
+	)
 
 	CFLAGS="${saved_CFLAGS}"
 	LIBS="${saved_LIBS}"
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index d375fab..340d452 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -661,7 +661,7 @@  EC_GROUP_order_bits(const EC_GROUP *group)
 #define RSA_F_RSA_OSSL_PRIVATE_ENCRYPT       RSA_F_RSA_EAY_PRIVATE_ENCRYPT
 #endif
 
-#ifndef SSL_CTX_get_min_proto_version
+#if !HAVE_DECL_SSL_CTX_GET_MIN_PROTO_VERSION
 /** Return the min SSL protocol version currently enabled in the context.
  *  If no valid version >= TLS1.0 is found, return 0. */
 static inline int
@@ -684,7 +684,7 @@  SSL_CTX_get_min_proto_version(SSL_CTX *ctx)
 }
 #endif /* SSL_CTX_get_min_proto_version */
 
-#ifndef SSL_CTX_get_max_proto_version
+#if !HAVE_DECL_SSL_CTX_GET_MAX_PROTO_VERSION
 /** Return the max SSL protocol version currently enabled in the context.
  *  If no valid version >= TLS1.0 is found, return 0. */
 static inline int
@@ -707,7 +707,7 @@  SSL_CTX_get_max_proto_version(SSL_CTX *ctx)
 }
 #endif /* SSL_CTX_get_max_proto_version */
 
-#ifndef SSL_CTX_set_min_proto_version
+#if !HAVE_DECL_SSL_CTX_SET_MIN_PROTO_VERSION
 /** Mimics SSL_CTX_set_min_proto_version for OpenSSL < 1.1 */
 static inline int
 SSL_CTX_set_min_proto_version(SSL_CTX *ctx, long tls_ver_min)
@@ -736,7 +736,7 @@  SSL_CTX_set_min_proto_version(SSL_CTX *ctx, long tls_ver_min)
 }
 #endif /* SSL_CTX_set_min_proto_version */
 
-#ifndef SSL_CTX_set_max_proto_version
+#if !HAVE_DECL_SSL_CTX_SET_MAX_PROTO_VERSION
 /** Mimics SSL_CTX_set_max_proto_version for OpenSSL < 1.1 */
 static inline int
 SSL_CTX_set_max_proto_version(SSL_CTX *ctx, long tls_ver_max)