From db64d36f004c34013e22c253b29694539e9d87ee Mon Sep 17 00:00:00 2001 From: Reda Chouk Date: Fri, 16 Aug 2024 23:04:47 +0200 Subject: [PATCH 1/6] properly handling the shutdown when multiple ones go on EAGAIN back to back. --- src/internal.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/internal.c b/src/internal.c index cd25cfc400..2bf201c9b7 100644 --- a/src/internal.c +++ b/src/internal.c @@ -25063,6 +25063,19 @@ static int SendAlert_ex(WOLFSSL* ssl, int severity, int type) } #endif + /* + * We check if we are trying to send a + * CLOSE_NOTIFY alert + * */ + if (type == 0) { + if (!ssl->options.sentNotify) { + ssl->options.sentNotify = 1; + } else { + /* CLOSE_NOTIFY already sent */ + return 0; + } + } + ssl->buffers.outputBuffer.length += sendSz; ret = SendBuffered(ssl); From 2356bec909f52e26dc771d11c0e9ad885d8920d0 Mon Sep 17 00:00:00 2001 From: Reda Chouk Date: Sat, 17 Aug 2024 00:32:32 +0200 Subject: [PATCH 2/6] no magic values --- src/internal.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/internal.c b/src/internal.c index 2bf201c9b7..a3b1138669 100644 --- a/src/internal.c +++ b/src/internal.c @@ -19,8 +19,6 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA */ - - #ifdef HAVE_CONFIG_H #include #endif @@ -25065,9 +25063,9 @@ static int SendAlert_ex(WOLFSSL* ssl, int severity, int type) /* * We check if we are trying to send a - * CLOSE_NOTIFY alert + * CLOSE_NOTIFY alert. * */ - if (type == 0) { + if (type == close_notify) { if (!ssl->options.sentNotify) { ssl->options.sentNotify = 1; } else { From 7d2ca8db5fba52356c879344d27cff33128d6ba9 Mon Sep 17 00:00:00 2001 From: Reda Chouk Date: Wed, 21 Aug 2024 14:37:51 +0200 Subject: [PATCH 3/6] addressing review: - added unit test; - formatting; --- src/internal.c | 3 +- tests/api.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/internal.c b/src/internal.c index a3b1138669..34fca4f8d6 100644 --- a/src/internal.c +++ b/src/internal.c @@ -25068,7 +25068,8 @@ static int SendAlert_ex(WOLFSSL* ssl, int severity, int type) if (type == close_notify) { if (!ssl->options.sentNotify) { ssl->options.sentNotify = 1; - } else { + } + else { /* CLOSE_NOTIFY already sent */ return 0; } diff --git a/tests/api.c b/tests/api.c index 68b1d4c361..4807833c29 100644 --- a/tests/api.c +++ b/tests/api.c @@ -81186,6 +81186,91 @@ static int test_extra_alerts_bad_psk(void) } #endif +#ifdef OPENSSL_EXTRA +/* + * Emulates wolfSSL_shutdown that goes on EAGAIN, + * by returning on output WOLFSSL_ERROR_WANT_WRITE.*/ +static int custom_wolfSSL_shutdown(WOLFSSL *ssl, char *buf, + int sz, void *ctx) +{ + (void)ssl; + (void)buf; + (void)ctx; + (void)sz; + + return WOLFSSL_CBIO_ERR_WANT_WRITE; +} + +static int test_multiple_alerts_EAGAIN(void) +{ + EXPECT_DECLS; + CallbackIOSend copy_current_io_cb = NULL; + size_t size_of_last_packet = 0; + + /* declare wolfSSL objects */ + struct test_memio_ctx test_ctx; + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + + /* Create and initialize WOLFSSL_CTX and WOLFSSL objects */ +#ifdef USE_TLSV13 + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); +#else + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); +#endif + ExpectNotNull(ctx_c); + ExpectNotNull(ssl_c); + ExpectNotNull(ctx_s); + ExpectNotNull(ssl_s); + + /* Load client certificates into WOLFSSL_CTX */ + ExpectIntEQ(wolfSSL_CTX_load_verify_locations(ctx_c, "./certs/ca-cert.pem", NULL), WOLFSSL_SUCCESS); + + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + + /* + * We set the custom callback for the IO to emulate multiple EAGAINs + * on shutdown, so we can check that we don't send multiple packets. + * */ + copy_current_io_cb = ssl_c->CBIOSend; + wolfSSL_SSLSetIOSend(ssl_c, custom_wolfSSL_shutdown); + + /* + * We call wolfSSL_shutdown multiple times to reproduce the behaviour, + * to check that it doesn't add the CLOSE_NOTIFY packet multiple times + * on the output buffer. + * */ + wolfSSL_shutdown(ssl_c); + wolfSSL_shutdown(ssl_c); + size_of_last_packet = ssl_c->buffers.outputBuffer.length; + wolfSSL_shutdown(ssl_c); + + /* + * Finally we check the length of the output buffer. + * */ + ExpectIntEQ((ssl_c->buffers.outputBuffer.length - size_of_last_packet), 0); + + wolfSSL_SSLSetIOSend(ssl_c, copy_current_io_cb); + + /* Cleanup and return */ + wolfSSL_CTX_free(ctx_c); + wolfSSL_free(ssl_c); + wolfSSL_CTX_free(ctx_s); + wolfSSL_free(ssl_s); + + return EXPECT_RESULT(); +} +#else +static int test_multiple_alerts_EAGAIN(void) +{ + return TEST_SKIPPED; +} +#endif + #if defined(WOLFSSL_TLS13) && defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES)\ && !defined(NO_PSK) static unsigned int test_tls13_bad_psk_binder_client_cb(WOLFSSL* ssl, @@ -86697,6 +86782,7 @@ TEST_CASE testCases[] = { TEST_DECL(test_extra_alerts_wrong_cs), TEST_DECL(test_extra_alerts_skip_hs), TEST_DECL(test_extra_alerts_bad_psk), + TEST_DECL(test_multiple_alerts_EAGAIN), TEST_DECL(test_tls13_bad_psk_binder), /* Can't memory test as client/server Asserts. */ TEST_DECL(test_harden_no_secure_renegotiation), From 577cce60dfb668a8309105d284cfa7a3f705b283 Mon Sep 17 00:00:00 2001 From: Reda Chouk Date: Wed, 21 Aug 2024 23:28:02 +0200 Subject: [PATCH 4/6] defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && !defined(WOLFSSL_NO_TLS12) --- tests/api.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/api.c b/tests/api.c index 4807833c29..43874da880 100644 --- a/tests/api.c +++ b/tests/api.c @@ -81186,7 +81186,7 @@ static int test_extra_alerts_bad_psk(void) } #endif -#ifdef OPENSSL_EXTRA +#if defined(OPENSSL_EXTRA) && defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && !defined(WOLFSSL_NO_TLS12) /* * Emulates wolfSSL_shutdown that goes on EAGAIN, * by returning on output WOLFSSL_ERROR_WANT_WRITE.*/ From f4a27772e064085c438cb224fe832eb4e8dc0c42 Mon Sep 17 00:00:00 2001 From: Reda Chouk Date: Fri, 23 Aug 2024 17:44:49 +0200 Subject: [PATCH 5/6] removed unnecessary copy of cb --- tests/api.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/api.c b/tests/api.c index 43874da880..6e5c93e04d 100644 --- a/tests/api.c +++ b/tests/api.c @@ -81204,7 +81204,6 @@ static int custom_wolfSSL_shutdown(WOLFSSL *ssl, char *buf, static int test_multiple_alerts_EAGAIN(void) { EXPECT_DECLS; - CallbackIOSend copy_current_io_cb = NULL; size_t size_of_last_packet = 0; /* declare wolfSSL objects */ @@ -81236,7 +81235,6 @@ static int test_multiple_alerts_EAGAIN(void) * We set the custom callback for the IO to emulate multiple EAGAINs * on shutdown, so we can check that we don't send multiple packets. * */ - copy_current_io_cb = ssl_c->CBIOSend; wolfSSL_SSLSetIOSend(ssl_c, custom_wolfSSL_shutdown); /* @@ -81254,8 +81252,6 @@ static int test_multiple_alerts_EAGAIN(void) * */ ExpectIntEQ((ssl_c->buffers.outputBuffer.length - size_of_last_packet), 0); - wolfSSL_SSLSetIOSend(ssl_c, copy_current_io_cb); - /* Cleanup and return */ wolfSSL_CTX_free(ctx_c); wolfSSL_free(ssl_c); From 8a6d7ff9a55bb4471143b137a7804daf72b73275 Mon Sep 17 00:00:00 2001 From: Reda Chouk Date: Fri, 23 Aug 2024 21:31:55 +0200 Subject: [PATCH 6/6] more clang-tidy edits --- tests/api.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/api.c b/tests/api.c index 6e5c93e04d..b920738521 100644 --- a/tests/api.c +++ b/tests/api.c @@ -81244,7 +81244,10 @@ static int test_multiple_alerts_EAGAIN(void) * */ wolfSSL_shutdown(ssl_c); wolfSSL_shutdown(ssl_c); - size_of_last_packet = ssl_c->buffers.outputBuffer.length; + + if (ssl_c != NULL) { + size_of_last_packet = ssl_c->buffers.outputBuffer.length; + } wolfSSL_shutdown(ssl_c); /*