Skip to contentSkip to conversation

Set tls-cipher restriction before loading certificates

PATCH In review Arne Schwabe 5 participants 10 messages Jun 26, 2017 – Jun 27, 2017

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Set tls-cipher restriction before loading certificates

OpenSSL 1.1 does not allow MD5 signed certificates by default anymore. This can be enabled again by settings tls-cipher "DEFAULT:@SECLEVEL=0" but only if the cipher list is set before loading the certificates. This patch changes the order of loading.

---
 src/openvpn/ssl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
src/openvpn/ssl.c +5 -3
index 29280dce..9ca300c0 100644
@@ -616,6 +616,11 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
         tls_ctx_client_new(new_ctx);
     }
 
+    /* Allowable ciphers */
+    /* Since @SECLEVEL also influces loading of certificates, set the
+     * cipher restrictions before loading certificates */
+    tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
+
     tls_ctx_set_options(new_ctx, options->ssl_flags);
 
     if (options->pkcs12_file)
@@ -708,9 +713,6 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
         tls_ctx_load_ecdh_params(new_ctx, options->ecdh_curve);
     }
 
-    /* Allowable ciphers */
-    tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
-
 #ifdef ENABLE_CRYPTO_MBEDTLS
     /* Personalise the random by mixing in the certificate */
     tls_ctx_personalise_random(new_ctx);
Signature · 1 line
2.11.0 (Apple Git-81)
#1 lore raw

Arne Schwabe <[email protected]...> on Mon, 2017/06/26 13:13:

Quoted 23 lines

OpenSSL 1.1 does not allow MD5 signed certificates by default anymore. This can be enabled again by settings tls-cipher "DEFAULT:@SECLEVEL=0" but only if the cipher list is set before loading the certificates. This patch changes the order of loading. --- src/openvpn/ssl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)

src/openvpn/ssl.c +5 -0
index 29280dce..9ca300c0 100644
@@ -616,6 +616,11 @@ init_ssl(const struct options *options, struct
 tls_root_ctx *new_ctx) tls_ctx_client_new(new_ctx);
     }
 
+    /* Allowable ciphers */
+    /* Since @SECLEVEL also influces loading of certificates, set the
+     * cipher restrictions before loading certificates */
+    tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
+
     tls_ctx_set_options(new_ctx, options->ssl_flags);
 
     if (options->pkcs12_file)

src/openvpn/ssl.c init_ssl(const struct options *options, struct · line 718

Quoted 9 lines
src/openvpn/ssl.c +0 -3
@@ -708,9 +713,6 @@ init_ssl(const struct options *options, struct
 tls_root_ctx *new_ctx) tls_ctx_load_ecdh_params(new_ctx,
 options->ecdh_curve); }
 
-    /* Allowable ciphers */
-    tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
-
 #ifdef ENABLE_CRYPTO_MBEDTLS
     /* Personalise the random by mixing in the certificate */
     tls_ctx_personalise_random(new_ctx);

Looks good and tested on a server with CA and server certificates using MD5. So ACK.

Signature · 3 lines
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
#2 lore raw

Hi,

Quoted 27 lines · Christian Hesse

Arne Schwabe <[email protected]...> on Mon, 2017/06/26 13:13:

Quoted 26 lines

OpenSSL 1.1 does not allow MD5 signed certificates by default anymore. This can be enabled again by settings tls-cipher "DEFAULT:@SECLEVEL=0" but only if the cipher list is set before loading the certificates. This patch changes the order of loading. --- src/openvpn/ssl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)

src/openvpn/ssl.c +5 -0
index 29280dce..9ca300c0 100644
@@ -616,6 +616,11 @@ init_ssl(const struct options *options, struct
 tls_root_ctx *new_ctx) tls_ctx_client_new(new_ctx);
     }
 
+    /* Allowable ciphers */
+    /* Since @SECLEVEL also influces loading of certificates, set the
+     * cipher restrictions before loading certificates */
+    tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
+
     tls_ctx_set_options(new_ctx, options->ssl_flags);
 
     if (options->pkcs12_file)
@@ -708,9 +713,6 @@ init_ssl(const struct options *options, struct
 tls_root_ctx *new_ctx) tls_ctx_load_ecdh_params(new_ctx,
 options->ecdh_curve); }
 
Quoted 9 lines

src/openvpn/ssl.c init_ssl(const struct options *options, struct tls_root_ctx *new_ctx) · line 718

Quoted 6 lines
src/openvpn/ssl.c +0 -3
-    /* Allowable ciphers */
-    tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
-
 #ifdef ENABLE_CRYPTO_MBEDTLS
     /* Personalise the random by mixing in the certificate */
     tls_ctx_personalise_random(new_ctx);

