Skip to content

Commit

Permalink
Check --ncp-ciphers list on startup
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
syzzer authored and dsommers committed Oct 13, 2016
1 parent 396d30c commit dc4fa3c
Show file tree
Hide file tree
Showing 7 changed files with 65 additions and 11 deletions.
5 changes: 5 additions & 0 deletions src/openvpn/crypto.c
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/openvpn/crypto_backend.h
Expand Up @@ -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);

Expand Down
15 changes: 10 additions & 5 deletions src/openvpn/crypto_mbedtls.c
Expand Up @@ -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;
}
Expand Down
17 changes: 12 additions & 5 deletions src/openvpn/crypto_openssl.c
Expand Up @@ -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;
}
Expand Down
5 changes: 5 additions & 0 deletions src/openvpn/options.c
Expand Up @@ -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
*/
Expand Down
22 changes: 22 additions & 0 deletions src/openvpn/ssl.c
Expand Up @@ -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.
Expand Down
9 changes: 9 additions & 0 deletions src/openvpn/ssl.h
Expand Up @@ -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
*/
Expand Down

0 comments on commit dc4fa3c

Please sign in to comment.