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

Author: bneradt

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

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

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

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;

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

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

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

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

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 <random>
 #include <openssl/crypto.h>
 #include <openssl/hmac.h>
+#include <openssl/rand.h>
 
 uint8_t QUICConnectionId::SCID_LEN = 0;
 
@@ -723,17 +723,22 @@ QUICConnectionId::ZERO()
   return QUICConnectionId(zero, 0);
 }
 
-QUICConnectionId::QUICConnectionId()
-{
-  this->randomize();
-}
-
 QUICConnectionId::QUICConnectionId(const uint8_t *buf, uint8_t len) : _len(len)
 {
   ink_assert(len <= QUICConnectionId::MAX_LENGTH);
   memcpy(this->_id, buf, std::min(static_cast<int>(len), QUICConnectionId::MAX_LENGTH));
 }
 
+QUICConnectionId
+QUICConnectionId::random()
+{
+  uint8_t id[MAX_LENGTH] = {0};
+
+  ink_release_assert(SCID_LEN <= MAX_LENGTH);
+  ink_release_assert(RAND_bytes(id, SCID_LEN) == 1);
+  return {id, SCID_LEN};
+}
+
 uint8_t
 QUICConnectionId::length() const
 {
@@ -751,20 +756,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
 {

src/iocore/net/unit_tests/test_QUICConnectionId.cc

@@ -0,0 +1,69 @@
+/** @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 <catch2/catch_test_macros.hpp>
+
+#include <cstdint>
+#include <type_traits>
+
+static_assert(!std::is_default_constructible_v<QUICConnectionId>);
+
+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<uint64_t>(cid) == 0);
+  }
+
+  SECTION("connection ID from bytes")
+  {
+    uint8_t const    raw[] = {0x01, 0x02, 0x03, 0x04};
+    QUICConnectionId cid{raw, static_cast<uint8_t>(sizeof(raw))};
+
+    CHECK(cid.length() == sizeof(raw));
+    CHECK_FALSE(cid.is_zero());
+    CHECK(cid.h32() == 0x01020304);
+    CHECK(static_cast<uint64_t>(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);
+  }
+}