Fix handling of keyupdate flood

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) {

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);

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."

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), \