diff --git a/changelogs/current.yaml b/changelogs/current.yaml index f2a6f89ae884..25f23c5dda3c 100644 --- a/changelogs/current.yaml +++ b/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. diff --git a/source/common/quic/envoy_quic_client_connection.cc b/source/common/quic/envoy_quic_client_connection.cc index 09a8745572bd..0616b93808a4 100644 --- a/source/common/quic/envoy_quic_client_connection.cc +++ b/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 socket) + : socket_(std::move(socket)) {} + + void deleteIsPending() override { socket_->close(); } + +private: + std::unique_ptr 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 probing_socket = + static_cast(context.get())->releaseSocket(); + // Extend the socket life time till the end of the current event loop. + dispatcher_.deferredDelete(std::make_unique(std::move(probing_socket))); } void EnvoyQuicClientConnection::onFileEvent(uint32_t events, diff --git a/source/common/quic/envoy_quic_client_connection.h b/source/common/quic/envoy_quic_client_connection.h index 2ddd3e523e76..d8b917e0f4dd 100644 --- a/source/common/quic/envoy_quic_client_connection.h +++ b/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 writer, + std::unique_ptr probing_socket); + + ~EnvoyQuicPathValidationContext() override; + + quic::QuicPacketWriter* WriterToUse() override; + + EnvoyQuicPacketWriter* releaseWriter(); + + Network::ConnectionSocket& probingSocket(); + + std::unique_ptr releaseSocket(); + + private: + std::unique_ptr 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 writer, - std::unique_ptr probing_socket); - - ~EnvoyQuicPathValidationContext() override; - - quic::QuicPacketWriter* WriterToUse() override; - - EnvoyQuicPacketWriter* releaseWriter(); - - Network::ConnectionSocket& probingSocket(); - - std::unique_ptr releaseSocket(); - - private: - std::unique_ptr writer_; - Network::ConnectionSocketPtr socket_; - }; - // Receives notifications from the Quiche layer on path validation results. class EnvoyPathValidationResultDelegate : public quic::QuicPathValidator::ResultDelegate { public: diff --git a/test/common/quic/envoy_quic_client_session_test.cc b/test/common/quic/envoy_quic_client_session_test.cc index 500423532dfd..153316969de0 100644 --- a/test/common/quic/envoy_quic_client_session_test.cc +++ b/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::ENCRYPTION_FORWARD_SECURE)); + InstallDecrypter(quic::ENCRYPTION_FORWARD_SECURE, + std::make_unique()); SetDefaultEncryptionLevel(quic::ENCRYPTION_FORWARD_SECURE); } @@ -64,10 +66,11 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParamallocateDispatcher("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::TestWithParammutable_num_timeouts_to_trigger_port_migration() + ->set_value(1); + http_connection_ = std::make_unique( + 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::TestWithParamclose(); } EnvoyQuicClientStream& sendGetRequest(Http::ResponseDecoder& response_decoder, Http::StreamCallbacks& stream_callbacks) { auto& stream = - dynamic_cast(http_connection_.newStream(response_decoder)); + dynamic_cast(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 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 write_current_; Http::Http3::CodecStats stats_; envoy::config::core::v3::Http3ProtocolOptions http3_options_; - QuicHttpClientConnectionImpl http_connection_; + std::unique_ptr 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( + 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 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(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 diff --git a/test/common/quic/test_utils.h b/test/common/quic/test_utils.h index 63a378bc3496..7a760158e411 100644 --- a/test/common/quic/test_utils.h +++ b/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 {