quic: fix socket UAF during port migration (envoyproxy#31702)

Commit Message: the probing socket is released when port migration fails. If this happens in response to an incoming packet during an I/O event, the follow socket read could cause use-after-free.

[2024-01-08 16:30:53.386][12][critical][backtrace] [./source/server/backtrace.h:104] Caught Segmentation fault, suspect faulting address 0x0
[2024-01-08 16:30:53.387][12][critical][backtrace] [./source/server/backtrace.h:91] Backtrace (use tools/stack_decode.py to get line numbers):
[2024-01-08 16:30:53.387][12][critical][backtrace] [./source/server/backtrace.h:92] Envoy version: 0/1.29.0-dev/test/DEBUG/BoringSSL
[2024-01-08 16:30:53.413][12][critical][backtrace] [./source/server/backtrace.h:96] #0: Envoy::SignalAction::sigHandler() [0x55bb876d499e]
[2024-01-08 16:30:53.413][12][critical][backtrace] [./source/server/backtrace.h:98] #1: [0x7f55fbf92510]
[2024-01-08 16:30:53.440][12][critical][backtrace] [./source/server/backtrace.h:96] #2: Envoy::Network::Utility::readPacketsFromSocket() [0x55bb875de0ef]
[2024-01-08 16:30:53.466][12][critical][backtrace] [./source/server/backtrace.h:96] #3: Envoy::Quic::EnvoyQuicClientConnection::onFileEvent() [0x55bb8663e1eb]
[2024-01-08 16:30:53.492][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#4: Envoy::Quic::EnvoyQuicClientConnection::setUpConnectionSocket()::$_0::operator()() [0x55bb8663f192]
[2024-01-08 16:30:53.518][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#5: std::__invoke_impl<>() [0x55bb8663f151]
[2024-01-08 16:30:53.544][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#6: std::__invoke_r<>() [0x55bb8663f0e2]
[2024-01-08 16:30:53.569][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#7: std::_Function_handler<>::_M_invoke() [0x55bb8663efc2]
[2024-01-08 16:30:53.595][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#8: std::function<>::operator()() [0x55bb85cb8f44]
[2024-01-08 16:30:53.621][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#9: Envoy::Event::DispatcherImpl::createFileEvent()::$_5::operator()() [0x55bb8722560f]
[2024-01-08 16:30:53.648][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#10: std::__invoke_impl<>() [0x55bb872255c1]
[2024-01-08 16:30:53.674][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#11: std::__invoke_r<>() [0x55bb87225562]
[2024-01-08 16:30:53.700][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#12: std::_Function_handler<>::_M_invoke() [0x55bb872253e2]
[2024-01-08 16:30:53.700][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#13: std::function<>::operator()() [0x55bb85cb8f44]
[2024-01-08 16:30:53.726][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#14: Envoy::Event::FileEventImpl::mergeInjectedEventsAndRunCb() [0x55bb872358ec]
[2024-01-08 16:30:53.752][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#15: Envoy::Event::FileEventImpl::assignEvents()::$_1::operator()() [0x55bb87235ed1]
[2024-01-08 16:30:53.778][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#16: Envoy::Event::FileEventImpl::assignEvents()::$_1::__invoke() [0x55bb87235949]
[2024-01-08 16:30:53.804][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#17: event_persist_closure [0x55bb87fab72b]
[2024-01-08 16:30:53.830][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#18: event_process_active_single_queue [0x55bb87faada2]
[2024-01-08 16:30:53.856][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#19: event_process_active [0x55bb87fa56c8]
[2024-01-08 16:30:53.882][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#20: event_base_loop [0x55bb87fa45cc]
[2024-01-08 16:30:53.908][12][critical][backtrace] [./source/server/backtrace.h:96] envoyproxy#21: Envoy::Event::LibeventScheduler::run() [0x55bb8760a59f]
Risk Level: low
Testing: new unit test
Docs Changes: N/A
Release Notes: Yes
Platform Specific Features: N/A

Signed-off-by: Dan Zhang <danzh@google.com>
Co-authored-by: Dan Zhang <danzh@google.com>

Author: danzh2010

changelogs/current.yaml

@@ -113,6 +113,9 @@ bug_fixes:
   change: |
     Fixed a bug in QUIC and HCM interaction which could cause ``use-after-free`` during asynchronous certificates retrieval.
     The fix is guarded by runtime ``envoy.reloadable_features.quic_fix_filter_manager_uaf``.
+- area: quic
+  change: |
+    Fixed a bug in QUIC upstream port migration which could cause use-after-free upon STATELESS_RESET packets.
 - area: redis
   change: |
     Fixed a bug causing crash if incoming redis key does not match against a ``prefix_route`` and ``catch_all_route`` is not defined.

source/common/quic/envoy_quic_client_connection.cc

@@ -11,6 +11,18 @@
 namespace Envoy {
 namespace Quic {
 
+// Used to defer deleting connection socket to avoid deleting IoHandle in a read loop.
+class DeferredDeletableSocket : public Event::DeferredDeletable {
+public:
+  explicit DeferredDeletableSocket(std::unique_ptr<Network::ConnectionSocket> socket)
+      : socket_(std::move(socket)) {}
+
+  void deleteIsPending() override { socket_->close(); }
+
+private:
+  std::unique_ptr<Network::ConnectionSocket> socket_;
+};
+
 EnvoyQuicClientConnection::EnvoyQuicClientConnection(
     const quic::QuicConnectionId& server_connection_id,
     Network::Address::InstanceConstSharedPtr& initial_peer_address,
@@ -188,6 +200,10 @@ void EnvoyQuicClientConnection::onPathValidationFailure(
   // Note that the probing socket and probing writer will be deleted once context goes out of
   // scope.
   OnPathValidationFailureAtClient(/*is_multi_port=*/false, *context);
+  std::unique_ptr<Network::ConnectionSocket> probing_socket =
+      static_cast<EnvoyQuicPathValidationContext*>(context.get())->releaseSocket();
+  // Extend the socket life time till the end of the current event loop.
+  dispatcher_.deferredDelete(std::make_unique<DeferredDeletableSocket>(std::move(probing_socket)));
 }
 
 void EnvoyQuicClientConnection::onFileEvent(uint32_t events,

source/common/quic/envoy_quic_client_connection.h

@@ -27,6 +27,29 @@ class EnvoyQuicClientConnection : public quic::QuicConnection,
                                   public QuicNetworkConnection,
                                   public Network::UdpPacketProcessor {
 public:
+  // Holds all components needed for a QUIC connection probing/migration.
+  class EnvoyQuicPathValidationContext : public quic::QuicPathValidationContext {
+  public:
+    EnvoyQuicPathValidationContext(const quic::QuicSocketAddress& self_address,
+                                   const quic::QuicSocketAddress& peer_address,
+                                   std::unique_ptr<EnvoyQuicPacketWriter> writer,
+                                   std::unique_ptr<Network::ConnectionSocket> probing_socket);
+
+    ~EnvoyQuicPathValidationContext() override;
+
+    quic::QuicPacketWriter* WriterToUse() override;
+
+    EnvoyQuicPacketWriter* releaseWriter();
+
+    Network::ConnectionSocket& probingSocket();
+
+    std::unique_ptr<Network::ConnectionSocket> releaseSocket();
+
+  private:
+    std::unique_ptr<EnvoyQuicPacketWriter> writer_;
+    Network::ConnectionSocketPtr socket_;
+  };
+
   // A connection socket will be created with given |local_addr|. If binding
   // port not provided in |local_addr|, pick up a random port.
   EnvoyQuicClientConnection(const quic::QuicConnectionId& server_connection_id,
@@ -91,29 +114,6 @@ class EnvoyQuicClientConnection : public quic::QuicConnection,
   probeAndMigrateToServerPreferredAddress(const quic::QuicSocketAddress& server_preferred_address);
 
 private:
-  // Holds all components needed for a QUIC connection probing/migration.
-  class EnvoyQuicPathValidationContext : public quic::QuicPathValidationContext {
-  public:
-    EnvoyQuicPathValidationContext(const quic::QuicSocketAddress& self_address,
-                                   const quic::QuicSocketAddress& peer_address,
-                                   std::unique_ptr<EnvoyQuicPacketWriter> writer,
-                                   std::unique_ptr<Network::ConnectionSocket> probing_socket);
-
-    ~EnvoyQuicPathValidationContext() override;
-
-    quic::QuicPacketWriter* WriterToUse() override;
-
-    EnvoyQuicPacketWriter* releaseWriter();
-
-    Network::ConnectionSocket& probingSocket();
-
-    std::unique_ptr<Network::ConnectionSocket> releaseSocket();
-
-  private:
-    std::unique_ptr<EnvoyQuicPacketWriter> writer_;
-    Network::ConnectionSocketPtr socket_;
-  };
-
   // Receives notifications from the Quiche layer on path validation results.
   class EnvoyPathValidationResultDelegate : public quic::QuicPathValidator::ResultDelegate {
   public:

test/common/quic/envoy_quic_client_session_test.cc

@@ -47,6 +47,8 @@ class TestEnvoyQuicClientConnection : public EnvoyQuicClientConnection {
                                   generator) {
     SetEncrypter(quic::ENCRYPTION_FORWARD_SECURE,
                  std::make_unique<quic::test::TaggingEncrypter>(quic::ENCRYPTION_FORWARD_SECURE));
+    InstallDecrypter(quic::ENCRYPTION_FORWARD_SECURE,
+                     std::make_unique<quic::test::TaggingDecrypter>());
     SetDefaultEncryptionLevel(quic::ENCRYPTION_FORWARD_SECURE);
   }
 
@@ -64,10 +66,11 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam<quic::ParsedQui
       : api_(Api::createApiForTest(time_system_)),
         dispatcher_(api_->allocateDispatcher("test_thread")), connection_helper_(*dispatcher_),
         alarm_factory_(*dispatcher_, *connection_helper_.GetClock()), quic_version_({GetParam()}),
-        peer_addr_(Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(),
-                                                        12345)),
+        peer_addr_(
+            Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(), 0)),
         self_addr_(Network::Utility::getAddressWithPort(*Network::Utility::getIpv6LoopbackAddress(),
                                                         54321)),
+        peer_socket_(createConnectionSocket(self_addr_, peer_addr_, nullptr)),
         quic_connection_(new TestEnvoyQuicClientConnection(
             quic::test::TestConnectionId(), connection_helper_, alarm_factory_, writer_,
             quic_version_, *dispatcher_, createConnectionSocket(peer_addr_, self_addr_, nullptr),
@@ -83,12 +86,15 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam<quic::ParsedQui
             /*send_buffer_limit*/ 1024 * 1024, crypto_stream_factory_, quic_stat_names_, {},
             *store_.rootScope(), transport_socket_options_, {}),
         stats_({ALL_HTTP3_CODEC_STATS(POOL_COUNTER_PREFIX(store_, "http3."),
-                                      POOL_GAUGE_PREFIX(store_, "http3."))}),
-        http_connection_(envoy_quic_session_, http_connection_callbacks_, stats_, http3_options_,
-                         64 * 1024, 100) {
+                                      POOL_GAUGE_PREFIX(store_, "http3."))}) {
+    http3_options_.mutable_quic_protocol_options()
+        ->mutable_num_timeouts_to_trigger_port_migration()
+        ->set_value(1);
+    http_connection_ = std::make_unique<QuicHttpClientConnectionImpl>(
+        envoy_quic_session_, http_connection_callbacks_, stats_, http3_options_, 64 * 1024, 100);
     EXPECT_EQ(time_system_.systemTime(), envoy_quic_session_.streamInfo().startTime());
     EXPECT_EQ(EMPTY_STRING, envoy_quic_session_.nextProtocol());
-    EXPECT_EQ(Http::Protocol::Http3, http_connection_.protocol());
+    EXPECT_EQ(Http::Protocol::Http3, http_connection_->protocol());
 
     time_system_.advanceTimeWait(std::chrono::milliseconds(1));
     ON_CALL(writer_, WritePacket(_, _, _, _, _, _))
@@ -98,6 +104,9 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam<quic::ParsedQui
   void SetUp() override {
     envoy_quic_session_.Initialize();
     setQuicConfigWithDefaultValues(envoy_quic_session_.config());
+    quic::test::QuicConfigPeer::SetReceivedStatelessResetToken(
+        envoy_quic_session_.config(),
+        quic::QuicUtils::GenerateStatelessResetToken(quic::test::TestConnectionId()));
     envoy_quic_session_.OnConfigNegotiated();
     envoy_quic_session_.addConnectionCallbacks(network_connection_callbacks_);
     envoy_quic_session_.setConnectionStats(
@@ -112,12 +121,13 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam<quic::ParsedQui
       EXPECT_CALL(network_connection_callbacks_, onEvent(Network::ConnectionEvent::LocalClose));
       envoy_quic_session_.close(Network::ConnectionCloseType::NoFlush);
     }
+    peer_socket_->close();
   }
 
   EnvoyQuicClientStream& sendGetRequest(Http::ResponseDecoder& response_decoder,
                                         Http::StreamCallbacks& stream_callbacks) {
     auto& stream =
-        dynamic_cast<EnvoyQuicClientStream&>(http_connection_.newStream(response_decoder));
+        dynamic_cast<EnvoyQuicClientStream&>(http_connection_->newStream(response_decoder));
     stream.getStream().addCallbacks(stream_callbacks);
 
     std::string host("www.abc.com");
@@ -136,8 +146,12 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam<quic::ParsedQui
   EnvoyQuicAlarmFactory alarm_factory_;
   quic::ParsedQuicVersionVector quic_version_;
   testing::NiceMock<quic::test::MockPacketWriter> writer_;
+  // Initialized with port 0 and modified during peer_socket_ creation.
   Network::Address::InstanceConstSharedPtr peer_addr_;
   Network::Address::InstanceConstSharedPtr self_addr_;
+  // Used in some tests to trigger a read event on the connection to test its full interaction with
+  // socket read utility functions.
+  Network::ConnectionSocketPtr peer_socket_;
   quic::DeterministicConnectionIdGenerator connection_id_generator_{
       quic::kQuicDefaultConnectionIdLength};
   TestEnvoyQuicClientConnection* quic_connection_;
@@ -156,13 +170,13 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam<quic::ParsedQui
   testing::StrictMock<Stats::MockGauge> write_current_;
   Http::Http3::CodecStats stats_;
   envoy::config::core::v3::Http3ProtocolOptions http3_options_;
-  QuicHttpClientConnectionImpl http_connection_;
+  std::unique_ptr<QuicHttpClientConnectionImpl> http_connection_;
 };
 
 INSTANTIATE_TEST_SUITE_P(EnvoyQuicClientSessionTests, EnvoyQuicClientSessionTest,
                          testing::ValuesIn(quic::CurrentSupportedHttp3Versions()));
 
-TEST_P(EnvoyQuicClientSessionTest, ShutdownNoOp) { http_connection_.shutdownNotice(); }
+TEST_P(EnvoyQuicClientSessionTest, ShutdownNoOp) { http_connection_->shutdownNotice(); }
 
 TEST_P(EnvoyQuicClientSessionTest, NewStream) {
   Http::MockResponseDecoder response_decoder;
@@ -412,5 +426,49 @@ TEST_P(EnvoyQuicClientSessionTest, VerifyContextAbortOnFlushWriteBuffer) {
                "unexpectedly reached");
 }
 
+// Tests that receiving a STATELESS_RESET packet on the probing socket doesn't cause crash.
+TEST_P(EnvoyQuicClientSessionTest, StatelessResetOnProbingSocket) {
+  quic::QuicNewConnectionIdFrame frame;
+  frame.connection_id = quic::test::TestConnectionId(1234);
+  ASSERT_NE(frame.connection_id, quic_connection_->connection_id());
+  frame.stateless_reset_token = quic::QuicUtils::GenerateStatelessResetToken(frame.connection_id);
+  frame.retire_prior_to = 0u;
+  frame.sequence_number = 1u;
+  quic_connection_->OnNewConnectionIdFrame(frame);
+  quic_connection_->SetSelfAddress(envoyIpAddressToQuicSocketAddress(self_addr_->ip()));
+
+  // Trigger port migration.
+  quic_connection_->OnPathDegradingDetected();
+  EXPECT_TRUE(envoy_quic_session_.HasPendingPathValidation());
+  auto* path_validation_context =
+      dynamic_cast<EnvoyQuicClientConnection::EnvoyQuicPathValidationContext*>(
+          quic_connection_->GetPathValidationContext());
+  Network::ConnectionSocket& probing_socket = path_validation_context->probingSocket();
+  const Network::Address::InstanceConstSharedPtr& new_self_address =
+      probing_socket.connectionInfoProvider().localAddress();
+  EXPECT_NE(new_self_address->asString(), self_addr_->asString());
+
+  // Send a STATELESS_RESET packet to the probing socket.
+  std::unique_ptr<quic::QuicEncryptedPacket> stateless_reset_packet =
+      quic::QuicFramer::BuildIetfStatelessResetPacket(
+          frame.connection_id, /*received_packet_length*/ 1200,
+          quic::QuicUtils::GenerateStatelessResetToken(quic::test::TestConnectionId()));
+  Buffer::RawSlice slice;
+  slice.mem_ = const_cast<char*>(stateless_reset_packet->data());
+  slice.len_ = stateless_reset_packet->length();
+  peer_socket_->ioHandle().sendmsg(&slice, 1, 0, peer_addr_->ip(), *new_self_address);
+
+  // Receiving the STATELESS_RESET on the probing socket shouldn't close the connection but should
+  // fail the probing.
+  EXPECT_CALL(network_connection_callbacks_, onEvent(Network::ConnectionEvent::RemoteClose))
+      .Times(0);
+  while (envoy_quic_session_.HasPendingPathValidation()) {
+    // Running event loop to receive the STATELESS_RESET and following socket reads shouldn't cause
+    // crash.
+    dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
+  }
+  EXPECT_EQ(self_addr_->asString(), quic_connection_->self_address().ToString());
+}
+
 } // namespace Quic
 } // namespace Envoy

test/common/quic/test_utils.h

@@ -162,6 +162,7 @@ class TestQuicCryptoClientStream : public quic::QuicCryptoClientStream {
                                      proof_handler, has_application_state) {}
 
   bool encryption_established() const override { return true; }
+  quic::HandshakeState GetHandshakeState() const override { return quic::HANDSHAKE_CONFIRMED; }
 };
 
 class TestQuicCryptoClientStreamFactory : public EnvoyQuicCryptoClientStreamFactoryInterface {