Restrict default TLS cipher list

In the past years, the internet has been moving forward wrt deprecating older and less secure ciphers. Let's follow this example in OpenVPN and also restrict the default list of negotiable TLS ciphers in 2.3.x.

This disables the following:
* Export ciphers (these are broken on purpose...)
* Ciphers in the LOW and MEDIUM security cipher list of OpenSSL The LOW suite will be completely removed from OpenSSL in 1.1.0, the MEDIUM suite contains ciphers like RC4 and SEED.
* Ciphers that are not supported by OpenVPN anyway (cleans up the list)

Note that users are able to override this default, using --tls-cipher, if they for some reason need ciphers that are now disabled by default.

---
 doc/openvpn.8             |  5 +++--
 src/openvpn/ssl.c         |  5 +----
 src/openvpn/ssl_openssl.c | 17 +++++++++++++++--
 3 files changed, 19 insertions(+), 8 deletions(-)

index 148a62a..1cad9be 100644
@@ -4555,8 +4555,9 @@ is an expert feature, which - if used correcly - can improve the security of
 your VPN connection.  But it is also easy to unwittingly use it to carefully
 align a gun with your foot, or just break your connection.  Use with care!
 
-The default for --tls-cipher is to use PolarSSL's default cipher list
-when using PolarSSL or "DEFAULT:!EXP:!PSK:!SRP:!kRSA" when using OpenSSL.
+The default for \-\-tls\-cipher is to use PolarSSL's default cipher list
+when using PolarSSL or "DEFAULT:!EXP:!LOW:!MEDIUM:!PSK:!SRP:!kRSA" when using
+OpenSSL.
 .\"*********************************************************
 .TP
 .B \-\-tls\-timeout n

index 0679890..9e31c3c 100644
@@ -561,10 +561,7 @@ init_ssl (const struct options *options, struct tls_root_ctx *new_ctx)
   tls_ctx_check_cert_time(new_ctx);
 
   /* Allowable ciphers */
-  if (options->cipher_list)
-    {
-      tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
-    }
+  tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
 
 #ifdef ENABLE_CRYPTO_POLARSSL
   /* Personalise the random by mixing in the certificate */

