From 330dd1b5f075e2a947c8495fa130976a5aec5d50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=96=B5=E5=90=89?= Date: Fri, 22 May 2026 11:01:40 +0800 Subject: [PATCH 1/2] [~] fix issue #581: reject coalesced packets with mismatched DCID per RFC 9000 12.2 The receive path in xqc_conn_process_packet walks every QUIC packet inside a single UDP datagram but never compares the Destination Connection ID of the second-and-later packets against the first one. RFC 9000 Section 12.2 requires receivers to ignore those mismatching follow-up packets while still processing the rest of the datagram; without that check a peer that holds more than one valid SCID for the connection can sneak frames in under a different CID once handshake is past, and during handshake the long header path skips the connection-level DCID set entirely for INIT and 0-RTT. Anchor the datagram on the DCID of its first successfully parsed packet and drop any subsequent packet whose pi_pkt.pkt_dcid does not match, emitting a WARN log plus a TRA_PACKET_DROPPED qlog event and bumping packet_dropped_count. The rest of the datagram keeps flowing. Coverage: tests/unittest/xqc_coalesced_test.c adds five CUnit cases that build coalesced Initials with controlled DCIDs and assert single-packet, match, mismatch, length-mismatch, and A+B+A patterns. Fixes: #581 --- src/transport/xqc_conn.c | 51 ++- tests/CMakeLists.txt | 1 + tests/unittest/main.c | 8 +- tests/unittest/xqc_coalesced_test.c | 474 ++++++++++++++++++++++++++++ tests/unittest/xqc_coalesced_test.h | 18 ++ 5 files changed, 550 insertions(+), 2 deletions(-) create mode 100644 tests/unittest/xqc_coalesced_test.c create mode 100644 tests/unittest/xqc_coalesced_test.h diff --git a/src/transport/xqc_conn.c b/src/transport/xqc_conn.c index 3765e961c..4d1ce24f5 100644 --- a/src/transport/xqc_conn.c +++ b/src/transport/xqc_conn.c @@ -5012,7 +5012,7 @@ xqc_conn_log_recvd_packet(xqc_connection_t *c, xqc_packet_in_t *pi, xqc_int_t xqc_conn_process_packet(xqc_connection_t *c, - const unsigned char *packet_in_buf, size_t packet_in_size, + const unsigned char *packet_in_buf, size_t packet_in_size, xqc_usec_t recv_time) { xqc_int_t ret = XQC_OK; @@ -5022,6 +5022,17 @@ xqc_conn_process_packet(xqc_connection_t *c, xqc_packet_in_t packet; unsigned char decrypt_payload[XQC_MAX_PACKET_IN_LEN]; + /* + * RFC 9000 Section 12.2: every coalesced packet that follows the first + * one in a UDP datagram is required to carry the same Destination + * Connection ID as the first packet; receivers SHOULD discard any + * mismatching subsequent packet but continue to process the rest. + * first_dcid only needs to be valid once first_dcid_seen flips to TRUE, + * so leaving the rest of the struct uninitialized is intentional. + */ + xqc_cid_t first_dcid; + xqc_bool_t first_dcid_seen = XQC_FALSE; + /* process all QUIC packets in UDP datagram */ while (pos < end) { last_pos = pos; @@ -5039,6 +5050,44 @@ xqc_conn_process_packet(xqc_connection_t *c, xqc_conn_log_recvd_packet(c, packet_in, packet_in_size, ret, recv_time); if (ret == XQC_OK) { + /* + * Anchor the datagram on the DCID of its first successfully + * parsed packet. Subsequent packets must match, otherwise the + * peer is either confused or attempting to smuggle traffic + * onto another connection's path through coalescing. + */ + if (first_dcid_seen == XQC_FALSE) { + xqc_cid_set(&first_dcid, packet_in->pi_pkt.pkt_dcid.cid_buf, + packet_in->pi_pkt.pkt_dcid.cid_len); + first_dcid_seen = XQC_TRUE; + + } else if (xqc_cid_is_equal(&first_dcid, + &packet_in->pi_pkt.pkt_dcid) != XQC_OK) + { + /* + * Drop only the offending packet, not the whole datagram; + * xqc_dcid_str/xqc_scid_str use distinct engine-side + * scratch buffers, so it is safe to print both CIDs in one + * log statement. + */ + xqc_log(c->log, XQC_LOG_WARN, + "|coalesced pkt dcid mismatch|first_dcid:%s|pkt_dcid:%s|pkt_type:%s|pkt_num:%ui|", + xqc_dcid_str(c->engine, &first_dcid), + xqc_scid_str(c->engine, &packet_in->pi_pkt.pkt_dcid), + xqc_pkt_type_2_str(packet_in->pi_pkt.pkt_type), + packet_in->pi_pkt.pkt_num); + + c->packet_dropped_count++; + xqc_log_event(c->log, TRA_PACKET_DROPPED, + "coalesced pkt dcid mismatch", XQC_OK, + xqc_pkt_type_2_str(packet_in->pi_pkt.pkt_type), + packet_in->pi_pkt.pkt_num); + + pos = packet_in->last; + xqc_log_event(c->log, TRA_PACKET_RECEIVED, packet_in); + continue; + } + ret = xqc_conn_on_pkt_processed(c, packet_in, recv_time); } else if (xqc_conn_tolerant_error(ret)) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8b530b488..dae468d4a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -114,6 +114,7 @@ if(HAVE_CUNIT) ${UNIT_TEST_DIR}/xqc_h3_ext_test.c ${UNIT_TEST_DIR}/xqc_ack_with_timestamp_test.c ${UNIT_TEST_DIR}/xqc_send_ctl_test.c + ${UNIT_TEST_DIR}/xqc_coalesced_test.c ) if(XQC_ENABLE_FEC) diff --git a/tests/unittest/main.c b/tests/unittest/main.c index 9e4441993..7917d317d 100644 --- a/tests/unittest/main.c +++ b/tests/unittest/main.c @@ -42,6 +42,7 @@ #include "xqc_fec_test.h" #include "xqc_ack_with_timestamp_test.h" #include "xqc_send_ctl_test.h" +#include "xqc_coalesced_test.h" static int xqc_init_suite(void) { return 0; } static int xqc_clean_suite(void) { return 0; } @@ -126,7 +127,12 @@ main() xqc_test_pto_uses_remote_max_ack_delay) || !CU_add_test(pSuite, "xqc_test_pto_remote_default_when_unset", xqc_test_pto_remote_default_when_unset) - /* ADD TESTS HERE */) + || !CU_add_test(pSuite, "xqc_test_coalesced_single_pkt", xqc_test_coalesced_single_pkt) + || !CU_add_test(pSuite, "xqc_test_coalesced_dcid_match", xqc_test_coalesced_dcid_match) + || !CU_add_test(pSuite, "xqc_test_coalesced_dcid_mismatch", xqc_test_coalesced_dcid_mismatch) + || !CU_add_test(pSuite, "xqc_test_coalesced_dcid_len_mismatch", xqc_test_coalesced_dcid_len_mismatch) + || !CU_add_test(pSuite, "xqc_test_coalesced_dcid_a_b_a", xqc_test_coalesced_dcid_a_b_a) + /* ADD TESTS HERE */) { CU_cleanup_registry(); return (int)CU_get_error(); diff --git a/tests/unittest/xqc_coalesced_test.c b/tests/unittest/xqc_coalesced_test.c new file mode 100644 index 000000000..ed34e0f9d --- /dev/null +++ b/tests/unittest/xqc_coalesced_test.c @@ -0,0 +1,474 @@ +/** + * @copyright Copyright (c) 2026, Alibaba Group Holding Limited + * + * Reverse-validation of the RFC 9000 Section 12.2 fix: lighting up the + * coalesced-DCID-mismatch branch in xqc_conn_process_packet, while keeping + * the happy path quiet. + * + * The trick is to encrypt each Initial under the *same* client-side Initial + * keys (which were derived from the original DCID) and only flip bytes in + * the wire-level DCID field. The AEAD AAD is the on-wire header, so as long + * as both endpoints see the same bytes the tag still validates - the only + * thing that diverges is what xqc_packet_parse_long_header records into + * pi_pkt.pkt_dcid, which is exactly what the new validation inspects. + */ + +#include +#include +#include + +#include "xqc_coalesced_test.h" + +#include "xquic/xquic.h" +#include "xquic/xquic_typedef.h" +#include "src/transport/xqc_conn.h" +#include "src/transport/xqc_engine.h" +#include "src/transport/xqc_packet.h" +#include "src/transport/xqc_packet_out.h" +#include "src/transport/xqc_packet_parser.h" +#include "src/transport/xqc_cid.h" + + +extern xqc_usec_t xqc_now(void); + + +/* Each Initial must be at least 1200 bytes on the server's view; pad to + * 1300 to leave headroom for variable-length header fields. */ +#define COALESCED_TEST_PKT_TARGET 1300 + + +typedef struct coalesced_ctx_s { + xqc_engine_t *engine; + xqc_connection_t *c; + xqc_cid_t cid; + unsigned char buf[2048]; + size_t buf_len; +} coalesced_ctx_t; + + +static ssize_t +coalesced_write_socket(const unsigned char *buf, size_t size, + const struct sockaddr *peer_addr, socklen_t peer_addrlen, + void *conn_user_data) +{ + coalesced_ctx_t *ctx = (coalesced_ctx_t *)conn_user_data; + if (size > sizeof(ctx->buf)) { + size = sizeof(ctx->buf); + } + memcpy(ctx->buf, buf, size); + ctx->buf_len = size; + return size; +} + +static int +coalesced_conn_create_notify(xqc_connection_t *conn, const xqc_cid_t *cid, + void *user_data, void *conn_proto_data) +{ + coalesced_ctx_t *ctx = (coalesced_ctx_t *)user_data; + ctx->c = conn; + memcpy(&ctx->cid, cid, sizeof(xqc_cid_t)); + xqc_conn_set_alp_user_data(conn, ctx); + return 0; +} + +static void +coalesced_set_event_timer(xqc_msec_t wake_after, void *engine_user_data) +{ + return; +} + +static xqc_engine_t * +coalesced_create_engine(coalesced_ctx_t *ctx, xqc_engine_type_t etype) +{ + xqc_engine_ssl_config_t ssl_config; + memset(&ssl_config, 0, sizeof(ssl_config)); + ssl_config.private_key_file = "./server.key"; + ssl_config.cert_file = "./server.crt"; + ssl_config.ciphers = XQC_TLS_CIPHERS; + ssl_config.groups = XQC_TLS_GROUPS; + + xqc_engine_callback_t engine_cb = { + .set_event_timer = coalesced_set_event_timer, + }; + + xqc_transport_callbacks_t tcbs = { + .write_socket = coalesced_write_socket, + }; + + xqc_app_proto_callbacks_t alp_cbs = { + .conn_cbs.conn_create_notify = coalesced_conn_create_notify, + }; + + xqc_engine_t *engine = xqc_engine_create(etype, NULL, &ssl_config, + &engine_cb, &tcbs, ctx); + if (engine == NULL) { + return NULL; + } + xqc_engine_register_alpn(engine, "transport", 9, &alp_cbs, NULL); + return engine; +} + + +/* + * Build a single Initial packet with a caller-supplied DCID, padding-only + * payload, and a chosen packet number. Returns 0 on failure, otherwise the + * size written into out_buf (which must be at least 2048 bytes). + * + * The encryption is performed against cli_conn so the keys come from the + * client's Initial secret; the DCID we put in the header is purely a wire + * value that ends up in the AEAD AAD on both sides. + */ +static size_t +build_initial_pkt(xqc_connection_t *cli_conn, + const unsigned char *dcid_buf, uint8_t dcid_len, + xqc_packet_number_t pkt_num, + uint8_t *out_buf, size_t out_buf_cap) +{ + xqc_packet_out_t *po = xqc_packet_out_create(2048); + if (po == NULL) { + return 0; + } + + po->po_pkt.pkt_type = XQC_PTYPE_INIT; + po->po_pkt.pkt_pns = XQC_PNS_INIT; + po->po_pkt.pkt_num = pkt_num; + + /* SCID = client's user_scid; DCID supplied by caller. */ + memcpy(po->po_pkt.pkt_scid.cid_buf, + cli_conn->scid_set.user_scid.cid_buf, + cli_conn->scid_set.user_scid.cid_len); + po->po_pkt.pkt_scid.cid_len = cli_conn->scid_set.user_scid.cid_len; + + memcpy(po->po_pkt.pkt_dcid.cid_buf, dcid_buf, dcid_len); + po->po_pkt.pkt_dcid.cid_len = dcid_len; + + ssize_t hdr_len = xqc_gen_long_packet_header( + po, + po->po_pkt.pkt_dcid.cid_buf, po->po_pkt.pkt_dcid.cid_len, + po->po_pkt.pkt_scid.cid_buf, po->po_pkt.pkt_scid.cid_len, + NULL, 0, XQC_VERSION_V1, XQC_PKTNO_BITS); + if (hdr_len <= 0) { + xqc_packet_out_destroy(po); + return 0; + } + po->po_used_size += hdr_len; + + /* Pad with zero bytes so the wire-format Initial passes the server-side + * 1200-byte minimum check (xqc_packet_parse_initial). All zero bytes + * become a single PADDING frame on the receive path. */ + size_t aead_tag = 16; /* AES-128-GCM, matches BoringSSL initial AEAD */ + size_t target_payload = + (COALESCED_TEST_PKT_TARGET > (size_t)hdr_len + aead_tag) + ? COALESCED_TEST_PKT_TARGET - (size_t)hdr_len - aead_tag + : 64; + + if (po->po_used_size + target_payload > po->po_buf_cap) { + xqc_packet_out_destroy(po); + return 0; + } + memset(po->po_buf + po->po_used_size, 0x00, target_payload); + po->po_used_size += target_payload; + + size_t enc_len = 0; + xqc_int_t ret = xqc_packet_encrypt_buf(cli_conn, po, out_buf, + out_buf_cap, &enc_len); + xqc_packet_out_destroy(po); + if (ret != XQC_OK) { + return 0; + } + return enc_len; +} + + +/* + * Boilerplate: stand up a client+server engine pair, drive a real Initial + * from client to server so the server-side connection exists. + */ +static int +coalesced_test_setup(coalesced_ctx_t *cli, coalesced_ctx_t *svr) +{ + cli->engine = coalesced_create_engine(cli, XQC_ENGINE_CLIENT); + svr->engine = coalesced_create_engine(svr, XQC_ENGINE_SERVER); + if (cli->engine == NULL || svr->engine == NULL) { + return -1; + } + + xqc_conn_settings_t conn_settings; + memset(&conn_settings, 0, sizeof(conn_settings)); + conn_settings.proto_version = XQC_VERSION_V1; + xqc_conn_ssl_config_t conn_ssl_config; + memset(&conn_ssl_config, 0, sizeof(conn_ssl_config)); + + const xqc_cid_t *cid = xqc_connect(cli->engine, &conn_settings, + NULL, 0, "", 0, &conn_ssl_config, + NULL, 0, "transport", cli); + if (cid == NULL || cli->c == NULL) { + return -1; + } + + struct sockaddr_in6 peer_addr; + socklen_t peer_addrlen = sizeof(peer_addr); + struct sockaddr_in6 local_addr; + socklen_t local_addrlen = sizeof(local_addr); + + xqc_engine_packet_process(svr->engine, cli->buf, cli->buf_len, + (struct sockaddr *)&local_addr, local_addrlen, + (struct sockaddr *)&peer_addr, peer_addrlen, + xqc_now(), svr); + + return svr->c == NULL ? -1 : 0; +} + + +static void +coalesced_test_teardown(coalesced_ctx_t *cli, coalesced_ctx_t *svr) +{ + if (cli->engine != NULL) { + if (cli->c != NULL) { + xqc_conn_close(cli->engine, &cli->cid); + } + xqc_engine_destroy(cli->engine); + } + if (svr->engine != NULL) { + if (svr->c != NULL) { + xqc_conn_close(svr->engine, &svr->cid); + } + xqc_engine_destroy(svr->engine); + } +} + + +/* T1 - regression: a single Initial in a datagram never trips the new + * mismatch branch (drop counter must remain at its baseline). */ +void +xqc_test_coalesced_single_pkt(void) +{ + coalesced_ctx_t cli = {0}; + coalesced_ctx_t svr = {0}; + + if (coalesced_test_setup(&cli, &svr) != 0) { + CU_FAIL("coalesced_test_setup failed"); + coalesced_test_teardown(&cli, &svr); + return; + } + + uint32_t baseline = svr.c->packet_dropped_count; + + uint8_t enc[2048]; + size_t enc_len = build_initial_pkt( + cli.c, + cli.c->dcid_set.current_dcid.cid_buf, + cli.c->dcid_set.current_dcid.cid_len, + 1, enc, sizeof(enc)); + CU_ASSERT(enc_len > 0); + + (void)xqc_conn_process_packet(svr.c, enc, enc_len, xqc_now()); + + /* Single packet must never increment the dropped count via the new + * coalesced-mismatch branch. */ + CU_ASSERT_EQUAL(svr.c->packet_dropped_count, baseline); + + coalesced_test_teardown(&cli, &svr); +} + + +/* T2 - core reverse-validation: a coalesced Initial(A) + Initial(B) datagram + * with A != B must drop exactly one packet (the offender) and keep + * processing the rest of the datagram. */ +void +xqc_test_coalesced_dcid_mismatch(void) +{ + coalesced_ctx_t cli = {0}; + coalesced_ctx_t svr = {0}; + + if (coalesced_test_setup(&cli, &svr) != 0) { + CU_FAIL("coalesced_test_setup failed"); + coalesced_test_teardown(&cli, &svr); + return; + } + + uint32_t baseline_dropped = svr.c->packet_dropped_count; + uint32_t baseline_rcvd = svr.c->rcv_pkt_stats.conn_rcvd_pkts; + + /* DCID A: client's view of the current DCID. */ + uint8_t dcid_a[XQC_MAX_CID_LEN]; + uint8_t dcid_a_len = cli.c->dcid_set.current_dcid.cid_len; + memcpy(dcid_a, cli.c->dcid_set.current_dcid.cid_buf, dcid_a_len); + + /* DCID B: same length, first byte flipped. */ + uint8_t dcid_b[XQC_MAX_CID_LEN]; + uint8_t dcid_b_len = dcid_a_len; + memcpy(dcid_b, dcid_a, dcid_a_len); + dcid_b[0] ^= 0xFF; + + uint8_t enc1[2048]; + size_t enc1_len = build_initial_pkt(cli.c, dcid_a, dcid_a_len, 1, + enc1, sizeof(enc1)); + CU_ASSERT(enc1_len > 0); + + uint8_t enc2[2048]; + size_t enc2_len = build_initial_pkt(cli.c, dcid_b, dcid_b_len, 2, + enc2, sizeof(enc2)); + CU_ASSERT(enc2_len > 0); + + uint8_t combined[8192]; + CU_ASSERT(enc1_len + enc2_len <= sizeof(combined)); + memcpy(combined, enc1, enc1_len); + memcpy(combined + enc1_len, enc2, enc2_len); + + xqc_int_t ret = xqc_conn_process_packet(svr.c, combined, + enc1_len + enc2_len, xqc_now()); + /* The function must return cleanly (non-fatal): the second packet was + * dropped but the first was processed. */ + CU_ASSERT_EQUAL(ret, XQC_OK); + + /* Exactly one drop attributable to the new branch. */ + CU_ASSERT_EQUAL(svr.c->packet_dropped_count, baseline_dropped + 1); + + /* The first packet must have been logged as received. */ + CU_ASSERT(svr.c->rcv_pkt_stats.conn_rcvd_pkts > baseline_rcvd); + + coalesced_test_teardown(&cli, &svr); +} + + +/* T3 - boundary: when both coalesced Initials share the same DCID, the new + * branch must NOT trip. */ +void +xqc_test_coalesced_dcid_match(void) +{ + coalesced_ctx_t cli = {0}; + coalesced_ctx_t svr = {0}; + + if (coalesced_test_setup(&cli, &svr) != 0) { + CU_FAIL("coalesced_test_setup failed"); + coalesced_test_teardown(&cli, &svr); + return; + } + + uint32_t baseline_dropped = svr.c->packet_dropped_count; + + uint8_t dcid_a[XQC_MAX_CID_LEN]; + uint8_t dcid_a_len = cli.c->dcid_set.current_dcid.cid_len; + memcpy(dcid_a, cli.c->dcid_set.current_dcid.cid_buf, dcid_a_len); + + uint8_t enc1[2048]; + size_t enc1_len = build_initial_pkt(cli.c, dcid_a, dcid_a_len, 1, + enc1, sizeof(enc1)); + CU_ASSERT(enc1_len > 0); + + uint8_t enc2[2048]; + size_t enc2_len = build_initial_pkt(cli.c, dcid_a, dcid_a_len, 2, + enc2, sizeof(enc2)); + CU_ASSERT(enc2_len > 0); + + uint8_t combined[8192]; + CU_ASSERT(enc1_len + enc2_len <= sizeof(combined)); + memcpy(combined, enc1, enc1_len); + memcpy(combined + enc1_len, enc2, enc2_len); + + (void)xqc_conn_process_packet(svr.c, combined, + enc1_len + enc2_len, xqc_now()); + + /* DCIDs match; the new branch must stay silent. */ + CU_ASSERT_EQUAL(svr.c->packet_dropped_count, baseline_dropped); + + coalesced_test_teardown(&cli, &svr); +} + + +/* T4 - boundary: same byte prefix but different length must still mismatch + * (xqc_cid_is_equal first compares cid_len). */ +void +xqc_test_coalesced_dcid_len_mismatch(void) +{ + coalesced_ctx_t cli = {0}; + coalesced_ctx_t svr = {0}; + + if (coalesced_test_setup(&cli, &svr) != 0) { + CU_FAIL("coalesced_test_setup failed"); + coalesced_test_teardown(&cli, &svr); + return; + } + + uint32_t baseline_dropped = svr.c->packet_dropped_count; + + uint8_t dcid_a[XQC_MAX_CID_LEN]; + uint8_t dcid_a_len = cli.c->dcid_set.current_dcid.cid_len; + memcpy(dcid_a, cli.c->dcid_set.current_dcid.cid_buf, dcid_a_len); + + /* DCID B: prefix of A, shorter length (dcid_a_len > 4 by default). */ + uint8_t dcid_b[XQC_MAX_CID_LEN]; + uint8_t dcid_b_len = (dcid_a_len > 4) ? (uint8_t)(dcid_a_len - 4) : (uint8_t)4; + memcpy(dcid_b, dcid_a, dcid_b_len); + + uint8_t enc1[2048]; + size_t enc1_len = build_initial_pkt(cli.c, dcid_a, dcid_a_len, 1, + enc1, sizeof(enc1)); + CU_ASSERT(enc1_len > 0); + + uint8_t enc2[2048]; + size_t enc2_len = build_initial_pkt(cli.c, dcid_b, dcid_b_len, 2, + enc2, sizeof(enc2)); + CU_ASSERT(enc2_len > 0); + + uint8_t combined[8192]; + CU_ASSERT(enc1_len + enc2_len <= sizeof(combined)); + memcpy(combined, enc1, enc1_len); + memcpy(combined + enc1_len, enc2, enc2_len); + + (void)xqc_conn_process_packet(svr.c, combined, + enc1_len + enc2_len, xqc_now()); + + CU_ASSERT_EQUAL(svr.c->packet_dropped_count, baseline_dropped + 1); + + coalesced_test_teardown(&cli, &svr); +} + + +/* T5 - extension: A + B + A. The middle packet must be dropped, the third + * (which matches first_dcid) must be accepted again - so drop count is + * exactly 1 (no double-counting, no spurious accept-then-mismatch). */ +void +xqc_test_coalesced_dcid_a_b_a(void) +{ + coalesced_ctx_t cli = {0}; + coalesced_ctx_t svr = {0}; + + if (coalesced_test_setup(&cli, &svr) != 0) { + CU_FAIL("coalesced_test_setup failed"); + coalesced_test_teardown(&cli, &svr); + return; + } + + uint32_t baseline_dropped = svr.c->packet_dropped_count; + + uint8_t dcid_a[XQC_MAX_CID_LEN]; + uint8_t dcid_a_len = cli.c->dcid_set.current_dcid.cid_len; + memcpy(dcid_a, cli.c->dcid_set.current_dcid.cid_buf, dcid_a_len); + + uint8_t dcid_b[XQC_MAX_CID_LEN]; + uint8_t dcid_b_len = dcid_a_len; + memcpy(dcid_b, dcid_a, dcid_a_len); + dcid_b[0] ^= 0xFF; + + uint8_t enc1[2048], enc2[2048], enc3[2048]; + size_t l1 = build_initial_pkt(cli.c, dcid_a, dcid_a_len, 1, enc1, sizeof(enc1)); + size_t l2 = build_initial_pkt(cli.c, dcid_b, dcid_b_len, 2, enc2, sizeof(enc2)); + size_t l3 = build_initial_pkt(cli.c, dcid_a, dcid_a_len, 3, enc3, sizeof(enc3)); + CU_ASSERT(l1 > 0 && l2 > 0 && l3 > 0); + + uint8_t combined[8192]; + CU_ASSERT(l1 + l2 + l3 <= sizeof(combined)); + size_t off = 0; + memcpy(combined + off, enc1, l1); off += l1; + memcpy(combined + off, enc2, l2); off += l2; + memcpy(combined + off, enc3, l3); off += l3; + + (void)xqc_conn_process_packet(svr.c, combined, off, xqc_now()); + + /* Only the middle packet should have been dropped. */ + CU_ASSERT_EQUAL(svr.c->packet_dropped_count, baseline_dropped + 1); + + coalesced_test_teardown(&cli, &svr); +} diff --git a/tests/unittest/xqc_coalesced_test.h b/tests/unittest/xqc_coalesced_test.h new file mode 100644 index 000000000..2cfcb8491 --- /dev/null +++ b/tests/unittest/xqc_coalesced_test.h @@ -0,0 +1,18 @@ +/** + * @copyright Copyright (c) 2026, Alibaba Group Holding Limited + * + * Tests for RFC 9000 Section 12.2: every coalesced QUIC packet that follows + * the first one in a UDP datagram MUST share the same Destination Connection + * ID. Receivers SHOULD discard offenders but continue processing the rest. + */ + +#ifndef _XQC_COALESCED_TEST_H_INCLUDED_ +#define _XQC_COALESCED_TEST_H_INCLUDED_ + +void xqc_test_coalesced_single_pkt(void); +void xqc_test_coalesced_dcid_match(void); +void xqc_test_coalesced_dcid_mismatch(void); +void xqc_test_coalesced_dcid_len_mismatch(void); +void xqc_test_coalesced_dcid_a_b_a(void); + +#endif /* _XQC_COALESCED_TEST_H_INCLUDED_ */ From eacf8abc35a0d2beb817cebc0587e4124a6b9a41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=96=B5=E5=90=89?= Date: Fri, 22 May 2026 15:11:14 +0800 Subject: [PATCH 2/2] [~] fix issue #581 follow-up: enforce coalesced DCID check before frame application The earlier patch placed the DCID consistency check after xqc_packet_process_single() returned, which turned out to be too late: xqc_packet_process_single internally invokes xqc_packet_decrypt_single, and that function calls xqc_process_frames() right after AEAD succeeds (src/transport/xqc_packet.c:221). So by the time the caller-side check fired, every frame in the offending coalesced packet had already been applied to the connection state - STREAM data accepted, ACK ranges updated, CRYPTO fed to the TLS stack, CONNECTION_CLOSE advanced the state to DRAINING. The continue statement only skipped xqc_conn_on_pkt_processed bookkeeping, leaving the actual threat that RFC 9000 Section 12.2 cares about wide open. Move the check inside xqc_packet_process_single, between the parse and the decrypt step. The new entry xqc_packet_process_single_anchored takes the datagram-scope (first_dcid, first_dcid_seen) cursor and performs the comparison before xqc_packet_decrypt_single is ever called, so mismatched packets cost zero AEAD work and never reach xqc_process_frames. The legacy xqc_packet_process_single is kept as a NULL-anchor wrapper for ABI compatibility. A new library-internal errno XQC_EVERSION_NEGOTIATION-style code, XQC_ECOALESCED_DCID_MISMATCH (691), lets xqc_conn_process_packet distinguish the drop from the existing tolerant-error path: tolerant errors do "goto end" and abandon the rest of the datagram, while the new code advances pos and continues with the next coalesced packet, which is what Section 12.2 actually mandates. packet_dropped_count is now incremented inside packet.c next to the decrypt-failure counter (no double counting with the caller), and no TRA_PACKET_RECEIVED qlog event is emitted for the dropped packet. The previous CUnit cases only asserted on packet_dropped_count and conn_rcvd_pkts, neither of which is sensitive to whether xqc_process_frames was actually invoked - they would have stayed green regardless of the bug. Added xqc_test_coalesced_mismatch_frames_not_applied: the offending packet carries a CONNECTION_CLOSE frame, and the test asserts conn_close_recv_time stays 0 and conn_state stays below DRAINING. That would have failed on the prior tip and passes here. ./tests/run_tests: 62/62 PASS. --- include/xquic/xqc_errno.h | 2 + src/transport/xqc_conn.c | 61 ++++------- src/transport/xqc_packet.c | 60 +++++++++++ src/transport/xqc_packet.h | 17 ++++ tests/unittest/main.c | 1 + tests/unittest/xqc_coalesced_test.c | 152 ++++++++++++++++++++++++++++ tests/unittest/xqc_coalesced_test.h | 1 + 7 files changed, 254 insertions(+), 40 deletions(-) diff --git a/include/xquic/xqc_errno.h b/include/xquic/xqc_errno.h index 862fc9cc5..d1bc106e6 100644 --- a/include/xquic/xqc_errno.h +++ b/include/xquic/xqc_errno.h @@ -155,6 +155,8 @@ typedef enum { XQC_EACK_EXT_ABN_VAL = 690, /**< ACK Extension - abnormal value */ + XQC_ECOALESCED_DCID_MISMATCH = 691, /**< RFC 9000 §12.2: coalesced packet DCID differs from first packet in datagram, drop just this packet */ + XQC_E_MAX, } xqc_transport_error_t; diff --git a/src/transport/xqc_conn.c b/src/transport/xqc_conn.c index 4d1ce24f5..e4c94721e 100644 --- a/src/transport/xqc_conn.c +++ b/src/transport/xqc_conn.c @@ -5025,8 +5025,14 @@ xqc_conn_process_packet(xqc_connection_t *c, /* * RFC 9000 Section 12.2: every coalesced packet that follows the first * one in a UDP datagram is required to carry the same Destination - * Connection ID as the first packet; receivers SHOULD discard any - * mismatching subsequent packet but continue to process the rest. + * Connection ID as the first packet; receivers MUST discard any + * mismatching subsequent packet (without delivering its frames to the + * connection state machine) but continue to process the rest of the + * datagram. The anchor is set inside xqc_packet_process_single_anchored + * after parsing succeeds and before xqc_packet_decrypt_single performs + * AEAD and frame application, so mismatched packets cost neither cycles + * nor side effects. + * * first_dcid only needs to be valid once first_dcid_seen flips to TRUE, * so leaving the rest of the struct uninitialized is intentional. */ @@ -5045,50 +5051,25 @@ xqc_conn_process_packet(xqc_connection_t *c, packet_in->pi_path_id = XQC_UNKNOWN_PATH_ID; /* packet_in->pos will update inside */ - ret = xqc_packet_process_single(c, packet_in); + ret = xqc_packet_process_single_anchored(c, packet_in, + &first_dcid, &first_dcid_seen); xqc_conn_log_recvd_packet(c, packet_in, packet_in_size, ret, recv_time); if (ret == XQC_OK) { + ret = xqc_conn_on_pkt_processed(c, packet_in, recv_time); + + } else if (ret == -XQC_ECOALESCED_DCID_MISMATCH) { /* - * Anchor the datagram on the DCID of its first successfully - * parsed packet. Subsequent packets must match, otherwise the - * peer is either confused or attempting to smuggle traffic - * onto another connection's path through coalescing. + * §12.2 drop: skip on_pkt_processed for this packet (no PN + * space update, no ACK arming, no idle timer refresh) but + * keep walking the datagram. packet_in->last already points + * to the end of this coalesced packet because parsing + * succeeded before the DCID check fired. */ - if (first_dcid_seen == XQC_FALSE) { - xqc_cid_set(&first_dcid, packet_in->pi_pkt.pkt_dcid.cid_buf, - packet_in->pi_pkt.pkt_dcid.cid_len); - first_dcid_seen = XQC_TRUE; - - } else if (xqc_cid_is_equal(&first_dcid, - &packet_in->pi_pkt.pkt_dcid) != XQC_OK) - { - /* - * Drop only the offending packet, not the whole datagram; - * xqc_dcid_str/xqc_scid_str use distinct engine-side - * scratch buffers, so it is safe to print both CIDs in one - * log statement. - */ - xqc_log(c->log, XQC_LOG_WARN, - "|coalesced pkt dcid mismatch|first_dcid:%s|pkt_dcid:%s|pkt_type:%s|pkt_num:%ui|", - xqc_dcid_str(c->engine, &first_dcid), - xqc_scid_str(c->engine, &packet_in->pi_pkt.pkt_dcid), - xqc_pkt_type_2_str(packet_in->pi_pkt.pkt_type), - packet_in->pi_pkt.pkt_num); - - c->packet_dropped_count++; - xqc_log_event(c->log, TRA_PACKET_DROPPED, - "coalesced pkt dcid mismatch", XQC_OK, - xqc_pkt_type_2_str(packet_in->pi_pkt.pkt_type), - packet_in->pi_pkt.pkt_num); - - pos = packet_in->last; - xqc_log_event(c->log, TRA_PACKET_RECEIVED, packet_in); - continue; - } - - ret = xqc_conn_on_pkt_processed(c, packet_in, recv_time); + pos = packet_in->last; + ret = XQC_OK; + continue; } else if (xqc_conn_tolerant_error(ret)) { /* ignore the remain bytes */ diff --git a/src/transport/xqc_packet.c b/src/transport/xqc_packet.c index 40cac595e..52b13f678 100644 --- a/src/transport/xqc_packet.c +++ b/src/transport/xqc_packet.c @@ -12,6 +12,7 @@ #include "src/transport/xqc_send_ctl.h" #include "src/transport/xqc_recv_record.h" #include "src/transport/xqc_packet_parser.h" +#include "src/transport/xqc_cid.h" #include "src/transport/xqc_utils.h" #include "src/transport/xqc_engine.h" #include "src/tls/xqc_tls.h" @@ -247,6 +248,34 @@ xqc_packet_decrypt_single(xqc_connection_t *c, xqc_packet_in_t *packet_in) xqc_int_t xqc_packet_process_single(xqc_connection_t *c, xqc_packet_in_t *packet_in) +{ + return xqc_packet_process_single_anchored(c, packet_in, NULL, NULL); +} + +/* + * RFC 9000 Section 12.2: every coalesced packet that follows the first + * one in a UDP datagram is required to carry the same Destination + * Connection ID as the first packet; receivers MUST discard any + * mismatching subsequent packet without processing its payload, while + * continuing to process the rest of the datagram. + * + * The check has to happen after xqc_packet_parse_single (so that + * pi_pkt.pkt_dcid and packet_in->last are populated for the current + * packet) but BEFORE xqc_packet_decrypt_single, because the "decrypt" + * step internally calls xqc_process_frames and applies STREAM/ACK/CRYPTO + * frames to the connection state. Once that returns, dropping the packet + * at the caller level cannot un-apply those frames - it would only skip + * xqc_conn_on_pkt_processed bookkeeping, which is not what §12.2 requires. + * + * Only packets that carry a packet number participate in anchoring and + * mismatch detection. VERSION_NEGOTIATION and RETRY are stand-alone by + * spec and must not be coalesced, so they are skipped here to avoid + * polluting the anchor. + */ +xqc_int_t +xqc_packet_process_single_anchored(xqc_connection_t *c, + xqc_packet_in_t *packet_in, + xqc_cid_t *first_dcid, xqc_bool_t *first_dcid_seen) { xqc_int_t ret = XQC_ERROR; @@ -261,6 +290,37 @@ xqc_packet_process_single(xqc_connection_t *c, return XQC_OK; } + /* + * Anchor on the DCID of the first parsed packet that needs decryption, + * and reject every subsequent packet whose DCID does not match. Doing + * this before xqc_packet_decrypt_single guarantees that the offending + * packet's frames are never delivered to the connection state machine + * and that no AEAD work is spent on a packet we are required to drop. + */ + if (first_dcid != NULL && first_dcid_seen != NULL) { + if (*first_dcid_seen == XQC_FALSE) { + xqc_cid_set(first_dcid, packet_in->pi_pkt.pkt_dcid.cid_buf, + packet_in->pi_pkt.pkt_dcid.cid_len); + *first_dcid_seen = XQC_TRUE; + + } else if (xqc_cid_is_equal(first_dcid, + &packet_in->pi_pkt.pkt_dcid) != XQC_OK) + { + xqc_log(c->log, XQC_LOG_WARN, + "|coalesced pkt dcid mismatch|first_dcid:%s|pkt_dcid:%s|pkt_type:%s|", + xqc_dcid_str(c->engine, first_dcid), + xqc_scid_str(c->engine, &packet_in->pi_pkt.pkt_dcid), + xqc_pkt_type_2_str(packet_in->pi_pkt.pkt_type)); + + xqc_log_event(c->log, TRA_PACKET_DROPPED, + "coalesced pkt dcid mismatch", XQC_OK, + xqc_pkt_type_2_str(packet_in->pi_pkt.pkt_type), + (uint64_t)0); + c->packet_dropped_count++; + return -XQC_ECOALESCED_DCID_MISMATCH; + } + } + /* decrypt packet */ ret = xqc_packet_decrypt_single(c, packet_in); if (ret != XQC_OK) { diff --git a/src/transport/xqc_packet.h b/src/transport/xqc_packet.h index 70036a326..a87e20288 100644 --- a/src/transport/xqc_packet.h +++ b/src/transport/xqc_packet.h @@ -100,5 +100,22 @@ xqc_pkt_type_t xqc_state_to_pkt_type(xqc_connection_t *conn); */ xqc_int_t xqc_packet_process_single(xqc_connection_t *c, xqc_packet_in_t *packet_in); +/** + * Process a single QUIC packet, enforcing the RFC 9000 §12.2 coalesced + * DCID rule before decryption. first_dcid / first_dcid_seen are owned by + * the datagram-level caller and persist across packets in the same UDP + * datagram. Passing NULL for either pointer disables the check (used by + * the legacy xqc_packet_process_single wrapper). + * + * Returns -XQC_ECOALESCED_DCID_MISMATCH when a subsequent coalesced + * packet's DCID does not match the anchor; the caller must skip the + * offending packet without invoking xqc_conn_on_pkt_processed but still + * advance pos past packet_in->last to continue with the next coalesced + * packet in the same UDP datagram. + */ +xqc_int_t xqc_packet_process_single_anchored(xqc_connection_t *c, + xqc_packet_in_t *packet_in, + xqc_cid_t *first_dcid, xqc_bool_t *first_dcid_seen); + #endif /* _XQC_PACKET_H_INCLUDED_ */ diff --git a/tests/unittest/main.c b/tests/unittest/main.c index 7917d317d..a8c002c78 100644 --- a/tests/unittest/main.c +++ b/tests/unittest/main.c @@ -132,6 +132,7 @@ main() || !CU_add_test(pSuite, "xqc_test_coalesced_dcid_mismatch", xqc_test_coalesced_dcid_mismatch) || !CU_add_test(pSuite, "xqc_test_coalesced_dcid_len_mismatch", xqc_test_coalesced_dcid_len_mismatch) || !CU_add_test(pSuite, "xqc_test_coalesced_dcid_a_b_a", xqc_test_coalesced_dcid_a_b_a) + || !CU_add_test(pSuite, "xqc_test_coalesced_mismatch_frames_not_applied", xqc_test_coalesced_mismatch_frames_not_applied) /* ADD TESTS HERE */) { CU_cleanup_registry(); diff --git a/tests/unittest/xqc_coalesced_test.c b/tests/unittest/xqc_coalesced_test.c index ed34e0f9d..634998004 100644 --- a/tests/unittest/xqc_coalesced_test.c +++ b/tests/unittest/xqc_coalesced_test.c @@ -26,6 +26,7 @@ #include "src/transport/xqc_packet.h" #include "src/transport/xqc_packet_out.h" #include "src/transport/xqc_packet_parser.h" +#include "src/transport/xqc_frame_parser.h" #include "src/transport/xqc_cid.h" @@ -180,6 +181,87 @@ build_initial_pkt(xqc_connection_t *cli_conn, } +/* + * Variant of build_initial_pkt that injects a CONNECTION_CLOSE frame + * (0x1c, transport-level, err_code=0x0a PROTOCOL_VIOLATION) at the head + * of the payload before padding. The frame is observable through two + * server-side side effects produced by xqc_process_conn_close_frame: + * + * - conn_close_recv_time flips from 0 to a non-zero timestamp + * - conn_state advances to XQC_CONN_STATE_DRAINING + * + * Neither of those is touched by xqc_conn_on_pkt_processed, so they + * only appear if the frame really reached xqc_process_frames. This + * makes the helper a smoking-gun probe for "were the offending packet's + * frames actually applied to connection state?". + */ +static size_t +build_initial_pkt_with_conn_close(xqc_connection_t *cli_conn, + const unsigned char *dcid_buf, + uint8_t dcid_len, + xqc_packet_number_t pkt_num, + uint8_t *out_buf, size_t out_buf_cap) +{ + xqc_packet_out_t *po = xqc_packet_out_create(2048); + if (po == NULL) { + return 0; + } + + po->po_pkt.pkt_type = XQC_PTYPE_INIT; + po->po_pkt.pkt_pns = XQC_PNS_INIT; + po->po_pkt.pkt_num = pkt_num; + + memcpy(po->po_pkt.pkt_scid.cid_buf, + cli_conn->scid_set.user_scid.cid_buf, + cli_conn->scid_set.user_scid.cid_len); + po->po_pkt.pkt_scid.cid_len = cli_conn->scid_set.user_scid.cid_len; + + memcpy(po->po_pkt.pkt_dcid.cid_buf, dcid_buf, dcid_len); + po->po_pkt.pkt_dcid.cid_len = dcid_len; + + ssize_t hdr_len = xqc_gen_long_packet_header( + po, + po->po_pkt.pkt_dcid.cid_buf, po->po_pkt.pkt_dcid.cid_len, + po->po_pkt.pkt_scid.cid_buf, po->po_pkt.pkt_scid.cid_len, + NULL, 0, XQC_VERSION_V1, XQC_PKTNO_BITS); + if (hdr_len <= 0) { + xqc_packet_out_destroy(po); + return 0; + } + po->po_used_size += hdr_len; + + /* CONNECTION_CLOSE (transport, PROTOCOL_VIOLATION, no triggering frame). */ + ssize_t cc_len = xqc_gen_conn_close_frame(po, 0x0a, 0, 0); + if (cc_len <= 0) { + xqc_packet_out_destroy(po); + return 0; + } + po->po_used_size += cc_len; + + /* Pad to roughly the same nominal size as build_initial_pkt. */ + size_t aead_tag = 16; + size_t fixed = (size_t)hdr_len + (size_t)cc_len + aead_tag; + size_t target_payload = + (COALESCED_TEST_PKT_TARGET > fixed) ? COALESCED_TEST_PKT_TARGET - fixed : 64; + + if (po->po_used_size + target_payload > po->po_buf_cap) { + xqc_packet_out_destroy(po); + return 0; + } + memset(po->po_buf + po->po_used_size, 0x00, target_payload); + po->po_used_size += target_payload; + + size_t enc_len = 0; + xqc_int_t ret = xqc_packet_encrypt_buf(cli_conn, po, out_buf, + out_buf_cap, &enc_len); + xqc_packet_out_destroy(po); + if (ret != XQC_OK) { + return 0; + } + return enc_len; +} + + /* * Boilerplate: stand up a client+server engine pair, drive a real Initial * from client to server so the server-side connection exists. @@ -472,3 +554,73 @@ xqc_test_coalesced_dcid_a_b_a(void) coalesced_test_teardown(&cli, &svr); } + + +/* + * Smoking-gun test for the §12.2 semantics that drop counters alone + * cannot prove: when the offending packet carries an observable frame + * (CONNECTION_CLOSE), the receiver state must be untouched. If the + * mismatch check fires only after xqc_packet_decrypt_single (which + * internally calls xqc_process_frames), the CLOSE frame still lands + * and drives the connection to DRAINING - which is exactly the bug + * this case is here to catch. + */ +void +xqc_test_coalesced_mismatch_frames_not_applied(void) +{ + coalesced_ctx_t cli = {0}; + coalesced_ctx_t svr = {0}; + + if (coalesced_test_setup(&cli, &svr) != 0) { + CU_FAIL("coalesced_test_setup failed"); + coalesced_test_teardown(&cli, &svr); + return; + } + + uint32_t baseline_dropped = svr.c->packet_dropped_count; + xqc_conn_state_t baseline_state = svr.c->conn_state; + CU_ASSERT_EQUAL(svr.c->conn_close_recv_time, 0); + CU_ASSERT(baseline_state < XQC_CONN_STATE_DRAINING); + + uint8_t dcid_a[XQC_MAX_CID_LEN]; + uint8_t dcid_a_len = cli.c->dcid_set.current_dcid.cid_len; + memcpy(dcid_a, cli.c->dcid_set.current_dcid.cid_buf, dcid_a_len); + + uint8_t dcid_b[XQC_MAX_CID_LEN]; + uint8_t dcid_b_len = dcid_a_len; + memcpy(dcid_b, dcid_a, dcid_a_len); + dcid_b[0] ^= 0xFF; + + /* First packet: legitimate anchor, padding-only. */ + uint8_t enc1[2048]; + size_t enc1_len = build_initial_pkt(cli.c, dcid_a, dcid_a_len, 1, + enc1, sizeof(enc1)); + CU_ASSERT(enc1_len > 0); + + /* Second packet: mismatching DCID, CONNECTION_CLOSE payload. */ + uint8_t enc2[2048]; + size_t enc2_len = build_initial_pkt_with_conn_close( + cli.c, dcid_b, dcid_b_len, 2, enc2, sizeof(enc2)); + CU_ASSERT(enc2_len > 0); + + uint8_t combined[8192]; + CU_ASSERT(enc1_len + enc2_len <= sizeof(combined)); + memcpy(combined, enc1, enc1_len); + memcpy(combined + enc1_len, enc2, enc2_len); + + (void)xqc_conn_process_packet(svr.c, combined, + enc1_len + enc2_len, xqc_now()); + + /* + * Core invariants of RFC 9000 §12.2: the offending packet's frames + * MUST NOT take effect on the connection. + */ + CU_ASSERT_EQUAL(svr.c->conn_close_recv_time, 0); + CU_ASSERT(svr.c->conn_state < XQC_CONN_STATE_DRAINING); + + /* And the drop counter still has to move forward, otherwise we would + * silently accept the rogue packet. */ + CU_ASSERT_EQUAL(svr.c->packet_dropped_count, baseline_dropped + 1); + + coalesced_test_teardown(&cli, &svr); +} diff --git a/tests/unittest/xqc_coalesced_test.h b/tests/unittest/xqc_coalesced_test.h index 2cfcb8491..009ff314f 100644 --- a/tests/unittest/xqc_coalesced_test.h +++ b/tests/unittest/xqc_coalesced_test.h @@ -14,5 +14,6 @@ void xqc_test_coalesced_dcid_match(void); void xqc_test_coalesced_dcid_mismatch(void); void xqc_test_coalesced_dcid_len_mismatch(void); void xqc_test_coalesced_dcid_a_b_a(void); +void xqc_test_coalesced_mismatch_frames_not_applied(void); #endif /* _XQC_COALESCED_TEST_H_INCLUDED_ */