Looks good and tested on a server with CA and server certificates using MD5. So ACK.

MD5 must die, die, die. People still using MD5 certs should be thankful that OpenSSL pointed out this security hole in their setup, and fix that by immediately issuing SHA2 certs...

That said, the patch does not specifically allow MD5, makes sense by itself, and passes my tests. So, ACK.

-Steffan

#3 lore raw
In reply to Arne Schwabe
Quoted 1 line · Arne Schwabe

OpenSSL 1.1 does not allow MD5 signed certificates by default anymore. This can be enabled again by settings tls-cipher "DEFAULT:@SECLEVEL=0" but only if the cipher list is set before loading the certificates. This patch changes the order of loading.

I'm not fully convinced of the argumentation for this feature - unless something have changed in OpenSSL 1.1. I believe the same can be achieved by setting an environment variable before starting OpenVPN.

  $ OPENSSL_ENABLE_MD5_VERIFY=1 /usr/sbin/openvpn ....

I know several Fedora users have deployed this, even when systemd is involved. This is needed on systems with OpenSSL 1.0 as well when they connect to a server having an MD5 based certificate or signed by a CA with an MD5 based certificate.

So unless OpenSSL 1.1 have changed this behaviour from OpenSSL 1.0, I'm not really convinced we need this.

Signature · 4 lines
kind regards,

David Sommerseth
OpenVPN Technologies, Inc
#4 lore raw

Am 26.06.17 um 13:51 schrieb David Sommerseth:

Quoted 9 lines · Arne Schwabe
Quoted 1 line

OpenSSL 1.1 does not allow MD5 signed certificates by default anymore. This can be enabled again by settings tls-cipher "DEFAULT:@SECLEVEL=0" but only if the cipher list is set before loading the certificates. This patch changes the order of loading.

I'm not fully convinced of the argumentation for this feature - unless something have changed in OpenSSL 1.1. I believe the same can be achieved by setting an environment variable before starting OpenVPN.

  $ OPENSSL_ENABLE_MD5_VERIFY=1 /usr/sbin/openvpn ....
Quoted 9 lines

I know several Fedora users have deployed this, even when systemd is involved. This is needed on systems with OpenSSL 1.0 as well when they connect to a server having an MD5 based certificate or signed by a CA with an MD5 based certificate.

So unless OpenSSL 1.1 have changed this behaviour from OpenSSL 1.0, I'm not really convinced we need this.

See this also a bugfix. Since tls-cipher options affect certificate loading, it is good to set it before certificate loading. E.g. you might want to use @SECLEVEL=5 to only allow loading of SHA256 based certificates.

Also I think your option is Fedora specific as I could not find anything in the source code in my OSSL copy and the message also mentions it being Fedora specific:

** WARNING ** [Fedora modification] MD5 certificate hash re-enabled via OPENSSL_ENABLE_MD5_VERIFY environment variable.

Arne

#5 lore raw
Quoted 24 lines · Arne Schwabe

Am 26.06.17 um 13:51 schrieb David Sommerseth:

Quoted 9 lines · Arne Schwabe
Quoted 1 line

OpenSSL 1.1 does not allow MD5 signed certificates by default anymore. This can be enabled again by settings tls-cipher "DEFAULT:@SECLEVEL=0" but only if the cipher list is set before loading the certificates. This patch changes the order of loading.

I'm not fully convinced of the argumentation for this feature - unless something have changed in OpenSSL 1.1. I believe the same can be achieved by setting an environment variable before starting OpenVPN.

  $ OPENSSL_ENABLE_MD5_VERIFY=1 /usr/sbin/openvpn ....
Quoted 9 lines

I know several Fedora users have deployed this, even when systemd is involved. This is needed on systems with OpenSSL 1.0 as well when they connect to a server having an MD5 based certificate or signed by a CA with an MD5 based certificate.

So unless OpenSSL 1.1 have changed this behaviour from OpenSSL 1.0, I'm not really convinced we need this.

See this also a bugfix. Since tls-cipher options affect certificate loading, it is good to set it before certificate loading. E.g. you might want to use @SECLEVEL=5 to only allow loading of SHA256 based certificates.

Quoted 6 lines

Also I think your option is Fedora specific as I could not find anything in the source code in my OSSL copy and the message also mentions it being Fedora specific:

** WARNING ** [Fedora modification] MD5 certificate hash re-enabled via OPENSSL_ENABLE_MD5_VERIFY environment variable.