index e67f60e..1dfbb23 100644
@@ -267,6 +267,20 @@ tls_ctx_set_options (struct tls_root_ctx *ctx, unsigned int ssl_flags)
 void
 tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 {
+  if (ciphers == NULL)
+    {
+      /* Use sane default TLS cipher list */
+      if(!SSL_CTX_set_cipher_list(ctx->ctx,
+	  /* Use openssl's default list as a basis */
+	  "DEFAULT"
+	  /* Disable export ciphers and openssl's 'low' and 'medium' ciphers */
+	  ":!EXP:!LOW:!MEDIUM"
+	  /* Disable unsupported TLS modes */
+	  ":!PSK:!SRP:!kRSA"))
+	crypto_msg (M_FATAL, "Failed to set default TLS cipher list.");
+      return;
+    }
+
   size_t begin_of_cipher, end_of_cipher;
 
   const char *current_cipher;

@@ -1383,8 +1397,7 @@ show_available_tls_ciphers (const char *cipher_list)
   if (!ssl)
     crypto_msg (M_FATAL, "Cannot create SSL object");
 
-  if (cipher_list)
-    tls_ctx_restrict_ciphers(&tls_ctx, cipher_list);
+  tls_ctx_restrict_ciphers(&tls_ctx, cipher_list);
 
   printf ("Available TLS Ciphers,\n");
   printf ("listed in order of preference:\n\n");

2.5.0

Am 17.04.16 um 18:23 schrieb Steffan Karger:

ACK (like in master)
Changing this in 2.3.x might break some configs. In my Android client the last change broke various config, a few broken OpenSSL installations but mainly custom "hardening" by specifying a very small cipher list with tls-cipher.

Arne

In the past years, the internet has been moving forward wrt deprecating older and less secure ciphers. Let's follow this example in OpenVPN and also restrict the default list of negotiable TLS ciphers in 2.3.x.

This disables the following:
* Export ciphers (these are broken on purpose...)
* Ciphers in the LOW and MEDIUM security cipher list of OpenSSL The LOW suite will be completely removed from OpenSSL in 1.1.0, the MEDIUM suite contains ciphers like RC4 and SEED.
* Ciphers that are not supported by OpenVPN anyway (cleans up the list)

Note that users are able to override this default, using --tls-cipher, if they for some reason need ciphers that are now disabled by default.

v2: add Changes.rst entry.

---
 doc/openvpn.8             |  5 +++--
 src/openvpn/ssl.c         |  5 +----
 src/openvpn/ssl_openssl.c | 17 +++++++++++++++--
 3 files changed, 19 insertions(+), 8 deletions(-)

index 148a62a..1cad9be 100644
@@ -4555,8 +4555,9 @@ is an expert feature, which - if used correcly - can improve the security of
 your VPN connection.  But it is also easy to unwittingly use it to carefully
 align a gun with your foot, or just break your connection.  Use with care!
 
-The default for --tls-cipher is to use PolarSSL's default cipher list
-when using PolarSSL or "DEFAULT:!EXP:!PSK:!SRP:!kRSA" when using OpenSSL.
+The default for \-\-tls\-cipher is to use PolarSSL's default cipher list
+when using PolarSSL or "DEFAULT:!EXP:!LOW:!MEDIUM:!PSK:!SRP:!kRSA" when using
+OpenSSL.
 .\"*********************************************************
 .TP
 .B \-\-tls\-timeout n

index 0679890..9e31c3c 100644
@@ -561,10 +561,7 @@ init_ssl (const struct options *options, struct tls_root_ctx *new_ctx)
   tls_ctx_check_cert_time(new_ctx);
 
   /* Allowable ciphers */
-  if (options->cipher_list)
-    {
-      tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
-    }
+  tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
 
 #ifdef ENABLE_CRYPTO_POLARSSL
   /* Personalise the random by mixing in the certificate */

index e67f60e..1dfbb23 100644
@@ -267,6 +267,20 @@ tls_ctx_set_options (struct tls_root_ctx *ctx, unsigned int ssl_flags)
 void
 tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 {
+  if (ciphers == NULL)
+    {
+      /* Use sane default TLS cipher list */
+      if(!SSL_CTX_set_cipher_list(ctx->ctx,
+	  /* Use openssl's default list as a basis */
+	  "DEFAULT"
+	  /* Disable export ciphers and openssl's 'low' and 'medium' ciphers */
+	  ":!EXP:!LOW:!MEDIUM"
+	  /* Disable unsupported TLS modes */
+	  ":!PSK:!SRP:!kRSA"))
+	crypto_msg (M_FATAL, "Failed to set default TLS cipher list.");
+      return;
+    }
+
   size_t begin_of_cipher, end_of_cipher;
 
   const char *current_cipher;

@@ -1383,8 +1397,7 @@ show_available_tls_ciphers (const char *cipher_list)
   if (!ssl)
     crypto_msg (M_FATAL, "Cannot create SSL object");
 
-  if (cipher_list)
-    tls_ctx_restrict_ciphers(&tls_ctx, cipher_list);
+  tls_ctx_restrict_ciphers(&tls_ctx, cipher_list);
 
   printf ("Available TLS Ciphers,\n");
   printf ("listed in order of preference:\n\n");

2.5.0

Your patch has been applied to the release/2.3 branch (Changes.rst was missing, taken from "master" patch and put into the "New in 2.3.11" section).

commit 8a399cd30ae19f521794f3dfe7ef194abd440d1b (release/2.3)

Author: Steffan Karger
Date:   Sun Apr 17 20:23:32 2016 +0200

     Restrict default TLS cipher list

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11455
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Am 17.04.2016 um 20:23 schrieb Steffan Karger:

Sorry, too late for my
NAK - reasons below the code patch.

Causes SIGSEGV on PolarSSL-based builds unless --tls-cipher is specified in a useful way.

"Too late for NAK" because this code has already been committed and released in 2.3.11.

I am not sure if the corresponding master branch code is OK, please double check. I'm too tired for an assessment, and too busy to hold this back for as long as I'd have to.

Commit 8a399cd3 hardened the OpenSSL default cipher list, but also introduced a change in shared code that causes polarssl / mbedtls builds to break when no --tls-cipher is specified.

This fix is backported code from the master branch.

---
 src/openvpn/ssl_polarssl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 1f58369..9263698 100644
@@ -176,7 +176,12 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 {
   char *tmp_ciphers, *tmp_ciphers_orig, *token;
   int i, cipher_count;
-  int ciphers_len = strlen (ciphers);
+  int ciphers_len;
+
+  if (NULL == ciphers)
+    return; /* Nothing to do */
+
+  ciphers_len = strlen (ciphers);
 
   ASSERT (NULL != ctx);
   ASSERT (0 != ciphers_len);

2.7.4

ACK. Tested on FreeBSD 9.3, all 3 tests in "make check", works.

It is in line with the much better tested code in master, and with the man page:

"The default for --tls-cipher is to use PolarSSL's default cipher
list when using PolarSSL or (...) when using OpenSSL"

Your patch has been applied to the release/2.3 branch.

commit 629baad8f89af261445a2ace03694601f8e476f9 (release/2.3)

Author: Steffan Karger
Date:   Fri May 13 08:54:52 2016 +0200

     Fix polarssl / mbedtls builds

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11647
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Am 13.05.2016 um 09:32 schrieb Gert Doering:

Thanks to both of you. The FreeBSD ports' POLARSSL option is now fixed, and I have removed the BROKEN mark.

Hi,

Thanks. There a few additional minor patches that can go into 2.3, and I'll wait a few more days if we broke anything else in 2.3.11, and then we'll do 2.3.12

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...