From b67fd6f29c2b63d6723c327cd5a445dce1366952 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Wed, 28 Aug 2024 16:47:48 +0200 Subject: [PATCH] Fix failing test_dtls_frag_ch - Add option to disable ECH - InitSuites: clean up DTLS paths - wolfSSL_parse_cipher_list: remove WOLFSSL_MAX_SUITE_SZ setting - wolfSSL_parse_cipher_list: add rationale for keeping ciphersuites - test_dtls_frag_ch: ECH and ciphersuites were pushing the ClientHello message over the fragmentation limit. Disabling ECH and limiting ciphersuites fixes the test. --- .github/workflows/os-check.yml | 3 +++ src/internal.c | 26 +++++++++++--------------- src/ssl.c | 34 ++++++++++++++++++++++++++++++---- src/tls.c | 28 +++++++++++++++++++--------- src/tls13.c | 16 +++++++--------- tests/api.c | 14 ++++++++++++++ wolfssl/internal.h | 8 +++++++- wolfssl/ssl.h | 4 ++++ 8 files changed, 95 insertions(+), 38 deletions(-) diff --git a/.github/workflows/os-check.yml b/.github/workflows/os-check.yml index fcad81210..1402afe57 100644 --- a/.github/workflows/os-check.yml +++ b/.github/workflows/os-check.yml @@ -35,6 +35,9 @@ jobs: CPPFLAGS=''-DWOLFSSL_DTLS13_NO_HRR_ON_RESUME'' ', '--enable-experimental --enable-kyber --enable-dtls --enable-dtls13 --enable-dtls-frag-ch', + '--enable-all --enable-dtls13 --enable-dtls-frag-ch', + '--enable-dtls --enable-dtls13 --enable-dtls-frag-ch + --enable-dtls-mtu', ] name: make check runs-on: ${{ matrix.os }} diff --git a/src/internal.c b/src/internal.c index 0f9d093ab..593501ed1 100644 --- a/src/internal.c +++ b/src/internal.c @@ -2563,7 +2563,7 @@ void wolfSSL_CRYPTO_cleanup_ex_data(WOLFSSL_CRYPTO_EX_DATA* ex_data) #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH) /* free all ech configs in the list */ -static void FreeEchConfigs(WOLFSSL_EchConfig* configs, void* heap) +void FreeEchConfigs(WOLFSSL_EchConfig* configs, void* heap) { WOLFSSL_EchConfig* working_config = configs; WOLFSSL_EchConfig* next_config; @@ -3268,9 +3268,13 @@ void InitSuites(Suites* suites, ProtocolVersion pv, int keySz, word16 haveRSA, int haveRSAsig = 1; #ifdef WOLFSSL_DTLS - /* If DTLS v1.2 or later than set tls1_2 flag */ - if (pv.major == DTLS_MAJOR && pv.minor <= DTLSv1_2_MINOR) { - tls1_2 = 1; + if (pv.major == DTLS_MAJOR) { + dtls = 1; + tls = 1; + /* May be dead assignments dependent upon configuration */ + (void) dtls; + (void) tls; + tls1_2 = pv.minor <= DTLSv1_2_MINOR; } #endif @@ -3381,17 +3385,6 @@ void InitSuites(Suites* suites, ProtocolVersion pv, int keySz, word16 haveRSA, haveRSAsig = 0; /* can't have RSA sig if don't have RSA */ #endif -#ifdef WOLFSSL_DTLS - if (pv.major == DTLS_MAJOR) { - dtls = 1; - tls = 1; - /* May be dead assignments dependent upon configuration */ - (void) dtls; - (void) tls; - tls1_2 = pv.minor <= DTLSv1_2_MINOR; - } -#endif - #ifdef HAVE_RENEGOTIATION_INDICATION if (side == WOLFSSL_CLIENT_END) { suites->suites[idx++] = CIPHER_BYTE; @@ -7512,6 +7505,9 @@ int InitSSL(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup) #if defined(HAVE_ENCRYPT_THEN_MAC) && !defined(WOLFSSL_AEAD_ONLY) ssl->options.disallowEncThenMac = ctx->disallowEncThenMac; #endif +#if defined(WOLFSSL_TLS13) && defined(HAVE_ECH) + ssl->options.disableECH = ctx->disableECH; +#endif /* default alert state (none) */ ssl->alert_history.last_rx.code = -1; diff --git a/src/ssl.c b/src/ssl.c index b4bf40744..18104ba43 100644 --- a/src/ssl.c +++ b/src/ssl.c @@ -553,6 +553,18 @@ int wolfSSL_CTX_GetEchConfigs(WOLFSSL_CTX* ctx, byte* output, return GetEchConfigsEx(ctx->echConfigs, output, outputLen); } +void wolfSSL_CTX_SetEchEnable(WOLFSSL_CTX* ctx, byte enable) +{ + if (ctx != NULL) { + ctx->disableECH = !enable; + if (ctx->disableECH) { + TLSX_Remove(&ctx->extensions, TLSX_ECH, ctx->heap); + FreeEchConfigs(ctx->echConfigs, ctx->heap); + ctx->echConfigs = NULL; + } + } +} + /* set the ech config from base64 for our client ssl object, base64 is the * format ech configs are sent using dns records */ int wolfSSL_SetEchConfigsBase64(WOLFSSL* ssl, char* echConfigs64, @@ -942,6 +954,18 @@ int wolfSSL_GetEchConfigs(WOLFSSL* ssl, byte* output, word32* outputLen) return GetEchConfigsEx(ssl->echConfigs, output, outputLen); } +void wolfSSL_SetEchEnable(WOLFSSL* ssl, byte enable) +{ + if (ssl != NULL) { + ssl->options.disableECH = !enable; + if (ssl->options.disableECH) { + TLSX_Remove(&ssl->extensions, TLSX_ECH, ssl->heap); + FreeEchConfigs(ssl->echConfigs, ssl->heap); + ssl->echConfigs = NULL; + } + } +} + /* get the raw ech configs from our linked list of ech config structs */ int GetEchConfigsEx(WOLFSSL_EchConfig* configs, byte* output, word32* outputLen) { @@ -8480,10 +8504,6 @@ static int wolfSSL_parse_cipher_list(WOLFSSL_CTX* ctx, WOLFSSL* ssl, } /* list contains ciphers either only for TLS 1.3 or <= TLS 1.2 */ - if (suites->suiteSz == 0) { - WOLFSSL_MSG("Warning suites->suiteSz = 0 set to WOLFSSL_MAX_SUITE_SZ"); - suites->suiteSz = WOLFSSL_MAX_SUITE_SZ; - } #ifdef WOLFSSL_SMALL_STACK if (suites->suiteSz > 0) { suitesCpy = (byte*)XMALLOC(suites->suiteSz, NULL, @@ -8510,6 +8530,12 @@ static int wolfSSL_parse_cipher_list(WOLFSSL_CTX* ctx, WOLFSSL* ssl, return WOLFSSL_FAILURE; } + /* The idea in this section is that OpenSSL has two API to set ciphersuites. + * - SSL_CTX_set_cipher_list for setting TLS <= 1.2 suites + * - SSL_CTX_set_ciphersuites for setting TLS 1.3 suites + * Since we direct both API here we attempt to provide API compatibility. If + * we only get suites from <= 1.2 or == 1.3 then we will only update those + * suites and keep the suites from the other group. */ for (i = 0; i < suitesCpySz && suites->suiteSz <= (WOLFSSL_MAX_SUITE_SZ - SUITE_LEN); i += 2) { /* Check for duplicates */ diff --git a/src/tls.c b/src/tls.c index f61a6e25e..49768444d 100644 --- a/src/tls.c +++ b/src/tls.c @@ -12083,6 +12083,11 @@ static int TLSX_ECH_Parse(WOLFSSL* ssl, const byte* readBuf, word16 size, if (size == 0) return BAD_FUNC_ARG; + if (ssl->options.disableECH) { + WOLFSSL_MSG("TLSX_ECH_Parse: ECH disabled. Ignoring."); + return 0; + } + if (msgType == encrypted_extensions) { ret = wolfSSL_SetEchConfigs(ssl, readBuf, size); @@ -13588,18 +13593,21 @@ int TLSX_PopulateExtensions(WOLFSSL* ssl, byte isServer) #endif #if defined(HAVE_ECH) /* GREASE ECH */ - if (ssl->echConfigs == NULL) { - ret = GREASE_ECH_USE(&(ssl->extensions), ssl->heap, ssl->rng); - } - else if (ssl->echConfigs != NULL) { - ret = ECH_USE(ssl->echConfigs, &(ssl->extensions), ssl->heap, - ssl->rng); + if (!ssl->options.disableECH) { + if (ssl->echConfigs == NULL) { + ret = GREASE_ECH_USE(&(ssl->extensions), ssl->heap, + ssl->rng); + } + else if (ssl->echConfigs != NULL) { + ret = ECH_USE(ssl->echConfigs, &(ssl->extensions), + ssl->heap, ssl->rng); + } } #endif } #if defined(HAVE_ECH) else if (IsAtLeastTLSv1_3(ssl->version)) { - if (ssl->ctx->echConfigs != NULL) { + if (ssl->ctx->echConfigs != NULL && !ssl->options.disableECH) { ret = SERVER_ECH_USE(&(ssl->extensions), ssl->heap, ssl->ctx->echConfigs); @@ -13789,7 +13797,8 @@ int TLSX_GetRequestSize(WOLFSSL* ssl, byte msgType, word32* pLength) } #endif #if defined(HAVE_ECH) - if (ssl->options.useEch == 1 && msgType == client_hello) { + if (ssl->options.useEch == 1 && !ssl->options.disableECH + && msgType == client_hello) { ret = TLSX_GetSizeWithEch(ssl, semaphore, msgType, &length); if (ret != 0) return ret; @@ -14034,7 +14043,8 @@ int TLSX_WriteRequest(WOLFSSL* ssl, byte* output, byte msgType, word32* pOffset) #endif #endif #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH) - if (ssl->options.useEch == 1 && msgType == client_hello) { + if (ssl->options.useEch == 1 && !ssl->options.disableECH + && msgType == client_hello) { ret = TLSX_WriteWithEch(ssl, output, semaphore, msgType, &offset); if (ret != 0) diff --git a/src/tls13.c b/src/tls13.c index bbca4fac5..10e578edd 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -4415,7 +4415,7 @@ int SendTls13ClientHello(WOLFSSL* ssl) /* find length of outer and inner */ #if defined(HAVE_ECH) - if (ssl->options.useEch == 1) { + if (ssl->options.useEch == 1 && !ssl->options.disableECH) { TLSX* echX = TLSX_Find(ssl->extensions, TLSX_ECH); if (echX == NULL) return -1; @@ -4566,7 +4566,7 @@ int SendTls13ClientHello(WOLFSSL* ssl) #if defined(HAVE_ECH) /* write inner then outer */ - if (ssl->options.useEch == 1) { + if (ssl->options.useEch == 1 && !ssl->options.disableECH) { /* set the type to inner */ args->ech->type = ECH_TYPE_INNER; @@ -4626,7 +4626,7 @@ int SendTls13ClientHello(WOLFSSL* ssl) #if defined(HAVE_ECH) /* encrypt and pack the ech innerClientHello */ - if (ssl->options.useEch == 1) { + if (ssl->options.useEch == 1 && !ssl->options.disableECH) { ret = TLSX_FinalizeEch(args->ech, args->output + RECORD_HEADER_SZ + HANDSHAKE_HEADER_SZ, (word32)(args->sendSz - (RECORD_HEADER_SZ + HANDSHAKE_HEADER_SZ))); @@ -4656,11 +4656,9 @@ int SendTls13ClientHello(WOLFSSL* ssl) { #if defined(HAVE_ECH) /* compute the inner hash */ - if (ssl->options.useEch == 1) { + if (ssl->options.useEch == 1 && !ssl->options.disableECH) ret = EchHashHelloInner(ssl, args->ech); - } #endif - /* compute the outer hash */ if (ret == 0) ret = HashOutput(ssl, args->output, (int)args->idx, 0); @@ -5475,7 +5473,7 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, #if defined(HAVE_ECH) /* check for acceptConfirmation and HashInput with 8 0 bytes */ - if (ssl->options.useEch == 1) { + if (ssl->options.useEch == 1 && !ssl->options.disableECH) { ret = EchCheckAcceptance(ssl, input, args->serverRandomOffset, (int)helloSz); if (ret != 0) return ret; @@ -6935,7 +6933,7 @@ int DoTls13ClientHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx, goto exit_dch; #if defined(HAVE_ECH) - if (ssl->ctx->echConfigs != NULL) { + if (ssl->ctx->echConfigs != NULL && !ssl->options.disableECH) { /* save the start of the buffer so we can use it when parsing ech */ echX = TLSX_Find(ssl->extensions, TLSX_ECH); @@ -7407,7 +7405,7 @@ int SendTls13ServerHello(WOLFSSL* ssl, byte extMsgType) #endif /* WOLFSSL_DTLS13 */ { #if defined(HAVE_ECH) - if (ssl->ctx->echConfigs != NULL) { + if (ssl->ctx->echConfigs != NULL && !ssl->options.disableECH) { echX = TLSX_Find(ssl->extensions, TLSX_ECH); if (echX == NULL) diff --git a/tests/api.c b/tests/api.c index 5b4be95e7..41edd3dd8 100644 --- a/tests/api.c +++ b/tests/api.c @@ -88883,6 +88883,20 @@ static int test_dtls_frag_ch(void) /* Expect quietly dropping fragmented first CH */ ExpectIntEQ(test_ctx.c_len, 0); +#if defined(WOLFSSL_TLS13) && defined(HAVE_ECH) + /* Disable ECH as it pushes it over our MTU */ + wolfSSL_SetEchEnable(ssl_c, 0); +#endif + + /* Limit options to make the CH a fixed length */ + /* See wolfSSL_parse_cipher_list for reason why we provide 1.3 AND 1.2 + * ciphersuite. This is only necessary when building with OPENSSL_EXTRA. */ + ExpectTrue(wolfSSL_set_cipher_list(ssl_c, "TLS13-AES256-GCM-SHA384" +#ifdef OPENSSL_EXTRA + ":DHE-RSA-AES256-GCM-SHA384" +#endif + )); + /* CH1 */ ExpectIntEQ(wolfSSL_negotiate(ssl_c), -1); ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); diff --git a/wolfssl/internal.h b/wolfssl/internal.h index 9e57ab0c4..39ef18c7e 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -3088,6 +3088,8 @@ WOLFSSL_LOCAL int GetEchConfig(WOLFSSL_EchConfig* config, byte* output, WOLFSSL_LOCAL int GetEchConfigsEx(WOLFSSL_EchConfig* configs, byte* output, word32* outputLen); + +WOLFSSL_LOCAL void FreeEchConfigs(WOLFSSL_EchConfig* configs, void* heap); #endif struct TLSX { @@ -3806,6 +3808,9 @@ struct WOLFSSL_CTX { #endif #if defined(WOLFSSL_DTLS) && defined(WOLFSSL_SCTP) byte dtlsSctp:1; /* DTLS-over-SCTP mode */ +#endif +#if defined(WOLFSSL_TLS13) && defined(HAVE_ECH) + byte disableECH:1; #endif word16 minProto:1; /* sets min to min available */ word16 maxProto:1; /* sets max to max available */ @@ -4957,7 +4962,8 @@ struct Options { word16 useDtlsCID:1; #endif /* WOLFSSL_DTLS_CID */ #if defined(WOLFSSL_TLS13) && defined(HAVE_ECH) - word16 useEch:1; + word16 useEch:1; /* Do we have a valid config */ + byte disableECH:1; /* Did the user disable ech */ #endif #ifdef WOLFSSL_SEND_HRR_COOKIE word16 cookieGood:1; diff --git a/wolfssl/ssl.h b/wolfssl/ssl.h index 9adc0be00..c4483003e 100644 --- a/wolfssl/ssl.h +++ b/wolfssl/ssl.h @@ -988,6 +988,8 @@ WOLFSSL_API int wolfSSL_CTX_GenerateEchConfig(WOLFSSL_CTX* ctx, WOLFSSL_API int wolfSSL_CTX_GetEchConfigs(WOLFSSL_CTX* ctx, byte* output, word32* outputLen); +WOLFSSL_API void wolfSSL_CTX_SetEchEnable(WOLFSSL_CTX* ctx, byte enable); + WOLFSSL_API int wolfSSL_SetEchConfigsBase64(WOLFSSL* ssl, char* echConfigs64, word32 echConfigs64Len); @@ -996,6 +998,8 @@ WOLFSSL_API int wolfSSL_SetEchConfigs(WOLFSSL* ssl, const byte* echConfigs, WOLFSSL_API int wolfSSL_GetEchConfigs(WOLFSSL* ssl, byte* echConfigs, word32* echConfigsLen); + +WOLFSSL_API void wolfSSL_SetEchEnable(WOLFSSL* ssl, byte enable); #endif /* WOLFSSL_TLS13 && HAVE_ECH */ #ifdef HAVE_POLY1305