| Message ID | 20181007073413.9778-1-steffan@karger.me |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Add support for CHACHA20-POLY1305 in the data channel | expand |
Hi, On 07/10/18 15:34, Steffan Karger wrote: > 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 > > 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! > > Signed-off-by: Steffan Karger <steffan@karger.me> > --- > 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(-) > > diff --git a/Changes.rst b/Changes.rst > index a6090cf5..70ce2e10 100644 > --- a/Changes.rst > +++ b/Changes.rst > @@ -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 > ========================== > > diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c > index c8d95f3b..6d34acd7 100644 > --- a/src/openvpn/crypto.c > +++ b/src/openvpn/crypto.c > @@ -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 " > diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h > index f917c8d7..38b2c175 100644 > --- a/src/openvpn/crypto_backend.h > +++ b/src/openvpn/crypto_backend.h > @@ -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. > * > diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c > index 82f4e574..a5a096b1 100644 > --- a/src/openvpn/crypto_mbedtls.c > +++ b/src/openvpn/crypto_mbedtls.c > @@ -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 Since you are touching the line above (you moved it), please put spaces around '/'. > +#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? > + || 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) ditto > + || cipher_kt_mode(cipher) == MBEDTLS_MODE_CHACHAPOLY > +#endif > + ); > } > > > diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c > index 9ec2048d..0004be9e 100644 > --- a/src/openvpn/crypto_openssl.c > +++ b/src/openvpn/crypto_openssl.c > @@ -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; > } > > /* > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index 4257c33d..315303b0 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -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,
Hi, On 07/10/18 21:28, Antonio Quartulli wrote: >> +#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,
Hi, On 07/10/18 21:28, Antonio Quartulli wrote: > Hi, > > On 07/10/18 15:34, Steffan Karger wrote: >> 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 > >> >> 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! > >> >> Signed-off-by: Steffan Karger <steffan@karger.me> >> --- >> 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(-) >> >> diff --git a/Changes.rst b/Changes.rst >> index a6090cf5..70ce2e10 100644 >> --- a/Changes.rst >> +++ b/Changes.rst >> @@ -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 >> ========================== >> >> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c >> index c8d95f3b..6d34acd7 100644 >> --- a/src/openvpn/crypto.c >> +++ b/src/openvpn/crypto.c >> @@ -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 " >> diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h >> index f917c8d7..38b2c175 100644 >> --- a/src/openvpn/crypto_backend.h >> +++ b/src/openvpn/crypto_backend.h >> @@ -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. >> * >> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c >> index 82f4e574..a5a096b1 100644 >> --- a/src/openvpn/crypto_mbedtls.c >> +++ b/src/openvpn/crypto_mbedtls.c >> @@ -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 > > Since you are touching the line above (you moved it), please put spaces > around '/'. > >> +#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? > >> + || 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) > > ditto > >> + || cipher_kt_mode(cipher) == MBEDTLS_MODE_CHACHAPOLY >> +#endif >> + ); >> } >> >> >> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c >> index 9ec2048d..0004be9e 100644 >> --- a/src/openvpn/crypto_openssl.c >> +++ b/src/openvpn/crypto_openssl.c >> @@ -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; >> } >> >> /* >> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c >> index 4257c33d..315303b0 100644 >> --- a/src/openvpn/ssl.c >> +++ b/src/openvpn/ssl.c >> @@ -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. > 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,
Hi, Thank for review. I'l be getting back on your other comments later, for now just: On 07-10-18 15:36, Antonio Quartulli wrote: > On 07/10/18 21:28, Antonio Quartulli wrote: >>> +#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) 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
Hi, On 08/10/18 02:13, Steffan Karger wrote: > On 07-10-18 15:36, Antonio Quartulli wrote: >> On 07/10/18 21:28, Antonio Quartulli wrote: >>>> +#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) > > 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,
Hi, On 07-10-18 22:18, Antonio Quartulli wrote: > 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
diff --git a/Changes.rst b/Changes.rst index a6090cf5..70ce2e10 100644 --- a/Changes.rst +++ b/Changes.rst @@ -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 ========================== diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c index c8d95f3b..6d34acd7 100644 --- a/src/openvpn/crypto.c +++ b/src/openvpn/crypto.c @@ -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 " diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h index f917c8d7..38b2c175 100644 --- a/src/openvpn/crypto_backend.h +++ b/src/openvpn/crypto_backend.h @@ -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. * diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c index 82f4e574..a5a096b1 100644 --- a/src/openvpn/crypto_mbedtls.c +++ b/src/openvpn/crypto_mbedtls.c @@ -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 + ); } diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c index 9ec2048d..0004be9e 100644 --- a/src/openvpn/crypto_openssl.c +++ b/src/openvpn/crypto_openssl.c @@ -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; } /* diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 4257c33d..315303b0 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -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 */ {
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 <steffan@karger.me> --- 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(-)