[Openvpn-devel,v2,1/2] Make tls_version_max return the actual maximum version

Message ID 20181031165222.5997-1-arne@rfc2549.org
State New
Headers show
Series
  • [Openvpn-devel,v2,1/2] Make tls_version_max return the actual maximum version
Related show

Commit Message

Arne Schwabe Oct. 31, 2018, 4:52 p.m.
Before OpenSSL 1.1.1 there could be no mismatch between
compiled and actual OpenSSL version. With OpenSSL 1.1.1 we need
runtime detection to detect the actual best TLS version supported.

Allowing this runtime detection also allows removing some of the
TLS 1.3/OpenSSL 1.1.1 #ifdefs

Without this patch tls-min-version 1.3 or-highest will actually
downgrade to TLS 1.3 in the "compiled with 1.1.0 and linked against
1.1.1" scenario.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl.c         | 11 +++++------
 src/openvpn/ssl_openssl.c | 33 ++++++++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 11 deletions(-)

Comments

Steffan Karger Dec. 5, 2018, 10:51 a.m. | #1
Hi,

On Wed, 31 Oct 2018 at 17:53, Arne Schwabe <arne@rfc2549.org> wrote:
> Before OpenSSL 1.1.1 there could be no mismatch between
> compiled and actual OpenSSL version. With OpenSSL 1.1.1 we need
> runtime detection to detect the actual best TLS version supported.
>
> Allowing this runtime detection also allows removing some of the
> TLS 1.3/OpenSSL 1.1.1 #ifdefs
>
> Without this patch tls-min-version 1.3 or-highest will actually
> downgrade to TLS 1.3 in the "compiled with 1.1.0 and linked against
> 1.1.1" scenario.

"Downgrade to TLS 1.2", I guess?

But more fundamental: do want to support runtime-upgrading the TLS
library? Are we sure that this is the only place where this will
create unexpected behaviour? Does it even make sense to upgrade a
dependency to a version that contains all sorts of API/ABI changes and
then expect that you do not have to recompile? Honest questions; I
don't understand why one would want or do this.

-Steffan
Arne Schwabe Dec. 5, 2018, 2:09 p.m. | #2
Am 05.12.18 um 11:51 schrieb Steffan Karger:
> Hi,
> 
> On Wed, 31 Oct 2018 at 17:53, Arne Schwabe <arne@rfc2549.org> wrote:
>> Before OpenSSL 1.1.1 there could be no mismatch between
>> compiled and actual OpenSSL version. With OpenSSL 1.1.1 we need
>> runtime detection to detect the actual best TLS version supported.
>>
>> Allowing this runtime detection also allows removing some of the
>> TLS 1.3/OpenSSL 1.1.1 #ifdefs
>>
>> Without this patch tls-min-version 1.3 or-highest will actually
>> downgrade to TLS 1.3 in the "compiled with 1.1.0 and linked against
>> 1.1.1" scenario.
> 
> "Downgrade to TLS 1.2", I guess?
> 
> But more fundamental: do want to support runtime-upgrading the TLS
> library? Are we sure that this is the only place where this will
> create unexpected behaviour? Does it even make sense to upgrade a
> dependency to a version that contains all sorts of API/ABI changes and
> then expect that you do not have to recompile? Honest questions; I
> don't understand why one would want or do this.

I think on some installation, especially when the user compiled OpenVPN
him/herself, we just need to live with the fact that the OpenSSL library
got upgraded from OpenSSL 1.1.0 to OpenSSL 1.1.1.

But one art of decting the newer library here is that we can bail out on
some of the corner cases instead of continuing, like refusing external
auth if the OpenSSL libary is too new.

I still think is an unsupported scenario and we might add an warning
message aka like

"Detected OpenSSL 1.1.1 but compiled against OpenSSL 1.1.0, recompile to
use all OpenSSL 1.1.1 features"

