Check --ncp-ciphers list on startup

PATCH Superseded Steffan Karger 3 participants 6 messages Oct 11, 2016 – Oct 13, 2016

Share & tools

Steffan Karger

author

2016-10-11 19:35 UTC #0 lore raw

PATCH 1/1 Check --ncp-ciphers list on startup

Currently, if --ncp-ciphers contains an invalid cipher, OpenVPN will only error out when that cipher is selected by negotiation. That's not very friendly to the user, so check the list on startup, and give a clear error message immediately.

This patches changes the cipher_kt_get() to let the caller decide what action to take if no valid cipher was found. This enables us to print all invalid ciphers in the list, instead of just the first invalid cipher.

This should fix trac #737.

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

---
 src/openvpn/crypto.c         |  5 +++++
 src/openvpn/crypto_backend.h |  3 ++-
 src/openvpn/crypto_mbedtls.c | 15 ++++++++++-----
 src/openvpn/crypto_openssl.c | 17 ++++++++++++-----
 src/openvpn/options.c        |  5 +++++
 src/openvpn/ssl.c            | 21 +++++++++++++++++++++
 src/openvpn/ssl.h            |  9 +++++++++
 7 files changed, 64 insertions(+), 11 deletions(-)

src/openvpn/crypto.c +5 -0

index 6f57841..1b17f9d 100644
@@ -763,6 +763,11 @@ init_key_type (struct key_type *kt, const char *ciphername,
   if (ciphername)
     {
       kt->cipher = cipher_kt_get (translate_cipher_name_from_openvpn(ciphername));
+      if (!kt->cipher)
+	{
+	  msg (M_FATAL, "Cipher %s not supported", ciphername);
+	}
+
       kt->cipher_length = cipher_kt_key_size (kt->cipher);
       if (keysize > 0 && keysize <= MAX_CIPHER_KEY_LENGTH)
 	kt->cipher_length = keysize;

src/openvpn/crypto_backend.h +2 -1

index a699673..bf7d78c 100644
@@ -195,7 +195,8 @@ void cipher_des_encrypt_ecb (const unsigned char key[DES_KEY_LENGTH],
  * 			\c AES-128-CBC).
  *
  * @return		A statically allocated structure containing parameters
- * 			for the given cipher.
+ * 			for the given cipher, or NULL if no matching parameters
+ * 			were found.
  */
 const cipher_kt_t * cipher_kt_get (const char *ciphername);

src/openvpn/crypto_mbedtls.c +10 -5

index 92cba49..6ad5924 100644
@@ -384,13 +384,18 @@ cipher_kt_get (const char *ciphername)
   cipher = mbedtls_cipher_info_from_string(ciphername);
 
   if (NULL == cipher)
-    msg (M_FATAL, "Cipher algorithm '%s' not found", ciphername);
+    {
+      msg (D_LOW, "Cipher algorithm '%s' not found", ciphername);
+      return NULL;
+    }
 
   if (cipher->key_bitlen/8 > MAX_CIPHER_KEY_LENGTH)
-    msg (M_FATAL, "Cipher algorithm '%s' uses a default key size (%d bytes) which is larger than " PACKAGE_NAME "'s current maximum key size (%d bytes)",
-	 ciphername,
-	 cipher->key_bitlen/8,
-	 MAX_CIPHER_KEY_LENGTH);
+    {
+      msg (D_LOW, "Cipher algorithm '%s' uses a default key size (%d bytes) "
+	  "which is larger than " PACKAGE_NAME "'s current maximum key size "
+	  "(%d bytes)", ciphername, cipher->key_bitlen/8, MAX_CIPHER_KEY_LENGTH);
+      return NULL;
+    }
 
   return cipher;
 }

src/openvpn/crypto_openssl.c +12 -5

index 3484c77..1ea06bb 100644
@@ -504,13 +504,20 @@ cipher_kt_get (const char *ciphername)
   cipher = EVP_get_cipherbyname (ciphername);
 
   if (NULL == cipher)
-    crypto_msg (M_FATAL, "Cipher algorithm '%s' not found", ciphername);
+    {
+      crypto_msg (D_LOW, "Cipher algorithm '%s' not found", ciphername);
+      return NULL;
+    }
+
 
   if (EVP_CIPHER_key_length (cipher) > MAX_CIPHER_KEY_LENGTH)
-    msg (M_FATAL, "Cipher algorithm '%s' uses a default key size (%d bytes) which is larger than " PACKAGE_NAME "'s current maximum key size (%d bytes)",
-	 ciphername,
-	 EVP_CIPHER_key_length (cipher),
-	 MAX_CIPHER_KEY_LENGTH);
+    {
+      msg (D_LOW, "Cipher algorithm '%s' uses a default key size (%d bytes) "
+	  "which is larger than " PACKAGE_NAME "'s current maximum key size "
+	  "(%d bytes)", ciphername, EVP_CIPHER_key_length (cipher),
+	  MAX_CIPHER_KEY_LENGTH);
+      return NULL;
+    }
 
   return cipher;
 }

src/openvpn/options.c +5 -0

index 98ae930..1358237 100644
@@ -2208,6 +2208,11 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
 
 #ifdef ENABLE_CRYPTO
 
+  if (options->ncp_enabled && !tls_check_ncp_cipher_list(options->ncp_ciphers))
+    {
+      msg (M_USAGE, "NCP cipher list contains unsupported ciphers.");
+    }
+
   /*
    * Check consistency of replay options
    */

src/openvpn/ssl.c +21 -0

index caf3b1f..a078ce2 100644
@@ -3715,6 +3715,27 @@ tls_peer_info_ncp_ver(const char *peer_info)
   return 0;
 }
 
+bool
+tls_check_ncp_cipher_list(const char *list) {
+  char *tmp_ciphers = string_alloc (list, NULL);
+  char *tmp_ciphers_orig = tmp_ciphers;
+  bool unsupported_cipher_found = false;
+
+  const char *token = strtok (tmp_ciphers, ":");
+  while (token)
+    {
+      if (!cipher_kt_get (translate_cipher_name_from_openvpn (token)))
+	{
+	  msg (M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
+	  unsupported_cipher_found = true;
+	}
+      token = strtok (NULL, ":");
+    }
+  free (tmp_ciphers_orig);
+
+  return list && 0 < strlen(list) && !unsupported_cipher_found;
+}
+
 /*
  * Dump a human-readable rendition of an openvpn packet
  * into a garbage collectable string which is returned.

src/openvpn/ssl.h +9 -0

index de68b69..e6963a4 100644
@@ -503,6 +503,15 @@ tls_get_peer_info(const struct tls_multi *multi)
  */
 int tls_peer_info_ncp_ver(const char *peer_info);
 
+/**
+ * Check whether the ciphers in the supplied list are supported.
+ *
+ * @param list		Colon-separated list of ciphers
+ *
+ * @returns true iff all ciphers in list are supported.
+ */
+bool tls_check_ncp_cipher_list(const char *list);
+
 /*
  * inline functions
  */

Signature · 1 line

2.7.4

Selva Nair

2016-10-11 20:11 UTC #1 lore raw

Hi,

To nit-pick,

Quoted 8 lines · Steffan Karger

src/openvpn/ssl.c +7 -0

+bool
+tls_check_ncp_cipher_list(const char *list) {
+  char *tmp_ciphers = string_alloc (list, NULL);
+  char *tmp_ciphers_orig = tmp_ciphers;
+  bool unsupported_cipher_found = false;
+
+  const char *token = strtok (tmp_ciphers, ":");

src/openvpn/ssl.c tls_peer_info_ncp_ver(const char *peer_info) · line 3734

Quoted 10 lines

src/openvpn/ssl.c +10 -0

+  while (token)
+    {
+      if (!cipher_kt_get (translate_cipher_name_from_openvpn (token)))
+       {
+         msg (M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
+         unsupported_cipher_found = true;
+       }
+      token = strtok (NULL, ":");
+    }
+  free (tmp_ciphers_orig);

the extra "tmp_ciphers_orig" is redundant. Just "free (tmp_ciphers)" will do.

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+  return list && 0 < strlen(list) && !unsupported_cipher_found;

Checking for list == NULL here looks too late. If list is null, so is tmp_ciphers and then the first call to strtok will not reset the internal state of strtok(). The behaviour of strtok then depends on whether any previous use of it was was iterated to the end or not.

If list could be NULL, better check it and do an early return from the top.

Quoted 2 lines

+

Selva

Steffan Karger

author

2016-10-12 07:31 UTC #2 lore raw

Quoted 1 line · Selva Nair

To nit-pick,

Yes, please!

Quoted 14 lines · Steffan Karger

Quoted 13 lines

src/openvpn/ssl.c +12 -0

+bool
+tls_check_ncp_cipher_list(const char *list) {
+  char *tmp_ciphers = string_alloc (list, NULL);
+  char *tmp_ciphers_orig = tmp_ciphers;
+  bool unsupported_cipher_found = false;
+
+  const char *token = strtok (tmp_ciphers, ":");
+  while (token)
+    {
+      if (!cipher_kt_get (translate_cipher_name_from_openvpn (token)))
+       {
+         msg (M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);

Quoted 8 lines

src/openvpn/ssl.c tls_peer_info_ncp_ver(const char *peer_info) · line 3734
Quoted 5 lines
src/openvpn/ssl.c +5 -0
+         unsupported_cipher_found = true;
+       }
+      token = strtok (NULL, ":");
+    }
+  free (tmp_ciphers_orig);
the extra "tmp_ciphers_orig" is redundant. Just "free (tmp_ciphers)" will do.

Indeed, will fix.

Quoted 9 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+
+  return list && 0 < strlen(list) && !unsupported_cipher_found;
Checking for list == NULL here looks too late. If list is null, so is tmp_ciphers and then the first call to strtok will not reset the internal state of strtok(). The behaviour of strtok then depends on whether any previous use of it was was iterated to the end or not.
If list could be NULL, better check it and do an early return from the top.

list can not be NULL, but that should indeed be ASSERT()ed at the top, instead of 'checked' here. Will fix too.

I'll send a v2 in a minute.

-Steffan

Steffan Karger

author

2016-10-12 07:32 UTC #3 lore raw

PATCH v2 1/1 Check --ncp-ciphers list on startup

This should fix trac #737.

v2: improve tls_check_ncp_cipher_list() with Selva's review suggestions.

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

---
 src/openvpn/crypto.c         |  5 +++++
 src/openvpn/crypto_backend.h |  3 ++-
 src/openvpn/crypto_mbedtls.c | 15 ++++++++++-----
 src/openvpn/crypto_openssl.c | 17 ++++++++++++-----
 src/openvpn/options.c        |  5 +++++
 src/openvpn/ssl.c            | 22 ++++++++++++++++++++++
 src/openvpn/ssl.h            |  9 +++++++++
 7 files changed, 65 insertions(+), 11 deletions(-)

src/openvpn/crypto.c +5 -0

index 4ea0082..3dd4a9e 100644
@@ -766,6 +766,11 @@ init_key_type (struct key_type *kt, const char *ciphername,
   if (strcmp (ciphername, "none") != 0)
     {
       kt->cipher = cipher_kt_get (translate_cipher_name_from_openvpn(ciphername));
+      if (!kt->cipher)
+	{
+	  msg (M_FATAL, "Cipher %s not supported", ciphername);
+	}
+
       kt->cipher_length = cipher_kt_key_size (kt->cipher);
       if (keysize > 0 && keysize <= MAX_CIPHER_KEY_LENGTH)
 	kt->cipher_length = keysize;

src/openvpn/crypto_backend.h +2 -1

index a699673..bf7d78c 100644
@@ -195,7 +195,8 @@ void cipher_des_encrypt_ecb (const unsigned char key[DES_KEY_LENGTH],
  * 			\c AES-128-CBC).
  *
  * @return		A statically allocated structure containing parameters
- * 			for the given cipher.
+ * 			for the given cipher, or NULL if no matching parameters
+ * 			were found.
  */
 const cipher_kt_t * cipher_kt_get (const char *ciphername);

src/openvpn/crypto_mbedtls.c +10 -5

index 92cba49..6ad5924 100644
@@ -384,13 +384,18 @@ cipher_kt_get (const char *ciphername)
   cipher = mbedtls_cipher_info_from_string(ciphername);
 
   if (NULL == cipher)
-    msg (M_FATAL, "Cipher algorithm '%s' not found", ciphername);
+    {
+      msg (D_LOW, "Cipher algorithm '%s' not found", ciphername);
+      return NULL;
+    }
 
   if (cipher->key_bitlen/8 > MAX_CIPHER_KEY_LENGTH)
-    msg (M_FATAL, "Cipher algorithm '%s' uses a default key size (%d bytes) which is larger than " PACKAGE_NAME "'s current maximum key size (%d bytes)",
-	 ciphername,
-	 cipher->key_bitlen/8,
-	 MAX_CIPHER_KEY_LENGTH);
+    {
+      msg (D_LOW, "Cipher algorithm '%s' uses a default key size (%d bytes) "
+	  "which is larger than " PACKAGE_NAME "'s current maximum key size "
+	  "(%d bytes)", ciphername, cipher->key_bitlen/8, MAX_CIPHER_KEY_LENGTH);
+      return NULL;
+    }
 
   return cipher;
 }

src/openvpn/crypto_openssl.c +12 -5

index 3484c77..1ea06bb 100644
@@ -504,13 +504,20 @@ cipher_kt_get (const char *ciphername)
   cipher = EVP_get_cipherbyname (ciphername);
 
   if (NULL == cipher)
-    crypto_msg (M_FATAL, "Cipher algorithm '%s' not found", ciphername);
+    {
+      crypto_msg (D_LOW, "Cipher algorithm '%s' not found", ciphername);
+      return NULL;
+    }
+
 
   if (EVP_CIPHER_key_length (cipher) > MAX_CIPHER_KEY_LENGTH)
-    msg (M_FATAL, "Cipher algorithm '%s' uses a default key size (%d bytes) which is larger than " PACKAGE_NAME "'s current maximum key size (%d bytes)",
-	 ciphername,
-	 EVP_CIPHER_key_length (cipher),
-	 MAX_CIPHER_KEY_LENGTH);
+    {
+      msg (D_LOW, "Cipher algorithm '%s' uses a default key size (%d bytes) "
+	  "which is larger than " PACKAGE_NAME "'s current maximum key size "
+	  "(%d bytes)", ciphername, EVP_CIPHER_key_length (cipher),
+	  MAX_CIPHER_KEY_LENGTH);
+      return NULL;
+    }
 
   return cipher;
 }

src/openvpn/options.c +5 -0

index d7b4948..b52c6ff 100644
@@ -2208,6 +2208,11 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
 
 #ifdef ENABLE_CRYPTO
 
+  if (options->ncp_enabled && !tls_check_ncp_cipher_list(options->ncp_ciphers))
+    {
+      msg (M_USAGE, "NCP cipher list contains unsupported ciphers.");
+    }
+
   /*
    * Check consistency of replay options
    */

src/openvpn/ssl.c +22 -0

index 420164e..c7cf78d 100644
@@ -3714,6 +3714,28 @@ tls_peer_info_ncp_ver(const char *peer_info)
   return 0;
 }
 
+bool
+tls_check_ncp_cipher_list(const char *list) {
+  bool unsupported_cipher_found = false;
+
+  ASSERT (list);
+
+  char * const tmp_ciphers = string_alloc (list, NULL);
+  const char *token = strtok (tmp_ciphers, ":");
+  while (token)
+    {
+      if (!cipher_kt_get (translate_cipher_name_from_openvpn (token)))
+	{
+	  msg (M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
+	  unsupported_cipher_found = true;
+	}
+      token = strtok (NULL, ":");
+    }
+  free (tmp_ciphers);
+
+  return 0 < strlen(list) && !unsupported_cipher_found;
+}
+
 /*
  * Dump a human-readable rendition of an openvpn packet
  * into a garbage collectable string which is returned.

src/openvpn/ssl.h +9 -0

index de68b69..e6963a4 100644
@@ -503,6 +503,15 @@ tls_get_peer_info(const struct tls_multi *multi)
  */
 int tls_peer_info_ncp_ver(const char *peer_info);
 
+/**
+ * Check whether the ciphers in the supplied list are supported.
+ *
+ * @param list		Colon-separated list of ciphers
+ *
+ * @returns true iff all ciphers in list are supported.
+ */
+bool tls_check_ncp_cipher_list(const char *list);
+
 /*
  * inline functions
  */

Signature · 1 line

2.7.4

Selva Nair

2016-10-13 04:31 UTC #4 lore raw

Quoted 14 lines · Steffan Karger

Currently, if --ncp-ciphers contains an invalid cipher, OpenVPN will only error out when that cipher is selected by negotiation. That's not very friendly to the user, so check the list on startup, and give a clear error message immediately.
This patches changes the cipher_kt_get() to let the caller decide what action to take if no valid cipher was found. This enables us to print all invalid ciphers in the list, instead of just the first invalid cipher.
This should fix trac #737.
v2: improve tls_check_ncp_cipher_list() with Selva's review suggestions.
Signed-off-by Steffan Karger <[email protected]...>

Quoted 9 lines

---
 src/openvpn/crypto.c         |  5 +++++
 src/openvpn/crypto_backend.h |  3 ++-
 src/openvpn/crypto_mbedtls.c | 15 ++++++++++-----
 src/openvpn/crypto_openssl.c | 17 ++++++++++++-----
 src/openvpn/options.c        |  5 +++++
 src/openvpn/ssl.c            | 22 ++++++++++++++++++++++
 src/openvpn/ssl.h            |  9 +++++++++
 7 files changed, 65 insertions(+), 11 deletions(-)

Looks good now. ACK.

Selva

David Sommerseth

2016-10-13 16:17 UTC #5 lore raw

In reply to Steffan Karger

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Your patch has been applied to the master branch

commit dc4fa3c4656b92aff3f26d4134c509410add142e

Author: Steffan Karger
Date:   Wed Oct 12 09:32:49 2016 +0200

     Check --ncp-ciphers list on startup

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg12671.html
     Trac: #737
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJX/7OcAAoJEIbPlEyWcf3ym58P/0egwsxlOL4tSdSLc7Lwo41r kxacwdshA+1hlJMRXeNoXh+5hv7JktTnisN9Skk/kvbpHXLNpKUPklbWgup0tLq+ GIQAeHcAeuocDYwIqzH2mdqqqqlZV7txCq6oES25+XGQY04FTLJIXANF0W37WEL8 XjUzeFGbO7hkqA+f6h68n6kFGrWyVJ0VmAsUt8PWtoPEo3gPXymzferqF0GAKmTs bwINagyNLr6FtvwoJVAQt4k69Mkp+9idManUabeXRKb1IyVrepHLpDKC0ESvv7Vs oEOri+A6QzljCuc/rFrL0XMjCRg2E0+ocABbawtGwEUCJivj5YM6mnr37WrdFvgN PWPt0xToKLe/e01KjoJH6dsgma38M822LbwcMxUszygzURGKBLZm/d4Tl/4IBUms nru3w4YiHNDa9P/F2d3XUTaah6FmFsRowatszuA1D0zfj5IOqS3nkb85ph9lRDvb nPwizqVzZD+aELPXNKZgU46cXFeI8WD2q3ORS4jOH6aUjsvg2OPwakMI/UnXNoFi bHotkerLP+AizHNPrkYm2T1agEJHN5sv82FmqLEEw9NGiZKA3zRvndLrBTb5LNJY CwBGaJ+coDKdpGCmgs2naySOYnVfJgxZY0rElsYmIAQ8C8s0vjqwATqnvYtceIZ9 tojHc8KC4m01MEF7/Neo
=t9Hn
-----END PGP SIGNATURE-----