Add support for CHACHA20-POLY1305 in the data channel

PATCH Superseded Steffan Karger 3 participants 15 messages Oct 7, 2018 – Oct 10, 2018

Share & tools

Steffan Karger

author

2018-10-07 07:34 UTC #0 lore raw

PATCH 1/1 Add support for CHACHA20-POLY1305 in the data channel

We explicitly only supported GCM as a valid AEAD mode, change that to also allow ChaCha20-Poly1305 as an AEAD cipher. That works nicely with our new (GCM) data channel format, because is has the same 96-bit IV.

Note that we need some tricks to not treat the cipher as insecure, because we used to only look at the block size of a cipher to determine if find a cipher insecure. But ChaCha20-Poly1305 is a stream cipher, which essentially has a 'block size' of 1 byte and is reported as such. So, special-case this cipher to be in the list of secure ciphers.

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

---
 Changes.rst                  | 10 ++++++++++
 src/openvpn/crypto.c         |  2 +-
 src/openvpn/crypto_backend.h |  5 +++++
 src/openvpn/crypto_mbedtls.c | 21 ++++++++++++++++++---
 src/openvpn/crypto_openssl.c | 33 ++++++++++++++++++++++++++++-----
 src/openvpn/ssl.c            |  2 +-
 6 files changed, 63 insertions(+), 10 deletions(-)

Changes.rst +10 -0

index a6090cf5..70ce2e10 100644
@@ -1,3 +1,13 @@
+Overview of changes in 2.5
+==========================
+
+New features
+------------
+ChaCha20-Poly1305 cipher support
+    Added support for using the ChaCha20-Poly1305 cipher in the OpenVPN data
+    channel.
+
+
 Overview of changes in 2.4
 ==========================

src/openvpn/crypto.c +1 -1

index c8d95f3b..6d34acd7 100644
@@ -841,7 +841,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_block_size(kt->cipher) < 128/8)
+        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 "

src/openvpn/crypto_backend.h +5 -0

index f917c8d7..38b2c175 100644
@@ -284,6 +284,11 @@ int cipher_kt_block_size(const cipher_kt_t *cipher_kt);
  */
 int cipher_kt_tag_size(const cipher_kt_t *cipher_kt);
 
+/**
+ * Returns true if we consider this cipher to be insecure.
+ */
+bool cipher_kt_insecure(const cipher_kt_t *cipher);
+
 /**
  * Returns the mode that the cipher runs in.
  *

src/openvpn/crypto_mbedtls.c +18 -3

index 82f4e574..a5a096b1 100644
@@ -51,6 +51,7 @@
 #include <mbedtls/cipher.h>
 #include <mbedtls/havege.h>
 #include <mbedtls/pem.h>
+#include <mbedtls/version.h>
 
 #include <mbedtls/entropy.h>

@@ -175,7 +176,7 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && cipher_kt_block_size(info) >= 128/8)
+        if (info && !cipher_kt_insecure(info))
         {
             print_cipher(info);
         }

@@ -188,7 +189,7 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && cipher_kt_block_size(info) < 128/8)
+        if (info && cipher_kt_insecure(info))
         {
             print_cipher(info);
         }

@@ -550,6 +551,16 @@ cipher_kt_tag_size(const mbedtls_cipher_info_t *cipher_kt)
     return 0;
 }
 
+bool
+cipher_kt_insecure(const mbedtls_cipher_info_t *cipher_kt)
+{
+    return !(cipher_kt_block_size(cipher_kt) >= 128/8
+#if defined(MBEDTLS_CHACHAPOLY_C) && (MBEDTLS_VERSION_NUMBER >= 0x020C0000)
+             || cipher_kt->type == MBEDTLS_CIPHER_CHACHA20_POLY1305
+#endif
+             );
+}
+
 int
 cipher_kt_mode(const mbedtls_cipher_info_t *cipher_kt)
 {

@@ -573,7 +584,11 @@ cipher_kt_mode_ofb_cfb(const cipher_kt_t *cipher)
 bool
 cipher_kt_mode_aead(const cipher_kt_t *cipher)
 {
-    return cipher && cipher_kt_mode(cipher) == OPENVPN_MODE_GCM;
+    return cipher && (cipher_kt_mode(cipher) == OPENVPN_MODE_GCM
+#if defined(MBEDTLS_CHACHAPOLY_C) && (MBEDTLS_VERSION_NUMBER >= 0x020C0000)
+                      || cipher_kt_mode(cipher) == MBEDTLS_MODE_CHACHAPOLY
+#endif
+                      );
 }

src/openvpn/crypto_openssl.c +28 -5

index 9ec2048d..0004be9e 100644
@@ -245,6 +245,7 @@ const cipher_name_pair cipher_name_translation_table[] = {
     { "AES-128-GCM", "id-aes128-GCM" },
     { "AES-192-GCM", "id-aes192-GCM" },
     { "AES-256-GCM", "id-aes256-GCM" },
+    { "CHACHA20-POLY1305", "ChaCha20-Poly1305" },
 };
 const size_t cipher_name_translation_table_count =
     sizeof(cipher_name_translation_table) / sizeof(*cipher_name_translation_table);

@@ -321,7 +322,7 @@ show_available_ciphers(void)
     qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
 
     for (i = 0; i < num_ciphers; i++) {
-        if (cipher_kt_block_size(cipher_list[i]) >= 128/8)
+        if (!cipher_kt_insecure(cipher_list[i]))
         {
             print_cipher(cipher_list[i]);
         }

@@ -330,7 +331,7 @@ show_available_ciphers(void)
     printf("\nThe following ciphers have a block size of less than 128 bits, \n"
            "and are therefore deprecated.  Do not use unless you have to.\n\n");
     for (i = 0; i < num_ciphers; i++) {
-        if (cipher_kt_block_size(cipher_list[i]) < 128/8)
+        if (cipher_kt_insecure(cipher_list[i]))
         {
             print_cipher(cipher_list[i]);
         }

@@ -686,6 +687,16 @@ cipher_kt_tag_size(const EVP_CIPHER *cipher_kt)
     }
 }
 
+bool
+cipher_kt_insecure(const EVP_CIPHER *cipher)
+{
+    return !(cipher_kt_block_size(cipher) >= 128/8
+#ifdef NID_chacha20_poly1305
+             || EVP_CIPHER_nid(cipher) == NID_chacha20_poly1305
+#endif
+            );
+}
+
 int
 cipher_kt_mode(const EVP_CIPHER *cipher_kt)
 {

@@ -720,10 +731,22 @@ bool
 cipher_kt_mode_aead(const cipher_kt_t *cipher)
 {
 #ifdef HAVE_AEAD_CIPHER_MODES
-    return cipher && (cipher_kt_mode(cipher) == OPENVPN_MODE_GCM);
-#else
-    return false;
+    if (cipher)
+    {
+        switch (EVP_CIPHER_nid(cipher))
+        {
+        case NID_aes_128_gcm:
+        case NID_aes_192_gcm:
+        case NID_aes_256_gcm:
+#ifdef NID_chacha20_poly1305
+        case NID_chacha20_poly1305:
 #endif
+            return true;
+        }
+    }
+#endif
+
+    return false;
 }
 
 /*

src/openvpn/ssl.c +1 -1

index 4257c33d..315303b0 100644
@@ -294,7 +294,7 @@ tls_get_cipher_name_pair(const char *cipher_name, size_t len)
 static void
 tls_limit_reneg_bytes(const cipher_kt_t *cipher, int *reneg_bytes)
 {
-    if (cipher && (cipher_kt_block_size(cipher) < 128/8))
+    if (cipher && cipher_kt_insecure(cipher))
     {
         if (*reneg_bytes == -1) /* Not user-specified */
         {

Signature · 1 line

2.17.1

Antonio Quartulli

2018-10-07 13:28 UTC #1 lore raw

Hi,

Quoted 3 lines · Steffan Karger

We explicitly only supported GCM as a valid AEAD mode, change that to also allow ChaCha20-Poly1305 as an AEAD cipher. That works nicely with our new (GCM) data channel format, because is has the same 96-bit IV.

                                     ^^ it

Quoted 6 lines

Note that we need some tricks to not treat the cipher as insecure, because we used to only look at the block size of a cipher to determine if find a cipher insecure. But ChaCha20-Poly1305 is a stream cipher, which essentially has a 'block size' of 1 byte and is reported as such. So, special-case this cipher to be in the list of secure ciphers.

I am so excited we are finally including ChaCha20-Poly1305 support! :) Thanks!