Arne
Steffan Karger Dec. 6, 2018, 8:29 a.m. | #3
On 05-12-18 15:09, Arne Schwabe wrote:
> Am 05.12.18 um 11:51 schrieb Steffan Karger:
>> On Wed, 31 Oct 2018 at 17:53, Arne Schwabe <arne@rfc2549.org> wrote:
>>> Before OpenSSL 1.1.1 there could be no mismatch between
>>> compiled and actual OpenSSL version. With OpenSSL 1.1.1 we need
>>> runtime detection to detect the actual best TLS version supported.
>>>
>>> Allowing this runtime detection also allows removing some of the
>>> TLS 1.3/OpenSSL 1.1.1 #ifdefs
>>>
>>> Without this patch tls-min-version 1.3 or-highest will actually
>>> downgrade to TLS 1.3 in the "compiled with 1.1.0 and linked against
>>> 1.1.1" scenario.
>>
>> "Downgrade to TLS 1.2", I guess?
>>
>> But more fundamental: do want to support runtime-upgrading the TLS
>> library? Are we sure that this is the only place where this will
>> create unexpected behaviour? Does it even make sense to upgrade a
>> dependency to a version that contains all sorts of API/ABI changes and
>> then expect that you do not have to recompile? Honest questions; I
>> don't understand why one would want or do this.
> 
> I think on some installation, especially when the user compiled OpenVPN
> him/herself, we just need to live with the fact that the OpenSSL library
> got upgraded from OpenSSL 1.1.0 to OpenSSL 1.1.1.
> 
> But one art of decting the newer library here is that we can bail out on
> some of the corner cases instead of continuing, like refusing external
> auth if the OpenSSL libary is too new.
> 
> I still think is an unsupported scenario and we might add an warning
> message aka like
> 
> "Detected OpenSSL 1.1.1 but compiled against OpenSSL 1.1.0, recompile to
> use all OpenSSL 1.1.1 features"

Do you mean "add a warning message *in addition to* the patch
workarounds", or "add a warning message "instead of* the patch workarounds"?

I'm definitely in favour of adding the warning, but would need to think
a bit more about whether adding the workarounds will give net-positive
results.

-Steffan
Selva Nair Dec. 6, 2018, 3:38 p.m. | #4
Hi,

On Thu, Dec 6, 2018 at 3:29 AM Steffan Karger <steffan.karger@fox-it.com> wrote:
>
> On 05-12-18 15:09, Arne Schwabe wrote:
> > Am 05.12.18 um 11:51 schrieb Steffan Karger:
> >> On Wed, 31 Oct 2018 at 17:53, Arne Schwabe <arne@rfc2549.org> wrote:
> >>> Before OpenSSL 1.1.1 there could be no mismatch between
> >>> compiled and actual OpenSSL version. With OpenSSL 1.1.1 we need
> >>> runtime detection to detect the actual best TLS version supported.
> >>>
> >>> Allowing this runtime detection also allows removing some of the
> >>> TLS 1.3/OpenSSL 1.1.1 #ifdefs
> >>>
> >>> Without this patch tls-min-version 1.3 or-highest will actually
> >>> downgrade to TLS 1.3 in the "compiled with 1.1.0 and linked against
> >>> 1.1.1" scenario.
> >>
> >> "Downgrade to TLS 1.2", I guess?
> >>
> >> But more fundamental: do want to support runtime-upgrading the TLS
> >> library? Are we sure that this is the only place where this will
> >> create unexpected behaviour? Does it even make sense to upgrade a
> >> dependency to a version that contains all sorts of API/ABI changes and
> >> then expect that you do not have to recompile? Honest questions; I
> >> don't understand why one would want or do this.
> >
> > I think on some installation, especially when the user compiled OpenVPN
> > him/herself, we just need to live with the fact that the OpenSSL library
> > got upgraded from OpenSSL 1.1.0 to OpenSSL 1.1.1.
> >
> > But one art of decting the newer library here is that we can bail out on
> > some of the corner cases instead of continuing, like refusing external
> > auth if the OpenSSL libary is too new.
> >
> > I still think is an unsupported scenario and we might add an warning
> > message aka like
> >
> > "Detected OpenSSL 1.1.1 but compiled against OpenSSL 1.1.0, recompile to
> > use all OpenSSL 1.1.1 features"
>
> Do you mean "add a warning message *in addition to* the patch
> workarounds", or "add a warning message "instead of* the patch workarounds"?
>
> I'm definitely in favour of adding the warning, but would need to think
> a bit more about whether adding the workarounds will give net-positive
> results.

I was of the view that we should support upgrade to OpenSSL 1.1.1
without recompiling, but on closer encounters with 1.1.1, I have a
change of mind.

In our case, the only real problem is: if OpenSSL 1.1.1 is in use at
run time, external certificate support is broken and needs fixing. But
does that need this patch? Two cases to consider:

