[Openvpn-devel] Always disable SSL renegotiations

Message ID 20210325174449.26948-1-arne@rfc2549.org
State Superseded
Headers show
Series
  • [Openvpn-devel] Always disable SSL renegotiations
Related show

Commit Message

Arne Schwabe March 25, 2021, 5:44 p.m.
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(+)

Comments

Antonio Quartulli March 25, 2021, 11:15 p.m. | #1
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);
>  
>
tincanteksup March 26, 2021, 12:20 a.m. | #2
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);
>>   
>>
>
Arne Schwabe March 26, 2021, 11:03 a.m. | #3
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
Antonio Quartulli March 26, 2021, 1:30 p.m. | #4
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,
Antonio Quartulli March 26, 2021, 2:20 p.m. | #5
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,

Patch

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);