[Openvpn-devel,v3] Always disable TLS renegotiations

Message ID 20210401110003.19689-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,v3] Always disable TLS renegotiations
Related show

Commit Message

Arne Schwabe April 1, 2021, 11 a.m.
Renegotiations have been troublesome in the past and also the recent OpenSSL
security problem (CVE-2021-3449) is only exploitable if TLS renegotiation
is enabled.

mbed TLS disables it by default and says in the documentation:

Warning: It is recommended to always disable renegotation unless you know you
need it and you know what you're doing. In the past, there have been
several issues associated with renegotiation or a poor understanding of
its properties.

TLS renegotiation can be used to restart a session with diffferent
parameters (e.g. now with client certs). This somethign that OpenVPN does
not use.

For OpenSSL 1.0.2 the workaround to disable renegotiation is rather
cumbersome. So we keep this to 1.1.1 only since 1.0.2 is on its way to
deprecation anyway.

Furthermore because of all these problems, also TLS 1.3 completely
drops support for renegotiations.

Patch V2: Improve commments and commit message
Patch V3: Only disable renegotiation where the SSL_OP_NO_RENEGOTIATION
          define is available. LibreSSL, wolfSSL and OpenSSL 1.0.2 are
          lacking this macro.
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl_mbedtls.c | 4 ++++
 src/openvpn/ssl_openssl.c | 6 ++++++
 2 files changed, 10 insertions(+)

Comments

Gert Doering April 2, 2021, 6:49 p.m. | #1
Acked-by: Gert Doering <gert@greenie.muc.de>

I have taken Antonio's ACK on v2, and added my own on v3 (since
I NAKed the v2).  This is basically the same patch just with a 
conditional on SSL_OP_NO_RENEGOTIATION - so for everything Antonio
has tested, we know it works, and in addition it does not break
OpenSSL 1.0.x builds.

Built and cliend-side tested on FreeBSD and Linux with mbedTLS
2.16 and 2.26.0 and OpenSSL 1.0.2 and 1.1.1

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

commit 9e702a5d0f1d8ca0443d95ba13fc821deaa81d48 (master)
commit a31c4b73f56e1dddda64ba15b27f0c5b2c6a26d4 (release/2.5)
Author: Arne Schwabe
Date:   Thu Apr 1 13:00:03 2021 +0200

     Always disable TLS renegotiations

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


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 4626e9838..8917fb188 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -1086,6 +1086,10 @@  key_state_ssl_init(struct key_state_ssl *ks_ssl,
     {
         mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
     }
+    /* Disable TLS renegotiations. OpenVPN's renegotiation creates new SSL
+     * session and does not depend on this feature. And TLS renegotiations have
+     * been problematic in the past */
+    mbedtls_ssl_conf_renegotiation(ks_ssl->ssl_config, MBEDTLS_SSL_RENEGOTIATION_DISABLED);
 
     /* Disable record splitting (for now).  OpenVPN assumes records are sent
      * unfragmented, and changing that will require thorough review and
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index b85f95be1..cb8ac7727 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -320,6 +320,12 @@  tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags)
     sslopt |= SSL_OP_CIPHER_SERVER_PREFERENCE;
 #endif
     sslopt |= SSL_OP_NO_COMPRESSION;
+    /* Disable TLS renegotiations. OpenVPN's renegotiation creates new SSL
+     * session and does not depend on this feature. And TLS renegotiations have
+     * been problematic in the past */
+#ifdef SSL_OP_NO_RENEGOTIATION
+    sslopt |= SSL_OP_NO_RENEGOTIATION;
+#endif
 
     SSL_CTX_set_options(ctx->ctx, sslopt);