Merge pull request #10349 from rizlik/dtls13_rtx_fixes

DTLS13:  Fixes unnecessary client rtx and increase server robustness

Author: SparkiDev

src/dtls13.c

@@ -897,7 +897,7 @@ static void Dtls13RtxRemoveCurAck(WOLFSSL* ssl)
 #endif
 }
 
-static void Dtls13MaybeSaveClientHello(WOLFSSL* ssl)
+static void Dtls13SaveOrFlushClientHello(WOLFSSL* ssl)
 {
     Dtls13RtxRecord *r, **prev_next;
 
@@ -906,15 +906,18 @@ static void Dtls13MaybeSaveClientHello(WOLFSSL* ssl)
 
     if (ssl->options.side == WOLFSSL_CLIENT_END &&
         ssl->options.connectState >= CLIENT_HELLO_SENT &&
-        ssl->options.connectState <= HELLO_AGAIN_REPLY &&
-        ssl->options.downgrade && ssl->options.minDowngrade >= DTLSv1_2_MINOR) {
+        ssl->options.connectState <= HELLO_AGAIN_REPLY) {
         while (r != NULL) {
             if (r->handshakeType == client_hello) {
                 Dtls13RtxRecordUnlink(ssl, prev_next, r);
-                XFREE(ssl->dtls13ClientHello, ssl->heap, DYNAMIC_TYPE_DTLS_MSG);
-                ssl->dtls13ClientHello = r->data;
-                ssl->dtls13ClientHelloSz = r->length;
-                r->data = NULL;
+                if (ssl->options.downgrade &&
+                        ssl->options.minDowngrade >= DTLSv1_2_MINOR) {
+                    XFREE(ssl->dtls13ClientHello, ssl->heap,
+                        DYNAMIC_TYPE_DTLS_MSG);
+                    ssl->dtls13ClientHello = r->data;
+                    ssl->dtls13ClientHelloSz = r->length;
+                    r->data = NULL;
+                }
                 Dtls13FreeRtxBufferRecord(ssl, r);
                 return;
             }
@@ -934,7 +937,7 @@ static int Dtls13RtxMsgRecvd(WOLFSSL* ssl, enum HandShakeType hs,
             ssl->keys.dtls_expected_peer_handshake_number) {
 
         if (hs == server_hello)
-            Dtls13MaybeSaveClientHello(ssl);
+            Dtls13SaveOrFlushClientHello(ssl);
 
         /* In the handshake, receiving part of the next flight, acknowledge the
          * sent flight. */
@@ -1869,13 +1872,15 @@ static int _Dtls13HandshakeRecv(WOLFSSL* ssl, byte* input, word32 size,
             *processedSize = size;
             return 0;
         }
-        /* To be able to operate in stateless mode, we assume the ClientHello
-         * is in order and we use its Handshake Message number and Sequence
-         * Number for our Tx. */
-        ssl->keys.dtls_expected_peer_handshake_number =
-            ssl->keys.dtls_handshake_number =
-                ssl->keys.dtls_peer_handshake_number;
-        ssl->dtls13Epochs[0].nextSeqNumber = ssl->keys.curSeq;
+        if (!ssl->options.dtlsStateful) {
+            /* To be able to operate in stateless mode, we assume the
+             * ClientHello is in order and we use its Handshake Message number
+             * and Sequence Number for our Tx. */
+            ssl->keys.dtls_expected_peer_handshake_number =
+                ssl->keys.dtls_handshake_number =
+                    ssl->keys.dtls_peer_handshake_number;
+            ssl->dtls13Epochs[0].nextSeqNumber = ssl->keys.curSeq;
+        }
     }
 
     if (idx + fragLength > size) {

tests/api/test_dtls.c

@@ -1836,6 +1836,225 @@ int test_dtls_rtx_across_epoch_change(void)
     return EXPECT_RESULT();
 }
 
+#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) &&                           \
+    defined(WOLFSSL_DTLS13) && defined(WOLFSSL_DTLS)
+static int test_dtls13_get_message_seq(const char* msg, int msgSz,
+    word16* msgSeq)
+{
+    int hsOff = DTLS_RECORD_HEADER_SZ;
+
+    if (msg == NULL || msgSeq == NULL ||
+            msgSz < DTLS_RECORD_HEADER_SZ + DTLS_HANDSHAKE_HEADER_SZ) {
+        return BAD_FUNC_ARG;
+    }
+
+    *msgSeq = ((word16)(byte)msg[hsOff + 4] << 8) |
+              (word16)(byte)msg[hsOff + 5];
+
+    return WOLFSSL_SUCCESS;
+}
+#endif
+
+int test_dtls13_ch2_rtx_no_ch1(void)
+{
+    EXPECT_DECLS;
+#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) &&                           \
+    defined(WOLFSSL_DTLS13) && defined(WOLFSSL_DTLS)
+    WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
+    WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
+    struct test_memio_ctx test_ctx;
+    const char* msg = NULL;
+    int msgSz = 0;
+    word16 ch1Seq = 0;
+    int i;
+    int foundCh1Seq = 0;
+
+    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);
+
+    /* To force HRR */
+    ExpectIntEQ(wolfSSL_NoKeyShares(ssl_c), WOLFSSL_SUCCESS);
+
+    /* CH1 */
+    ExpectIntEQ(wolfSSL_connect(ssl_c), -1);
+    ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ);
+    ExpectIntEQ(test_memio_get_message(&test_ctx, 0, &msg, &msgSz, 0), 0);
+    ExpectIntGE(msgSz, DTLS_RECORD_HEADER_SZ + DTLS_HANDSHAKE_HEADER_SZ);
+    ExpectIntEQ(test_dtls13_get_message_seq(msg, msgSz, &ch1Seq),
+        WOLFSSL_SUCCESS);
+
+    /* HRR */
+    ExpectIntEQ(wolfSSL_accept(ssl_s), -1);
+    ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), WOLFSSL_ERROR_WANT_READ);
+    ExpectIntGT(test_ctx.c_msg_count, 0);
+
+    /* CH2 */
+    ExpectIntEQ(wolfSSL_connect(ssl_c), -1);
+    ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ);
+    ExpectIntGT(test_ctx.s_msg_count, 0);
+
+    /* Drop CH2 and trigger the client retransmission timeout. */
+    test_memio_clear_buffer(&test_ctx, 0);
+    if (wolfSSL_dtls13_use_quick_timeout(ssl_c))
+        ExpectIntEQ(wolfSSL_dtls_got_timeout(ssl_c), WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_dtls_got_timeout(ssl_c), WOLFSSL_SUCCESS);
+    ExpectIntGT(test_ctx.s_msg_count, 0);
+
+    for (i = 0; i < test_ctx.s_msg_count && EXPECT_SUCCESS(); i++) {
+        int hsOff = DTLS_RECORD_HEADER_SZ;
+        word16 msgSeq = 0;
+
+        ExpectIntEQ(test_memio_get_message(&test_ctx, 0, &msg, &msgSz, i), 0);
+        /* memio stores one DTLS record per message in this handshake path. */
+        if (msgSz >= DTLS_RECORD_HEADER_SZ + DTLS_HANDSHAKE_HEADER_SZ &&
+                (byte)msg[0] == handshake && msg[hsOff] == client_hello) {
+            ExpectIntEQ(test_dtls13_get_message_seq(msg, msgSz, &msgSeq),
+                WOLFSSL_SUCCESS);
+            if (msgSeq == ch1Seq)
+                foundCh1Seq = 1;
+        }
+    }
+
+    ExpectIntEQ(foundCh1Seq, 0);
+
+    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_frag_ch2_with_ch1_rtx(void)
+{
+    EXPECT_DECLS;
+#if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) &&                           \
+    defined(WOLFSSL_DTLS13) && defined(WOLFSSL_DTLS) &&                         \
+    defined(WOLFSSL_DTLS_MTU) && defined(WOLFSSL_DTLS_CH_FRAG)
+    WOLFSSL_CTX *ctx_c = NULL, *ctx_s = NULL;
+    WOLFSSL *ssl_c = NULL, *ssl_s = NULL;
+    struct test_memio_ctx test_ctx;
+    char hrr[TEST_MEMIO_BUF_SZ];
+    int hrrSz = (int)sizeof(hrr);
+    char ch1Rtx[TEST_MEMIO_BUF_SZ];
+    int ch1RtxSz = (int)sizeof(ch1Rtx);
+    char ch2[TEST_MEMIO_BUF_SZ];
+    int ch2Sz = 0;
+    int ch2MsgCount = 0;
+    int ch2MsgSizes[TEST_MEMIO_MAX_MSGS] = {0};
+    /* The DTLS record sequence number occupies the last 8 bytes of the
+     * record header. */
+    int recordSeqOff = DTLS_RECORD_HEADER_SZ - 8;
+    int ch2Seq = 0;
+    int ch1RtxSeq = 0;
+    int off;
+    int i;
+
+    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);
+
+    /* To force HRR */
+    ExpectIntEQ(wolfSSL_NoKeyShares(ssl_c), WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_dtls13_allow_ch_frag(ssl_s, 1), WOLFSSL_SUCCESS);
+
+    /* CH1 */
+    ExpectIntEQ(wolfSSL_connect(ssl_c), -1);
+    ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ);
+
+    /* HRR */
+    ExpectIntEQ(wolfSSL_accept(ssl_s), -1);
+    ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), WOLFSSL_ERROR_WANT_READ);
+    ExpectIntEQ(test_memio_copy_message(&test_ctx, 1, hrr, &hrrSz, 0), 0);
+
+    /* Drop HRR, trigger CH1 retransmission, copy and drop it */
+    test_memio_clear_buffer(&test_ctx, 1);
+    if (wolfSSL_dtls13_use_quick_timeout(ssl_c))
+        ExpectIntEQ(wolfSSL_dtls_got_timeout(ssl_c), WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_dtls_got_timeout(ssl_c), WOLFSSL_SUCCESS);
+    ExpectIntEQ(test_memio_copy_message(&test_ctx, 0, ch1Rtx, &ch1RtxSz, 0), 0);
+    test_memio_clear_buffer(&test_ctx, 0);
+
+    /* Force CH2 fragmentation. MTU must be small enough to fragment but large
+     * enough that the cookie extension lands in the first fragment, otherwise
+     * the server can't validate it statelessly and the test scenario (server
+     * stateful after frag 1) does not hold. With --enable-all (PQ groups in
+     * supported_groups), the cookie extension can sit ~400 bytes into CH2; 600
+     * gives margin while still producing multiple fragments (CH2 is ~2KB). */
+    ExpectIntEQ(wolfSSL_dtls_set_mtu(ssl_c, 600), WOLFSSL_SUCCESS);
+
+    /* Forward HRR and let the client create fragmented CH2 */
+    ExpectIntEQ(test_memio_inject_message(&test_ctx, 1, hrr, hrrSz), 0);
+    ExpectIntEQ(wolfSSL_connect(ssl_c), -1);
+    ExpectIntEQ(wolfSSL_get_error(ssl_c, -1), WOLFSSL_ERROR_WANT_READ);
+
+    ExpectIntGT(test_ctx.s_msg_count, 1);
+    ExpectIntLE(test_ctx.s_msg_count, TEST_MEMIO_MAX_MSGS);
+    ExpectIntLE(test_ctx.s_len, (int)sizeof(ch2));
+    if (EXPECT_SUCCESS()) {
+        ch2Sz = test_ctx.s_len;
+        ch2MsgCount = test_ctx.s_msg_count;
+        XMEMCPY(ch2, test_ctx.s_buff, ch2Sz);
+        XMEMCPY(ch2MsgSizes, test_ctx.s_msg_sizes,
+            sizeof(ch2MsgSizes[0]) * (size_t)ch2MsgCount);
+
+        ch2Seq = ((byte)ch2[recordSeqOff + 4] << 8) |
+                 (byte)ch2[recordSeqOff + 5];
+        ch1RtxSeq = ch2Seq + ch2MsgCount;
+
+        /* Synthesize a CH1 retransmission that can pass the replay window after
+         * the first CH2 fragment makes the server stateful. The handshake
+         * message_seq remains the original CH1 value; only the DTLS record
+         * sequence is moved past the fragmented CH2 flight */
+        ch1Rtx[recordSeqOff + 0] = 0;
+        ch1Rtx[recordSeqOff + 1] = 0;
+        ch1Rtx[recordSeqOff + 2] = 0;
+        ch1Rtx[recordSeqOff + 3] = 0;
+        ch1Rtx[recordSeqOff + 4] = (byte)(ch1RtxSeq >> 8);
+        ch1Rtx[recordSeqOff + 5] = (byte)ch1RtxSeq;
+    }
+
+    test_memio_clear_buffer(&test_ctx, 0);
+
+    /* Deliver CH2 first fragment only. Now server is stateful */
+    ExpectIntEQ(test_memio_inject_message(&test_ctx, 0, ch2, ch2MsgSizes[0]), 0);
+    ExpectIntEQ(wolfSSL_accept(ssl_s), -1);
+    ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), WOLFSSL_ERROR_WANT_READ);
+
+    /* Deliver the retransmitted CH1 between CH2 fragments, it should be
+     * discarded as rtx */
+    ExpectIntEQ(test_memio_inject_message(&test_ctx, 0, ch1Rtx, ch1RtxSz), 0);
+    ExpectIntEQ(wolfSSL_accept(ssl_s), -1);
+    ExpectIntEQ(wolfSSL_get_error(ssl_s, -1), WOLFSSL_ERROR_WANT_READ);
+    test_memio_clear_buffer(&test_ctx, 1);
+
+    /* Deliver the rest of CH2 */
+    off = ch2MsgSizes[0];
+    for (i = 1; i < ch2MsgCount && EXPECT_SUCCESS(); i++) {
+        ExpectIntEQ(test_memio_inject_message(&test_ctx, 0, ch2 + off,
+            ch2MsgSizes[i]), 0);
+        off += ch2MsgSizes[i];
+    }
+
+    /* Restore MTU so the client's input buffer can hold the full server
+     * flight (e.g. an SH carrying a hybrid PQC key share). */
+    ExpectIntEQ(wolfSSL_dtls_set_mtu(ssl_c, 1500), WOLFSSL_SUCCESS);
+
+    ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+
+    wolfSSL_free(ssl_c);
+    wolfSSL_CTX_free(ctx_c);
+    wolfSSL_free(ssl_s);
+    wolfSSL_CTX_free(ctx_s);
+#endif
+    return EXPECT_RESULT();
+}
+
 int test_dtls_drop_client_ack(void)
 {
     EXPECT_DECLS;

tests/api/test_dtls.h

@@ -41,6 +41,8 @@ int test_dtls13_short_read(void);
 int test_records_span_network_boundaries(void);
 int test_dtls_record_cross_boundaries(void);
 int test_dtls_rtx_across_epoch_change(void);
+int test_dtls13_ch2_rtx_no_ch1(void);
+int test_dtls13_frag_ch2_with_ch1_rtx(void);
 int test_dtls_drop_client_ack(void);
 int test_dtls_bogus_finished_epoch_zero(void);
 int test_dtls_replay(void);
@@ -75,6 +77,8 @@ int test_dtls13_oversized_cert_chain(void);
         TEST_DECL_GROUP("dtls", test_records_span_network_boundaries),         \
         TEST_DECL_GROUP("dtls", test_dtls_record_cross_boundaries),            \
         TEST_DECL_GROUP("dtls", test_dtls_rtx_across_epoch_change),            \
+        TEST_DECL_GROUP("dtls", test_dtls13_ch2_rtx_no_ch1),                   \
+        TEST_DECL_GROUP("dtls", test_dtls13_frag_ch2_with_ch1_rtx),            \
         TEST_DECL_GROUP("dtls", test_dtls_drop_client_ack),                    \
         TEST_DECL_GROUP("dtls", test_dtls_bogus_finished_epoch_zero),          \
         TEST_DECL_GROUP("dtls", test_dtls_replay),                             \