| Message ID | 20210316124421.1635-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] Avoid generating unecessary mbed debug messages | expand |
Hi, On 16-03-2021 13:44, Arne Schwabe wrote: > The main motivation to make this change is to 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> > > Patch V2: Replace magic constant with proper define. Highlight more this > avoding generating unessary debug output than crash workaround. > --- > src/openvpn/options.c | 6 ++++++ > src/openvpn/ssl_common.h | 1 + > src/openvpn/ssl_mbedtls.c | 13 ++++++++++++- > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index 0eb049d8..c4646d48 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 >= (D_TLS_DEBUG_MED & M_DEBUG_LEVEL)) > + { > + /* We pass this flag to the SSL library to avoid a > + * mbed TLS always generating debug level logging */ Typo: "a mbed TLS" -> "mbed TLS". > + 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 b493cbb7..2e3c98db 100644 > --- a/src/openvpn/ssl_common.h > +++ b/src/openvpn/ssl_common.h > @@ -352,6 +352,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..4626e983 100644 > --- a/src/openvpn/ssl_mbedtls.c > +++ b/src/openvpn/ssl_mbedtls.c > @@ -1058,7 +1058,18 @@ 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); > + /* We only want to have mbed TLS generate debug level logging when we would > + * also display it. > + * In fact mbed TLS 2.25.0 crashes generating debug log 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 now looks good me. Acked-by: Steffan Karger <steffan@karger.me> Thanks, -Steffan
Acked-by: Gert Doering <gert@greenie.muc.de> Tested with exactly that testbed that initially discovered the SIGSEGV, and as expected, it no longer crashes. Verified that it still crashes with "--verb 8" (it does). Code also looks secure and does what it says on the lid. That said, ACKed and merged, I'm still not totally happy with the approach chosen - this is quite some new lines of code to change one "3" into a "2", depending on the value of one other variable... but we discussed this before, and Arne did not want to have the mbedTLS code "peek" into get_debug_level() to get at the --verb setting. Since Steffan is fine with this approach, I'm clearly the minority here... .. while writing this, the ACK from Steffan came in... so, recording that as well... MULTI-ACK! :-) - and fixing the comment as requested. Your patch has been applied to the master and release/2.5 branch (stability / long-term compat thing). commit 4524feb2bbbb6d1bd463a0c5c2d53aae5bdf360a (master) commit 475d17a53eba85591f270008f8b583383a5b9afa (release/2.5) Author: Arne Schwabe Date: Tue Mar 16 13:44:21 2021 +0100 Avoid generating unecessary mbed debug messages Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20210316124421.1635-1-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21667.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 0eb049d8..c4646d48 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 >= (D_TLS_DEBUG_MED & M_DEBUG_LEVEL)) + { + /* We pass this flag to the SSL library to avoid a + * mbed TLS always generating debug level logging */ + 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 b493cbb7..2e3c98db 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -352,6 +352,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..4626e983 100644 --- a/src/openvpn/ssl_mbedtls.c +++ b/src/openvpn/ssl_mbedtls.c @@ -1058,7 +1058,18 @@ 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); + /* We only want to have mbed TLS generate debug level logging when we would + * also display it. + * In fact mbed TLS 2.25.0 crashes generating debug log 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,
The main motivation to make this change is to 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> Patch V2: Replace magic constant with proper define. Highlight more this avoding generating unessary debug output than crash workaround. --- src/openvpn/options.c | 6 ++++++ src/openvpn/ssl_common.h | 1 + src/openvpn/ssl_mbedtls.c | 13 ++++++++++++- 3 files changed, 19 insertions(+), 1 deletion(-)