Nope, that is actually a warning I added in one of the earlier mbed TLS builds for Fedora. That patch have been removed again, as I moved back to compat-openssl10 when compat-openssl10-pkcs11-helper became available.

The patch which adds that warning is a workaround so that uses already having deployed MD5 support when they used OpenSSL ... so we needed to ensure users did have this feature enabled. So instead of requiring users to define MBEDTLS_ENABLE_MD5_VERIFY, I re-used the OPENSSL_ENABLE_MD5_VERIFY variable name. And complained about it in the logs.

But I'm actually a bit fascinated you found a Fedora build with that warning. IIRC, that build was from a scratch build, testing out this issue - used by a user which got into troubles during the mbed TLS based builds in Fedora 26 (not yet released) and Fedora Rawhide. With openvpn-2.4.2-1, I switched back to OpenSSL.

As of the next Fedora 26 openvpn build, I will move further forward to OpenSSL 1.1 instead of compat-openssl10. Fedora Rawhide is already on openssl-1.1.

Signature · 4 lines
kind regards,

David Sommerseth
OpenVPN Technologies, Inc
#6 lore raw
Quoted 16 lines · David Sommerseth
Quoted 15 lines · Arne Schwabe

Am 26.06.17 um 13:51 schrieb David Sommerseth:

Quoted 14 lines · Arne Schwabe
Quoted 1 line

OpenSSL 1.1 does not allow MD5 signed certificates by default anymore. This can be enabled again by settings tls-cipher "DEFAULT:@SECLEVEL=0" but only if the cipher list is set before loading the certificates. This patch changes the order of loading.

I'm not fully convinced of the argumentation for this feature - unless something have changed in OpenSSL 1.1. I believe the same can be achieved by setting an environment variable before starting OpenVPN.

  $ OPENSSL_ENABLE_MD5_VERIFY=1 /usr/sbin/openvpn ....

I know several Fedora users have deployed this, even when systemd is involved. This is needed on systems with OpenSSL 1.0 as well when they connect to a server having an MD5 based certificate or signed by a CA with an MD5 based certificate.

Quoted 8 lines
Quoted 8 lines
Quoted 4 lines

So unless OpenSSL 1.1 have changed this behaviour from OpenSSL 1.0, I'm not really convinced we need this.

See this also a bugfix. Since tls-cipher options affect certificate loading, it is good to set it before certificate loading. E.g. you might want to use @SECLEVEL=5 to only allow loading of SHA256 based certificates.

Oh, btw ... We need to align this with another patch-set from Steffan. There is a feature we have in OpenVPN 3 and the proposed patches from Steffan which adds --tls-cert-profile. This is aimed at controlling what kind of TLS security parameters which is required.

Currently there is an agreement of the following profiles:

- legacy: SHA1 and newer, RSA 2048-bit+, any elliptic curve.
- preferred: SHA2 and newer, RSA 2048-bit+, any elliptic curve.

               (default in v2.5)

- suiteb: SHA256/SHA384, ECDSA with P-256 or P-384.

The mbed TLS definition of these profiles in OpenVPN 3 is like this.

  const mbedtls_x509_crt_profile crt_profile_legacy = // CONST GLOBAL
        {
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA1 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_RIPEMD160 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ),
          0xFFFFFFF, /* Any PK alg    */
          0xFFFFFFF, /* Any curve     */
          1024,      /* Minimum size for RSA keys */
        };

  const mbedtls_x509_crt_profile crt_profile_preferred = // CONST GLOBAL
        {
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ),
          0xFFFFFFF, /* Any PK alg    */
          0xFFFFFFF, /* Any curve     */
          2048,      /* Minimum size for RSA keys */
        };

The suiteb profile is just reusing the mbed TLS definition directly.

With that said ... The legacy profile does not include MD5. So either we allow MD5 into the legacy profile; or we need legacy-md5.

Signature · 4 lines
kind regards,

David Sommerseth
OpenVPN Technologies, Inc
#7 lore raw
Quoted 43 lines
Quoted 5 lines
Quoted 5 lines

See this also a bugfix. Since tls-cipher options affect certificate loading, it is good to set it before certificate loading. E.g. you might want to use @SECLEVEL=5 to only allow loading of SHA256 based certificates.

Oh, btw ... We need to align this with another patch-set from Steffan. There is a feature we have in OpenVPN 3 and the proposed patches from Steffan which adds --tls-cert-profile. This is aimed at controlling what kind of TLS security parameters which is required.

Currently there is an agreement of the following profiles:

