[Openvpn-devel,v2] Allow all GCM ciphers

Message ID 20210421123415.1942917-1-arne@rfc2549.org
State Accepted
Delegated to: Antonio Quartulli
Headers show
Series [Openvpn-devel,v2] Allow all GCM ciphers | expand

Commit Message

Arne Schwabe April 21, 2021, 2:34 a.m. UTC
OpenSSL also allows ARIA-GCM and that works well with our implementation
While the handpicked list was needed for earlier OpenSSL versions (and
is still needed for Chacha20-Poly1305), the API nowadays with OpenSSL
1.0.2 and 1.1.x works as expected.

Patch V2: Remove special cases for AES-GCM ciphers.

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

Comments

Antonio Quartulli April 22, 2021, 5:48 a.m. UTC | #1
Hi,

On 21/04/2021 14:34, Arne Schwabe wrote:
> OpenSSL also allows ARIA-GCM and that works well with our implementation
> While the handpicked list was needed for earlier OpenSSL versions (and
> is still needed for Chacha20-Poly1305), the API nowadays with OpenSSL
> 1.0.2 and 1.1.x works as expected.
> 
> Patch V2: Remove special cases for AES-GCM ciphers.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Testing AES-GCM on various OpenSSL versions just works as usual.
ARIA-GCM works as expected on 1.1.0 and 1.1.1 (1.0.2 does not seem to
support it).

CHACHA20-POLY1305 is also confirmed to still be working on OpenSSL 1.1.0
and 1.1.1.

LibreSSL compiles and works as expected (basic tests performed).

wolfSSL compiles but does not work anymore. I presume it does not
properly implement EVP_CIPHER_mode() as it returns 0 for AES-256-GCM.

IMHO this is one of those cases where we should merge this patch and
then wait for wolfSSL to fix the library.

So this patch gets my ACK, but I am not sure how we want to proceed.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering April 27, 2021, 1:37 a.m. UTC | #2
Looks good to me, and passes regular client side tests :-) - I have 
not actually tested the ARIA ciphers, but "they now show up in
--show-cipher mode".  Seems I'll need to add this to one of the
server test cases and see what explodes...

WolfSSL folks, please fix the incompatibility Antonio found :-)

Your patch has been applied to the master branch.

commit 0f168c9ac6c0be5145b5c19c6e79634edf158262
Author: Arne Schwabe
Date:   Wed Apr 21 14:34:15 2021 +0200

     Allow all GCM ciphers

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20210421123415.1942917-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22168.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index f8b36bf85..57731ed79 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -728,16 +728,17 @@  cipher_kt_mode_aead(const cipher_kt_t *cipher)
 {
     if (cipher)
     {
-        switch (EVP_CIPHER_nid(cipher))
+        if (EVP_CIPHER_mode(cipher) == OPENVPN_MODE_GCM)
         {
-            case NID_aes_128_gcm:
-            case NID_aes_192_gcm:
-            case NID_aes_256_gcm:
+            return true;
+        }
+
 #ifdef NID_chacha20_poly1305
-            case NID_chacha20_poly1305:
-#endif
-                return true;
+        if (EVP_CIPHER_nid(cipher) == NID_chacha20_poly1305)
+        {
+            return true;
         }
+#endif
     }
 
     return false;