Quoted 89 lines

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

---
 Changes.rst                  | 10 ++++++++++
 src/openvpn/crypto.c         |  2 +-
 src/openvpn/crypto_backend.h |  5 +++++
 src/openvpn/crypto_mbedtls.c | 21 ++++++++++++++++++---
 src/openvpn/crypto_openssl.c | 33 ++++++++++++++++++++++++++++-----
 src/openvpn/ssl.c            |  2 +-
 6 files changed, 63 insertions(+), 10 deletions(-)

Changes.rst +10 -0

index a6090cf5..70ce2e10 100644
@@ -1,3 +1,13 @@
+Overview of changes in 2.5
+==========================
+
+New features
+------------
+ChaCha20-Poly1305 cipher support
+    Added support for using the ChaCha20-Poly1305 cipher in the OpenVPN data
+    channel.
+
+
 Overview of changes in 2.4
 ==========================

src/openvpn/crypto.c +1 -1

index c8d95f3b..6d34acd7 100644
@@ -841,7 +841,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_block_size(kt->cipher) < 128/8)
+        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 "

src/openvpn/crypto_backend.h +5 -0

index f917c8d7..38b2c175 100644
@@ -284,6 +284,11 @@ int cipher_kt_block_size(const cipher_kt_t *cipher_kt);
  */
 int cipher_kt_tag_size(const cipher_kt_t *cipher_kt);
 
