| Message ID | 20210325174449.26948-1-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Always disable SSL renegotiations | expand |
Hi, On 25/03/2021 18:44, Arne Schwabe wrote: > These have been troublesome in the past and also today's CVE-2021-3449 > DOS is only exploitable if renegotiation is enabled. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> What is the practical effect of this change? With the current code (before this patch) when would OpenSSL/mbedTLS start a renegotiation on its own? May it have had any impact on the OpenVPN protocol until now? Cheers, > --- > src/openvpn/ssl_mbedtls.c | 3 +++ > src/openvpn/ssl_openssl.c | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c > index 4626e9838..1628a02e3 100644 > --- a/src/openvpn/ssl_mbedtls.c > +++ b/src/openvpn/ssl_mbedtls.c > @@ -1086,6 +1086,9 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl, > { > mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups); > } > + /* Disable renegotiations. OpenVPN has its own mechanism to create whole > + * new SSL session. And these 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 d161f48b8..a11ca5b97 100644 > --- a/src/openvpn/ssl_openssl.c > +++ b/src/openvpn/ssl_openssl.c > @@ -320,6 +320,9 @@ 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 renegotiations. OpenVPN has its own mechanism to create whole > + * new SSL session. And these have been probelmatic in the past */ > + sslopt |= SSL_OP_NO_RENEGOTIATION; > > SSL_CTX_set_options(ctx->ctx, sslopt); > >
Hi, On 25/03/2021 23:15, Antonio Quartulli wrote: > Hi, > > On 25/03/2021 18:44, Arne Schwabe wrote: >> These have been troublesome in the past and also today's CVE-2021-3449 >> DOS is only exploitable if renegotiation is enabled. >> >> Signed-off-by: Arne Schwabe <arne@rfc2549.org> > > What is the practical effect of this change? > With the current code (before this patch) when would OpenSSL/mbedTLS > start a renegotiation on its own? > > May it have had any impact on the OpenVPN protocol until now? > > > Cheers, > > >> --- >> src/openvpn/ssl_mbedtls.c | 3 +++ >> src/openvpn/ssl_openssl.c | 3 +++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c >> index 4626e9838..1628a02e3 100644 >> --- a/src/openvpn/ssl_mbedtls.c >> +++ b/src/openvpn/ssl_mbedtls.c >> @@ -1086,6 +1086,9 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl, >> { >> mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups); >> } >> + /* Disable renegotiations. OpenVPN has its own mechanism to create whole >> + * new SSL session. And these have been problematic in the past */ This comment reads like so: Disable "unspecified" renegotiations. OpenVPN has "specific" renegotiation mechanism(s), which 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 d161f48b8..a11ca5b97 100644 >> --- a/src/openvpn/ssl_openssl.c >> +++ b/src/openvpn/ssl_openssl.c >> @@ -320,6 +320,9 @@ 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 renegotiations. OpenVPN has its own mechanism to create whole >> + * new SSL session. And these have been probelmatic in the past */ >> + sslopt |= SSL_OP_NO_RENEGOTIATION; >> >> SSL_CTX_set_options(ctx->ctx, sslopt); >> >> >
Am 26.03.21 um 00:15 schrieb Antonio Quartulli: > Hi, > > On 25/03/2021 18:44, Arne Schwabe wrote: >> These have been troublesome in the past and also today's CVE-2021-3449 >> DOS is only exploitable if renegotiation is enabled. >> >> Signed-off-by: Arne Schwabe <arne@rfc2549.org> > > What is the practical effect of this change? > With the current code (before this patch) when would OpenSSL/mbedTLS > start a renegotiation on its own? > Documentation from OpenSSL: SSL_OP_NO_RENEGOTIATION Disable all renegotiation in TLSv1.2 and earlier. Do not send HelloRequest messages, and ignore renegotiation requests via ClientHello. From mbed TLS: Enable / Disable renegotiation support for connection when initiated by peer (Default: MBEDTLS_SSL_RENEGOTIATION_DISABLED) 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. Note Server-side, enabling renegotiation also makes the server susceptible to a resource DoS by a malicious client. So for mbed TLS it was off by default anyway, this patch just doesn't trust the default for mbed TLS. Renegotiation is also dropped from TLS 1.3. Furthermore in TLS 1.2 you would use it to restart a session with diffferent parameter (now with client certs). So the is no real use case in OpenVPN to have it enabled. Arne
Hi, On 26/03/2021 12:03, Arne Schwabe wrote: > Am 26.03.21 um 00:15 schrieb Antonio Quartulli: >> Hi, >> >> On 25/03/2021 18:44, Arne Schwabe wrote: >>> These have been troublesome in the past and also today's CVE-2021-3449 >>> DOS is only exploitable if renegotiation is enabled. >>> >>> Signed-off-by: Arne Schwabe <arne@rfc2549.org> >> >> What is the practical effect of this change? >> With the current code (before this patch) when would OpenSSL/mbedTLS >> start a renegotiation on its own? >> > > > Documentation from OpenSSL: > > SSL_OP_NO_RENEGOTIATION > Disable all renegotiation in TLSv1.2 and earlier. Do not send > HelloRequest messages, and ignore renegotiation requests via ClientHello. > > From mbed TLS: > > Enable / Disable renegotiation support for connection when initiated by > peer (Default: MBEDTLS_SSL_RENEGOTIATION_DISABLED) > > 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. > Note > Server-side, enabling renegotiation also makes the server susceptible to > a resource DoS by a malicious client. > > > So for mbed TLS it was off by default anyway, this patch just doesn't > trust the default for mbed TLS. Renegotiation is also dropped from TLS > 1.3. Furthermore in TLS 1.2 you would use it to restart a session with > diffferent parameter (now with client certs). So the is no real use case > in OpenVPN to have it enabled. > Thanks a lot! This was definitely eye-opening, especially for somebody that does not eat oat and SSL for breakfast :-) It'd be nice though to have a little summary of this in the commit message (no need to copy/paste verbatim text from openssl/mbedtls though). I have a couple minor comments for the patch...incoming soon! Cheers,
Hi, On 25/03/2021 18:44, Arne Schwabe wrote: > These have been troublesome in the past and also today's CVE-2021-3449 > DOS is only exploitable if renegotiation is enabled. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > src/openvpn/ssl_mbedtls.c | 3 +++ > src/openvpn/ssl_openssl.c | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c > index 4626e9838..1628a02e3 100644 > --- a/src/openvpn/ssl_mbedtls.c > +++ b/src/openvpn/ssl_mbedtls.c > @@ -1086,6 +1086,9 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl, > { > mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups); > } > + /* Disable renegotiations. OpenVPN has its own mechanism to create whole > + * new SSL session. And these have been problematic in the past */ I would rephrase a bit the last sentence because "these" is a bit ambiguous: And these have been problematic in the past => Also, renegotiations initiated by the SSL library have already proven to be problematic. > + 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 d161f48b8..a11ca5b97 100644 > --- a/src/openvpn/ssl_openssl.c > +++ b/src/openvpn/ssl_openssl.c > @@ -320,6 +320,9 @@ 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 renegotiations. OpenVPN has its own mechanism to create whole > + * new SSL session. And these have been probelmatic in the past */ same as above. > + sslopt |= SSL_OP_NO_RENEGOTIATION; > > SSL_CTX_set_options(ctx->ctx, sslopt); > > The rest looks good to me! I tested running an OpenVPN connection with multiple renegotiations (triggered by OpenVPN) and nothing broke. I am not sure there is a specific test for testing when the "SSL renegotiation should have happened". Cheers,
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c index 4626e9838..1628a02e3 100644 --- a/src/openvpn/ssl_mbedtls.c +++ b/src/openvpn/ssl_mbedtls.c @@ -1086,6 +1086,9 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl, { mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups); } + /* Disable renegotiations. OpenVPN has its own mechanism to create whole + * new SSL session. And these 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 d161f48b8..a11ca5b97 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -320,6 +320,9 @@ 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 renegotiations. OpenVPN has its own mechanism to create whole + * new SSL session. And these have been probelmatic in the past */ + sslopt |= SSL_OP_NO_RENEGOTIATION; SSL_CTX_set_options(ctx->ctx, sslopt);
These have been troublesome in the past and also today's CVE-2021-3449 DOS is only exploitable if renegotiation is enabled. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/ssl_mbedtls.c | 3 +++ src/openvpn/ssl_openssl.c | 3 +++ 2 files changed, 6 insertions(+)