From 28383b165da7653d5796a3df38e966e4b0b4508d Mon Sep 17 00:00:00 2001 From: bneradt Date: Fri, 7 Aug 2026 15:17:09 -0500 Subject: [PATCH] Make QUIC connection ID creation explicit Default-constructing a QUIC connection ID generates random bytes, even when the value is only a placeholder that will be overwritten. Several generation paths also randomize the same ID a second time. This patch makes empty, decoded, and newly generated IDs explicit. It deletes default construction, adds a checked CSPRNG-backed factory, and adds coverage for the connection ID representation and initialization contract. Fixes: #5504 --- include/iocore/net/quic/QUICTypes.h | 11 +-- src/iocore/net/CMakeLists.txt | 3 + src/iocore/net/OpenSSLQUICNetVConnection.cc | 14 ++-- src/iocore/net/P_QUICNetVConnection.h | 15 ++-- src/iocore/net/QUICNetProcessor.cc | 3 +- src/iocore/net/QUICNetVConnection.cc | 22 ++--- src/iocore/net/QUICPacketHandler.cc | 7 +- src/iocore/net/qmux/QMuxConnection.cc | 3 +- src/iocore/net/quic/QUICTypes.cc | 35 ++++---- .../net/unit_tests/test_QUICConnectionId.cc | 81 +++++++++++++++++++ 10 files changed, 136 insertions(+), 58 deletions(-) create mode 100644 src/iocore/net/unit_tests/test_QUICConnectionId.cc diff --git a/include/iocore/net/quic/QUICTypes.h b/include/iocore/net/quic/QUICTypes.h index f48b2c7c8bb..d03a33df028 100644 --- a/include/iocore/net/quic/QUICTypes.h +++ b/include/iocore/net/quic/QUICTypes.h @@ -230,7 +230,9 @@ class QUICConnectionId static constexpr int MAX_LENGTH = 20; static constexpr size_t MAX_HEX_STR_LENGTH = MAX_LENGTH * 2 + 1; static QUICConnectionId ZERO(); - QUICConnectionId(); + static QUICConnectionId random(); + /// Force callers to explicitly choose zero, random, or byte-based initialization. + QUICConnectionId() = delete; QUICConnectionId(const uint8_t *buf, uint8_t len); explicit @@ -269,12 +271,11 @@ class QUICConnectionId uint8_t length() const; bool is_zero() const; - void randomize(); private: uint64_t _hashcode() const; - uint8_t _id[MAX_LENGTH]; - uint8_t _len = 0; + uint8_t _id[MAX_LENGTH] = {0}; + uint8_t _len = 0; }; class QUICStatelessResetToken @@ -432,7 +433,7 @@ class QUICPreferredAddress private: IpEndpoint _endpoint_ipv4 = {}; IpEndpoint _endpoint_ipv6 = {}; - QUICConnectionId _cid; + QUICConnectionId _cid = QUICConnectionId::ZERO(); QUICStatelessResetToken _token; bool _valid = false; }; diff --git a/src/iocore/net/CMakeLists.txt b/src/iocore/net/CMakeLists.txt index b317e26b3ea..795b7381c11 100644 --- a/src/iocore/net/CMakeLists.txt +++ b/src/iocore/net/CMakeLists.txt @@ -156,6 +156,9 @@ if(BUILD_TESTING) if(SSLLIB_IS_AT_LEAST_OPENSSL3) target_sources(test_net PRIVATE unit_tests/test_SSLDHParams.cc) endif() + if(TS_USE_QUIC OR TS_USE_QMUX) + target_sources(test_net PRIVATE unit_tests/test_QUICConnectionId.cc) + endif() if(TS_USE_QUIC) target_sources(test_net PRIVATE unit_tests/test_QUICTokenKeyConfig.cc) endif() diff --git a/src/iocore/net/OpenSSLQUICNetVConnection.cc b/src/iocore/net/OpenSSLQUICNetVConnection.cc index 9a06fb94d26..d63003368d7 100644 --- a/src/iocore/net/OpenSSLQUICNetVConnection.cc +++ b/src/iocore/net/OpenSSLQUICNetVConnection.cc @@ -101,9 +101,9 @@ QUICNetVConnection::init(SSL *ssl, QUICPacketHandler *packet_handler) { SET_HANDLER((NetVConnHandler)&QUICNetVConnection::acceptEvent); - this->_ssl = ssl; - this->_packet_handler = packet_handler; - this->_quic_connection_id.randomize(); + this->_ssl = ssl; + this->_packet_handler = packet_handler; + this->_quic_connection_id = QUICConnectionId::random(); this->_initial_source_connection_id = this->_quic_connection_id; this->_cid_text = this->_quic_connection_id.hex(); @@ -477,25 +477,25 @@ QUICNetVConnection::ping() QUICConnectionId QUICNetVConnection::peer_connection_id() const { - return {}; + return QUICConnectionId::ZERO(); } QUICConnectionId QUICNetVConnection::original_connection_id() const { - return {}; + return QUICConnectionId::ZERO(); } QUICConnectionId QUICNetVConnection::first_connection_id() const { - return {}; + return QUICConnectionId::ZERO(); } QUICConnectionId QUICNetVConnection::retry_source_connection_id() const { - return {}; + return QUICConnectionId::ZERO(); } QUICConnectionId diff --git a/src/iocore/net/P_QUICNetVConnection.h b/src/iocore/net/P_QUICNetVConnection.h index a43ed31c345..55dd5034abb 100644 --- a/src/iocore/net/P_QUICNetVConnection.h +++ b/src/iocore/net/P_QUICNetVConnection.h @@ -213,13 +213,14 @@ class QUICNetVConnection : public UnixNetVConnection, SSL *_ssl; QUICConfig::scoped_config _quic_config; - QUICConnectionId _peer_quic_connection_id; // dst cid in local - QUICConnectionId _peer_old_quic_connection_id; // dst previous cid in local - QUICConnectionId _original_quic_connection_id; // dst cid of initial packet from client - QUICConnectionId _first_quic_connection_id; // dst cid of initial packet from client that doesn't have retry token - QUICConnectionId _retry_source_connection_id; // src cid used for sending Retry packet - QUICConnectionId _initial_source_connection_id; // src cid used for Initial packet - QUICConnectionId _quic_connection_id; // src cid in local + QUICConnectionId _peer_quic_connection_id = QUICConnectionId::ZERO(); // dst cid in local + QUICConnectionId _peer_old_quic_connection_id = QUICConnectionId::ZERO(); // dst previous cid in local + QUICConnectionId _original_quic_connection_id = QUICConnectionId::ZERO(); // dst cid of initial packet from client + QUICConnectionId _first_quic_connection_id = + QUICConnectionId::ZERO(); // dst cid of initial packet from client without retry token + QUICConnectionId _retry_source_connection_id = QUICConnectionId::ZERO(); // src cid used for sending Retry packet + QUICConnectionId _initial_source_connection_id = QUICConnectionId::ZERO(); // src cid used for Initial packet + QUICConnectionId _quic_connection_id = QUICConnectionId::ZERO(); // src cid in local #if TS_HAS_QUICHE QUICConnectionTable *_ctable = nullptr; diff --git a/src/iocore/net/QUICNetProcessor.cc b/src/iocore/net/QUICNetProcessor.cc index d7f7012606a..b877e54b64a 100644 --- a/src/iocore/net/QUICNetProcessor.cc +++ b/src/iocore/net/QUICNetProcessor.cc @@ -185,8 +185,7 @@ QUICNetProcessor::connect_re(Continuation *cont, sockaddr const *remote_addr, Ne } // Setup QUICNetVConnection - QUICConnectionId client_dst_cid; - client_dst_cid.randomize(); + QUICConnectionId client_dst_cid = QUICConnectionId::random(); // vc->init set handler of vc `QUICNetVConnection::startEvent` vc->init(QUIC_SUPPORTED_VERSIONS[0], client_dst_cid, client_dst_cid, con, packet_handler); packet_handler->init(vc); diff --git a/src/iocore/net/QUICNetVConnection.cc b/src/iocore/net/QUICNetVConnection.cc index 451b1fc1243..e80dfb587bd 100644 --- a/src/iocore/net/QUICNetVConnection.cc +++ b/src/iocore/net/QUICNetVConnection.cc @@ -79,11 +79,11 @@ QUICNetVConnection::init(QUICVersion /* version ATS_UNUSED */, QUICConnectionId QUICPacketHandler *packet_handler, QUICConnectionTable *ctable, SSL *ssl) { SET_HANDLER((NetVConnHandler)&QUICNetVConnection::acceptEvent); - this->_udp_con = udp_con; - this->_quiche_con = quiche_con; - this->_packet_handler = packet_handler; - this->_original_quic_connection_id = original_cid; - this->_quic_connection_id.randomize(); + this->_udp_con = udp_con; + this->_quiche_con = quiche_con; + this->_packet_handler = packet_handler; + this->_original_quic_connection_id = original_cid; + this->_quic_connection_id = QUICConnectionId::random(); this->_initial_source_connection_id = this->_quic_connection_id; if (ctable) { @@ -448,37 +448,37 @@ QUICNetVConnection::ping() QUICConnectionId QUICNetVConnection::peer_connection_id() const { - return {}; + return QUICConnectionId::ZERO(); } QUICConnectionId QUICNetVConnection::original_connection_id() const { - return {}; + return QUICConnectionId::ZERO(); } QUICConnectionId QUICNetVConnection::first_connection_id() const { - return {}; + return QUICConnectionId::ZERO(); } QUICConnectionId QUICNetVConnection::retry_source_connection_id() const { - return {}; + return QUICConnectionId::ZERO(); } QUICConnectionId QUICNetVConnection::initial_source_connection_id() const { - return {}; + return QUICConnectionId::ZERO(); } QUICConnectionId QUICNetVConnection::connection_id() const { - return {}; + return QUICConnectionId::ZERO(); } std::string_view diff --git a/src/iocore/net/QUICPacketHandler.cc b/src/iocore/net/QUICPacketHandler.cc index 406c9ef7204..3f4a62b521c 100644 --- a/src/iocore/net/QUICPacketHandler.cc +++ b/src/iocore/net/QUICPacketHandler.cc @@ -240,9 +240,8 @@ QUICPacketHandlerIn::_recv_packet(int /* event ATS_UNUSED */, UDPPacket *udp_pac QUICConfig::scoped_config params; if (params->stateless_retry() && token_len == 0) { - QUICConnectionId new_cid; - new_cid.randomize(); - QUICRetryToken retry_token = { + QUICConnectionId new_cid = QUICConnectionId::random(); + QUICRetryToken retry_token = { udp_packet->from, {dcid, static_cast(dcid_len)}, new_cid @@ -278,7 +277,7 @@ QUICPacketHandlerIn::_recv_packet(int /* event ATS_UNUSED */, UDPPacket *udp_pac return; } - QUICConnectionId new_cid; + QUICConnectionId new_cid = QUICConnectionId::random(); QUICCertConfig::scoped_config server_cert; auto default_ctx = server_cert->defaultContext(); diff --git a/src/iocore/net/qmux/QMuxConnection.cc b/src/iocore/net/qmux/QMuxConnection.cc index 3b73b16c5bc..9d6f76c32df 100644 --- a/src/iocore/net/qmux/QMuxConnection.cc +++ b/src/iocore/net/qmux/QMuxConnection.cc @@ -92,12 +92,11 @@ QMuxConnection::_init_shared_config() }); } -QMuxConnection::QMuxConnection(NetVConnection *netvc) : Continuation(netvc->mutex) +QMuxConnection::QMuxConnection(NetVConnection *netvc) : Continuation(netvc->mutex), _synthetic_cid(QUICConnectionId::random()) { _init_shared_config(); SET_HANDLER(&QMuxConnection::main_event); - _synthetic_cid.randomize(); _cids_str = _synthetic_cid.hex(); auto *local_ep = netvc->get_local_addr(); diff --git a/src/iocore/net/quic/QUICTypes.cc b/src/iocore/net/quic/QUICTypes.cc index 9de55694fbf..0628a2a354f 100644 --- a/src/iocore/net/quic/QUICTypes.cc +++ b/src/iocore/net/quic/QUICTypes.cc @@ -29,9 +29,9 @@ #include "iocore/net/quic/QUICTypes.h" #include "iocore/net/quic/QUICConfig.h" #include "iocore/net/quic/QUICIntUtil.h" -#include #include #include +#include uint8_t QUICConnectionId::SCID_LEN = 0; @@ -723,15 +723,24 @@ QUICConnectionId::ZERO() return QUICConnectionId(zero, 0); } -QUICConnectionId::QUICConnectionId() +QUICConnectionId::QUICConnectionId(const uint8_t *buf, uint8_t len) : _len(std::min(len, MAX_LENGTH)) { - this->randomize(); + ink_assert(len <= QUICConnectionId::MAX_LENGTH); + memcpy(this->_id, buf, this->_len); } -QUICConnectionId::QUICConnectionId(const uint8_t *buf, uint8_t len) : _len(len) +QUICConnectionId +QUICConnectionId::random() { - ink_assert(len <= QUICConnectionId::MAX_LENGTH); - memcpy(this->_id, buf, std::min(static_cast(len), QUICConnectionId::MAX_LENGTH)); + uint8_t const length = SCID_LEN; + uint8_t id[MAX_LENGTH] = {0}; + + ink_release_assert(length <= MAX_LENGTH); + if (length == 0) { + return ZERO(); + } + ink_release_assert(RAND_bytes(id, length) == 1); + return {id, length}; } uint8_t @@ -751,20 +760,6 @@ QUICConnectionId::is_zero() const return true; } -void -QUICConnectionId::randomize() -{ - std::random_device rnd; - uint32_t x = rnd(); - for (int i = QUICConnectionId::SCID_LEN - 1; i >= 0; --i) { - if (i % 4 == 0) { - x = rnd(); - } - this->_id[i] = (x >> (8 * (i % 4))) & 0xFF; - } - this->_len = QUICConnectionId::SCID_LEN; -} - uint64_t QUICConnectionId::_hashcode() const { diff --git a/src/iocore/net/unit_tests/test_QUICConnectionId.cc b/src/iocore/net/unit_tests/test_QUICConnectionId.cc new file mode 100644 index 00000000000..c74516354a0 --- /dev/null +++ b/src/iocore/net/unit_tests/test_QUICConnectionId.cc @@ -0,0 +1,81 @@ +/** @file + + Tests for QUIC connection ID initialization. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include "iocore/net/quic/QUICTypes.h" +#include "tsutil/PostScript.h" + +#include + +#include +#include + +static_assert(!std::is_default_constructible_v); + +TEST_CASE("QUICConnectionId requires explicit initialization", "[quic]") +{ + SECTION("empty connection ID") + { + QUICConnectionId cid = QUICConnectionId::ZERO(); + + CHECK(cid.length() == 0); + CHECK(cid.is_zero()); + CHECK(cid.hex() == "0x"); + CHECK(static_cast(cid) == 0); + } + + SECTION("connection ID from bytes") + { + uint8_t const raw[] = {0x01, 0x02, 0x03, 0x04}; + QUICConnectionId cid{raw, static_cast(sizeof(raw))}; + + CHECK(cid.length() == sizeof(raw)); + CHECK_FALSE(cid.is_zero()); + CHECK(cid.h32() == 0x01020304); + CHECK(static_cast(cid) == 0x0102030400000000ULL); + CHECK(cid.hex() == "0x01020304"); + } + + SECTION("random connection ID") + { + uint8_t const previous_scid_len = QUICConnectionId::SCID_LEN; + ts::PostScript restore_scid_len([previous_scid_len]() -> void { QUICConnectionId::SCID_LEN = previous_scid_len; }); + + QUICConnectionId::SCID_LEN = 18; + QUICConnectionId cid = QUICConnectionId::random(); + + CHECK(cid.length() == 18); + CHECK(cid.hex().size() == 2 + 18 * 2); + } + + SECTION("zero-length random connection ID") + { + uint8_t const previous_scid_len = QUICConnectionId::SCID_LEN; + ts::PostScript restore_scid_len([previous_scid_len]() -> void { QUICConnectionId::SCID_LEN = previous_scid_len; }); + + QUICConnectionId::SCID_LEN = 0; + QUICConnectionId cid = QUICConnectionId::random(); + + CHECK(cid.length() == 0); + CHECK(cid.is_zero()); + } +}