+/**
+ * Returns true if we consider this cipher to be insecure.
+ */
+bool cipher_kt_insecure(const cipher_kt_t *cipher);
+
 /**
  * Returns the mode that the cipher runs in.
  *

src/openvpn/crypto_mbedtls.c +3 -2

index 82f4e574..a5a096b1 100644
@@ -51,6 +51,7 @@
 #include <mbedtls/cipher.h>
 #include <mbedtls/havege.h>
 #include <mbedtls/pem.h>
+#include <mbedtls/version.h>
 
 #include <mbedtls/entropy.h>

@@ -175,7 +176,7 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && cipher_kt_block_size(info) >= 128/8)
+        if (info && !cipher_kt_insecure(info))
         {
             print_cipher(info);
         }

@@ -188,7 +189,7 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && cipher_kt_block_size(info) < 128/8)
+        if (info && cipher_kt_insecure(info))
         {
             print_cipher(info);
         }

src/openvpn/crypto_mbedtls.c cipher_kt_tag_size(const mbedtls_cipher_info_t *cipher_kt) · line 557

Quoted 7 lines

src/openvpn/crypto_mbedtls.c +4 -0

@@ -550,6 +551,16 @@ cipher_kt_tag_size(const mbedtls_cipher_info_t *cipher_kt)
     return 0;
 }
 
+bool
+cipher_kt_insecure(const mbedtls_cipher_info_t *cipher_kt)
+{
+    return !(cipher_kt_block_size(cipher_kt) >= 128/8

Since you are touching the line above (you moved it), please put spaces around '/'.

src/openvpn/crypto_mbedtls.c cipher_kt_tag_size(const mbedtls_cipher_info_t *cipher_kt) · line 558

Quoted 1 line

src/openvpn/crypto_mbedtls.c +1 -0

+#if defined(MBEDTLS_CHACHAPOLY_C) && (MBEDTLS_VERSION_NUMBER >= 0x020C0000)

Why do we need the dual condition? Isn't MBEDTLS_CHACHAPOLY_C enough to know mbedTLS has what we need? Or you feel like we have to demand a specific mbedTLS version when using ChaCha20?

Quoted 9 lines

src/openvpn/crypto_mbedtls.c +5 -0

+             || cipher_kt->type == MBEDTLS_CIPHER_CHACHA20_POLY1305
+#endif
+             );
+}
+
 int
 cipher_kt_mode(const mbedtls_cipher_info_t *cipher_kt)
 {

src/openvpn/crypto_mbedtls.c cipher_kt_mode_ofb_cfb(const cipher_kt_t *cipher) · line 588

Quoted 6 lines

src/openvpn/crypto_mbedtls.c +2 -1

@@ -573,7 +584,11 @@ cipher_kt_mode_ofb_cfb(const cipher_kt_t *cipher)
 bool
 cipher_kt_mode_aead(const cipher_kt_t *cipher)
 {
-    return cipher && cipher_kt_mode(cipher) == OPENVPN_MODE_GCM;
+    return cipher && (cipher_kt_mode(cipher) == OPENVPN_MODE_GCM
+#if defined(MBEDTLS_CHACHAPOLY_C) && (MBEDTLS_VERSION_NUMBER >= 0x020C0000)

ditto

Quoted 85 lines

src/openvpn/crypto_mbedtls.c +3 -0

+                      || cipher_kt_mode(cipher) == MBEDTLS_MODE_CHACHAPOLY
+#endif
+                      );
 }

src/openvpn/crypto_openssl.c +28 -5

index 9ec2048d..0004be9e 100644
@@ -245,6 +245,7 @@ const cipher_name_pair cipher_name_translation_table[] = {
     { "AES-128-GCM", "id-aes128-GCM" },
     { "AES-192-GCM", "id-aes192-GCM" },
     { "AES-256-GCM", "id-aes256-GCM" },
+    { "CHACHA20-POLY1305", "ChaCha20-Poly1305" },
 };
 const size_t cipher_name_translation_table_count =
     sizeof(cipher_name_translation_table) / sizeof(*cipher_name_translation_table);

@@ -321,7 +322,7 @@ show_available_ciphers(void)
     qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
 
     for (i = 0; i < num_ciphers; i++) {
-        if (cipher_kt_block_size(cipher_list[i]) >= 128/8)
+        if (!cipher_kt_insecure(cipher_list[i]))
         {
             print_cipher(cipher_list[i]);
         }

@@ -330,7 +331,7 @@ show_available_ciphers(void)
     printf("\nThe following ciphers have a block size of less than 128 bits, \n"
            "and are therefore deprecated.  Do not use unless you have to.\n\n");
     for (i = 0; i < num_ciphers; i++) {
-        if (cipher_kt_block_size(cipher_list[i]) < 128/8)
+        if (cipher_kt_insecure(cipher_list[i]))
         {
             print_cipher(cipher_list[i]);
         }

@@ -686,6 +687,16 @@ cipher_kt_tag_size(const EVP_CIPHER *cipher_kt)
     }
 }
 
+bool
+cipher_kt_insecure(const EVP_CIPHER *cipher)
+{
+    return !(cipher_kt_block_size(cipher) >= 128/8
+#ifdef NID_chacha20_poly1305
+             || EVP_CIPHER_nid(cipher) == NID_chacha20_poly1305
+#endif
+            );
+}
+
 int
 cipher_kt_mode(const EVP_CIPHER *cipher_kt)
 {

@@ -720,10 +731,22 @@ bool
 cipher_kt_mode_aead(const cipher_kt_t *cipher)
 {
 #ifdef HAVE_AEAD_CIPHER_MODES
-    return cipher && (cipher_kt_mode(cipher) == OPENVPN_MODE_GCM);
-#else
-    return false;
+    if (cipher)
+    {
+        switch (EVP_CIPHER_nid(cipher))
+        {
+        case NID_aes_128_gcm:
+        case NID_aes_192_gcm:
+        case NID_aes_256_gcm:
+#ifdef NID_chacha20_poly1305
+        case NID_chacha20_poly1305:
 #endif
+            return true;
+        }
+    }
+#endif
+
+    return false;
 }
 
 /*

src/openvpn/ssl.c +0 -0

index 4257c33d..315303b0 100644

src/openvpn/ssl.c tls_get_cipher_name_pair(const char *cipher_name, size_t len) · line 300

Quoted 8 lines

src/openvpn/ssl.c +1 -1

@@ -294,7 +294,7 @@ tls_get_cipher_name_pair(const char *cipher_name, size_t len)
 static void
 tls_limit_reneg_bytes(const cipher_kt_t *cipher, int *reneg_bytes)
 {
-    if (cipher && (cipher_kt_block_size(cipher) < 128/8))
+    if (cipher && cipher_kt_insecure(cipher))
     {
         if (*reneg_bytes == -1) /* Not user-specified */
         {

The code looks good and I like having a opaque function now doing the "cipher security check" in one place only.

I was just wondering if you want to change the text "8 bit block" to "stream cipher" in --show-ciphers, in order to make it more clear to the casual reader. What do you think?

Regards,

Signature · 1 line

Antonio Quartulli

Antonio Quartulli

2018-10-07 13:36 UTC #2 lore raw

Hi,

Quoted 5 lines · Antonio Quartulli

Quoted 1 line
+#if defined(MBEDTLS_CHACHAPOLY_C) && (MBEDTLS_VERSION_NUMBER >= 0x020C0000)
Why do we need the dual condition? Isn't MBEDTLS_CHACHAPOLY_C enough to know mbedTLS has what we need? Or you feel like we have to demand a specific mbedTLS version when using ChaCha20?

Just to reinforce my argument: ChaCha20-Poly1305 was introduced with ce66d5e8e (in mbedTLS upstream) and a quick search shows:

mbedtls $ git tag --contains ce66d5e8e
mbedtls-2.12.0
mbedtls-2.13.0
mbedtls-2.13.1

Given that 2.12 is 0x020C0000, I am fairly sure there is no reason to have both conditions in the if guard.
But I might still be missing something.

(p.s. I am arguing simply because I want to avoid more version-based ifdef that may become new nightmares)

Cheers,

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2018-10-07 18:13 UTC #3 lore raw

Hi,

Thank for review. I'l be getting back on your other comments later, for now just:

Quoted 15 lines · Antonio Quartulli

Quoted 5 lines · Antonio Quartulli
Quoted 1 line
+#if defined(MBEDTLS_CHACHAPOLY_C) && (MBEDTLS_VERSION_NUMBER >= 0x020C0000)
Why do we need the dual condition? Isn't MBEDTLS_CHACHAPOLY_C enough to know mbedTLS has what we need? Or you feel like we have to demand a specific mbedTLS version when using ChaCha20?
Just to reinforce my argument: ChaCha20-Poly1305 was introduced with ce66d5e8e (in mbedTLS upstream) and a quick search shows:
mbedtls $ git tag --contains ce66d5e8e
mbedtls-2.12.0
mbedtls-2.13.0
mbedtls-2.13.1

Quoted 6 lines

Given that 2.12 is 0x020C0000, I am fairly sure there is no reason to have both conditions in the if guard.
But I might still be missing something.
(p.s. I am arguing simply because I want to avoid more version-based ifdef that may become new nightmares)

Which is a good cause, but that unfortunately won't work here. That is because the actual implementation was introduced in dca3a5d8, which is already in 2.9.0. So we do need the version. Unless you've got another trick up your sleeve to avoid it, what I would love to hear.

-Steffan

Antonio Quartulli

2018-10-07 20:18 UTC #4 lore raw

Hi,

Quoted 20 lines · Steffan Karger

Quoted 19 lines · Antonio Quartulli
Quoted 5 lines · Antonio Quartulli
Quoted 1 line
+#if defined(MBEDTLS_CHACHAPOLY_C) && (MBEDTLS_VERSION_NUMBER >= 0x020C0000)
Why do we need the dual condition? Isn't MBEDTLS_CHACHAPOLY_C enough to know mbedTLS has what we need? Or you feel like we have to demand a specific mbedTLS version when using ChaCha20?
Just to reinforce my argument: ChaCha20-Poly1305 was introduced with ce66d5e8e (in mbedTLS upstream) and a quick search shows:
mbedtls $ git tag --contains ce66d5e8e
mbedtls-2.12.0
mbedtls-2.13.0
mbedtls-2.13.1
Given that 2.12 is 0x020C0000, I am fairly sure there is no reason to have both conditions in the if guard.
But I might still be missing something.

Quoted 5 lines

Quoted 2 lines
(p.s. I am arguing simply because I want to avoid more version-based ifdef that may become new nightmares)
Which is a good cause, but that unfortunately won't work here. That is because the actual implementation was introduced in dca3a5d8,

[CUT]

mbedtls $ git tag --contains dca3a5d8
mbedtls-2.12.0
mbedtls-2.13.0
mbedtls-2.13.1

To be safe I checked out 2.9.0 and grepped the define:

mbedtls $ git checkout mbedtls-2.9.0
Note: checking out 'mbedtls-2.9.0'.

You are in 'detached HEAD' state. You can look around, make experimental changes and commit them, and you can discard any commits you make in this state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may do so (now or later) by using -b with the checkout command again. Example:

git checkout -b <new-branch-name>

HEAD is now at 070e35647 Merge remote-tracking branch 'upstream-restricted/pr/481' into development-restricted mbedtls $ grep -rn CHACHAPOLY_C
mbedtls $

am I missing something?
2.12.0 is the first release where my grep command prints something.

Cheers,

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2018-10-07 21:26 UTC #5 lore raw

Hi,

Quoted 2 lines · Antonio Quartulli

am I missing something?
2.12.0 is the first release where my grep command prints something.

No, you're absolutely right. I was misinterpreting my gitk. I'll tackle this in v2.

-Steffan

Antonio Quartulli

2018-10-07 14:18 UTC #6 lore raw

In reply to Antonio Quartulli

Hi,

Quoted 230 lines · Antonio Quartulli

Hi,

Quoted 3 lines · Steffan Karger

We explicitly only supported GCM as a valid AEAD mode, change that to also allow ChaCha20-Poly1305 as an AEAD cipher. That works nicely with our new (GCM) data channel format, because is has the same 96-bit IV.

                                     ^^ it

Quoted 6 lines

Note that we need some tricks to not treat the cipher as insecure, because we used to only look at the block size of a cipher to determine if find a cipher insecure. But ChaCha20-Poly1305 is a stream cipher, which essentially has a 'block size' of 1 byte and is reported as such. So, special-case this cipher to be in the list of secure ciphers.

I am so excited we are finally including ChaCha20-Poly1305 support! :) Thanks!

