| 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 | expand |
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
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; }
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(-)