diff --git a/src/internal.c b/src/internal.c index e4585216985..76fc95f923a 100644 --- a/src/internal.c +++ b/src/internal.c @@ -26412,7 +26412,7 @@ static int DoProcessAppDataRecord(WOLFSSL* ssl) #endif #ifndef WOLFSSL_RW_THREADED #ifdef WOLFSSL_TLS13 - if (ssl->keys.keyUpdateRespond) { + if (ssl->keys.keyUpdateRespond && !ssl->options.sendKeyUpdate) { WOLFSSL_MSG("No KeyUpdate from peer seen"); WOLFSSL_ERROR_VERBOSE(SANITY_MSG_E); return SANITY_MSG_E; @@ -30439,16 +30439,6 @@ int SendData(WOLFSSL* ssl, const void* data, size_t sz) } } #endif /* WOLFSSL_DTLS13 */ -#ifdef WOLFSSL_TLS13 - if (ssl->options.sendKeyUpdate) { - ssl->options.sendKeyUpdate = 0; - ret = IsWriteClosed(ssl) ? 0 : SendTls13KeyUpdate(ssl); - if (ret != 0) { - ssl->error = BUILD_MSG_ERROR; - return WOLFSSL_FATAL_ERROR; - } - } -#endif #endif /* last time system socket output buffer was full, try again to send */ @@ -30492,6 +30482,15 @@ int SendData(WOLFSSL* ssl, const void* data, size_t sz) /* RFC 8446 6.1, 6.2 - no new data after close_notify or a fatal alert */ if (IsWriteClosed(ssl)) { WOLFSSL_MSG("Connection is closed, not sending new data"); + #ifdef WOLFSSL_TLS13 + /* remove a KeyUpdate response that was left for this call to send */ + if (ssl->options.sendKeyUpdate) { + ssl->options.sendKeyUpdate = 0; + #ifndef WOLFSSL_RW_THREADED + ssl->keys.keyUpdateRespond = 0; + #endif + } + #endif if (ssl->error == 0 || ssl->error == WC_NO_ERR_TRACE(WANT_READ) || ssl->error == WOLFSSL_ERROR_WANT_READ) { ssl->error = ssl->options.fatalAlert ? SOCKET_PEER_CLOSED_E : @@ -30500,6 +30499,36 @@ int SendData(WOLFSSL* ssl, const void* data, size_t sz) return WOLFSSL_FATAL_ERROR; } +#ifdef WOLFSSL_TLS13 + /* A KeyUpdate response the read side left for the write side to send. It + * goes out ahead of the application data, and only once the output buffer + * is flushed, so that the record of an earlier call is accounted for above + * instead of being sent from here and then built again. */ + if (ssl->options.sendKeyUpdate) { + /* RFC 9846 Section 4.7.3: at the key update limit, ignore the + * request rather than fail the write. */ + if (Tls13KeyUpdateLimitReached(ssl)) { + WOLFSSL_MSG("Key update limit reached; ignoring update_requested"); + ssl->options.sendKeyUpdate = 0; + #ifndef WOLFSSL_RW_THREADED + ssl->keys.keyUpdateRespond = 0; + #endif + ret = 0; + } + else { + /* For TLS, SendTls13KeyUpdate() clears the flag itself and sends + * the KeyUpdate as a response. */ + if (ssl->options.dtls) + ssl->options.sendKeyUpdate = 0; + ret = SendTls13KeyUpdate(ssl); + } + if (ret != 0) { + ssl->error = BUILD_MSG_ERROR; + return WOLFSSL_FATAL_ERROR; + } + } +#endif + #ifdef WOLFSSL_THREADED_CRYPT ret = SendAsyncData(ssl); if (ret != 0) { diff --git a/src/tls13.c b/src/tls13.c index 40946c8e0e6..ce3803111e2 100644 --- a/src/tls13.c +++ b/src/tls13.c @@ -14325,6 +14325,14 @@ int SendTls13KeyUpdate(WOLFSSL* ssl) AddTls13Headers(output, OPAQUE8_LEN, key_update, ssl); + /* A TLS response left for SendData() is owed even if a later + * update_not_requested cleared keyUpdateRespond, and this KeyUpdate, + * whoever asked for it, answers it. */ + if (!ssl->options.dtls && ssl->options.sendKeyUpdate) { + ssl->keys.keyUpdateRespond = 1; + ssl->options.sendKeyUpdate = 0; + } + /* If: * 1. I haven't sent a KeyUpdate requesting a response and * 2. This isn't responding to peer KeyUpdate requiring a response then, @@ -14510,11 +14518,20 @@ static int DoTls13KeyUpdate(WOLFSSL* ssl, const byte* input, word32* inOutIdx, } #ifndef WOLFSSL_RW_THREADED - return SendTls13KeyUpdate(ssl); -#else + /* RFC 8446 Section 4.6.3: the response is due before the next + * application data record, and one KeyUpdate answers every request + * received while we are silent. While earlier output is still waiting + * for the peer to read it, don't queue a response per request behind + * it; leave one for SendData() to send. DTLS has at most one KeyUpdate + * in flight, see above. */ + if (ssl->options.dtls || ssl->buffers.outputBuffer.length == 0) { + /* This response answers a deferred request as well. */ + ssl->options.sendKeyUpdate = 0; + return SendTls13KeyUpdate(ssl); + } +#endif ssl->options.sendKeyUpdate = 1; return 0; -#endif } WOLFSSL_LEAVE("DoTls13KeyUpdate", ret); diff --git a/tests/api/test_tls13.c b/tests/api/test_tls13.c index 18730cb07f5..5ebc7164b44 100644 --- a/tests/api/test_tls13.c +++ b/tests/api/test_tls13.c @@ -14716,6 +14716,217 @@ int test_tls13_KeyUpdate_limit_ignores_update_requested(void) return EXPECT_RESULT(); } +/* RFC 8446 Section 4.6.3: the response to a KeyUpdate is due before the next + * application data record, which "causes an implementation which receives + * multiple KeyUpdates while it is silent to respond with a single update". + * + * Block the client's transport and have the server request one update after + * the other. The first one is queued and a single response is + * left for the next wolfSSL_write() to send ahead of its data. + * + * Not with HAVE_IO_POOL: its one output buffer per thread can't hold the + * client's queued response while the server, on the same thread, writes. */ +int test_tls13_KeyUpdate_blocked_write(void) +{ + EXPECT_DECLS; +#if defined(WOLFSSL_TLS13) && !defined(NO_WOLFSSL_CLIENT) && \ + !defined(NO_WOLFSSL_SERVER) && !defined(WOLFSSL_RW_THREADED) && \ + defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && !defined(HAVE_IO_POOL) + struct test_memio_ctx test_ctx; + WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL; + WOLFSSL *ssl_c = NULL, *ssl_s = NULL; + const char msg[] = "key-update-blocked-write"; + char buf[sizeof(msg)]; + word32 queued = 0; + w64wrapper sCount = w64From32(0, 0); + int sLen = 0; + int i; + + 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); + + test_memio_simulate_want_write(&test_ctx, 1, 1); + for (i = 0; i < 8; i++) { + /* Have every KeyUpdate of the server request a response. */ + if (ssl_s != NULL) + ssl_s->keys.updateResponseReq = 0; + ExpectIntEQ(wolfSSL_update_keys(ssl_s), WOLFSSL_SUCCESS); + ExpectIntLT(wolfSSL_read(ssl_c, buf, (int)sizeof(buf)), 0); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + if (EXPECT_SUCCESS() && ssl_c != NULL) { + /* The first response is queued, the others are not. */ + if (i == 0) + queued = ssl_c->buffers.outputBuffer.length; + ExpectIntGT(queued, 0); + ExpectIntEQ(ssl_c->buffers.outputBuffer.length, queued); + } + } + if (EXPECT_SUCCESS() && ssl_c != NULL) { + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 1))); + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 1); + } + + /* Owing a response doesn't stop the client from reading. */ + ExpectIntEQ(wolfSSL_write(ssl_s, msg, (int)sizeof(msg)), (int)sizeof(msg)); + XMEMSET(buf, 0, sizeof(buf)); + ExpectIntEQ(wolfSSL_read(ssl_c, buf, (int)sizeof(buf)), (int)sizeof(msg)); + ExpectIntEQ(XMEMCMP(buf, msg, sizeof(msg)), 0); + + /* The response goes out ahead of the next application data. */ + test_memio_simulate_want_write(&test_ctx, 1, 0); + ExpectIntEQ(wolfSSL_write(ssl_c, msg, (int)sizeof(msg)), (int)sizeof(msg)); + if (EXPECT_SUCCESS() && ssl_c != NULL) { + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 2))); + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 0); + ExpectIntEQ(ssl_c->keys.keyUpdateRespond, 0); + ExpectIntEQ(ssl_c->buffers.outputBuffer.length, 0); + } + XMEMSET(buf, 0, sizeof(buf)); + ExpectIntEQ(wolfSSL_read(ssl_s, buf, (int)sizeof(buf)), (int)sizeof(msg)); + ExpectIntEQ(XMEMCMP(buf, msg, sizeof(msg)), 0); + if (EXPECT_SUCCESS() && ssl_s != NULL) + ExpectIntEQ(ssl_s->keys.updateResponseReq, 0); + + /* A record that is waiting for its wolfSSL_write() to be retried is sent + * by that retry, once, and the response follows it. */ + test_memio_simulate_want_write(&test_ctx, 1, 1); + ExpectIntLT(wolfSSL_write(ssl_c, msg, (int)sizeof(msg)), 0); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_WRITE); + ExpectIntEQ(wolfSSL_update_keys(ssl_s), WOLFSSL_SUCCESS); + ExpectIntLT(wolfSSL_read(ssl_c, buf, (int)sizeof(buf)), 0); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + if (EXPECT_SUCCESS() && ssl_c != NULL) { + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 2))); + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 1); + } + test_memio_simulate_want_write(&test_ctx, 1, 0); + ExpectIntEQ(wolfSSL_write(ssl_c, msg, (int)sizeof(msg)), (int)sizeof(msg)); + if (EXPECT_SUCCESS() && ssl_c != NULL) { + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 3))); + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 0); + } + XMEMSET(buf, 0, sizeof(buf)); + ExpectIntEQ(wolfSSL_read(ssl_s, buf, (int)sizeof(buf)), (int)sizeof(msg)); + ExpectIntEQ(XMEMCMP(buf, msg, sizeof(msg)), 0); + ExpectIntLT(wolfSSL_read(ssl_s, buf, (int)sizeof(buf)), 0); + ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), WOLFSSL_ERROR_WANT_READ); + if (EXPECT_SUCCESS() && ssl_s != NULL) + ExpectIntEQ(ssl_s->keys.updateResponseReq, 0); + + /* Once the output has drained, here through the application's own + * wolfSSL_update_keys(), the read side answers the next request itself + * and the deferred response is not owed a second time. */ + test_memio_simulate_want_write(&test_ctx, 1, 1); + for (i = 0; i < 2; i++) { + if (ssl_s != NULL) + ssl_s->keys.updateResponseReq = 0; + ExpectIntEQ(wolfSSL_update_keys(ssl_s), WOLFSSL_SUCCESS); + ExpectIntLT(wolfSSL_read(ssl_c, buf, (int)sizeof(buf)), 0); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + } + if (EXPECT_SUCCESS() && ssl_c != NULL) { + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 4))); + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 1); + } + test_memio_simulate_want_write(&test_ctx, 1, 0); + ExpectIntEQ(wolfSSL_update_keys(ssl_c), WOLFSSL_SUCCESS); + if (EXPECT_SUCCESS() && ssl_c != NULL) { + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 5))); + ExpectIntEQ(ssl_c->keys.keyUpdateRespond, 0); + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 0); + ExpectIntEQ(ssl_c->buffers.outputBuffer.length, 0); + } + if (ssl_s != NULL) + ssl_s->keys.updateResponseReq = 0; + ExpectIntEQ(wolfSSL_update_keys(ssl_s), WOLFSSL_SUCCESS); + ExpectIntLT(wolfSSL_read(ssl_c, buf, (int)sizeof(buf)), 0); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + if (EXPECT_SUCCESS() && ssl_c != NULL) { + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 6))); + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 0); + ExpectIntEQ(ssl_c->buffers.outputBuffer.length, 0); + } + ExpectIntEQ(wolfSSL_write(ssl_c, msg, (int)sizeof(msg)), (int)sizeof(msg)); + if (EXPECT_SUCCESS() && ssl_c != NULL) + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 6))); + XMEMSET(buf, 0, sizeof(buf)); + ExpectIntEQ(wolfSSL_read(ssl_s, buf, (int)sizeof(buf)), (int)sizeof(msg)); + ExpectIntEQ(XMEMCMP(buf, msg, sizeof(msg)), 0); + if (EXPECT_SUCCESS() && ssl_s != NULL) + ExpectIntEQ(ssl_s->keys.updateResponseReq, 0); + + /* An update_not_requested that arrives after a response was deferred + * doesn't cancel it, and the response is still update_not_requested: + * the server doesn't have to answer it. Requests: answered and queued, + * deferred, then update_not_requested. */ + test_memio_simulate_want_write(&test_ctx, 1, 1); + for (i = 0; i < 3; i++) { + if (ssl_s != NULL) + ssl_s->keys.updateResponseReq = (i == 2); + ExpectIntEQ(wolfSSL_update_keys(ssl_s), WOLFSSL_SUCCESS); + ExpectIntLT(wolfSSL_read(ssl_c, buf, (int)sizeof(buf)), 0); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + } + if (EXPECT_SUCCESS() && ssl_c != NULL) { + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 7))); + ExpectIntEQ(ssl_c->keys.keyUpdateRespond, 0); + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 1); + } + if (ssl_s != NULL) + sCount = ssl_s->keys.keyUpdateCount; + test_memio_simulate_want_write(&test_ctx, 1, 0); + ExpectIntEQ(wolfSSL_write(ssl_c, msg, (int)sizeof(msg)), (int)sizeof(msg)); + if (EXPECT_SUCCESS() && ssl_c != NULL) { + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 8))); + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 0); + ExpectIntEQ(ssl_c->keys.updateResponseReq, 0); + } + XMEMSET(buf, 0, sizeof(buf)); + ExpectIntEQ(wolfSSL_read(ssl_s, buf, (int)sizeof(buf)), (int)sizeof(msg)); + ExpectIntEQ(XMEMCMP(buf, msg, sizeof(msg)), 0); + if (EXPECT_SUCCESS() && ssl_s != NULL) { + /* Neither KeyUpdate from the client asked the server to answer. */ + ExpectTrue(w64Equal(ssl_s->keys.keyUpdateCount, sCount)); + ExpectIntEQ(ssl_s->keys.keyUpdateRespond, 0); + } + + /* A deferred response is dropped, not sent, once the client has sent + * close_notify. */ + test_memio_simulate_want_write(&test_ctx, 1, 1); + for (i = 0; i < 2; i++) { + if (ssl_s != NULL) + ssl_s->keys.updateResponseReq = 0; + ExpectIntEQ(wolfSSL_update_keys(ssl_s), WOLFSSL_SUCCESS); + ExpectIntLT(wolfSSL_read(ssl_c, buf, (int)sizeof(buf)), 0); + ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ); + } + if (EXPECT_SUCCESS() && ssl_c != NULL) + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 1); + test_memio_simulate_want_write(&test_ctx, 1, 0); + ExpectIntEQ(wolfSSL_shutdown(ssl_c), WOLFSSL_SHUTDOWN_NOT_DONE); + sLen = test_ctx.s_len; + ExpectIntEQ(wolfSSL_write(ssl_c, msg, (int)sizeof(msg)), + 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)); + ExpectIntEQ(test_ctx.s_len, sLen); + if (EXPECT_SUCCESS() && ssl_c != NULL) { + ExpectTrue(w64Equal(ssl_c->keys.keyUpdateCount, w64From32(0, 9))); + ExpectIntEQ(ssl_c->options.sendKeyUpdate, 0); + ExpectIntEQ(ssl_c->keys.keyUpdateRespond, 0); + } + + wolfSSL_free(ssl_c); + wolfSSL_free(ssl_s); + wolfSSL_CTX_free(ctx_c); + wolfSSL_CTX_free(ctx_s); +#endif + return EXPECT_RESULT(); +} + /* RFC 9846 Section 5.5: "it is not possible to perform a KeyUpdate for early * data; therefore, implementations MUST NOT exceed the limits when sending * early data." diff --git a/tests/api/test_tls13.h b/tests/api/test_tls13.h index 123831145b1..387ba8bf922 100644 --- a/tests/api/test_tls13.h +++ b/tests/api/test_tls13.h @@ -150,6 +150,7 @@ int test_tls13_AEAD_limit_KU_aes128_ccm_sha256(void); int test_tls13_AEAD_limit_KU_aes128_ccm_8_sha256(void); int test_tls13_KeyUpdate_sender_limit(void); int test_tls13_KeyUpdate_limit_ignores_update_requested(void); +int test_tls13_KeyUpdate_blocked_write(void); int test_tls13_KeyUpdate_limit_writedup(void); int test_tls13_extension_trailing_data_alert(void); int test_tls13_early_data_AEAD_limit(void); @@ -330,6 +331,7 @@ int test_tls13_cks_hrr_reparse(void); TEST_DECL_GROUP("tls13", test_tls13_AEAD_limit_KU_aes128_ccm_8_sha256), \ TEST_DECL_GROUP("tls13", test_tls13_KeyUpdate_sender_limit), \ TEST_DECL_GROUP("tls13", test_tls13_KeyUpdate_limit_ignores_update_requested), \ + TEST_DECL_GROUP("tls13", test_tls13_KeyUpdate_blocked_write), \ TEST_DECL_GROUP("tls13", test_tls13_KeyUpdate_limit_writedup), \ TEST_DECL_GROUP("tls13", test_tls13_extension_trailing_data_alert), \ TEST_DECL_GROUP("tls13", test_tls13_early_data_AEAD_limit), \