Quoted 89 lines

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

---
 Changes.rst                  | 10 ++++++++++
 src/openvpn/crypto.c         |  2 +-
 src/openvpn/crypto_backend.h |  5 +++++
 src/openvpn/crypto_mbedtls.c | 21 ++++++++++++++++++---
 src/openvpn/crypto_openssl.c | 33 ++++++++++++++++++++++++++++-----
 src/openvpn/ssl.c            |  2 +-
 6 files changed, 63 insertions(+), 10 deletions(-)

Changes.rst +10 -0

index a6090cf5..70ce2e10 100644
@@ -1,3 +1,13 @@
+Overview of changes in 2.5
+==========================
+
+New features
+------------
+ChaCha20-Poly1305 cipher support
+    Added support for using the ChaCha20-Poly1305 cipher in the OpenVPN data
+    channel.
+
+
 Overview of changes in 2.4
 ==========================

src/openvpn/crypto.c +1 -1

index c8d95f3b..6d34acd7 100644
@@ -841,7 +841,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_block_size(kt->cipher) < 128/8)
+        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 "

src/openvpn/crypto_backend.h +5 -0

index f917c8d7..38b2c175 100644
@@ -284,6 +284,11 @@ int cipher_kt_block_size(const cipher_kt_t *cipher_kt);
  */
 int cipher_kt_tag_size(const cipher_kt_t *cipher_kt);
 
+/**
+ * Returns true if we consider this cipher to be insecure.
+ */
+bool cipher_kt_insecure(const cipher_kt_t *cipher);
+
 /**
  * Returns the mode that the cipher runs in.
  *

src/openvpn/crypto_mbedtls.c +3 -2

index 82f4e574..a5a096b1 100644
@@ -51,6 +51,7 @@
 #include <mbedtls/cipher.h>
 #include <mbedtls/havege.h>
 #include <mbedtls/pem.h>
+#include <mbedtls/version.h>
 
 #include <mbedtls/entropy.h>

@@ -175,7 +176,7 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && cipher_kt_block_size(info) >= 128/8)
+        if (info && !cipher_kt_insecure(info))
         {
             print_cipher(info);
         }

@@ -188,7 +189,7 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && cipher_kt_block_size(info) < 128/8)
+        if (info && cipher_kt_insecure(info))
         {
             print_cipher(info);
         }

src/openvpn/crypto_mbedtls.c cipher_kt_tag_size(const mbedtls_cipher_info_t *cipher_kt) · line 557

Quoted 7 lines

src/openvpn/crypto_mbedtls.c +4 -0

@@ -550,6 +551,16 @@ cipher_kt_tag_size(const mbedtls_cipher_info_t *cipher_kt)
     return 0;
 }
 
+bool
+cipher_kt_insecure(const mbedtls_cipher_info_t *cipher_kt)
+{
+    return !(cipher_kt_block_size(cipher_kt) >= 128/8

Since you are touching the line above (you moved it), please put spaces around '/'.

src/openvpn/crypto_mbedtls.c cipher_kt_tag_size(const mbedtls_cipher_info_t *cipher_kt) · line 558

Quoted 1 line

src/openvpn/crypto_mbedtls.c +1 -0

+#if defined(MBEDTLS_CHACHAPOLY_C) && (MBEDTLS_VERSION_NUMBER >= 0x020C0000)

Why do we need the dual condition? Isn't MBEDTLS_CHACHAPOLY_C enough to know mbedTLS has what we need? Or you feel like we have to demand a specific mbedTLS version when using ChaCha20?

Quoted 9 lines

src/openvpn/crypto_mbedtls.c +5 -0

+             || cipher_kt->type == MBEDTLS_CIPHER_CHACHA20_POLY1305
+#endif
+             );
+}
+
 int
 cipher_kt_mode(const mbedtls_cipher_info_t *cipher_kt)
 {

src/openvpn/crypto_mbedtls.c cipher_kt_mode_ofb_cfb(const cipher_kt_t *cipher) · line 588

Quoted 6 lines

src/openvpn/crypto_mbedtls.c +2 -1

@@ -573,7 +584,11 @@ cipher_kt_mode_ofb_cfb(const cipher_kt_t *cipher)
 bool
 cipher_kt_mode_aead(const cipher_kt_t *cipher)
 {
-    return cipher && cipher_kt_mode(cipher) == OPENVPN_MODE_GCM;
+    return cipher && (cipher_kt_mode(cipher) == OPENVPN_MODE_GCM
+#if defined(MBEDTLS_CHACHAPOLY_C) && (MBEDTLS_VERSION_NUMBER >= 0x020C0000)

ditto

Quoted 88 lines

src/openvpn/crypto_mbedtls.c +3 -0

+                      || cipher_kt_mode(cipher) == MBEDTLS_MODE_CHACHAPOLY
+#endif
+                      );
 }

src/openvpn/crypto_openssl.c +28 -5

index 9ec2048d..0004be9e 100644
@@ -245,6 +245,7 @@ const cipher_name_pair cipher_name_translation_table[] = {
     { "AES-128-GCM", "id-aes128-GCM" },
     { "AES-192-GCM", "id-aes192-GCM" },
     { "AES-256-GCM", "id-aes256-GCM" },
+    { "CHACHA20-POLY1305", "ChaCha20-Poly1305" },
 };
 const size_t cipher_name_translation_table_count =
     sizeof(cipher_name_translation_table) / sizeof(*cipher_name_translation_table);

@@ -321,7 +322,7 @@ show_available_ciphers(void)
     qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
 
     for (i = 0; i < num_ciphers; i++) {
-        if (cipher_kt_block_size(cipher_list[i]) >= 128/8)
+        if (!cipher_kt_insecure(cipher_list[i]))
         {
             print_cipher(cipher_list[i]);
         }

@@ -330,7 +331,7 @@ show_available_ciphers(void)
     printf("\nThe following ciphers have a block size of less than 128 bits, \n"
            "and are therefore deprecated.  Do not use unless you have to.\n\n");
     for (i = 0; i < num_ciphers; i++) {
-        if (cipher_kt_block_size(cipher_list[i]) < 128/8)
+        if (cipher_kt_insecure(cipher_list[i]))
         {
             print_cipher(cipher_list[i]);
         }

@@ -686,6 +687,16 @@ cipher_kt_tag_size(const EVP_CIPHER *cipher_kt)
     }
 }
 
+bool
+cipher_kt_insecure(const EVP_CIPHER *cipher)
+{
+    return !(cipher_kt_block_size(cipher) >= 128/8
+#ifdef NID_chacha20_poly1305
+             || EVP_CIPHER_nid(cipher) == NID_chacha20_poly1305
+#endif
+            );
+}
+
 int
 cipher_kt_mode(const EVP_CIPHER *cipher_kt)
 {

@@ -720,10 +731,22 @@ bool
 cipher_kt_mode_aead(const cipher_kt_t *cipher)
 {
 #ifdef HAVE_AEAD_CIPHER_MODES
-    return cipher && (cipher_kt_mode(cipher) == OPENVPN_MODE_GCM);
-#else
-    return false;
+    if (cipher)
+    {
+        switch (EVP_CIPHER_nid(cipher))
+        {
+        case NID_aes_128_gcm:
+        case NID_aes_192_gcm:
+        case NID_aes_256_gcm:
+#ifdef NID_chacha20_poly1305
+        case NID_chacha20_poly1305:
 #endif
+            return true;
+        }
+    }
+#endif
+
+    return false;
 }
 
 /*

src/openvpn/ssl.c +0 -1

index 4257c33d..315303b0 100644
@@ -294,7 +294,7 @@ tls_get_cipher_name_pair(const char *cipher_name, size_t len)
 static void
 tls_limit_reneg_bytes(const cipher_kt_t *cipher, int *reneg_bytes)
 {
-    if (cipher && (cipher_kt_block_size(cipher) < 128/8))

Quoted 9 lines

src/openvpn/ssl.c tls_get_cipher_name_pair(const char *cipher_name, size_t len) · line 300
Quoted 5 lines
src/openvpn/ssl.c +1 -0
+    if (cipher && cipher_kt_insecure(cipher))
     {
         if (*reneg_bytes == -1) /* Not user-specified */
         {
The code looks good and I like having a opaque function now doing the "cipher security check" in one place only.

It also just works as expected:

Sun Oct 7 22:14:48 2018 us=264551 127.0.0.1 Outgoing Data Channel:
Cipher 'CHACHA20-POLY1305' initialized with 256 bit key
Sun Oct 7 22:14:48 2018 us=264557 127.0.0.1 Incoming Data Channel:
Cipher 'CHACHA20-POLY1305' initialized with 256 bit key
Sun Oct 7 22:14:48 2018 us=264676 127.0.0.1 Control Channel: TLSv1.2, cipher TLS-ECDHE-RSA-WITH-CHACHA20-POLY1305-SHA256, 2048 bit key

Tested between a VM and my laptop.
Data inside the tunnel was flowing as expected.

Cheers,

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2018-10-07 22:30 UTC #7 lore raw

PATCH v2 1/2 Add support for CHACHA20-POLY1305 in the data channel

In reply to Antonio Quartulli

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

---
v2: code style fixes, remove unneeded version check

 Changes.rst                  | 10 ++++++++++
 src/openvpn/crypto.c         |  2 +-
 src/openvpn/crypto_backend.h |  5 +++++
 src/openvpn/crypto_mbedtls.c | 20 +++++++++++++++++---
 src/openvpn/crypto_openssl.c | 33 ++++++++++++++++++++++++++++-----
 src/openvpn/ssl.c            |  2 +-
 6 files changed, 62 insertions(+), 10 deletions(-)

Changes.rst +10 -0

index a6090cf5..70ce2e10 100644
@@ -1,3 +1,13 @@
+Overview of changes in 2.5
+==========================
+
+New features
+------------
+ChaCha20-Poly1305 cipher support
+    Added support for using the ChaCha20-Poly1305 cipher in the OpenVPN data
+    channel.
+
+
 Overview of changes in 2.4
 ==========================

src/openvpn/crypto.c +1 -1

index c8d95f3b..6d34acd7 100644
@@ -841,7 +841,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_block_size(kt->cipher) < 128/8)
+        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 "

