From d76eb72c5b6a25ef6b081cb5da391c3cc18af0b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Frauenschl=C3=A4ger?= Date: Wed, 21 Oct 2026 11:58:27 +0200 Subject: [PATCH] Close the remaining paths that write after close_notify Review of the previous commit found more records, state changes and misleading errors after our close_notify or a fatal alert: - A TLS 1.2 server that sent close_notify still reset its handshake state for a renegotiation ClientHello, and with WOLFSSL_OP_NO_RENEGOTIATION, or when a DTLS epoch would wrap, still sent a no_renegotiation alert. It now skips the message, as the client does for a HelloRequest. - An ignored post-handshake CertificateRequest was still parsed, and its context stayed linked on ssl->certReqCtx, so a peer could grow that list without bound while we waited for its close_notify. The message is now skipped before parsing. - Dtls13DoScheduledWork() still sent the KeyUpdate that Dtls13CheckAEADFailLimit() schedules after too many failed decryptions. - SendAlert_ex() only marked the connection closed for alert_fatal. In TLS 1.3 every alert except close_notify and user_canceled is fatal, which DoAlert() already applies on receipt. An SNI callback returning warning_return sends such an alert, after which the server could still send 0.5-RTT application data. - A refused write reported the positive WOLFSSL_ERROR_WANT_READ, which wolfSSL_peek() leaves behind after a post-handshake message, instead of SSL_SHUTDOWN_ALREADY_DONE_E or SOCKET_PEER_CLOSED_E, so an event loop would wait for readability and retry forever. Add IsWriteClosed() for the sentNotify || fatalAlert check that now guards every one of these paths. Extend test_tls12_no_renegotiation_after_close_notify (handshake state and WOLFSSL_OP_NO_RENEGOTIATION) and test_tls13_no_pha_response_after_close_notify (certReqCtx), and add test_tls13_refused_write_after_peek, test_dtls13_no_key_update_after_close_notify and test_tls13_sent_warning_error_alert_is_fatal. --- src/dtls13.c | 8 ++- src/internal.c | 40 +++++++---- src/ssl_api_rw.c | 2 +- src/tls13.c | 11 +++- tests/api/test_tls13.c | 146 ++++++++++++++++++++++++++++++++++++++++- tests/api/test_tls13.h | 6 ++ wolfssl/internal.h | 1 + 7 files changed, 192 insertions(+), 22 deletions(-) diff --git a/src/dtls13.c b/src/dtls13.c index fd3da647c0..2397ebffff 100644 --- a/src/dtls13.c +++ b/src/dtls13.c @@ -3444,9 +3444,11 @@ int Dtls13DoScheduledWork(WOLFSSL* ssl) if (ssl->dtls13DoKeyUpdate) { ssl->dtls13DoKeyUpdate = 0; - ret = Tls13UpdateKeys(ssl); - if (ret != 0) - return ret; + if (!IsWriteClosed(ssl)) { + ret = Tls13UpdateKeys(ssl); + if (ret != 0) + return ret; + } } return 0; diff --git a/src/internal.c b/src/internal.c index e382b42d0f..b71e7d25de 100644 --- a/src/internal.c +++ b/src/internal.c @@ -805,6 +805,11 @@ int IsAtLeastTLSv1_3(const ProtocolVersion pv) return ret; } +int IsWriteClosed(const WOLFSSL* ssl) +{ + return ssl->options.sentNotify || ssl->options.fatalAlert; +} + int IsEncryptionOn(const WOLFSSL* ssl, int isSend) { if (ssl == NULL) { @@ -20608,7 +20613,7 @@ static int DoHelloRequest(WOLFSSL* ssl, word32 size) WOLFSSL_ERROR_VERBOSE(FATAL_ERROR); return FATAL_ERROR; } - else if (ssl->options.sentNotify || ssl->options.fatalAlert) { + else if (IsWriteClosed(ssl)) { WOLFSSL_MSG("Ignoring HelloRequest after close_notify"); WOLFSSL_LEAVE("DoHelloRequest", 0); WOLFSSL_END(WC_FUNC_HELLO_REQUEST_DO); @@ -21439,6 +21444,16 @@ int DoHandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx, expectedIdx = *inOutIdx + size; +#ifndef NO_WOLFSSL_SERVER + if (ssl->options.side == WOLFSSL_SERVER_END && + ssl->options.handShakeDone && type == client_hello && + IsWriteClosed(ssl)) { + WOLFSSL_MSG("Ignoring ClientHello after close_notify"); + *inOutIdx = expectedIdx; + return 0; + } +#endif + #if !defined(NO_WOLFSSL_SERVER) && \ defined(HAVE_SECURE_RENEGOTIATION) && \ defined(HAVE_SERVER_RENEGOTIATION_INFO) @@ -30388,9 +30403,7 @@ int SendData(WOLFSSL* ssl, const void* data, size_t sz) /* We can send data without waiting on peer finished msg */ WOLFSSL_MSG("server sending data before receiving client finished"); } - /* No handshake records after our close_notify or a fatal alert */ - else if (!ssl->options.sentNotify && !ssl->options.fatalAlert && - ssl_in_handshake(ssl, 1)) { + else if (!IsWriteClosed(ssl) && ssl_in_handshake(ssl, 1)) { int err; WOLFSSL_MSG("handshake not complete, trying to finish"); if ( (err = wolfSSL_negotiate(ssl)) != WOLFSSL_SUCCESS) { @@ -30416,8 +30429,7 @@ int SendData(WOLFSSL* ssl, const void* data, size_t sz) } #endif /* WOLFSSL_DTLS13 */ #ifdef WOLFSSL_TLS13 - if (ssl->options.sendKeyUpdate && !ssl->options.sentNotify && - !ssl->options.fatalAlert) { + if (ssl->options.sendKeyUpdate && !IsWriteClosed(ssl)) { ssl->options.sendKeyUpdate = 0; ret = SendTls13KeyUpdate(ssl); if (ret != 0) { @@ -30469,7 +30481,8 @@ int SendData(WOLFSSL* ssl, const void* data, size_t sz) /* RFC 8446 6.2 - no new data write after a fatal alert */ if (ssl->options.fatalAlert) { WOLFSSL_MSG("Connection is closed, not sending new data"); - if (ssl->error == 0) + if (ssl->error == 0 || ssl->error == WC_NO_ERR_TRACE(WANT_READ) || + ssl->error == WOLFSSL_ERROR_WANT_READ) ssl->error = SOCKET_PEER_CLOSED_E; return WOLFSSL_FATAL_ERROR; } @@ -30477,7 +30490,8 @@ int SendData(WOLFSSL* ssl, const void* data, size_t sz) /* Reads and wolfSSL_shutdown() clear this; the peer may still send data */ if (ssl->options.sentNotify) { WOLFSSL_MSG("close_notify already sent, not sending new data"); - if (ssl->error == 0 || ssl->error == WC_NO_ERR_TRACE(WANT_READ)) + if (ssl->error == 0 || ssl->error == WC_NO_ERR_TRACE(WANT_READ) || + ssl->error == WOLFSSL_ERROR_WANT_READ) ssl->error = SSL_SHUTDOWN_ALREADY_DONE_E; return WOLFSSL_FATAL_ERROR; } @@ -30856,9 +30870,7 @@ int ReceiveData(WOLFSSL* ssl, byte* output, size_t sz, int peek) else #endif { - /* No handshake records after our close_notify or a fatal alert */ - if (!ssl->options.sentNotify && !ssl->options.fatalAlert && - ssl_in_handshake(ssl, 0)) { + if (!IsWriteClosed(ssl) && ssl_in_handshake(ssl, 0)) { int err; WOLFSSL_MSG("Handshake not complete, trying to finish"); if ( (err = wolfSSL_negotiate(ssl)) != WOLFSSL_SUCCESS) { @@ -30927,7 +30939,7 @@ startScr: if (ssl->secure_renegotiation && ssl->secure_renegotiation->enabled && ssl->options.handShakeState != HANDSHAKE_DONE && ssl->buffers.clearOutputBuffer.length == 0 - && !ssl->options.sentNotify && !ssl->options.fatalAlert) { + && !IsWriteClosed(ssl)) { /* ProcessReply processed a handshake packet and not any APP DATA * so let's move the handshake along */ int err; @@ -31084,7 +31096,9 @@ static int SendAlert_ex(WOLFSSL* ssl, int severity, int type) input[1] = (byte)type; ssl->alert_history.last_tx.code = type; ssl->alert_history.last_tx.level = severity; - if (severity == alert_fatal) { + /* RFC 9846 Section 6.2: TLS 1.3 error alerts are fatal at any level */ + if (severity == alert_fatal || (IsAtLeastTLSv1_3(ssl->version) && + type != close_notify && type != user_canceled)) { #ifdef WOLFSSL_DTLS /* Mark as closed in dtls only once we enter stateful mode. */ if (!ssl->options.dtls || ssl->options.dtlsStateful) diff --git a/src/ssl_api_rw.c b/src/ssl_api_rw.c index 1d0a8ce18e..7913b08193 100644 --- a/src/ssl_api_rw.c +++ b/src/ssl_api_rw.c @@ -142,7 +142,7 @@ static int wolfssl_write_dup_do_tls13_work(WOLFSSL* ssl) { int ret = 0; - if (ssl->options.sentNotify || ssl->options.fatalAlert) { + if (IsWriteClosed(ssl)) { WOLFSSL_MSG("Dropping delegated responses after close_notify"); #ifdef WOLFSSL_POST_HANDSHAKE_AUTH ssl->postHandshakeAuthPending = 0; diff --git a/src/tls13.c b/src/tls13.c index acae5d5121..80123e709e 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -6617,6 +6617,13 @@ static int DoTls13CertificateRequest(WOLFSSL* ssl, const byte* input, WOLFSSL_START(WC_FUNC_CERTIFICATE_REQUEST_DO); WOLFSSL_ENTER("DoTls13CertificateRequest"); +#ifdef WOLFSSL_POST_HANDSHAKE_AUTH + if (ssl->options.handShakeState == HANDSHAKE_DONE && IsWriteClosed(ssl)) { + *inOutIdx = begin + size; + return 0; + } +#endif + XMEMSET(&peerSuites, 0, sizeof(Suites)); #if !defined(WOLFSSL_NO_SIGALG) /* Post-handshake auth can deliver several requests; each one's cert @@ -14445,7 +14452,7 @@ static int DoTls13KeyUpdate(WOLFSSL* ssl, const byte* input, word32* inOutIdx, #endif /* WOLFSSL_DTLS13 */ if (ssl->keys.keyUpdateRespond) { - if (ssl->options.sentNotify || ssl->options.fatalAlert) { + if (IsWriteClosed(ssl)) { WOLFSSL_MSG("Not answering KeyUpdate after close_notify"); ssl->keys.keyUpdateRespond = 0; return 0; @@ -16401,7 +16408,7 @@ int DoTls13HandShakeMsgType(WOLFSSL* ssl, byte* input, word32* inOutIdx, if (ssl->options.side == WOLFSSL_CLIENT_END) { if (type == certificate_request && ssl->options.handShakeState == HANDSHAKE_DONE) { - if (ssl->options.sentNotify || ssl->options.fatalAlert) { + if (IsWriteClosed(ssl)) { WOLFSSL_MSG("Ignoring CertificateRequest after " "close_notify"); } diff --git a/tests/api/test_tls13.c b/tests/api/test_tls13.c index 20ecc20d64..5c2097ecd5 100644 --- a/tests/api/test_tls13.c +++ b/tests/api/test_tls13.c @@ -13768,8 +13768,10 @@ int test_tls13_no_pha_response_after_close_notify(void) (int)sizeof(msg)); ExpectBufEQ(readBuf, msg, sizeof(msg)); ExpectIntEQ(test_ctx.s_len, s_len_before); - if (ssl_c != NULL) + if (ssl_c != NULL) { ExpectIntEQ(ssl_c->options.handShakeState, HANDSHAKE_DONE); + ExpectNull(ssl_c->certReqCtx); + } wolfSSL_free(ssl_c); wolfSSL_CTX_free(ctx_c); @@ -13855,14 +13857,18 @@ int test_tls12_no_renegotiation_after_close_notify(void) int *closerOutLen; int lenBefore; int serverCloses; + int mode; - for (serverCloses = 0; serverCloses < 2 && EXPECT_SUCCESS(); - serverCloses++) { + /* 0: client closes, 1: server closes, 2: also WOLFSSL_OP_NO_RENEGOTIATION */ + for (mode = 0; mode < 3 && EXPECT_SUCCESS(); mode++) { + serverCloses = (mode > 0); XMEMSET(&test_ctx, 0, sizeof(test_ctx)); ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, wolfTLSv1_2_client_method, wolfTLSv1_2_server_method), 0); ExpectIntEQ(wolfSSL_UseSecureRenegotiation(ssl_c), WOLFSSL_SUCCESS); ExpectIntEQ(wolfSSL_UseSecureRenegotiation(ssl_s), WOLFSSL_SUCCESS); + if (mode == 2) + wolfSSL_set_options(ssl_s, WOLFSSL_OP_NO_RENEGOTIATION); ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); closer = serverCloses ? ssl_s : ssl_c; peer = serverCloses ? ssl_c : ssl_s; @@ -13878,6 +13884,8 @@ int test_tls12_no_renegotiation_after_close_notify(void) ExpectIntEQ(wolfSSL_get_error(closer, WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)), WOLFSSL_ERROR_WANT_READ); ExpectIntEQ(*closerOutLen, lenBefore); + if (closer != NULL) + ExpectIntEQ(closer->options.handShakeState, HANDSHAKE_DONE); wolfSSL_free(ssl_c); ssl_c = NULL; @@ -13892,6 +13900,138 @@ int test_tls12_no_renegotiation_after_close_notify(void) return EXPECT_RESULT(); } +int test_tls13_refused_write_after_peek(void) +{ + EXPECT_DECLS; +#if defined(WOLFSSL_TLS13) && defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ + defined(HAVE_SESSION_TICKET) && \ + !defined(WOLFSSL_TLS13_NO_PEEK_HANDSHAKE_DONE) && \ + !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) + WOLFSSL_CTX *ctx_c = NULL; + WOLFSSL_CTX *ctx_s = NULL; + WOLFSSL *ssl_c = NULL; + WOLFSSL *ssl_s = NULL; + struct test_memio_ctx test_ctx; + char buf[32]; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + + ExpectIntEQ(wolfSSL_shutdown(ssl_c), WOLFSSL_SHUTDOWN_NOT_DONE); + ExpectIntEQ(wolfSSL_send_SessionTicket(ssl_s), WOLFSSL_SUCCESS); + /* The ticket leaves the positive WOLFSSL_ERROR_WANT_READ in ssl->error */ + ExpectIntEQ(wolfSSL_peek(ssl_c, buf, (int)sizeof(buf)), 0); + ExpectIntEQ(wolfSSL_get_error(ssl_c, 0), WOLFSSL_ERROR_WANT_READ); + ExpectIntEQ(wolfSSL_write(ssl_c, buf, (int)sizeof(buf)), + WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)); + ExpectIntEQ(wolfSSL_get_error(ssl_c, WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)), + WC_NO_ERR_TRACE(SSL_SHUTDOWN_ALREADY_DONE_E)); + + wolfSSL_free(ssl_c); + wolfSSL_CTX_free(ctx_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + +int test_dtls13_no_key_update_after_close_notify(void) +{ + EXPECT_DECLS; +#if defined(WOLFSSL_DTLS13) && !defined(WOLFSSL_RW_THREADED) && \ + defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ + !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) + WOLFSSL_CTX *ctx_c = NULL; + WOLFSSL_CTX *ctx_s = NULL; + WOLFSSL *ssl_c = NULL; + WOLFSSL *ssl_s = NULL; + struct test_memio_ctx test_ctx; + char readBuf[32]; + char msg[] = "after close"; + int s_len_before; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfDTLSv1_3_client_method, wolfDTLSv1_3_server_method), 0); + ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0); + ExpectIntEQ(wolfSSL_write(ssl_s, msg, (int)sizeof(msg)), (int)sizeof(msg)); + ExpectIntEQ(wolfSSL_read(ssl_c, readBuf, (int)sizeof(readBuf)), + (int)sizeof(msg)); + ExpectIntEQ(wolfSSL_write(ssl_c, msg, (int)sizeof(msg)), (int)sizeof(msg)); + ExpectIntEQ(wolfSSL_read(ssl_s, readBuf, (int)sizeof(readBuf)), + (int)sizeof(msg)); + + ExpectIntEQ(wolfSSL_shutdown(ssl_c), WOLFSSL_SHUTDOWN_NOT_DONE); + ExpectIntEQ(wolfSSL_write(ssl_s, msg, (int)sizeof(msg)), (int)sizeof(msg)); + if (ssl_c != NULL) + ssl_c->dtls13DoKeyUpdate = 1; + s_len_before = test_ctx.s_len; + ExpectIntEQ(wolfSSL_read(ssl_c, readBuf, (int)sizeof(readBuf)), + (int)sizeof(msg)); + ExpectIntEQ(test_ctx.s_len, s_len_before); + + wolfSSL_free(ssl_c); + wolfSSL_CTX_free(ctx_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + +#if defined(WOLFSSL_TLS13) && defined(HAVE_SNI) && \ + defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ + !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) +static int test_sni_warn_cb(WOLFSSL* ssl, int* ad, void* arg) +{ + (void)ssl; + (void)arg; + *ad = unrecognized_name; + return warning_return; +} +#endif + +int test_tls13_sent_warning_error_alert_is_fatal(void) +{ + EXPECT_DECLS; +#if defined(WOLFSSL_TLS13) && defined(HAVE_SNI) && \ + defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \ + !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER) + WOLFSSL_CTX *ctx_c = NULL; + WOLFSSL_CTX *ctx_s = NULL; + WOLFSSL *ssl_c = NULL; + WOLFSSL *ssl_s = NULL; + struct test_memio_ctx test_ctx; + char msg[] = "early"; + int c_len_before; + + XMEMSET(&test_ctx, 0, sizeof(test_ctx)); + ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s, + wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0); + wolfSSL_CTX_set_servername_callback(ctx_s, test_sni_warn_cb); + ExpectIntEQ(wolfSSL_UseSNI(ssl_c, WOLFSSL_SNI_HOST_NAME, "example.com", + (word16)XSTRLEN("example.com")), WOLFSSL_SUCCESS); + + ExpectIntEQ(wolfSSL_connect(ssl_c), WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)); + ExpectIntEQ(wolfSSL_accept(ssl_s), WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)); + if (ssl_s != NULL) + ExpectIntEQ(ssl_s->options.fatalAlert, 1); + c_len_before = test_ctx.c_len; + ExpectIntEQ(wolfSSL_write(ssl_s, msg, (int)sizeof(msg)), + WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)); + if (ssl_s != NULL) + ExpectIntEQ(ssl_s->error, WC_NO_ERR_TRACE(SOCKET_PEER_CLOSED_E)); + ExpectIntEQ(test_ctx.c_len, c_len_before); + + wolfSSL_free(ssl_c); + wolfSSL_CTX_free(ctx_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + /* RFC 9846 Section 4.7.3, write-duplicate path. The read side of a write dup * cannot send, so a peer KeyUpdate(update_requested) is delegated to the write * side via dupWrite->keyUpdateRespond and sent from diff --git a/tests/api/test_tls13.h b/tests/api/test_tls13.h index b073f01de4..c0ce080b6e 100644 --- a/tests/api/test_tls13.h +++ b/tests/api/test_tls13.h @@ -167,6 +167,9 @@ int test_tls13_no_key_update_response_after_close_notify(void); int test_tls13_no_pha_response_after_close_notify(void); int test_tls13_write_dup_no_response_after_close_notify(void); int test_tls12_no_renegotiation_after_close_notify(void); +int test_tls13_refused_write_after_peek(void); +int test_dtls13_no_key_update_after_close_notify(void); +int test_tls13_sent_warning_error_alert_is_fatal(void); int test_tls13_pqc_hybrid_async_server(void); int test_tls13_pha_status_request(void); int test_tls13_x25519_keyshare_masks_reserved_bit(void); @@ -342,6 +345,9 @@ int test_tls13_cks_hrr_reparse(void); TEST_DECL_GROUP("tls13", test_tls13_no_pha_response_after_close_notify), \ TEST_DECL_GROUP("tls13", test_tls13_write_dup_no_response_after_close_notify), \ TEST_DECL_GROUP("tls13", test_tls12_no_renegotiation_after_close_notify), \ + TEST_DECL_GROUP("tls13", test_tls13_refused_write_after_peek), \ + TEST_DECL_GROUP("tls13", test_dtls13_no_key_update_after_close_notify), \ + TEST_DECL_GROUP("tls13", test_tls13_sent_warning_error_alert_is_fatal), \ TEST_DECL_GROUP("tls13", test_tls13_pqc_hybrid_async_server), \ TEST_DECL_GROUP("tls13", test_tls13_x25519_keyshare_masks_reserved_bit), \ TEST_DECL_GROUP("tls13", test_tls13_is_init_finished_want_write), \ diff --git a/wolfssl/internal.h b/wolfssl/internal.h index f5e541e8f1..6f9c8cddb6 100644 --- a/wolfssl/internal.h +++ b/wolfssl/internal.h @@ -7782,6 +7782,7 @@ WOLFSSL_LOCAL int IsTLS(const WOLFSSL* ssl); WOLFSSL_LOCAL int IsTLS_ex(const ProtocolVersion pv); WOLFSSL_LOCAL int IsAtLeastTLSv1_2(const WOLFSSL* ssl); WOLFSSL_LOCAL int IsAtLeastTLSv1_3(const ProtocolVersion pv); +WOLFSSL_LOCAL int IsWriteClosed(const WOLFSSL* ssl); WOLFSSL_LOCAL int IsEncryptionOn(const WOLFSSL* ssl, int isSend); WOLFSSL_LOCAL int TLSv1_3_Capable(WOLFSSL* ssl);