[Openvpn-devel,v2] Fix client NCP OCC fallback when server and client cipher are identical

Message ID 20200830131440.10933-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,v2] Fix client NCP OCC fallback when server and client cipher are identical
Related show

Commit Message

Arne Schwabe Aug. 30, 2020, 1:14 p.m.
If we do not get a cipher pushed we call tls_poor_mans_ncp to determine
whether we can use the server's cipher. Inherited from OpenVPN
2.4's code we only did this check when the ciphers were different.
Since OpenVPN 2.5 does not assume that our cipher we report in OCC
(options->ciphername) is always a valid cipher we always need  to perform
this check.

V2: Only call tls_item_in_cipher_list if remote_cipher is non-null to
    avoid calling strcmp with NULL.

Reported-By: Rafael Gava <gava100@gmail.com>
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl_ncp.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Gert Doering Aug. 30, 2020, 8:16 p.m. | #1
Acked-by: Gert Doering <gert@greenie.muc.de>

Besides the visual check ("nice, one level of indentation removed!"),
I have also added 5 more tests to our test environment (running on
phillip, so available for all t_client instances):

 - git master client to 2.3 server, "--cipher bf-cbc"
   ("OCC based NCP, same cipher on client and server")
 - git master client to 2.3 server, "--cipher aes-gcm, --data-ciphers bf-cbc"
   ("OCC based NCP, different cipher on client and server, but server
   client allowed on client")
 - git master client to 2.3 "--enable-small" server, --data-ciphers-fallback
   ("no NCP of any sort, falling back to the fallback cipher")
 - git master client to 2.4
   ("2.4 style NCP, receiving pushed AES-256-GCM")
 - git master client to 2.4 with "--ncp-disable"
   (fallback to OCC based NCP with "same cipher")

Previous git master nicely ran into the problem Gava reported for both
"same cipher on the client as OCC-signalled from the server" cases
(unsurprisingly) and with the patch, all 5 test cases succeeded.

Plus all the cases where this code is running on the server side (which
received much more scrutiny and testing than as-client-to-older-servers, 
alas).

Your patch has been applied to the master and release/2.5 branch.

commit 6ffe64e34004967a96514cc55abb22215fbe5640 (master)
commit ea3c0ecc11f64fe4a58441c6d9c889472f6ed606 (release/2.5)
Author: Arne Schwabe
Date:   Sun Aug 30 15:14:40 2020 +0200

     Fix client NCP OCC fallback when server and client cipher are identical

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20200830131440.10933-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/search?l=mid&q=20200830131440.10933-1-arne@rfc2549.org
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index c9ab85ce..55496395 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -269,14 +269,11 @@  static bool
 tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
 {
     if (remote_ciphername
-        && 0 != strcmp(o->ciphername, remote_ciphername))
+        && tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
     {
-        if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
-        {
-            o->ciphername = string_alloc(remote_ciphername, &o->gc);
-            msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
-            return true;
-        }
+        o->ciphername = string_alloc(remote_ciphername, &o->gc);
+        msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
+        return true;
     }
     return false;
 }