- legacy: SHA1 and newer, RSA 2048-bit+, any elliptic curve.
- preferred: SHA2 and newer, RSA 2048-bit+, any elliptic curve.

               (default in v2.5)

- suiteb: SHA256/SHA384, ECDSA with P-256 or P-384.

The mbed TLS definition of these profiles in OpenVPN 3 is like this.

  const mbedtls_x509_crt_profile crt_profile_legacy = // CONST GLOBAL
        {
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA1 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_RIPEMD160 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ),
          0xFFFFFFF, /* Any PK alg    */
          0xFFFFFFF, /* Any curve     */
          1024,      /* Minimum size for RSA keys */
        };

  const mbedtls_x509_crt_profile crt_profile_preferred = // CONST GLOBAL
        {
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) |
          MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ),
          0xFFFFFFF, /* Any PK alg    */
          0xFFFFFFF, /* Any curve     */
          2048,      /* Minimum size for RSA keys */
        };
Quoted 5 lines

The suiteb profile is just reusing the mbed TLS definition directly.

With that said ... The legacy profile does not include MD5. So either we allow MD5 into the legacy profile; or we need legacy-md5.

Yes but I think that is seperate effort. I am not sure how to probably implment that with OpenSSL. SECLEVEL is similar but does not have exactly the same consequences. YOu could probably emulate the profiles with some kind of tls-cipher settings. But if you do that, you still need this patch :)

Arne

#8 lore raw

On 26/06/17 16:00, Arne Schwabe wrote:
[...snip...]

Quoted 8 lines
Quoted 8 lines

Currently there is an agreement of the following profiles:

- legacy: SHA1 and newer, RSA 2048-bit+, any elliptic curve.
- preferred: SHA2 and newer, RSA 2048-bit+, any elliptic curve.

               (default in v2.5)

- suiteb: SHA256/SHA384, ECDSA with P-256 or P-384.

[...snip...]

Quoted 11 lines
Quoted 5 lines

The suiteb profile is just reusing the mbed TLS definition directly.

With that said ... The legacy profile does not include MD5. So either we allow MD5 into the legacy profile; or we need legacy-md5.

Yes but I think that is seperate effort. I am not sure how to probably implment that with OpenSSL. SECLEVEL is similar but does not have exactly the same consequences. YOu could probably emulate the profiles with some kind of tls-cipher settings. But if you do that, you still need this patch :)

I agree we need to have a mechanism for adjusting the SECLEVEL/--tls-cert-profile. The challenge is that we have users which expects a similar behaviour, regardless if their OpenVPN build is using OpenSSL or mbed TLS. For end users, that matters - and we can't tell them "for this OpenVPN variant, you need to use this syntax".

In addition, AFAIK the --tls-cert-profile support is already released for OpenVPN Connect. IIRC, that approach was agreed upon between James and Steffan at the last Hackathon.

Unless there are really strong reasons not to continue with --tls-cert-profile, I am of the opinion we should go that path. That is to ensure sites already rolled out --tls-cert-profile will not start yelling at us later on. OpenVPN 3 based clients needs to behave similarly to OpenVPN 2.x when it comes to configuration options. And OpenVPN 3 is what is inside OpenVPN Connect and PrivateTunnel clients, which again have ties to OpenVPN Access Server. We need to ensure we don't add fragmentation inside the OpenVPN environment.

Of course, it won't be easy to make the users have the same experience regardless if OpenVPN use mbed TLS or OpenSSL under the hood. But I am do strongly believe that is the proper way to handle this. OpenVPN need to "glue" this together so the user experience is unified.

I am also aware that we have a few mbed TLS specific features (--use-prediction-resistance) and there are some features only available in OpenSSL (f.ex. PKCS#12 support, --capath, --engine). This is unfortunate, and we should try to reduce such gaps to an absolute minimum. So I cannot give an ACK to any patches which contributes further to such a fragmentation - unless there are really strong reasons to do so.

In this particular case, both OpenSSL and mbed TLS have a similar features, so in this case it should be possible to get a unified experience. So lets try to aim for that.

Signature · 4 lines
kind regards,

David Sommerseth
OpenVPN Technologies, Inc
#9 lore raw
In reply to Arne Schwabe

Your patch has been applied to the master and release/2.4 branch.

commit 26345ba61b8d5bccb1331894ab6d1468e3b09adf (master) commit 95c07b13ce112ceb8b15175fcae0d95c70e93eee (release/2.4)

Author: Arne Schwabe
Date:   Mon Jun 26 13:13:26 2017 +0200
     Set tls-cipher restriction before loading certificates

     Acked-by: Christian Hesse <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg14961.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering