Use RSA_generate_key_ex() instead of deprecated, RSA_generate_key()

One note on the implementation though; the code generates one ephemeral RSA key that is used during the entire lifetime of an OpenVPN process. If OpenSSL requests a new (ephemeral) key, it will keep on returning the same (usually rather small) key. Not the best solution.

To actually run this code, I had to force usage by selecting the TLS-RSA-EXPORT-WITH-DES40-CBC-SHA tls-cipher. That generated a 512-bit ephemeral RSA key, and uses the outdated DES encryption protocol.

Using this mode could lead to a false sense of security. Then again, one should be using (Ephemeral) Diffie-Hellman anyway, and OpenVPN requires a tls-server to supply dh parameters. A user would need to deliberately choose a weak tls-cipher like TLS-RSA-EXPORT-WITH-DES40-CBC-SHA, which would be aligning a gun with his foot anyway. If one would decide this implementation is not good enough anymore, I'd suggest to just strip out support for this completely.

Hi,

Quoted 6 lines · Steffan Karger

One note on the implementation though; the code generates one ephemeral RSA key that is used during the entire lifetime of an OpenVPN process. If OpenSSL requests a new (ephemeral) key, it will keep on returning the same (usually rather small) key. Not the best solution.

Quoted 4 lines

To actually run this code, I had to force usage by selecting the TLS-RSA-EXPORT-WITH-DES40-CBC-SHA tls-cipher. That generated a 512-bit ephemeral RSA key, and uses the outdated DES encryption protocol.

I'm not sure I understand the whole picture here. What is happening in the "normal" scenario, and why is this particular cipher calling a different code path inside OpenVPN?

Quoted 9 lines

Using this mode could lead to a false sense of security. Then again, one should be using (Ephemeral) Diffie-Hellman anyway, and OpenVPN requires a tls-server to supply dh parameters. A user would need to deliberately choose a weak tls-cipher like TLS-RSA-EXPORT-WITH-DES40-CBC-SHA, which would be aligning a gun with his foot anyway. If one would decide this implementation is not good enough anymore, I'd suggest to just strip out support for this completely.

I'm tempted to agree with this :) - single-DES (and 40 bit to that!) really isn't what we want to use.

So what would happen if we remove this code, and a user tries to use this tls-cipher?

I've given the stripping out a little more thought and think I'm in favour of stripping this out of 2.4, but keep support in 2.3 to not break anything for minor releases. Removing this cipher mode sounds like a feature to me ;)

Could I ask you to provide a patch to remove this for 2.4?

This allows to check the available TLS ciphers for a specific configuration by supplying both --tls-cipher and --show-tls options.

This code would not really generate ephemeral keys every time it is called, but a single key that would be reused during process lifetime and returned each time the function was called; probably not what users would expect.

TLS allowes ephemeral keys to be used only when no other key exchange, such as (ephemeral) Diffie-Hellman, is performed. The end result is that it was only used by a number of (weak) export ciphers, which could give users a false sense of security.

So, instead of fixing a weak cipher mode, we'll just remove support for it completely. Plenty of better alternatives are available in TLS.

This diff look like a lot has changed, but this just adds some ifs to check for NULL in tls_ctx_restrict_ciphers() to prepare for disabling export ciphers by default in OpenVPN 2.4+.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/ssl.c          |   5 +-
 src/openvpn/ssl_backend.h  |   5 +-
 src/openvpn/ssl_openssl.c  | 130 ++++++++++++++++++++++++++-------------------
 src/openvpn/ssl_polarssl.c |   7 ++-
 4 files changed, 85 insertions(+), 62 deletions(-)

src/openvpn/ssl.c +1 -4

index bd19d75..93222c4 100644
@@ -543,10 +543,7 @@ init_ssl (const struct options *options, struct tls_root_ctx *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 */

src/openvpn/ssl_backend.h +3 -2

index 54383fe..a6fc3bd 100644
@@ -167,8 +167,9 @@ void tls_ctx_set_options (struct tls_root_ctx *ctx, unsigned int ssl_flags);
 /**
  * Restrict the list of ciphers that can be used within the TLS context.
  *
- * @param ctx		TLS context to restrict
- * @param ciphers	String containing : delimited cipher names.
+ * @param ctx		TLS context to restrict, must be valid.
+ * @param ciphers	String containing : delimited cipher names, or NULL to use
+ *					sane defaults.
  */
 void tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers);
 

src/openvpn/ssl_openssl.c +75 -55

index 08327a1..5f6c270 100644
@@ -217,71 +217,91 @@ 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)
 {
-  size_t begin_of_cipher, end_of_cipher;
-
-  const char *current_cipher;
-  size_t current_cipher_len;
+  if (ciphers == NULL)
+    {
+      /* Nothing to do */
+      return;
+    }
+  else
+    {
+      /* Parse supplied cipher list and pass on to OpenSSL */
+      size_t begin_of_cipher, end_of_cipher;
 
-  const tls_cipher_name_pair *cipher_pair;
+      const char *current_cipher;
+      size_t current_cipher_len;
 
-  char openssl_ciphers[4096];
-  size_t openssl_ciphers_len = 0;
-  openssl_ciphers[0] = '\0';
+      const tls_cipher_name_pair *cipher_pair;
 
-  ASSERT(NULL != ctx);
+      char openssl_ciphers[4096];
+      size_t openssl_ciphers_len = 0;
+      openssl_ciphers[0] = '\0';
 
-  // Translate IANA cipher suite names to OpenSSL names
-  begin_of_cipher = end_of_cipher = 0;
-  for (; begin_of_cipher < strlen(ciphers); begin_of_cipher = end_of_cipher) {
-      end_of_cipher += strcspn(&ciphers[begin_of_cipher], ":");
-      cipher_pair = tls_get_cipher_name_pair(&ciphers[begin_of_cipher], end_of_cipher - begin_of_cipher);
+      ASSERT(NULL != ctx);
 
-      if (NULL == cipher_pair)
+      // Translate IANA cipher suite names to OpenSSL names
+      begin_of_cipher = end_of_cipher = 0;
+      for (; begin_of_cipher < strlen(ciphers); begin_of_cipher = end_of_cipher)
         {
-          // No translation found, use original
-          current_cipher = &ciphers[begin_of_cipher];
-          current_cipher_len = end_of_cipher - begin_of_cipher;
-
-          // Issue warning on missing translation
-          // %.*s format specifier expects length of type int, so guarantee
-          // that length is small enough and cast to int.
-          msg (M_WARN, "No valid translation found for TLS cipher '%.*s'",
-                 constrain_int(current_cipher_len, 0, 256), current_cipher);
-        }
-      else
-	{
-	  // Use OpenSSL name
-          current_cipher = cipher_pair->openssl_name;
-          current_cipher_len = strlen(current_cipher);
-
-	  if (end_of_cipher - begin_of_cipher == current_cipher_len &&
-	      0 == memcmp (&ciphers[begin_of_cipher], cipher_pair->openssl_name, end_of_cipher - begin_of_cipher))
-	    {
-	      // Non-IANA name used, show warning
-	      msg (M_WARN, "Deprecated TLS cipher name '%s', please use IANA name '%s'", cipher_pair->openssl_name, cipher_pair->iana_name);
-	    }
-	}
-
-      // Make sure new cipher name fits in cipher string
-      if (((sizeof(openssl_ciphers)-1) - openssl_ciphers_len) < current_cipher_len) {
-	msg(M_SSLERR, "Failed to set restricted TLS cipher list, too long (>%d).", (int)sizeof(openssl_ciphers)-1);
-      }
+          end_of_cipher += strcspn(&ciphers[begin_of_cipher], ":");
+          cipher_pair = tls_get_cipher_name_pair(&ciphers[begin_of_cipher],
+              end_of_cipher - begin_of_cipher);
 
-      // Concatenate cipher name to OpenSSL cipher string
-      memcpy(&openssl_ciphers[openssl_ciphers_len], current_cipher, current_cipher_len);
-      openssl_ciphers_len += current_cipher_len;
-      openssl_ciphers[openssl_ciphers_len] = ':';
-      openssl_ciphers_len++;
+          if (NULL == cipher_pair)
+            {
+              // No translation found, use original
+              current_cipher = &ciphers[begin_of_cipher];
+              current_cipher_len = end_of_cipher - begin_of_cipher;
+
+              // Issue warning on missing translation
+              // %.*s format specifier expects length of type int, so guarantee
+              // that length is small enough and cast to int.
+              msg (M_WARN, "No valid translation found for TLS cipher '%.*s'",
+                     constrain_int(current_cipher_len, 0, 256), current_cipher);
+            }
+          else
+            {
+              // Use OpenSSL name
+              current_cipher = cipher_pair->openssl_name;
+              current_cipher_len = strlen(current_cipher);
+
+              if (end_of_cipher - begin_of_cipher == current_cipher_len &&
+                  0 == memcmp (&ciphers[begin_of_cipher],
+                      cipher_pair->openssl_name,
+                      end_of_cipher - begin_of_cipher))
+                {
+                  // Non-IANA name used, show warning
+                  msg (M_WARN, "Deprecated TLS cipher name '%s', "
+                      "please use IANA name '%s'", cipher_pair->openssl_name,
+                      cipher_pair->iana_name);
+                }
+            }
 
-      end_of_cipher++;
-  }
+          // Make sure new cipher name fits in cipher string
+          if (((sizeof(openssl_ciphers)-1) - openssl_ciphers_len) <
+              current_cipher_len) {
+            msg(M_SSLERR,
+                "Failed to set restricted TLS cipher list, too long (>%d).",
+                (int)sizeof(openssl_ciphers)-1);
+          }
+
+          // Concatenate cipher name to OpenSSL cipher string
+          memcpy(&openssl_ciphers[openssl_ciphers_len], current_cipher,
+              current_cipher_len);
+          openssl_ciphers_len += current_cipher_len;
+          openssl_ciphers[openssl_ciphers_len] = ':';
+          openssl_ciphers_len++;
+
+          end_of_cipher++;
+        }
 
-  if (openssl_ciphers_len > 0)
-    openssl_ciphers[openssl_ciphers_len-1] = '\0';
+      if (openssl_ciphers_len > 0)
+        openssl_ciphers[openssl_ciphers_len-1] = '\0';
 
-  // Set OpenSSL cipher list
-  if(!SSL_CTX_set_cipher_list(ctx->ctx, openssl_ciphers))
-    msg(M_SSLERR, "Failed to set restricted TLS cipher list: %s", openssl_ciphers);
+      // Set OpenSSL cipher list
+      if(!SSL_CTX_set_cipher_list(ctx->ctx, openssl_ciphers))
+        msg(M_SSLERR, "Failed to set restricted TLS cipher list: %s",
+            openssl_ciphers);
+    }
 }
 
 void

