[+] fix issue #756 key update initiator must wait for ACK before confirming

Author: cherylsy

include/xquic/xqc_errno.h

@@ -25,7 +25,15 @@ typedef enum {
     TRA_INVALID_TOKEN               =  0xB,
     TRA_APPLICATION_ERROR           =  0xC,
     TRA_CRYPTO_BUFFER_EXCEEDED      =  0xD,
-    TRA_0RTT_TRANS_PARAMS_ERROR     =  0xE,   /**< MUST delete the current saved 0RTT transport parameters */
+    TRA_KEY_UPDATE_ERROR            =  0xE,   /**< RFC 9001 §6.7: key update error */
+    /*
+     * Library-internal: 0-RTT transport parameter violation.
+     * RFC 9000 §7.4.1 does not define a dedicated wire error code for this;
+     * on the wire this value is non-standard (peer treats as INTERNAL_ERROR).
+     * Kept as a distinct value so xqc_conn_should_clear_0rtt_ticket() can
+     * distinguish it from other close reasons.
+     */
+    TRA_0RTT_TRANS_PARAMS_ERROR     =  0x54,
     /*
      * RFC 9000 Section 6.2 does not assign a CONNECTION_CLOSE code for
      * the Version Negotiation abort path, because the client cannot

scripts/case_test.sh

@@ -3880,7 +3880,7 @@ sleep 1
 clear_log
 echo -e "check_clear_0rtt_ticket_flag_in_close_notify...\c"
 ${CLIENT_BIN} -l d -T 1 -s 4800 -U 1 -Q 65535 -E > stdlog
-cli_res2=`grep "should_clear_0rtt_ticket, conn_err:14, clear_0rtt_ticket:1" stdlog`
+cli_res2=`grep "should_clear_0rtt_ticket, conn_err:84, clear_0rtt_ticket:1" stdlog`
 errlog=`grep_err_log`
 if [ -n "$cli_res2" ] && [ -n "$errlog" ]; then
     echo ">>>>>>>> pass:1"
@@ -3902,7 +3902,7 @@ sleep 1
 clear_log
 echo -e "check_clear_0rtt_ticket_flag_in_h3_close_notify...\c"
 ${CLIENT_BIN} -l d -s 4800 -Q 65535 -E > stdlog
-cli_res2=`grep "should_clear_0rtt_ticket, conn_err:14, clear_0rtt_ticket:1" stdlog`
+cli_res2=`grep "should_clear_0rtt_ticket, conn_err:84, clear_0rtt_ticket:1" stdlog`
 errlog=`grep_err_log`
 if [ -n "$cli_res2" ] && [ -n "$errlog" ]; then
     echo ">>>>>>>> pass:1"
@@ -3924,7 +3924,7 @@ sleep 1
 clear_log
 echo -e "check_clear_0rtt_ticket_flag_in_h3_close_notify...\c"
 ${CLIENT_BIN} -l d -s 4800 -Q 65535 -E > stdlog
-cli_res2=`grep "should_clear_0rtt_ticket, conn_err:14, clear_0rtt_ticket:1" stdlog`
+cli_res2=`grep "should_clear_0rtt_ticket, conn_err:84, clear_0rtt_ticket:1" stdlog`
 errlog=`grep_err_log`
 if [ -n "$cli_res2" ] && [ -n "$errlog" ]; then
     echo ">>>>>>>> pass:1"
@@ -5185,7 +5185,7 @@ fi
 
 # test 701: server reduces max_streams_bidi after first connection,
 # client detects reduction on 0-RTT resumption and closes with
-# TRANSPORT_PARAMETER_ERROR (0x0E = conn_err:14)
+# TRA_0RTT_TRANS_PARAMS_ERROR (0x54 = conn_err:84)
 killall test_server 2> /dev/null
 clear_log
 rm -f test_session xqc_token tp_localhost
@@ -5196,7 +5196,7 @@ sleep 1
 ${CLIENT_BIN} -s 1024 -l d -t 1 -E > stdlog
 # second connection: 0-RTT with reduced max_streams_bidi on server
 ${CLIENT_BIN} -s 1024 -l d -t 1 -E > stdlog
-conn_err=`grep "conn_err:14" stdlog`
+conn_err=`grep "conn_err:84" stdlog`
 if [ -n "$conn_err" ]; then
     echo ">>>>>>>> pass:1"
     case_print_result "0RTT_param_reduction" "pass"

src/transport/xqc_conn.c

@@ -657,7 +657,9 @@ xqc_conn_init_key_update_ctx(xqc_connection_t *conn)
     ctx->first_recv_pktno  = 0;
     ctx->enc_pkt_cnt       = 0;
 
-    ctx->initiate_time_guard   = 0;
+    ctx->initiate_time_guard       = 0;
+    ctx->key_update_initiator      = XQC_FALSE;
+    ctx->key_update_not_confirmed  = XQC_FALSE;
 }
 
 static inline void
@@ -6115,7 +6117,8 @@ xqc_conn_tls_transport_params_cb(const uint8_t *tp, size_t len, void *user_data)
      */
     if (conn->conn_type == XQC_CONN_TYPE_CLIENT
         && (conn->conn_flag & XQC_CONN_FLAG_HAS_0RTT)
-        && xqc_tls_is_early_data_accepted(conn->tls) == XQC_TLS_EARLY_DATA_ACCEPT)
+        && (xqc_tls_is_early_data_accepted(conn->tls) == XQC_TLS_EARLY_DATA_ACCEPT
+            || (conn->conn_flag & XQC_CONN_FLAG_0RTT_OK)))
     {
         xqc_trans_settings_t *remembered = &conn->remote_settings;
 

src/transport/xqc_conn.h

@@ -288,6 +288,12 @@ typedef struct {
     uint64_t                enc_pkt_cnt;       /* number of packet encrypt with each key phase */
     xqc_usec_t              initiate_time_guard;  /* time limit for initiating next key update */
 
+    /* RFC 9001 §6.1: initiator must wait for ACK before considering update confirmed */
+    xqc_bool_t              key_update_initiator; /* XQC_TRUE if this endpoint initiated the current key update */
+
+    /* RFC 9001 §6.1/6.2: TRUE while key update awaits peer confirmation */
+    xqc_bool_t              key_update_not_confirmed;
+
 } xqc_key_update_ctx_t;
 
 typedef struct xqc_ping_record_s {

src/transport/xqc_packet_parser.c

@@ -672,6 +672,16 @@ xqc_packet_encrypt_buf(xqc_connection_t *conn, xqc_packet_out_t *packet_out,
                 return ret;
             }
 
+            /*
+             * RFC 9001 §6.1: Initiator MUST NOT initiate a subsequent key update
+             * until it receives an ACK for a packet sent with the new key phase.
+             * This is enforced by the first_sent_pktno <= ctl_largest_acked check
+             * in the key update trigger condition (line 660 above).
+             * Mark as initiator so ACK processing can log confirmation.
+             */
+            conn->key_update_ctx.key_update_initiator = XQC_TRUE;
+            conn->key_update_ctx.key_update_not_confirmed = XQC_TRUE;
+
             ret = xqc_conn_confirm_key_update(conn);
             if (ret != XQC_OK) {
                 xqc_log(conn->log, XQC_LOG_ERROR, "|xqc_conn_confirm_key_update error|");
@@ -761,6 +771,29 @@ xqc_packet_decrypt(xqc_connection_t *conn, xqc_packet_in_t *packet_in)
 
     /* check key phase, determine weather to update read keys */
     xqc_uint_t key_phase = XQC_PACKET_SHORT_HEADER_KEY_PHASE(header);
+
+    /*
+     * RFC 9001 §6.2: Consecutive key update detection.
+     * If key_update_not_confirmed is TRUE, we are still awaiting confirmation
+     * of our own key update.  A new key_phase change from the peer before that
+     * confirmation means the peer updated keys twice without waiting.
+     * Treat as KEY_UPDATE_ERROR.
+     */
+    if (packet_in->pi_pkt.pkt_type == XQC_PTYPE_SHORT_HEADER && level == XQC_ENC_LEV_1RTT
+        && key_phase != conn->key_update_ctx.next_in_key_phase
+        && packet_in->pi_pkt.pkt_num > conn->key_update_ctx.first_recv_pktno
+        && conn->key_update_ctx.key_update_not_confirmed)
+    {
+        xqc_log(conn->log, XQC_LOG_ERROR,
+                "|consecutive key update from peer|pkt_num:%ui|key_phase:%ui|"
+                "expected:%ui|first_recv_pktno:%ui|",
+                packet_in->pi_pkt.pkt_num, key_phase,
+                conn->key_update_ctx.next_in_key_phase,
+                conn->key_update_ctx.first_recv_pktno);
+        XQC_CONN_ERR(conn, TRA_KEY_UPDATE_ERROR);
+        return -XQC_EPROTO;
+    }
+
     if (packet_in->pi_pkt.pkt_type == XQC_PTYPE_SHORT_HEADER && level == XQC_ENC_LEV_1RTT
         && key_phase != conn->key_update_ctx.next_in_key_phase
         && packet_in->pi_pkt.pkt_num > conn->key_update_ctx.first_recv_pktno
@@ -808,6 +841,11 @@ xqc_packet_decrypt(xqc_connection_t *conn, xqc_packet_in_t *packet_in)
                 return ret;
             }
 
+            /* Responder: confirmed by sending with new keys (TX_WRITE sets TRUE).
+             * Clear initiator flag in case a prior initiated update was pending. */
+            conn->key_update_ctx.key_update_initiator = XQC_FALSE;
+            conn->key_update_ctx.key_update_not_confirmed = XQC_FALSE;
+
             ret = xqc_conn_confirm_key_update(conn);
             if (ret != XQC_OK) {
                 xqc_log(conn->log, XQC_LOG_WARN, "|xqc_conn_confirm_key_update error|");
@@ -821,6 +859,14 @@ xqc_packet_decrypt(xqc_connection_t *conn, xqc_packet_in_t *packet_in)
         }
     }
 
+    /*
+     * TODO: RFC 9001 §6.4 — detect old-key packets with higher pkt_num
+     * than new-key packets.  xquic selects keys by key_phase bit (not trial
+     * decryption), so an old-key-phase packet with pkt_num > first_recv_pktno
+     * during the 3*PTO retention window should be treated as KEY_UPDATE_ERROR.
+     * Requires tracking max pkt_num per key phase.  See issue #756 BUG3.
+     */
+
     return XQC_OK;
 }
 

src/transport/xqc_send_ctl.c

@@ -958,6 +958,26 @@ xqc_send_ctl_on_ack_received(xqc_send_ctl_t *send_ctl, xqc_pn_ctl_t *pn_ctl, xqc
         return XQC_OK;
     }
 
+    /*
+     * RFC 9001 §6.1: Key update confirmation for the initiator.
+     * The initiator considers the key update confirmed once the peer ACKs a
+     * packet sent with the new key phase.  The practical enforcement is that
+     * the next key update cannot be initiated until first_sent_pktno <=
+     * ctl_largest_acked (checked at the trigger site in xqc_packet_parser.c).
+     * Here we just log the event and clear the initiator flag.
+     */
+    if (pns == XQC_PNS_APP_DATA
+        && conn->key_update_ctx.key_update_initiator
+        && send_ctl->ctl_largest_acked[pns] != XQC_MAX_UINT64_VALUE
+        && send_ctl->ctl_largest_acked[pns] >= conn->key_update_ctx.first_sent_pktno)
+    {
+        conn->key_update_ctx.key_update_initiator = XQC_FALSE;
+        conn->key_update_ctx.key_update_not_confirmed = XQC_FALSE;
+        xqc_log(conn->log, XQC_LOG_DEBUG,
+                 "|key update confirmed by ACK|largest_acked:%ui|first_sent_pktno:%ui|",
+                 send_ctl->ctl_largest_acked[pns], conn->key_update_ctx.first_sent_pktno);
+    }
+
     if (update_largest_ack && has_ack_eliciting && ack_on_same_path) {
         if (!ignore_rtt) {
             /* 更新 ctl_latest_rtt */

tests/unittest/main.c

@@ -105,6 +105,12 @@ main()
         || !CU_add_test(pSuite, "xqc_test_crypto_frame_dispatched_via_xqc_process_frame", xqc_test_crypto_frame_dispatched_via_xqc_process_frame)
         || !CU_add_test(pSuite, "xqc_test_crypto_in_0rtt_emits_connection_close", xqc_test_crypto_in_0rtt_emits_connection_close)
         || !CU_add_test(pSuite, "xqc_test_crypto", xqc_test_crypto)
+        /* issue #823: key update initiator confirmation (RFC 9001 §6.1) */
+        || !CU_add_test(pSuite, "xqc_test_key_update_initiator_confirmation",
+                        xqc_test_key_update_initiator_confirmation)
+        /* issue #756 BUG2: consecutive key update detection (RFC 9001 §6.2) */
+        || !CU_add_test(pSuite, "xqc_test_consecutive_key_update_detection",
+                        xqc_test_consecutive_key_update_detection)
         || !CU_add_test(pSuite, "xqc_test_hp_sample_boundary", xqc_test_hp_sample_boundary)
         || !CU_add_test(pSuite, "xqc_test_packet_encrypt_hp_sample_boundary", xqc_test_packet_encrypt_hp_sample_boundary)
         || !CU_add_test(pSuite, "xqc_test_empty_pkt", xqc_test_empty_pkt)

tests/unittest/xqc_conn_test.c

@@ -403,6 +403,7 @@ xqc_0rtt_test_make_conn(xqc_cid_t *out_server_scid)
 
     /* mark the connection as having 0-RTT */
     conn->conn_flag |= XQC_CONN_FLAG_HAS_0RTT;
+    conn->conn_flag |= XQC_CONN_FLAG_0RTT_OK;
     /* clear any prior errors */
     conn->conn_err = 0;
     conn->conn_flag &= ~XQC_CONN_FLAG_ERROR;
@@ -625,7 +626,9 @@ xqc_test_0rtt_params_each_reduced(void)
 
         /* reduce exactly one field below remembered */
         uint64_t *field = (uint64_t *)((char *)&params + cases[i].tp_offset);
-        *field = cases[i].remembered_val - 1;
+        *field = cases[i].remembered_val <= 4
+                 ? cases[i].remembered_val - 1
+                 : cases[i].remembered_val / 2;
 
         xqc_int_t err = xqc_0rtt_test_fire(conn, &params);
         CU_ASSERT_EQUAL(err, TRA_0RTT_TRANS_PARAMS_ERROR);