[Openvpn-devel] Avoid a crash in mbed TLS 2.25 with --verb < 8

mbed TLS 2.25 has a nasty bug that the print function for Montgomery style
EC curves (Curve25519 and Curve448) does segfault. See also the issue
reported here: https://github.com/ARMmbed/mbedtls/issues/4208

We request always debug level 3 from mbed TLS but filter out any debug
output of level 3 unless verb 8 or higher is set. This commeit sets
the debug level to 2 to avoid this problem by makeing mbed TLS not
generatin the problematic debug output.

For the affected version to still use --verb 8 with mbed TLS 2.25 is to
restrict the EC groups to ones that do not crash the print function
like with '--tls-groups secp521r1:secp384r1:secp256r1'.

This patch has no patch on user-visible behaviour on unaffected mbed TLS
versions.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/options.c     |  6 ++++++
 src/openvpn/ssl_common.h  |  1 +
 src/openvpn/ssl_mbedtls.c | 11 ++++++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

Hi,

On 08/03/2021 15:21, Arne Schwabe wrote:
[cut]

> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 0eb049d8..6d908e15 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -5883,6 +5883,12 @@ add_option(struct options *options,
>      {
>          VERIFY_PERMISSION(OPT_P_MESSAGES);
>          options->verbosity = positive_atoi(p[1]);
> +        if (options->verbosity > 7)
> +        {
> +            /* We pass this flag to the SSL library to avoid a bug
> +             * in mbed TLS 2.5.0 with high log level */
> +            options->ssl_flags |= SSLF_TLS_DEBUG_ENABLED;
> +        }

(2.5.0 is probably a typ0?)

If we know that a certain combination of options + the right mbedTLS
version will lead to a crash, shouldn't we catch it here (or somewhere
else) and error out right away?

>  #if !defined(ENABLE_DEBUG) && !defined(ENABLE_SMALL)
>          /* Warn when a debug verbosity is supplied when built without debug support */
>          if (options->verbosity >= 7)
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index bbb8135d..f821c654 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -350,6 +350,7 @@ struct tls_options
>  #define SSLF_TLS_VERSION_MIN_MASK     0xF  /* (uses bit positions 6 to 9) */
>  #define SSLF_TLS_VERSION_MAX_SHIFT    10
>  #define SSLF_TLS_VERSION_MAX_MASK     0xF  /* (uses bit positions 10 to 13) */
> +#define SSLF_TLS_DEBUG_ENABLED        (1<<14)
>      unsigned int ssl_flags;
>  
>  #ifdef ENABLE_MANAGEMENT
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index b30b6b9d..a7b6c2c6 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -1058,7 +1058,16 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>      mbedtls_ssl_config_defaults(ks_ssl->ssl_config, ssl_ctx->endpoint,
>                                  MBEDTLS_SSL_TRANSPORT_STREAM, MBEDTLS_SSL_PRESET_DEFAULT);
>  #ifdef MBEDTLS_DEBUG_C
> -    mbedtls_debug_set_threshold(3);
> +    /* This works around a crash in mbed TLS 2.25 if Curve25591 is selected
> +     * for DH (https://github.com/ARMmbed/mbedtls/issues/4208)*/
> +    if (session->opt->ssl_flags & SSLF_TLS_DEBUG_ENABLED)
> +    {
> +        mbedtls_debug_set_threshold(3);

If a user is on mbedTLS 2.25 and specifies verb = 8 we will then crash
(assuming the right combination of options was chosen).

Isn't it better to just always use 2 as threshold when the mbedTLS
version is known to be buggy (regardless of verb)?
An ifdef around this invocation would do it, without the need of
introducing another SSL flag.

> +    }
> +    else
> +    {
> +        mbedtls_debug_set_threshold(2);
> +    }>  #endif
>      mbedtls_ssl_conf_dbg(ks_ssl->ssl_config, my_debug, NULL);
>      mbedtls_ssl_conf_rng(ks_ssl->ssl_config, mbedtls_ctr_drbg_random,
> 


my 2 cents

Cheers,

Am 09.03.21 um 10:09 schrieb Antonio Quartulli:
> Hi,
> 
> On 08/03/2021 15:21, Arne Schwabe wrote:
> [cut]
> 
>> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
>> index 0eb049d8..6d908e15 100644
>> --- a/src/openvpn/options.c
>> +++ b/src/openvpn/options.c
>> @@ -5883,6 +5883,12 @@ add_option(struct options *options,
>>      {
>>          VERIFY_PERMISSION(OPT_P_MESSAGES);
>>          options->verbosity = positive_atoi(p[1]);
>> +        if (options->verbosity > 7)
>> +        {
>> +            /* We pass this flag to the SSL library to avoid a bug
>> +             * in mbed TLS 2.5.0 with high log level */
>> +            options->ssl_flags |= SSLF_TLS_DEBUG_ENABLED;
>> +        }
> 
> (2.5.0 is probably a typ0?)

Yes.

> 
> If we know that a certain combination of options + the right mbedTLS
> version will lead to a crash, shouldn't we catch it here (or somewhere
> else) and error out right away?

This one that we know of. But if we do a warning I would rather do a
generic one. The debug logging of mbed TLS seems to be not very well
tested. normally we don't explciitly try to detect and warn about every
obscure bug that we know of. But we could do for this bug:

#if mbed version >= 2.25.0
msg(M_INFO, "Note: mbed TLS 2.25.0 is known to crash on trying too
output x25519 and x448 parameters in debug. If you see mbed TLS crash
after printing out ECDH curve: x25519 you can workaround this bug by
using '--tls-groups secp521r1:secp384r1:secp256r1'");
#endif

>>  #if !defined(ENABLE_DEBUG) && !defined(ENABLE_SMALL)
>>          /* Warn when a debug verbosity is supplied when built without debug support */
>>          if (options->verbosity >= 7)
>> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
>> index bbb8135d..f821c654 100644
>> --- a/src/openvpn/ssl_common.h
>> +++ b/src/openvpn/ssl_common.h
>> @@ -350,6 +350,7 @@ struct tls_options
>>  #define SSLF_TLS_VERSION_MIN_MASK     0xF  /* (uses bit positions 6 to 9) */
>>  #define SSLF_TLS_VERSION_MAX_SHIFT    10
>>  #define SSLF_TLS_VERSION_MAX_MASK     0xF  /* (uses bit positions 10 to 13) */
>> +#define SSLF_TLS_DEBUG_ENABLED        (1<<14)
>>      unsigned int ssl_flags;
>>  
>>  #ifdef ENABLE_MANAGEMENT
>> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
>> index b30b6b9d..a7b6c2c6 100644
>> --- a/src/openvpn/ssl_mbedtls.c
>> +++ b/src/openvpn/ssl_mbedtls.c
>> @@ -1058,7 +1058,16 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>>      mbedtls_ssl_config_defaults(ks_ssl->ssl_config, ssl_ctx->endpoint,
>>                                  MBEDTLS_SSL_TRANSPORT_STREAM, MBEDTLS_SSL_PRESET_DEFAULT);
>>  #ifdef MBEDTLS_DEBUG_C
>> -    mbedtls_debug_set_threshold(3);
>> +    /* This works around a crash in mbed TLS 2.25 if Curve25591 is selected
>> +     * for DH (https://github.com/ARMmbed/mbedtls/issues/4208)*/
>> +    if (session->opt->ssl_flags & SSLF_TLS_DEBUG_ENABLED)
>> +    {
>> +        mbedtls_debug_set_threshold(3);
> 
> If a user is on mbedTLS 2.25 and specifies verb = 8 we will then crash
> (assuming the right combination of options was chosen).
> 
> Isn't it better to just always use 2 as threshold when the mbedTLS
> version is known to be buggy (regardless of verb)?
> An ifdef around this invocation would do it, without the need of
> introducing another SSL flag.

I am strongly against removing the possibility to debug mbed TLS
completely just because it *might* crash on a setting that is not used
outside of debugging (verb >= 8).

Also seeing that this bug, it seems that requesting level 3 logging from
mbed TLS is not a good idea in general as the code seems not to be well
tested and there might other high debug print crashes lurking. Also even
though it does not seem to affect performance, currently we generate a
number of debug prints for each packet by requesting level logging from
mbed TLS (crypto operation debug prints).

Arne

Hi,

On 08-03-2021 15:21, Arne Schwabe wrote:
> mbed TLS 2.25 has a nasty bug that the print function for Montgomery style
> EC curves (Curve25519 and Curve448) does segfault. See also the issue
> reported here: https://github.com/ARMmbed/mbedtls/issues/4208
> 
> We request always debug level 3 from mbed TLS but filter out any debug
> output of level 3 unless verb 8 or higher is set. This commeit sets
> the debug level to 2 to avoid this problem by makeing mbed TLS not
> generatin the problematic debug output.

Only setting level 3 when we actually need it is a better approach
anyway. So +1 that, even without this bug. Somewhat surprisingly, a
quick test shows that this does not matter for performance (perhaps that
is why I didn't do it like that back then, can't remember).

> For the affected version to still use --verb 8 with mbed TLS 2.25 is to
> restrict the EC groups to ones that do not crash the print function
> like with '--tls-groups secp521r1:secp384r1:secp256r1'.
> 
> This patch has no patch on user-visible behaviour on unaffected mbed TLS
> versions.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/options.c     |  6 ++++++
>  src/openvpn/ssl_common.h  |  1 +
>  src/openvpn/ssl_mbedtls.c | 11 ++++++++++-
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 0eb049d8..6d908e15 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -5883,6 +5883,12 @@ add_option(struct options *options,
>      {
>          VERIFY_PERMISSION(OPT_P_MESSAGES);
>          options->verbosity = positive_atoi(p[1]);
> +        if (options->verbosity > 7)

Instead of the magic 7, can we use "D_TLS_DEBUG_MED & M_DEBUG_LEVEL"?

> +        {
> +            /* We pass this flag to the SSL library to avoid a bug
> +             * in mbed TLS 2.5.0 with high log level */

(Repeating Antonio:) Should be 2.25.

> +            options->ssl_flags |= SSLF_TLS_DEBUG_ENABLED;
> +        }
>  #if !defined(ENABLE_DEBUG) && !defined(ENABLE_SMALL)
>          /* Warn when a debug verbosity is supplied when built without debug support */
>          if (options->verbosity >= 7)
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index bbb8135d..f821c654 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -350,6 +350,7 @@ struct tls_options
>  #define SSLF_TLS_VERSION_MIN_MASK     0xF  /* (uses bit positions 6 to 9) */
>  #define SSLF_TLS_VERSION_MAX_SHIFT    10
>  #define SSLF_TLS_VERSION_MAX_MASK     0xF  /* (uses bit positions 10 to 13) */
> +#define SSLF_TLS_DEBUG_ENABLED        (1<<14)
>      unsigned int ssl_flags;
>  
>  #ifdef ENABLE_MANAGEMENT
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index b30b6b9d..a7b6c2c6 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -1058,7 +1058,16 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>      mbedtls_ssl_config_defaults(ks_ssl->ssl_config, ssl_ctx->endpoint,
>                                  MBEDTLS_SSL_TRANSPORT_STREAM, MBEDTLS_SSL_PRESET_DEFAULT);
>  #ifdef MBEDTLS_DEBUG_C
> -    mbedtls_debug_set_threshold(3);
> +    /* This works around a crash in mbed TLS 2.25 if Curve25591 is selected
> +     * for DH (https://github.com/ARMmbed/mbedtls/issues/4208)*/
> +    if (session->opt->ssl_flags & SSLF_TLS_DEBUG_ENABLED)
> +    {
> +        mbedtls_debug_set_threshold(3);
> +    }
> +    else
> +    {
> +        mbedtls_debug_set_threshold(2);
> +    }
>  #endif
>      mbedtls_ssl_conf_dbg(ks_ssl->ssl_config, my_debug, NULL);
>      mbedtls_ssl_conf_rng(ks_ssl->ssl_config, mbedtls_ctr_drbg_random,
> 

Otherwise, this looks good to me. Just stare-at-code, no real testing done.

-Steffan