(i) compiled against 1.1.0 but the dynamic linker loads 1.1.1
(ii) compiled and run against 1.1.1

For fixing "management-external-cert", case (i) may need this but case
(ii) doesn't. That said, the ABI compat between 1.1.0 and 1.1.1 that
OpenSSL claims has caveats, so we need not promote use case (i). Just
warn this may not work and let things fail if and when it fails.

Even in case of (ii) my view is that we should just let external
signing fail if the management client is not capable of handling the
signature request (cases where the data is either prepadded or
requires PSS padding). Such an approach still requires some changes to
the code to support non PKCS1 padding but that's the topic of a
different set of patches.

Selva

Selva

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index c0bc7a47..2a92f2e6 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -4182,12 +4182,11 @@  show_available_tls_ciphers(const char *cipher_list,
 {
     printf("Available TLS Ciphers, listed in order of preference:\n");
 
-#if (ENABLE_CRYPTO_OPENSSL && OPENSSL_VERSION_NUMBER >= 0x1010100fL)
-    printf("\nFor TLS 1.3 and newer (--tls-ciphersuites):\n\n");
-    show_available_tls_ciphers_list(cipher_list_tls13, tls_cert_profile, true);
-#else
-    (void) cipher_list_tls13;  /* Avoid unused warning */
-#endif
+    if (tls_version_max() >= TLS_VER_1_3)
+    {
+        printf("\nFor TLS 1.3 and newer (--tls-ciphersuites):\n\n");
+        show_available_tls_ciphers_list(cipher_list_tls13, tls_cert_profile, true);
+    }
 
     printf("\nFor TLS 1.2 and older (--tls-cipher):\n\n");
     show_available_tls_ciphers_list(cipher_list, tls_cert_profile, false);
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index b5da7e13..c2c8fdc0 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -210,7 +210,23 @@  int
 tls_version_max(void)
 {
 #if defined(TLS1_3_VERSION)
+    /* If this is defined we can safely assume TLS 1.3 support */
     return TLS_VER_1_3;
+#elif OPENSSL_VERSION_NUMBER >= 0x10100000L
+    /*
+     * The library we are *linked* against is OpenSSL 1.1.1
+     * and therefore supports TLS 1.3. This needs to be checked at runtime
+     * since we can be compiled against 1.1.0 and then the library can be
+     * upgraded to 1.1.1
+     */
+    if (OpenSSL_version_num() >= 0x1010100fL)
+    {
+        return TLS_VER_1_3;
+    }
+    else
+    {
+        return TLS_VER_1_2;
+    }
 #elif defined(TLS1_2_VERSION) || defined(SSL_OP_NO_TLSv1_2)
     return TLS_VER_1_2;
 #elif defined(TLS1_1_VERSION) || defined(SSL_OP_NO_TLSv1_1)
@@ -236,12 +252,20 @@  openssl_tls_version(int ver)
     {
         return TLS1_2_VERSION;
     }
-#if defined(TLS1_3_VERSION)
     else if (ver == TLS_VER_1_3)
     {
+        /*
+         * Supporting the library upgraded to TLS1.3 without recompile
+         * is enough to support here with a simple constant that the same
+         * as in the TLS 1.3, so spec it is very unlikely that OpenSSL
+         * will change this constant
+         */
+#ifndef TLS1_3_VERSION
+        return 0x0304;
+#else
         return TLS1_3_VERSION;
-    }
 #endif
+    }
     return 0;
 }
 
@@ -1948,14 +1972,13 @@  show_available_tls_ciphers_list(const char *cipher_list,
         crypto_msg(M_FATAL, "Cannot create SSL_CTX object");
     }
 
-#if (OPENSSL_VERSION_NUMBER >= 0x1010100fL)
     if (tls13)
     {
-        SSL_CTX_set_min_proto_version(tls_ctx.ctx, TLS1_3_VERSION);
+        SSL_CTX_set_min_proto_version(tls_ctx.ctx,
+                                      openssl_tls_version(TLS_VER_1_3));
         tls_ctx_restrict_ciphers_tls13(&tls_ctx, cipher_list);
     }
     else
-#endif
     {
         SSL_CTX_set_max_proto_version(tls_ctx.ctx, TLS1_2_VERSION);
         tls_ctx_restrict_ciphers(&tls_ctx, cipher_list);