diff mbox series

[Openvpn-devel,v2] Warn about insecure ciphers also in init_key_type

Message ID 20200219112153.13013-1-arne@rfc2549.org
State Accepted
Headers
Series [Openvpn-devel,v2] Warn about insecure ciphers also in init_key_type |

Commit Message

Arne Schwabe Feb. 19, 2020, 12:21 a.m. UTC 
  With modern Clients and server initialising the crypto cipher later
and not when reading in the config, most users never the warning when
having selected BF-CBC in the configuration.

This patch adds the logic to print out warning to init_key_type.

Main reason for this patch is a personal experience with someone who was
strictly against putting 'cipher' into a config file because he did not
like hardcoding a cipher and "OpenVPN will do AES-GCM anyway" and thinks
that it is better to not have it in configuration even after told by me
that 15 year defaults might not be good anymore.

Patch V2: rebase on master, fix minor style issues

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/crypto.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

Comments

Steffan Karger Feb. 19, 2020, 4:13 a.m. UTC | #1 
On 19-02-2020 12:21, Arne Schwabe wrote:
> With modern Clients and server initialising the crypto cipher later
> and not when reading in the config, most users never the warning when
> having selected BF-CBC in the configuration.
> 
> This patch adds the logic to print out warning to init_key_type.
> 
> Main reason for this patch is a personal experience with someone who was
> strictly against putting 'cipher' into a config file because he did not
> like hardcoding a cipher and "OpenVPN will do AES-GCM anyway" and thinks
> that it is better to not have it in configuration even after told by me
> that 15 year defaults might not be good anymore.
> 
> Patch V2: rebase on master, fix minor style issues
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/crypto.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 65e789ed..453cb20a 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -736,6 +736,17 @@ crypto_max_overhead(void)
>             +max_int(OPENVPN_MAX_HMAC_SIZE, OPENVPN_AEAD_TAG_LENGTH);
>  }
>  
> +static void warn_insecure_key_type(const char* ciphername, const cipher_kt_t *cipher)
> +{
> +    if (cipher_kt_insecure(cipher))
> +    {
> +        msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than 128"
> +                    " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
> +                    "using a --cipher with a larger block size (e.g. AES-256-CBC).",
> +            ciphername, cipher_kt_block_size(cipher)*8);
> +    }
> +}
> +
>  /*
>   * Build a struct key_type.
>   */
> @@ -779,6 +790,10 @@ init_key_type(struct key_type *kt, const char *ciphername,
>          {
>              msg(M_FATAL, "Cipher '%s' not allowed: block size too big.", ciphername);
>          }
> +        if (warn)
> +        {
> +            warn_insecure_key_type(ciphername, kt->cipher);
> +        }
>      }
>      else
>      {
> @@ -831,9 +846,10 @@ init_key_ctx(struct key_ctx *ctx, const struct key *key,
>          cipher_ctx_init(ctx->cipher, key->cipher, kt->cipher_length,
>                          kt->cipher, enc);
>  
> +        const char* ciphername = translate_cipher_name_to_openvpn(cipher_kt_name(kt->cipher));
>          msg(D_HANDSHAKE, "%s: Cipher '%s' initialized with %d bit key",
>              prefix,
> -            translate_cipher_name_to_openvpn(cipher_kt_name(kt->cipher)),
> +            ciphername,
>              kt->cipher_length *8);
>  
>          dmsg(D_SHOW_KEYS, "%s: CIPHER KEY: %s", prefix,
> @@ -841,13 +857,7 @@ init_key_ctx(struct key_ctx *ctx, const struct key *key,
>          dmsg(D_CRYPTO_DEBUG, "%s: CIPHER block_size=%d iv_size=%d",
>               prefix, cipher_kt_block_size(kt->cipher),
>               cipher_kt_iv_size(kt->cipher));
> -        if (cipher_kt_insecure(kt->cipher))
> -        {
> -            msg(M_WARN, "WARNING: INSECURE cipher with block size less than 128"
> -                " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
> -                "using a --cipher with a larger block size (e.g. AES-256-CBC).",
> -                cipher_kt_block_size(kt->cipher)*8);
> -        }
> +        warn_insecure_key_type(ciphername, kt->cipher);
>      }
>      if (kt->digest && kt->hmac_length > 0)
>      {
> 

Tested, works as advertised.

Acked-by: Steffan Karger <steffan.karger@foxcrypto.com>

-Steffan
diff mbox series

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 65e789ed..453cb20a 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -736,6 +736,17 @@  crypto_max_overhead(void)
            +max_int(OPENVPN_MAX_HMAC_SIZE, OPENVPN_AEAD_TAG_LENGTH);
 }
 
+static void warn_insecure_key_type(const char* ciphername, const cipher_kt_t *cipher)
+{
+    if (cipher_kt_insecure(cipher))
+    {
+        msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than 128"
+                    " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
+                    "using a --cipher with a larger block size (e.g. AES-256-CBC).",
+            ciphername, cipher_kt_block_size(cipher)*8);
+    }
+}
+
 /*
  * Build a struct key_type.
  */
@@ -779,6 +790,10 @@  init_key_type(struct key_type *kt, const char *ciphername,
         {
             msg(M_FATAL, "Cipher '%s' not allowed: block size too big.", ciphername);
         }
+        if (warn)
+        {
+            warn_insecure_key_type(ciphername, kt->cipher);
+        }
     }
     else
     {
@@ -831,9 +846,10 @@  init_key_ctx(struct key_ctx *ctx, const struct key *key,
         cipher_ctx_init(ctx->cipher, key->cipher, kt->cipher_length,
                         kt->cipher, enc);
 
+        const char* ciphername = translate_cipher_name_to_openvpn(cipher_kt_name(kt->cipher));
         msg(D_HANDSHAKE, "%s: Cipher '%s' initialized with %d bit key",
             prefix,
-            translate_cipher_name_to_openvpn(cipher_kt_name(kt->cipher)),
+            ciphername,
             kt->cipher_length *8);
 
         dmsg(D_SHOW_KEYS, "%s: CIPHER KEY: %s", prefix,
@@ -841,13 +857,7 @@  init_key_ctx(struct key_ctx *ctx, const struct key *key,
         dmsg(D_CRYPTO_DEBUG, "%s: CIPHER block_size=%d iv_size=%d",
              prefix, cipher_kt_block_size(kt->cipher),
              cipher_kt_iv_size(kt->cipher));
-        if (cipher_kt_insecure(kt->cipher))
-        {
-            msg(M_WARN, "WARNING: INSECURE cipher with block size less than 128"
-                " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
-                "using a --cipher with a larger block size (e.g. AES-256-CBC).",
-                cipher_kt_block_size(kt->cipher)*8);
-        }
+        warn_insecure_key_type(ciphername, kt->cipher);
     }
     if (kt->digest && kt->hmac_length > 0)
     {