src/openvpn/crypto_backend.h +5 -0

index f917c8d7..38b2c175 100644
@@ -284,6 +284,11 @@ int cipher_kt_block_size(const cipher_kt_t *cipher_kt);
  */
 int cipher_kt_tag_size(const cipher_kt_t *cipher_kt);
 
+/**
+ * Returns true if we consider this cipher to be insecure.
+ */
+bool cipher_kt_insecure(const cipher_kt_t *cipher);
+
 /**
  * Returns the mode that the cipher runs in.
  *

src/openvpn/crypto_mbedtls.c +17 -3

index 82f4e574..0c39eccc 100644
@@ -175,7 +175,7 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && cipher_kt_block_size(info) >= 128/8)
+        if (info && !cipher_kt_insecure(info))
         {
             print_cipher(info);
         }

@@ -188,7 +188,7 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && cipher_kt_block_size(info) < 128/8)
+        if (info && cipher_kt_insecure(info))
         {
             print_cipher(info);
         }

@@ -550,6 +550,16 @@ cipher_kt_tag_size(const mbedtls_cipher_info_t *cipher_kt)
     return 0;
 }
 
+bool
+cipher_kt_insecure(const mbedtls_cipher_info_t *cipher_kt)
+{
+    return !(cipher_kt_block_size(cipher_kt) >= 128 / 8
+#ifdef MBEDTLS_CHACHAPOLY_C
+             || cipher_kt->type == MBEDTLS_CIPHER_CHACHA20_POLY1305
+#endif
+             );
+}
+
 int
 cipher_kt_mode(const mbedtls_cipher_info_t *cipher_kt)
 {

@@ -573,7 +583,11 @@ cipher_kt_mode_ofb_cfb(const cipher_kt_t *cipher)
 bool
 cipher_kt_mode_aead(const cipher_kt_t *cipher)
 {
-    return cipher && cipher_kt_mode(cipher) == OPENVPN_MODE_GCM;
+    return cipher && (cipher_kt_mode(cipher) == OPENVPN_MODE_GCM
+#ifdef MBEDTLS_CHACHAPOLY_C
+                      || cipher_kt_mode(cipher) == MBEDTLS_MODE_CHACHAPOLY
+#endif
+                      );
 }

src/openvpn/crypto_openssl.c +28 -5

index 9ec2048d..1c0fae86 100644
@@ -245,6 +245,7 @@ const cipher_name_pair cipher_name_translation_table[] = {
     { "AES-128-GCM", "id-aes128-GCM" },
     { "AES-192-GCM", "id-aes192-GCM" },
     { "AES-256-GCM", "id-aes256-GCM" },
+    { "CHACHA20-POLY1305", "ChaCha20-Poly1305" },
 };
 const size_t cipher_name_translation_table_count =
     sizeof(cipher_name_translation_table) / sizeof(*cipher_name_translation_table);

@@ -321,7 +322,7 @@ show_available_ciphers(void)
     qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
 
     for (i = 0; i < num_ciphers; i++) {
-        if (cipher_kt_block_size(cipher_list[i]) >= 128/8)
+        if (!cipher_kt_insecure(cipher_list[i]))
         {
             print_cipher(cipher_list[i]);
         }

@@ -330,7 +331,7 @@ show_available_ciphers(void)
     printf("\nThe following ciphers have a block size of less than 128 bits, \n"
            "and are therefore deprecated.  Do not use unless you have to.\n\n");
     for (i = 0; i < num_ciphers; i++) {
-        if (cipher_kt_block_size(cipher_list[i]) < 128/8)
+        if (cipher_kt_insecure(cipher_list[i]))
         {
             print_cipher(cipher_list[i]);
         }

@@ -686,6 +687,16 @@ cipher_kt_tag_size(const EVP_CIPHER *cipher_kt)
     }
 }
 
+bool
+cipher_kt_insecure(const EVP_CIPHER *cipher)
+{
+    return !(cipher_kt_block_size(cipher) >= 128 / 8
+#ifdef NID_chacha20_poly1305
+             || EVP_CIPHER_nid(cipher) == NID_chacha20_poly1305
+#endif
+            );
+}
+
 int
 cipher_kt_mode(const EVP_CIPHER *cipher_kt)
 {

@@ -720,10 +731,22 @@ bool
 cipher_kt_mode_aead(const cipher_kt_t *cipher)
 {
 #ifdef HAVE_AEAD_CIPHER_MODES
-    return cipher && (cipher_kt_mode(cipher) == OPENVPN_MODE_GCM);
-#else
-    return false;
+    if (cipher)
+    {
+        switch (EVP_CIPHER_nid(cipher))
+        {
+        case NID_aes_128_gcm:
+        case NID_aes_192_gcm:
+        case NID_aes_256_gcm:
+#ifdef NID_chacha20_poly1305
+        case NID_chacha20_poly1305:
 #endif
+            return true;
+        }
+    }
+#endif
+
+    return false;
 }
 
 /*

src/openvpn/ssl.c +1 -1

index 4257c33d..315303b0 100644
@@ -294,7 +294,7 @@ tls_get_cipher_name_pair(const char *cipher_name, size_t len)
 static void
 tls_limit_reneg_bytes(const cipher_kt_t *cipher, int *reneg_bytes)
 {
-    if (cipher && (cipher_kt_block_size(cipher) < 128/8))
+    if (cipher && cipher_kt_insecure(cipher))
     {
         if (*reneg_bytes == -1) /* Not user-specified */
         {

Signature · 1 line

2.17.1

Steffan Karger

author

2018-10-07 22:30 UTC #8 lore raw

PATCH v2 2/2 List ChaCha20-Poly1305 as stream cipher

As Antonio pointed out, "8-bit block cipher" is a bit funny. So teach print_cipher() to print such cipher as "stream cipher".

Because I didn't want to write the same code twice, I decided to merge the two print_cipher() implementations into one shared function. That should make it easier to keep both backends consistent.

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

---
v2: introduce this patch

 src/openvpn/crypto.c         | 27 +++++++++++++++++++++++++++
 src/openvpn/crypto.h         |  3 +++
 src/openvpn/crypto_mbedtls.c | 23 ++---------------------
 src/openvpn/crypto_mbedtls.h |  4 ++++
 src/openvpn/crypto_openssl.c | 15 ---------------
 src/openvpn/crypto_openssl.h |  4 ++++
 6 files changed, 40 insertions(+), 36 deletions(-)

src/openvpn/crypto.c +27 -0

index 6d34acd7..e81399b7 100644
@@ -1769,6 +1769,33 @@ get_random(void)
     return l;
 }
 