src/openvpn/ssl_polarssl.c +0 -0

index 551c352..d964b91 100644
@@ -173,7 +173,12 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 {
   char *tmp_ciphers, *tmp_ciphers_orig, *token;
   int i, cipher_count;

Hi,

Attached a v2 of the patch below, that removes the else to make the diff a lot smaller and changes a //-style comment to /* */-style.

-Steffan

Quoted 211 lines · Steffan Karger

This diff look like a lot has changed, but this just adds some ifs to check for NULL in tls_ctx_restrict_ciphers() to prepare for disabling export ciphers by default in OpenVPN 2.4+.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/ssl.c          |   5 +-
 src/openvpn/ssl_backend.h  |   5 +-
 src/openvpn/ssl_openssl.c  | 130 ++++++++++++++++++++++++++-------------------
 src/openvpn/ssl_polarssl.c |   7 ++-
 4 files changed, 85 insertions(+), 62 deletions(-)

src/openvpn/ssl.c +1 -4

index bd19d75..93222c4 100644
@@ -543,10 +543,7 @@ init_ssl (const struct options *options, struct tls_root_ctx *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 */

src/openvpn/ssl_backend.h +3 -2

index 54383fe..a6fc3bd 100644
@@ -167,8 +167,9 @@ void tls_ctx_set_options (struct tls_root_ctx *ctx, unsigned int ssl_flags);
 /**
  * Restrict the list of ciphers that can be used within the TLS context.
  *
- * @param ctx		TLS context to restrict
- * @param ciphers	String containing : delimited cipher names.
+ * @param ctx		TLS context to restrict, must be valid.
+ * @param ciphers	String containing : delimited cipher names, or NULL to use
+ *					sane defaults.
  */
 void tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers);
 

src/openvpn/ssl_openssl.c +75 -55

index 08327a1..5f6c270 100644
@@ -217,71 +217,91 @@ 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)
 {
-  size_t begin_of_cipher, end_of_cipher;
-
-  const char *current_cipher;
-  size_t current_cipher_len;
+  if (ciphers == NULL)
+    {
+      /* Nothing to do */
+      return;
+    }
+  else
+    {
+      /* Parse supplied cipher list and pass on to OpenSSL */
+      size_t begin_of_cipher, end_of_cipher;
 
-  const tls_cipher_name_pair *cipher_pair;
+      const char *current_cipher;
+      size_t current_cipher_len;
 
-  char openssl_ciphers[4096];
-  size_t openssl_ciphers_len = 0;
-  openssl_ciphers[0] = '\0';
+      const tls_cipher_name_pair *cipher_pair;
 
-  ASSERT(NULL != ctx);
+      char openssl_ciphers[4096];
+      size_t openssl_ciphers_len = 0;
+      openssl_ciphers[0] = '\0';
 
-  // Translate IANA cipher suite names to OpenSSL names
-  begin_of_cipher = end_of_cipher = 0;
-  for (; begin_of_cipher < strlen(ciphers); begin_of_cipher = end_of_cipher) {
-      end_of_cipher += strcspn(&ciphers[begin_of_cipher], ":");
-      cipher_pair = tls_get_cipher_name_pair(&ciphers[begin_of_cipher], end_of_cipher - begin_of_cipher);
+      ASSERT(NULL != ctx);
 
-      if (NULL == cipher_pair)
+      // Translate IANA cipher suite names to OpenSSL names
+      begin_of_cipher = end_of_cipher = 0;
+      for (; begin_of_cipher < strlen(ciphers); begin_of_cipher = end_of_cipher)
         {
-          // No translation found, use original
-          current_cipher = &ciphers[begin_of_cipher];
-          current_cipher_len = end_of_cipher - begin_of_cipher;
-
-          // Issue warning on missing translation
-          // %.*s format specifier expects length of type int, so guarantee
-          // that length is small enough and cast to int.
-          msg (M_WARN, "No valid translation found for TLS cipher '%.*s'",
-                 constrain_int(current_cipher_len, 0, 256), current_cipher);
-        }
-      else
-	{
-	  // Use OpenSSL name
-          current_cipher = cipher_pair->openssl_name;
-          current_cipher_len = strlen(current_cipher);
-
-	  if (end_of_cipher - begin_of_cipher == current_cipher_len &&
-	      0 == memcmp (&ciphers[begin_of_cipher], cipher_pair->openssl_name, end_of_cipher - begin_of_cipher))
-	    {
-	      // Non-IANA name used, show warning
-	      msg (M_WARN, "Deprecated TLS cipher name '%s', please use IANA name '%s'", cipher_pair->openssl_name, cipher_pair->iana_name);
-	    }
-	}
-
-      // Make sure new cipher name fits in cipher string
-      if (((sizeof(openssl_ciphers)-1) - openssl_ciphers_len) < current_cipher_len) {
-	msg(M_SSLERR, "Failed to set restricted TLS cipher list, too long (>%d).", (int)sizeof(openssl_ciphers)-1);
-      }
+          end_of_cipher += strcspn(&ciphers[begin_of_cipher], ":");
+          cipher_pair = tls_get_cipher_name_pair(&ciphers[begin_of_cipher],
+              end_of_cipher - begin_of_cipher);
 
-      // Concatenate cipher name to OpenSSL cipher string
-      memcpy(&openssl_ciphers[openssl_ciphers_len], current_cipher, current_cipher_len);
-      openssl_ciphers_len += current_cipher_len;
-      openssl_ciphers[openssl_ciphers_len] = ':';
-      openssl_ciphers_len++;
+          if (NULL == cipher_pair)
+            {
+              // No translation found, use original
+              current_cipher = &ciphers[begin_of_cipher];
+              current_cipher_len = end_of_cipher - begin_of_cipher;
+
+              // Issue warning on missing translation
+              // %.*s format specifier expects length of type int, so guarantee
+              // that length is small enough and cast to int.
+              msg (M_WARN, "No valid translation found for TLS cipher '%.*s'",
+                     constrain_int(current_cipher_len, 0, 256), current_cipher);
+            }
+          else
+            {
+              // Use OpenSSL name
+              current_cipher = cipher_pair->openssl_name;
+              current_cipher_len = strlen(current_cipher);
+
+              if (end_of_cipher - begin_of_cipher == current_cipher_len &&
+                  0 == memcmp (&ciphers[begin_of_cipher],
+                      cipher_pair->openssl_name,
+                      end_of_cipher - begin_of_cipher))
+                {
+                  // Non-IANA name used, show warning
+                  msg (M_WARN, "Deprecated TLS cipher name '%s', "
+                      "please use IANA name '%s'", cipher_pair->openssl_name,
+                      cipher_pair->iana_name);
+                }
+            }
 
-      end_of_cipher++;
-  }
+          // Make sure new cipher name fits in cipher string
+          if (((sizeof(openssl_ciphers)-1) - openssl_ciphers_len) <
+              current_cipher_len) {
+            msg(M_SSLERR,
+                "Failed to set restricted TLS cipher list, too long (>%d).",
+                (int)sizeof(openssl_ciphers)-1);
+          }
+
+          // Concatenate cipher name to OpenSSL cipher string
+          memcpy(&openssl_ciphers[openssl_ciphers_len], current_cipher,
+              current_cipher_len);
+          openssl_ciphers_len += current_cipher_len;
+          openssl_ciphers[openssl_ciphers_len] = ':';
+          openssl_ciphers_len++;
+
+          end_of_cipher++;
+        }
 
-  if (openssl_ciphers_len > 0)
-    openssl_ciphers[openssl_ciphers_len-1] = '\0';
+      if (openssl_ciphers_len > 0)
+        openssl_ciphers[openssl_ciphers_len-1] = '\0';
 
-  // Set OpenSSL cipher list
-  if(!SSL_CTX_set_cipher_list(ctx->ctx, openssl_ciphers))
-    msg(M_SSLERR, "Failed to set restricted TLS cipher list: %s", openssl_ciphers);
+      // Set OpenSSL cipher list
+      if(!SSL_CTX_set_cipher_list(ctx->ctx, openssl_ciphers))
+        msg(M_SSLERR, "Failed to set restricted TLS cipher list: %s",
+            openssl_ciphers);
+    }
 }
 
 void

src/openvpn/ssl_polarssl.c +6 -1

index 551c352..d964b91 100644
@@ -173,7 +173,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);
 

src/openvpn/ssl_polarssl.c tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers) · line 184

Quoted 3 lines

src/openvpn/ssl_polarssl.c +0 -0

   ASSERT (NULL != ctx);
   ASSERT (0 != ciphers_len);

Export ciphers are deliberately weak ciphers, and not fully supported by OpenVPN since ephemeral RSA support has been removed a few commits ago. This commit removes them from the default cipher list to avoid confusion.

PolarSSL does not support export ciphers, so no action required there.

ACK. But isn't is better to use good to do the !EXP just in case and for consistency for PolarSSL as well.

Hi,

Attached a patch that should close issue #197. This patch moves from using the deprecated RSA_generate_key() to the 'new' RSA_generate_key_ex() to generate ephemeral RSA keys. This patch does not change OpenVPN's behaviour.

One note on the implementation though; the code generates one ephemeral RSA key that is used during the entire lifetime of an OpenVPN process. If OpenSSL requests a new (ephemeral) key, it will keep on returning the same (usually rather small) key. Not the best solution.