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.

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

Signed-off-by: Steffan Karger <steffan@karger.me>
Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <1476257569-16301-1-git-send-email-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg12671.html
Trac: #737
Signed-off-by: David Sommerseth <davids@openvpn.net>

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 4ea0082c423b92aa14c2397167a69fe5b29cc236..3dd4a9ebc0550740c6484a5e96e6f0cd1d7447d5 100644 (file)
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -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;

diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index a69967376cc865c41781124c367adba5ff97f4b3..bf7d78c4e41f4d241a2d40dee03d485b968530fd 100644 (file)
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -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);
 

diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 92cba492ffdf67f5662277b90cc073716edc58bf..6ad5924782ad241dfc74c637e0b5f6af0c92a816 100644 (file)
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -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;
 }

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 3484c77112e13c74be63ba4a40200b0152f63e54..1ea06bba1f3c4ac9aa665f42d5708522fddf7d2e 100644 (file)
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -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;
 }

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 2998f06ed62f782563b25053b5058ac8d5c9442f..1ed14b0da99826c578045d3c5d3eb0ad6ef82b59 100644 (file)
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -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
    */

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 420164e7726bc1723790b8f3ca88a4f4406e4f07..c7cf78df968ca3f0fd828a6fe08658b62cfd057d 100644 (file)
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -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.

diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index de68b69e93f7c9a64291e89dde1dcf80cce5cae4..e6963a4c2741854bf90b050b6fceba95ee53c5ff 100644 (file)
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -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
  */