Re: [Openvpn-devel] [PATCH v2] Check --ncp-ciphers list on startup

2016-10-12 Thread Selva Nair
On Wed, Oct 12, 2016 at 3:32 AM, Steffan Karger  wrote:

> 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 
> ---
>  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
--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2] Check --ncp-ciphers list on startup

2016-10-12 Thread 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 
---
 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(-)

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 4ea0082..3dd4a9e 100644
--- 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 a699673..bf7d78c 100644
--- 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 92cba49..6ad5924 100644
--- 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 3484c77..1ea06bb 100644
--- 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 d7b4948..b52c6ff 100644
--- 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 420164e..c7cf78d 100644
-