+void
+print_cipher(const cipher_kt_t *cipher)
+{
+    const char *var_key_size = cipher_kt_var_key_size(cipher) ?
+        " by default" : "";
+
+    printf("%s  (%d bit key%s, ",
+           translate_cipher_name_to_openvpn(cipher_kt_name(cipher)),
+           cipher_kt_key_size(cipher) * 8, var_key_size);
+
+    if (cipher_kt_block_size(cipher) == 1)
+    {
+        printf("stream cipher");
+    }
+    else
+    {
+        printf("%d bit block", cipher_kt_block_size(cipher) * 8);
+    }
+
+    if (!cipher_kt_mode_cbc(cipher))
+    {
+        printf(", TLS client/server mode only");
+    }
+
+    printf(")\n");
+}
+
 static const cipher_name_pair *
 get_cipher_name_pair(const char *cipher_name)
 {

src/openvpn/crypto.h +3 -0

index f4b3dca3..cf87cb49 100644
@@ -465,6 +465,9 @@ void prng_uninit(void);
 /* an analogue to the random() function, but use prng_bytes */
 long int get_random(void);
 
+/** Print a cipher list entry */
+void print_cipher(const cipher_kt_t *cipher);
+
 void test_crypto(struct crypto_options *co, struct frame *f);

src/openvpn/crypto_mbedtls.c +2 ~~-21~~

index 0c39eccc..54ac1893 100644
@@ -140,26 +140,6 @@ const cipher_name_pair cipher_name_translation_table[] = {
 const size_t cipher_name_translation_table_count =
     sizeof(cipher_name_translation_table) / sizeof(*cipher_name_translation_table);
 
-static void
-print_cipher(const cipher_kt_t *info)
-{
-    if (info && (cipher_kt_mode_cbc(info)
-#ifdef HAVE_AEAD_CIPHER_MODES
-                 || cipher_kt_mode_aead(info)
-#endif
-                 ))
-    {
-        const char *ssl_only = cipher_kt_mode_cbc(info) ?
-                               "" : ", TLS client/server mode only";
-        const char *var_key_size = info->flags & MBEDTLS_CIPHER_VARIABLE_KEY_LEN ?
-                                   " by default" : "";
-
-        printf("%s  (%d bit key%s, %d bit block%s)\n",
-               cipher_kt_name(info), cipher_kt_key_size(info) * 8, var_key_size,
-               cipher_kt_block_size(info) * 8, ssl_only);
-    }
-}
-
 void
 show_available_ciphers(void)
 {

@@ -175,7 +155,8 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && !cipher_kt_insecure(info))
+        if (info && !cipher_kt_insecure(info)
+            && (cipher_kt_mode_aead(info) || cipher_kt_mode_cbc(info)))
         {
             print_cipher(info);
         }

src/openvpn/crypto_mbedtls.h +4 -0

index 452b06ed..81b542bc 100644
@@ -146,5 +146,9 @@ mbed_log_func_line_lite(unsigned int flags, int errval,
 #define mbed_ok(errval) \
     mbed_log_func_line_lite(D_CRYPT_ERRORS, errval, __func__, __LINE__)
 
+static inline bool cipher_kt_var_key_size(const cipher_kt_t *cipher)
+{
+    return cipher->flags & MBEDTLS_CIPHER_VARIABLE_KEY_LEN;
+}
 
 #endif /* CRYPTO_MBEDTLS_H_ */

src/openvpn/crypto_openssl.c +0 ~~-15~~

index 1c0fae86..7989127b 100644
@@ -265,21 +265,6 @@ cipher_name_cmp(const void *a, const void *b)
     return strcmp(cipher_name_a, cipher_name_b);
 }
 
-static void
-print_cipher(const EVP_CIPHER *cipher)
-{
-    const char *var_key_size =
-        (EVP_CIPHER_flags(cipher) & EVP_CIPH_VARIABLE_LENGTH) ?
-        " by default" : "";
-    const char *ssl_only = cipher_kt_mode_cbc(cipher) ?
-                           "" : ", TLS client/server mode only";
-
-    printf("%s  (%d bit key%s, %d bit block%s)\n",
-           translate_cipher_name_to_openvpn(EVP_CIPHER_name(cipher)),
-           EVP_CIPHER_key_length(cipher) * 8, var_key_size,
-           cipher_kt_block_size(cipher) * 8, ssl_only);
-}
-
 void
 show_available_ciphers(void)
 {

src/openvpn/crypto_openssl.h +4 -0

index 0a413705..1ea3e858 100644
@@ -101,5 +101,9 @@ void crypto_print_openssl_errors(const unsigned int flags);
         msg((flags), __VA_ARGS__); \
     } while (false)
 
+static inline bool cipher_kt_var_key_size(const cipher_kt_t *cipher)
+{
+    return EVP_CIPHER_flags(cipher) & EVP_CIPH_VARIABLE_LENGTH;
+}
 
 #endif /* CRYPTO_OPENSSL_H_ */

Signature · 1 line

2.17.1

Antonio Quartulli

2018-10-08 09:18 UTC #9 lore raw

Re: [Openvpn-devel] [PATCH v2 2/2] List ChaCha20-Poly1305 as stream cipher

Hi,

see below:

On 08/10/18 06:30, Steffan Karger wrote:
[CUT]

Quoted 32 lines

src/openvpn/crypto_mbedtls.c +0 ~~-20~~

index 0c39eccc..54ac1893 100644
@@ -140,26 +140,6 @@ const cipher_name_pair cipher_name_translation_table[] = {
 const size_t cipher_name_translation_table_count =
     sizeof(cipher_name_translation_table) / sizeof(*cipher_name_translation_table);
 
-static void
-print_cipher(const cipher_kt_t *info)
-{
-    if (info && (cipher_kt_mode_cbc(info)
-#ifdef HAVE_AEAD_CIPHER_MODES
-                 || cipher_kt_mode_aead(info)
-#endif
-                 ))
-    {
-        const char *ssl_only = cipher_kt_mode_cbc(info) ?
-                               "" : ", TLS client/server mode only";
-        const char *var_key_size = info->flags & MBEDTLS_CIPHER_VARIABLE_KEY_LEN ?
-                                   " by default" : "";
-
-        printf("%s  (%d bit key%s, %d bit block%s)\n",
-               cipher_kt_name(info), cipher_kt_key_size(info) * 8, var_key_size,
-               cipher_kt_block_size(info) * 8, ssl_only);
-    }
-}
-
 void
 show_available_ciphers(void)
 {

src/openvpn/crypto_mbedtls.c show_available_ciphers(void) · line 161

Quoted 8 lines

src/openvpn/crypto_mbedtls.c +2 -1

@@ -175,7 +155,8 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && !cipher_kt_insecure(info))
+        if (info && !cipher_kt_insecure(info)
+            && (cipher_kt_mode_aead(info) || cipher_kt_mode_cbc(info)))
         {
             print_cipher(info);

apparently crypto.h is not included:

crypto_mbedtls.c: In function ‘show_available_ciphers’:
crypto_mbedtls.c:161:13: warning: implicit declaration of function ‘print_cipher’; did you mean ‘print_argv’? [-Wimplicit-function-declaration]

             print_cipher(info);
             ^~~~~~~~~~~~

When compiling with OpenSSL I couldn't see any warning.

Cheers,

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2018-10-09 20:43 UTC #10 lore raw

PATCH v3 2/2 List ChaCha20-Poly1305 as stream cipher

As Antonio pointed out, "8-bit block cipher" is a bit funny. So teach print_cipher() to print such cipher as "stream cipher".

Because I didn't want to write the same code twice, I decided to merge the two print_cipher() implementations into one shared function. That should make it easier to keep both backends consistent.

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

---
 src/openvpn/crypto.c         | 27 +++++++++++++++++++++++++++
 src/openvpn/crypto.h         |  3 +++
 src/openvpn/crypto_mbedtls.c | 24 +++---------------------
 src/openvpn/crypto_mbedtls.h |  4 ++++
 src/openvpn/crypto_openssl.c | 15 ---------------
 src/openvpn/crypto_openssl.h |  4 ++++
 6 files changed, 41 insertions(+), 36 deletions(-)

src/openvpn/crypto.c +27 -0

index 6d34acd7..e81399b7 100644
@@ -1769,6 +1769,33 @@ get_random(void)
     return l;
 }
 
+void
+print_cipher(const cipher_kt_t *cipher)
+{
+    const char *var_key_size = cipher_kt_var_key_size(cipher) ?
+        " by default" : "";
+
+    printf("%s  (%d bit key%s, ",
+           translate_cipher_name_to_openvpn(cipher_kt_name(cipher)),
+           cipher_kt_key_size(cipher) * 8, var_key_size);
+
+    if (cipher_kt_block_size(cipher) == 1)
+    {
+        printf("stream cipher");
+    }
+    else
+    {
+        printf("%d bit block", cipher_kt_block_size(cipher) * 8);
+    }
+
+    if (!cipher_kt_mode_cbc(cipher))
+    {
+        printf(", TLS client/server mode only");
+    }
+
+    printf(")\n");
+}
+
 static const cipher_name_pair *
 get_cipher_name_pair(const char *cipher_name)
 {

src/openvpn/crypto.h +3 -0

index f4b3dca3..cf87cb49 100644
@@ -465,6 +465,9 @@ void prng_uninit(void);
 /* an analogue to the random() function, but use prng_bytes */
 long int get_random(void);
 
+/** Print a cipher list entry */
+void print_cipher(const cipher_kt_t *cipher);
+
 void test_crypto(struct crypto_options *co, struct frame *f);

src/openvpn/crypto_mbedtls.c +3 ~~-21~~

index 0c39eccc..46c3c606 100644
@@ -39,6 +39,7 @@
 #include "errlevel.h"
 #include "basic.h"
 #include "buffer.h"
+#include "crypto.h"
 #include "integer.h"
 #include "crypto_backend.h"
 #include "otime.h"

@@ -140,26 +141,6 @@ const cipher_name_pair cipher_name_translation_table[] = {
 const size_t cipher_name_translation_table_count =
     sizeof(cipher_name_translation_table) / sizeof(*cipher_name_translation_table);
 
-static void
-print_cipher(const cipher_kt_t *info)
-{
-    if (info && (cipher_kt_mode_cbc(info)
-#ifdef HAVE_AEAD_CIPHER_MODES
-                 || cipher_kt_mode_aead(info)
-#endif
-                 ))
-    {
-        const char *ssl_only = cipher_kt_mode_cbc(info) ?
-                               "" : ", TLS client/server mode only";
-        const char *var_key_size = info->flags & MBEDTLS_CIPHER_VARIABLE_KEY_LEN ?
-                                   " by default" : "";
-
-        printf("%s  (%d bit key%s, %d bit block%s)\n",
-               cipher_kt_name(info), cipher_kt_key_size(info) * 8, var_key_size,
-               cipher_kt_block_size(info) * 8, ssl_only);
-    }
-}
-
 void
 show_available_ciphers(void)
 {

@@ -175,7 +156,8 @@ show_available_ciphers(void)
     while (*ciphers != 0)
     {
         const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
-        if (info && !cipher_kt_insecure(info))
+        if (info && !cipher_kt_insecure(info)
+            && (cipher_kt_mode_aead(info) || cipher_kt_mode_cbc(info)))
         {
             print_cipher(info);
         }

src/openvpn/crypto_mbedtls.h +4 -0

index 452b06ed..81b542bc 100644
@@ -146,5 +146,9 @@ mbed_log_func_line_lite(unsigned int flags, int errval,
 #define mbed_ok(errval) \
     mbed_log_func_line_lite(D_CRYPT_ERRORS, errval, __func__, __LINE__)
 
+static inline bool cipher_kt_var_key_size(const cipher_kt_t *cipher)
+{
+    return cipher->flags & MBEDTLS_CIPHER_VARIABLE_KEY_LEN;
+}
 
 #endif /* CRYPTO_MBEDTLS_H_ */

src/openvpn/crypto_openssl.c +0 ~~-15~~

index 1c0fae86..7989127b 100644
@@ -265,21 +265,6 @@ cipher_name_cmp(const void *a, const void *b)
     return strcmp(cipher_name_a, cipher_name_b);
 }
 
-static void
-print_cipher(const EVP_CIPHER *cipher)
-{
-    const char *var_key_size =
-        (EVP_CIPHER_flags(cipher) & EVP_CIPH_VARIABLE_LENGTH) ?
-        " by default" : "";
-    const char *ssl_only = cipher_kt_mode_cbc(cipher) ?
-                           "" : ", TLS client/server mode only";
-
-    printf("%s  (%d bit key%s, %d bit block%s)\n",
-           translate_cipher_name_to_openvpn(EVP_CIPHER_name(cipher)),
-           EVP_CIPHER_key_length(cipher) * 8, var_key_size,
-           cipher_kt_block_size(cipher) * 8, ssl_only);
-}
-
 void
 show_available_ciphers(void)
 {

src/openvpn/crypto_openssl.h +4 -0

index 0a413705..1ea3e858 100644
@@ -101,5 +101,9 @@ void crypto_print_openssl_errors(const unsigned int flags);
         msg((flags), __VA_ARGS__); \
     } while (false)
 
+static inline bool cipher_kt_var_key_size(const cipher_kt_t *cipher)
+{
+    return EVP_CIPHER_flags(cipher) & EVP_CIPH_VARIABLE_LENGTH;
+}
 
 #endif /* CRYPTO_OPENSSL_H_ */

Signature · 1 line

2.17.1

Antonio Quartulli

Acked-by on v3 2/2

2018-10-10 09:50 UTC #11 lore raw

Re: [Openvpn-devel] [PATCH v3 2/2] List ChaCha20-Poly1305 as stream cipher

Hi,

Quoted 8 lines · Steffan Karger

As Antonio pointed out, "8-bit block cipher" is a bit funny. So teach print_cipher() to print such cipher as "stream cipher".
Because I didn't want to write the same code twice, I decided to merge the two print_cipher() implementations into one shared function. That should make it easier to keep both backends consistent.
Signed-off-by Steffan Karger <[email protected]...>

Does what it says. Simple and plain. No warning anymore. And now we have a more meaningful text ;)

Acked-by Antonio Quartulli <[email protected]...>

Signature · 1 line

Antonio Quartulli

Gert Doering

2018-10-10 20:56 UTC #12 lore raw

[Openvpn-devel] [PATCH applied] Re: List ChaCha20-Poly1305 as stream cipher

In reply to Steffan Karger

Your patch has been applied to the master branch.

"Does what it says on the lid"... before:

CHACHA20-POLY1305 (256 bit key, 8 bit block, TLS client/server mode only)

.. after:

CHACHA20-POLY1305 (256 bit key, stream cipher, TLS client/server mode only)

commit 447997dd83400bffc05db65a91f659dc87b4a367

Author: Steffan Karger
Date:   Tue Oct 9 22:43:15 2018 +0200

     List ChaCha20-Poly1305 as stream cipher

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Antonio Quartulli <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17682.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Antonio Quartulli

Acked-by on v2 1/2

2018-10-08 09:11 UTC #13 lore raw

In reply to Steffan Karger

Hi,

thanks for fixing the ifdef condition!
Tested again and it works as expected between two clients when disabling NCP and setting CHACHA20-POLY1305 as cipher:

Mon Oct 8 17:11:36 2018 us=670345 127.0.0.1 Outgoing Data Channel:
Cipher 'CHACHA20-POLY1305' initialized with 256 bit key Mon Oct 8 17:11:36 2018 us=670351 127.0.0.1 Incoming Data Channel:
Cipher 'CHACHA20-POLY1305' initialized with 256 bit key

Quoted 10 lines · Steffan Karger

We explicitly only supported GCM as a valid AEAD mode, change that to also allow ChaCha20-Poly1305 as an AEAD cipher. That works nicely with our new (GCM) data channel format, because is has the same 96-bit IV.
Note that we need some tricks to not treat the cipher as insecure, because we used to only look at the block size of a cipher to determine if find a cipher insecure. But ChaCha20-Poly1305 is a stream cipher, which essentially has a 'block size' of 1 byte and is reported as such. So, special-case this cipher to be in the list of secure ciphers.

Quoted 3 lines

Signed-off-by Steffan Karger <[email protected]...>
---
v2: code style fixes, remove unneeded version check

Acked-by Antonio Quartulli <[email protected]...>

Cheers,

Signature · 1 line

Antonio Quartulli

Gert Doering

2018-10-08 14:49 UTC #14 lore raw

In reply to Steffan Karger

Your patch has been applied to the master branch.

I have done a cursory stare-at code and it matches the grumblings given on the way to dinner ("check not only for GCM but accept all save ciphers using this API", IIRC). My current test rig is too old to do a full client-server test easily, but at least the mbedTLS and the OpenSSL 1.1.0 build both claim support...

CHACHA20-POLY1305 (256 bit key, 8 bit block, TLS client/server mode only) CHACHA20-POLY1305 (256 bit key, 8 bit block, TLS client/server mode only)

.. and pass t_client tests both for OpenSSL and mbedTLS...

commit 6d0d0af9883b9ae266c0468f2739557a53e94b68

Author: Steffan Karger
Date:   Mon Oct 8 00:30:34 2018 +0200

     Add support for CHACHA20-POLY1305 in the data channel

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Antonio Quartulli <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17629.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering